[FFmpeg-devel] [PATCH] use reordered_opaque in ffmpeg.c

Michael Niedermayer michaelni
Sun Jun 20 12:50:12 CEST 2010


On Sun, Jun 20, 2010 at 01:58:55AM -0700, Alexander Strange wrote:
> This changes ffmpeg.c to push PTS through the decoder using reordered_opaque instead of the old and not quite accurate thing it does now.
> It fixes a problem where initial decoder delay was treated as dropped frames.
> 
> Contrived example:
> 
> http://astrange.ithinksw.net/ffmpeg/camp_mot_mbaff0_full.mp4
> ffmpeg -strict 2 -i camp_mot_mbaff0_full.mp4 -y -an -f framecrc crc.txt
> 
> Unpatched: frame=   30 fps=  0 q=0.0 Lsize=       1kB time=1.00 bitrate=   6.9kbits/s dup=14 drop=14 
> Patched: frame=   30 fps=  0 q=0.0 Lsize=       1kB time=1.00 bitrate=   6.9kbits/s    
> 
> The first frame is emitted 14 times without the patch, and the last 14 frames are lost.
> 
> This is rare on mainline, but required for it to support ffmpeg-mt which has increased decoder delay.
> 
> A few things I tested still have timestamp problems - files with inaccurate dts or packed B-frames have non-monotonic pts coming out of reordered_opaque, and the issue with start_time isn't fixed - but I haven't found any files with worse sync.
> 
> Tested make test, but not make fate since it fails immediately on 4xm even without any patching.
> 

