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

[Reimar Döffinger]

On 8 Jan 2011, at 10:20, Clément Bœsch <ubitux at 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.