[MPlayer-dev-eng] Re: [PACTH] get_byte_pos slave command

[Alban Bedel]

On Wed, 19 Apr 2006 16:53:17 +0200
Reimar Döffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de> wrote:

> Hi,
> On Wed, Apr 19, 2006 at 12:53:59PM +0200, Alban Bedel wrote:
> > On Tue, 18 Apr 2006 23:16:44 +0200
> > Reimar Döffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de> wrote:
> > > I think this might be useful, esp. in combination with -sb.
> > > Comments?
> > 
> > Making it a property would make more sense imho.
> 
> Like the attached patch?

Looks good beside a few details. Best is probably if i simply describe
the stuff a bit, althought by the time a finished writing this you'll
probably already have worked much of it by yourself.

* GET/SET

The argument should be a pointer to the data type represented by the option
type. Before doing anything else the property should check that it make
any sense in the current context. For ex. an audio related property
should return M_PROPRETY_UNAVAILABLE right away if their is no sound.

If the argument is NULL the code should still perform its test but then
simply return 0 instead of really setting/reading the value.

If the value should stay within some fix bounds, this should be
reflected in the m_option struct. The SET code should also clamp the
"input" value before setting it so that the caller can directly know
which value was actually set. To make things simpler their is the
M_PROPERTY_CLAMP() macro that do "automatic" clamping from the value
in the m_option struct.

* PRINT

This is needed only if the print fonction at the m_option level doesn't
return a suitable value to show on the OSD and such. The exact same
test as for GET should be performed to avoid any inconsistency.
The argument is a char** that should be filled with a newly allocated
string. Like for GET/SET the argument might be NULL.

* PARSE

Invert version of PRINT, not used currently.

* STEP_UP/DOWN

These two are fully optional but should be implemented if they make sense.
Only properties how implement these 2 operations can currently be modified
in the parameters osd menu. If an argument is given the value should be
stepped according to both the sign of the argument and the action
(ie. STEP_UP with a negative value go down). If the argument is NULL it
should step with some "default" value (like 100ms for a delay, 1 for a
flag, and so on).

So regarding your patch all it need is a few if(!arg) return NULL;
Perhaps also set the minimum to 0 and clamp the input value on set.
The print stuff can also be ditched if it just does what the print
function for CONF_TYPE_POSITION do.
Dunno if set make much sense, althought it could be a good way to
test the error resistance of our demuxers i suppose :)

> But a few questions came up:
> What are M_PROPERTY_ERROR and M_PROPERTY_OK for? They are not used
> anywhere currently...

Good that you raise that. They are more left over from early version
than anything else atm, my bad that i didn't fixed that before commit.

Originaly ERROR (0) was meant for when things go really wrong. But
now properties return 0 when get/set/print is called w/o argument.
It would probably be better to rename the define for 0 to something like
M_PROPERTY_SUPPORTED and move M_PROPERTY_ERROR to a negative value.
And then perhaps also move all those return [1|0]; to their respective
define. Sorry i'm often a bit too lazy with those "trivial" defines :(

> Does all this property stuff really have to be in mplayer.c??

Probably not all. Some could be moved elsewhere, but where ? Some will
have to stay i fear, making sh_video, sh_audio, etc globals sound really
too bad to me.

> Am I missing something or is there really no (I really mean nada, zilch)
> documentation for this stuff?

Right :( I do plan to eventually, some day get around to do it, but it's
a quiet big work bcs i must first document the option stuff on which this
is based.

> Not even a simple list of supported
> properties?

Their is -list-properties ;)

> Shouldn't those property commands also be active in idle mode?

That's the plan, some could probably work in paused mode too.

	Albeu