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

[Michael Niedermayer]

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100401/bff31b87/attachment.pgp>