>  ffmpeg.c |   15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 516a736d70a632d20f9f4b19640dc16dbebf3612  0001-ffmpeg-Replace-EOF-test-with-equivalent-condition.patch
> From e2b0d1cecbf5e83d706c028a24bbd6c05a402133 Mon Sep 17 00:00:00 2001
> From: Alexander Strange <astrange at ithinksw.com>
> Date: Thu, 17 Jun 2010 01:17:06 -0700
> Subject: [PATCH 1/2] ffmpeg: Replace EOF test with equivalent condition
> 
> Explicitly tests whether the last decode call returned something.
> Required for next commit, which breaks the current test.
> ---
>  ffmpeg.c |   15 ++++++++-------
>  1 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/ffmpeg.c b/ffmpeg.c
> index b8dbe36..36ced7d 100644
> --- a/ffmpeg.c
> +++ b/ffmpeg.c
> @@ -1509,12 +1509,11 @@ static int output_packet(AVInputStream *ist, int ist_index,
>      AVFormatContext *os;
>      AVOutputStream *ost;
>      int ret, i;
> -    int got_picture;
> +    int got_output;
>      AVFrame picture;
>      void *buffer_to_free;
>      static unsigned int samples_size= 0;
>      AVSubtitle subtitle, *subtitle_to_free;
> -    int got_subtitle;

merging the 2 variables is ok, you can commit that

for the rest of patch #1 please see comments below

[...]

>  ffmpeg.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
> 46dcd61db8077e9b7beebf05d5a22b39ed964378  0002-ffmpeg-Send-decoded-frame-timestamps-through-reorder.patch
> From ec244d5f22b863fa9f6d0c2887c197e28db51cd7 Mon Sep 17 00:00:00 2001
> From: Alexander Strange <astrange at ithinksw.com>
> Date: Wed, 16 Jun 2010 23:05:48 -0700
> Subject: [PATCH 2/2] ffmpeg: Send decoded frame timestamps through reordered_opaque
> 
> Improves a/v sync in the presence of decoding delay, which could have been
> detected as dropped frames before.
> ---
>  ffmpeg.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/ffmpeg.c b/ffmpeg.c
> index 36ced7d..edc5722 100644
> --- a/ffmpeg.c
> +++ b/ffmpeg.c
> @@ -301,6 +301,7 @@ typedef struct AVInputStream {
>      int64_t       next_pts;  /* synthetic pts for cases where pkt.pts
>                                  is not defined */
>      int64_t       pts;       /* current pts */
> +    int64_t       last_pts;  /* the previously output pts, for detecting misordering */
>      int is_start;            /* is 1 at the start and after a discontinuity */
>      int showed_multi_packet_warning;
>      int is_past_recording_time;
> @@ -1514,6 +1515,7 @@ static int output_packet(AVInputStream *ist, int ist_index,
>      void *buffer_to_free;
>      static unsigned int samples_size= 0;
>      AVSubtitle subtitle, *subtitle_to_free;
> +    int64_t pkt_pts = AV_NOPTS_VALUE;
>  #if CONFIG_AVFILTER
>      int frame_available;
>  #endif
> @@ -1536,6 +1538,8 @@ static int output_packet(AVInputStream *ist, int ist_index,
>  
>      if(pkt->dts != AV_NOPTS_VALUE)
>          ist->next_pts = ist->pts = av_rescale_q(pkt->dts, ist->st->time_base, AV_TIME_BASE_Q);
> +    if(pkt->pts != AV_NOPTS_VALUE)
> +        pkt_pts = av_rescale_q(pkt->pts, ist->st->time_base, AV_TIME_BASE_Q);

I think its more ideal if we avoid (aka do as late as possible) to 
convert timebases so as to avoid rounding errors.


>  
>      //while we have more to decode or while the decoder did output something on EOF
>      while (avpkt.size > 0 || (!pkt && got_output)) {
> @@ -1590,6 +1594,7 @@ static int output_packet(AVInputStream *ist, int ist_index,
>                      decoded_data_size = (ist->st->codec->width * ist->st->codec->height * 3) / 2;
>                      /* XXX: allocate picture correctly */
>                      avcodec_get_frame_defaults(&picture);
> +                    ist->st->codec->reordered_opaque = pkt_pts;
>  
>                      ret = avcodec_decode_video2(ist->st->codec,
>                                                  &picture, &got_output, &avpkt);
> @@ -1600,12 +1605,17 @@ static int output_packet(AVInputStream *ist, int ist_index,
>                          /* no picture yet */
>                          goto discard_packet;
>                      }
> +                    if (picture.reordered_opaque != AV_NOPTS_VALUE && picture.reordered_opaque > ist->last_pts)
> +                        ist->pts = picture.reordered_opaque;
> +                    else if (verbose > 2 && picture.reordered_opaque <= ist->last_pts)
> +                        fprintf(stderr, "ignoring misordered pts %lld -> %lld\n", ist->last_pts, picture.reordered_opaque);

the else is missing a != AV_NOPTS_VALUE check
you forget updating ist->next_pts in line with the new ist->pts, which leads
to the need of patch #1. And iam not saying what patch #1 does is bad i just
think it only becomes needed due to this apparent bug.
also the check from ffplay should be used probably instead of the last_pts
stuff
i mean:
        if (got_picture) {
            if(pkt->dts != AV_NOPTS_VALUE){
                is->faulty_dts += pkt->dts <= is->last_dts_for_fault_detection;
                is->last_dts_for_fault_detection= pkt->dts;
            }
            if(frame->reordered_opaque != AV_NOPTS_VALUE){
                is->faulty_pts += frame->reordered_opaque <= is->last_pts_for_fault_detection;
                is->last_pts_for_fault_detection= frame->reordered_opaque;
            }
        }

        if(   (   decoder_reorder_pts==1
               || (decoder_reorder_pts && is->faulty_pts<is->faulty_dts)
               || pkt->dts == AV_NOPTS_VALUE)
           && frame->reordered_opaque != AV_NOPTS_VALUE)
            *pts= frame->reordered_opaque;
        else if(pkt->dts != AV_NOPTS_VALUE)
            *pts= pkt->dts;
        else
            *pts= 0;

This could ideally of course be moved into shared code (a macro if we dont
find a solution for accessing the fields of the struct cleaner)


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- 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/20100620/21ac093b/attachment.pgp>



More information about the ffmpeg-devel mailing list