[FFmpeg-devel] [PATCH] Add -t option to ffplay

[Robert Krüger]

On 01.04.2010, at 14:07, Michael Niedermayer wrote:

> On Thu, Apr 01, 2010 at 11:18:14AM +0200, Robert Kr?ger wrote:
>> 
>> On 31.03.2010, at 21:29, Michael Niedermayer wrote:
>> 
>>> On Wed, Mar 31, 2010 at 05:15:25PM +0200, Robert Kr?ger wrote:
>>>> 
>>>> On 31.03.2010, at 16:06, Michael Niedermayer wrote:
>>>> 
>>>>> On Wed, Mar 31, 2010 at 01:52:52PM +0200, Robert Kr?ger wrote:
>>>>>> 
>>>>>> On 27.03.2010, at 00:00, Stefano Sabatini wrote:
>>>>>> 
>>>>>>> On date Wednesday 2010-03-24 16:58:59 +0100, Robert Kr?ger encoded:
>>>>>>>> 
>>>>>>>> On 24.03.2010, at 16:37, Michael Niedermayer wrote:
>>>>>>>> 
>>>>>>>>> On Wed, Mar 24, 2010 at 04:34:16PM +0100, Robert Kr?ger wrote:
>>>>>>>>>> 
>>>>>>>>>> On 24.03.2010, at 15:43, Michael Niedermayer wrote:
>>>>>>>>>> 
>>>>>>>>>>> On Wed, Mar 24, 2010 at 10:10:46AM +0100, Robert Kr?ger wrote:
>>>>>>>>>>>> 
>>>>>> 
>>>>>>>>>>>> 
>>>>>>> 
>>>>>>> Missing manpage docs.
>>>>>> 
>>>>>> Oh, yes. Simply forgot.
>>>>>> 
>>>>>> Updated patch attached.
>>>>>> 
>>>>> [...]
>>>>>> @@ -2473,6 +2476,8 @@
>>>>>>           SDL_Delay(10);
>>>>>>           continue;
>>>>>>       }
>>>>>> +        if(start_time != AV_NOPTS_VALUE)
>>>>>> +            effective_start_time = start_time;
>>>>>>       if(url_feof(ic->pb) || eof) {
>>>>>>           if(is->video_stream >= 0){
>>>>>>               av_init_packet(pkt);
>>>>>> @@ -2484,7 +2489,7 @@
>>>>>>           SDL_Delay(10);
>>>>>>           if(is->audioq.size + is->videoq.size + is->subtitleq.size ==0){
>>>>>>               if(loop!=1 && (!loop || --loop)){
>>>>>> -                    stream_seek(cur_stream, start_time != AV_NOPTS_VALUE ? start_time : 0, 0, 0);
>>>>>> +                    stream_seek(cur_stream, effective_start_time, 0, 0);
>>>>>>               }else if(autoexit){
>>>>>>                   ret=AVERROR_EOF;
>>>>>>                   goto fail;
>>>>> 
>>>>> why is this changed?
>>>>> 
>>>> 
>>>> it is extracted into a variable because otherwise the conditional (start_time != AV_NOPTS_VALUE ? start_time : 0) would be used twice expressing the same thing.
>>> 
>>> i think using the conditional twice is more readable here than a distant
>>> variable
>>> 
>> 
>> OK, changed.
>> 
>>> 
>>>> 
>>>>> 
>>>>>> @@ -2501,11 +2506,16 @@
>>>>>>           SDL_Delay(100); /* wait for user event */
>>>>>>           continue;
>>>>>>       }
>>>>>> -        if (pkt->stream_index == is->audio_stream) {
>>>>>> +        /* check if packet is in play range specified by user, then queue, otherwise discard */
>>>>>> +        pkt_past_play_range = duration != AV_NOPTS_VALUE &&
>>>>>> +                (pkt->pts - ic->streams[pkt->stream_index]->start_time) *
>>>>>> +                av_q2d(ic->streams[pkt->stream_index]->time_base) - (double)effective_start_time/1000000
>>>>>> +                > ((double)duration/1000000);
>>>>>> +        if (!pkt_past_play_range && pkt->stream_index == is->audio_stream) {
>>>>>>           packet_queue_put(&is->audioq, pkt);
>>>>>> -        } else if (pkt->stream_index == is->video_stream) {
>>>>>> +        } else if (!pkt_past_play_range && pkt->stream_index == is->video_stream) {
>>>>>>           packet_queue_put(&is->videoq, pkt);
>>>>>> -        } else if (pkt->stream_index == is->subtitle_stream) {
>>>>>> +        } else if (!pkt_past_play_range && pkt->stream_index == is->subtitle_stream) {
>>>>>>           packet_queue_put(&is->subtitleq, pkt);
>>>>>>       } else {
>>>>>>           av_free_packet(pkt);
>>>>> 
>>>> 
>>>>> it seems to me that a pkt_in_play_range would lead to simpler code
>>>> 
>>>> I was trying to avoid two calls to av_free_packet (one for the case that it is not in playable range and the other for the packet not being audio, video or subtitle). That's why I ended up with the && in each if. Do you have a suggestion with one call to av-free-packet and less complex conditionals or do you mean it would be sufficient to reformulate it as pkt_in_play_range and get rid of the three negations?  
>>> 
>>> i just meant the 3 negations
>>> its just a small step but cleaner IMHO
>>> 
>> 
>> You're right. Changed.
>> 
>> Regards,
>> 
>> Robert
>> 
> 
>> doc/ffplay-doc.texi |    2 ++
>> ffplay.c            |   21 ++++++++++++++++++---
>> 2 files changed, 20 insertions(+), 3 deletions(-)
>> 9a135789ac72db2406e4a12ee80b18fd57a765f0  ffplay-duration-option.patch
> 
> ok if tested

I have tested with different combinations of -ss, -t, -autoexit, -loop and haven't found any problems so far.