[MPlayer-dev-eng] [RFC] defines for demux seek flags

[Uoti Urpala]

On Sun, 2008-01-27 at 09:13 +0100, Reimar Döffinger wrote:
> > -    demux_seek(priv->ad,pos,audio_delay,1);
> > +    demux_seek(priv->ad,pos,audio_delay,(struct seekflags){.absolute=1});
> 
> I find this really, really ugly.

While not particularly pretty I don't find it exceptionally ugly. I
don't think "DEMUXER_SEEKFLAG_ABSOLUTE" would be particularly pretty
either. Are you sure you're not judging it unnecessarily negatively due
to unfamiliarity with the syntax?

It would be possible to define separate constants for such flag values
but I don't think that would be worth it.

> > +struct seekflags {
> > +    unsigned absolute : 1;
> > +    unsigned percent : 1;
> > +};
> 
> Typedef to seekflags_t might be nicer, though this name still risks
> collisions either way.

It could be named mp_seekflags. I'm not sure about the typedef part. I
think current current MPlayer sources use typedefs excessively for
things that would better be used as normal structs, but maybe this is a
case where using a (shorter) typedef name would have some benefit.

> > diff --git a/mplayer.c b/mplayer.c
> > index 8d779d6..efcba69 100644
> > --- a/mplayer.c
> > +++ b/mplayer.c
> > @@ -2487,7 +2487,8 @@ static void edl_update(MPContext *mpctx)
> >  static int seek(MPContext *mpctx, double amount, int style)
> >  {
> >      current_module = "seek";
> > -    if (demux_seek(mpctx->demuxer, amount, audio_delay, style) == 0)
> > +    struct seekflags flags = {.absolute = style & 1, .percent = style >> 1};
> > +    if (demux_seek(mpctx->demuxer, amount, audio_delay, flags) == 0)
> >  	return -1;
> 
> Quite a cheat to avoid a even larger patch :-).

mplayer.c and command.c use the flags in the global "abs_seek_pos"
variable which should be removed rather than just converted. I left that
for another time. I don't think that is really a "cheat"...

> This way does not really look much better to me, also considering the
> amount of change it needs.

I think the use in demuxers does look significantly better than bitmask
defines would. How would the defines require significantly less change?
You'd still need to change most of the demuxers (unless you left some
"flags & 1" code in, but that's neither a fair comparison nor a
desirable thing to do IMO). And in general I don't think the "amount of
change" is an argument against something unless it makes implementation
impractical.