[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