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.