[MPlayer-dev-eng] [PATCH] Add size based support for -endpos in MPlayer

[Clément Bœsch]

On Wed, Jan 05, 2011 at 04:22:12PM +0100, Reimar Döffinger wrote:
> On Sat, Dec 25, 2010 at 04:56:50PM +0100, Clément Bœsch wrote:
> > A small patch to add size based support for -endpos in MPlayer. I tested
> > it with random videos, audio only files and -loop option and it seems to
> > work. Anything else I should test?
> 
> -loop 0 both before and after the filename might be a good idea (it
> behaves differently, putting it after will use seeking to loop).
> 

Ok. Both seem to be fine.

> > However, I don't understand why mpctx->sh_audio is tested when
> > mpctx->sh_video is NULL; is there a way to call the code around
> > mplayer.c:L3698 without having neither mpctx->sh_video and
> > mpctx->sh_audio?
> 
> Well, I guess the reason is that it's hard to know and adding that kind
> of stuff later is quite a pain.
> Usually I'd expect this kind of case to happen if you have an audio-only
> file and then the audio parameters change to something neither the ao
> nor automatic conversion can handle.
> It should drop out of that code very quickly, but I am not confident it
> might not run across this first.
> 

Ok, thanks.

> > And then, does this mean mpctx->stream could be NULL?
> 
> Unlikely, audio e.g. can also become null any time by audio switching
> (audio off is one of the states), we do not really have any such complex
> stuff for streams.
> 

Ok :)

> > @@ -3711,7 +3706,8 @@ if(!mpctx->sh_video) {
> >    if(!quiet)
> >      print_status(a_pos, 0, 0);
> >  
> > -  if(end_at.type == END_AT_TIME && end_at.pos < a_pos)
> > +  if(end_at.type == END_AT_TIME && end_at.pos < a_pos ||
> > +     end_at.type == END_AT_SIZE && end_at.pos < stream_tell(mpctx->stream))
> >      mpctx->eof = PT_NEXT_ENTRY;
> >    update_subtitles(NULL, a_pos, mpctx->d_sub, 0);
> >    update_osd_msg();
> > @@ -3819,10 +3815,9 @@ if(auto_quality>0){
> >       if (play_n_frames <= 0) mpctx->eof = PT_NEXT_ENTRY;
> >   }
> >  
> > -
> > -// FIXME: add size based support for -endpos
> > - if (end_at.type == END_AT_TIME &&
> > -         !frame_time_remaining && end_at.pos <= mpctx->sh_video->pts)
> > + if (!frame_time_remaining &&
> > +     ((end_at.type == END_AT_TIME &&       mpctx->sh_video->pts >= end_at.pos) ||
> > +      (end_at.type == END_AT_SIZE && stream_tell(mpctx->stream) >= end_at.pos)))
> 
> In principle fine, but maybe it's time to factor out this duplicated
> code?
> E.g. (not valid C, too lazy to look up the types)
> static int is_at_end(&end_at, mpctx) {
>     switch (end_at->type)
>     {
>     case END_AT_TIME:
>         return mpctx->sh_video->pts >= end_at->pos;
>     ...
>     }
>     return 0;
> }
> 

Sure. Since I don't want to do that in the first patch, here is a second
one. But there is a few things I think may be improved: doesn't end_at and
frame_time_remaining belong to the MPContext? Putting end_at in it will
likely clarify the code, while for the other one it could avoid an
unnecessary check before calling is_at_end (it could get it from the
mpctx, just like it would take end_at).

By the way, I will apply the first patch (not the one attached) adding the
mplayer -endpos bytes support in the next days.

-- 
Clément B.