[PATCH] Add size based support for -endpos in MPlayer
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? 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? And then, does this mean mpctx->stream could be NULL? -- Clément B. Not sent from a jesusPhone.
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?
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? And then, does this mean mpctx->stream could be NULL?
Ping? -- Clément B. Not sent from a jesusPhone.
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).
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.
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.
@@ -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; } Particularly when extending this the switch is far more readable than the if, too.
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.
On 8 Jan 2011, at 10:20, Clément Bœsch <ubitux@gmail.com> wrote:
On Wed, Jan 05, 2011 at 04:22:12PM +0100, Reimar Döffinger wrote:
@@ -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).
Doesn't seem like much of an improvement but I don't mind. However for this second patch I'd prefer if you applied merging the two if(!quiet) separately, also because the patch you have attached ends up with the comment at a wrong and confusing place.
By the way, I will apply the first patch (not the one attached) adding the mplayer -endpos bytes support in the next days.
No need to wait from my side.
On Sat, Jan 08, 2011 at 05:37:39PM +0100, Reimar Döffinger wrote:
On 8 Jan 2011, at 10:20, Clément Bœsch <ubitux@gmail.com> wrote:
On Wed, Jan 05, 2011 at 04:22:12PM +0100, Reimar Döffinger wrote:
@@ -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).
Doesn't seem like much of an improvement but I don't mind. However for this second patch I'd prefer if you applied merging the two if(!quiet) separately, also because the patch you have attached ends up with the comment at a wrong and confusing place.
Ok, I'll send a new patch. Btw, what is the "handle this better" refering to?
By the way, I will apply the first patch (not the one attached) adding the mplayer -endpos bytes support in the next days.
No need to wait from my side.
Applied. -- Clément B.
On Mon, Jan 10, 2011 at 08:23:23PM +0100, Clément Bœsch wrote:
Doesn't seem like much of an improvement but I don't mind. However for this second patch I'd prefer if you applied merging the two if(!quiet) separately, also because the patch you have attached ends up with the comment at a wrong and confusing place.
Ok, I'll send a new patch. Btw, what is the "handle this better" refering to?
I suspect the duplicated !quiet condition and generally that code was considered a bit ugly by whoever wrote it.
On Wed, Jan 05, 2011 at 04:22:12PM +0100, Reimar Döffinger wrote: [...]
- -// 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; }
Particularly when extending this the switch is far more readable than the if, too.
Patch attached. I'll apply it in 3 days. -- Clément B.
On Sat, Jun 11, 2011 at 04:21:32PM +0200, Clément Bœsch wrote:
On Wed, Jan 05, 2011 at 04:22:12PM +0100, Reimar Döffinger wrote: [...]
- -// 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; }
Particularly when extending this the switch is far more readable than the if, too.
Patch attached. I'll apply it in 3 days.
New patch attached, fixing pts for audio only files. -- Clément B.
On Sat, Jun 11, 2011 at 04:45:00PM +0200, Clément Bœsch wrote:
@@ -3802,8 +3816,7 @@ if (!quiet) print_status(a_pos, 0, 0);
- 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)) + if (is_at_end(mpctx, &end_at)) mpctx->eof = PT_NEXT_ENTRY;
You should also update the if ((!quiet || end_at.type == END_AT_TIME) && mpctx->sh_audio) condition right above I think. Though note that the update_subtitles also uses a_pos, so that should be updated to if (mpctx->sh_audio). Since that's a rather unrelated bug-fix I just applied that ;-)
On Sat, Jun 11, 2011 at 04:45:00PM +0200, Clément Bœsch wrote:
On Sat, Jun 11, 2011 at 04:21:32PM +0200, Clément Bœsch wrote:
On Wed, Jan 05, 2011 at 04:22:12PM +0100, Reimar Döffinger wrote: [...]
- -// 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; }
Particularly when extending this the switch is far more readable than the if, too.
Patch attached. I'll apply it in 3 days.
New patch attached, fixing pts for audio only files.
Alternate version attached to avoid recomputing audio pts. -- Clément B.
On Sat, Jun 11, 2011 at 05:09:39PM +0200, Clément Bœsch wrote:
On Sat, Jun 11, 2011 at 04:45:00PM +0200, Clément Bœsch wrote:
On Sat, Jun 11, 2011 at 04:21:32PM +0200, Clément Bœsch wrote:
On Wed, Jan 05, 2011 at 04:22:12PM +0100, Reimar Döffinger wrote: [...]
- -// 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; }
Particularly when extending this the switch is far more readable than the if, too.
Patch attached. I'll apply it in 3 days.
New patch attached, fixing pts for audio only files.
Alternate version attached to avoid recomputing audio pts.
I think that one's really nice. Not sure if it will stay so in the long run, but I'd say go ahead.
On Sat, Jun 11, 2011 at 05:21:21PM +0200, Reimar Döffinger wrote:
On Sat, Jun 11, 2011 at 05:09:39PM +0200, Clément Bœsch wrote:
On Sat, Jun 11, 2011 at 04:45:00PM +0200, Clément Bœsch wrote:
On Sat, Jun 11, 2011 at 04:21:32PM +0200, Clément Bœsch wrote:
On Wed, Jan 05, 2011 at 04:22:12PM +0100, Reimar Döffinger wrote: [...]
- -// 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; }
Particularly when extending this the switch is far more readable than the if, too.
Patch attached. I'll apply it in 3 days.
New patch attached, fixing pts for audio only files.
Alternate version attached to avoid recomputing audio pts.
I think that one's really nice. Not sure if it will stay so in the long run, but I'd say go ahead.
OK, applied. -- Clément B.
participants (2)
-
Clément Bœsch -
Reimar Döffinger