[FFmpeg-devel] [PATCH] use reordered_opaque in ffmpeg.c
Alexander Strange
astrange
Mon Jun 21 12:34:19 CEST 2010
On Jun 20, 2010, at 3:50 AM, Michael Niedermayer wrote:
> 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
Applied.
>
> 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.
Yes, I meant to work on that in another patch.
>> //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.
Seems so. Fixed locally.
> 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)
Hmm, ffmpeg and ffplay unfortunately don't share data structures besides cmd-line parsing.
This could be extracted and put in cmdutils, I guess.
Are there any files where faulty_dts > faulty_pts?
I have this one with faulty_dts==faulty_pts:
http://samples.mplayerhq.hu/archive/container/mpeg/mpeg%2bmpeg2video%2bac3%2b%2bnon_monotone_timestamps.mpg
but I'm not sure if we need last_dts to handle that.
More information about the ffmpeg-devel
mailing list