diff mbox

[FFmpeg-devel,4/4] lavf/mov: Add support for edit list parsing.

Message ID CAGD_KHdv3d_pGUxM6E-TB3pjHSJMZ8tEP9yn0tFt5cFnUVM2ng@mail.gmail.com
State Superseded
Headers show

Commit Message

Sasi Inguva Aug. 25, 2016, 7:31 p.m. UTC
oops. thanks for pointing that out. Even in our version of ffmpeg, that
assert doesn't get compiled so we never catched it. That assert logic is
not correct anymore. At the end of one edit list there can be frames marked
as discard, for which we would keep increasing the timestamp even if they
are marked as discard, so that when the timestamps are rerodered to compute
PTS B-frames get the correct PTS. But the next edit list should always
start with the timestamp of the last-non-discarded frame of the previous
edit list. Hence we will get non-increasing timestamps added as index
entries.

The test may have passed for you before, because before that line was
assert(..) instead of av_assert1(...) so maybe that line wasn't getting
compiled before. Attaching the 4 patches again.



On Wed, Aug 24, 2016 at 7:37 PM, Michael Niedermayer <michael@niedermayer.cc
> wrote:

> On Wed, Aug 24, 2016 at 06:42:16PM -0700, Sasi Inguva wrote:
> > hmm. strange. I just rebased my branch on top of head, and reran the
> test,
> > and it succeeds along with all other fate tests. I am attaching the 4
> > patches again here.
>
> you need to build with
> --assert-level=2
> to see the failure
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>

Comments

Michael Niedermayer Aug. 25, 2016, 9:39 p.m. UTC | #1
On Thu, Aug 25, 2016 at 12:31:19PM -0700, Sasi Inguva wrote:
> oops. thanks for pointing that out. Even in our version of ffmpeg, that
> assert doesn't get compiled so we never catched it. That assert logic is
> not correct anymore. At the end of one edit list there can be frames marked
> as discard, for which we would keep increasing the timestamp even if they
> are marked as discard, so that when the timestamps are rerodered to compute
> PTS B-frames get the correct PTS. But the next edit list should always
> start with the timestamp of the last-non-discarded frame of the previous
> edit list. Hence we will get non-increasing timestamps added as index
> entries.
> 
> The test may have passed for you before, because before that line was
> assert(..) instead of av_assert1(...) so maybe that line wasn't getting
> compiled before. Attaching the 4 patches again.

(i think 3rd one) breaks
./ffmpeg -i ~/tickets/5528/fire.mp3 test.wav
...
[mp3 @ 0x393b980] invalid new backstep -1
Assertion avctx->internal->skip_samples >= 0 failed at libavcodec/utils.c:2357
Aborted

http://trac.ffmpeg.org/attachment/ticket/5528/fire.mp3

[...]
Michael Niedermayer Aug. 25, 2016, 10:01 p.m. UTC | #2
On Thu, Aug 25, 2016 at 12:31:19PM -0700, Sasi Inguva wrote:
> oops. thanks for pointing that out. Even in our version of ffmpeg, that
> assert doesn't get compiled so we never catched it. That assert logic is
> not correct anymore. At the end of one edit list there can be frames marked
> as discard, for which we would keep increasing the timestamp even if they
> are marked as discard, so that when the timestamps are rerodered to compute
> PTS B-frames get the correct PTS. But the next edit list should always
> start with the timestamp of the last-non-discarded frame of the previous
> edit list. Hence we will get non-increasing timestamps added as index
> entries.
> 
> The test may have passed for you before, because before that line was
> assert(..) instead of av_assert1(...) so maybe that line wasn't getting
> compiled before. Attaching the 4 patches again.

patchset breaks timestamps for audio:
./ffmpeg -i matrixbench_mpeg2.mpg  -t 0.1  one.mov
./ffprobe  -show_packets -print_format compact one.mov
packet|codec_type=video|stream_index=0|pts=0|pts_time=0.000000|dts=-1024|dts_time=-0.080000|duration=512|duration_time=0.040000|convergence_duration=N/A|convergence_duration_time=N/A|size=9368|pos=36|flags=K
packet|codec_type=video|stream_index=0|pts=1024|pts_time=0.080000|dts=-512|dts_time=-0.040000|duration=512|duration_time=0.040000|convergence_duration=N/A|convergence_duration_time=N/A|size=2190|pos=9404|flags=_
packet|codec_type=audio|stream_index=1|pts=0|pts_time=0.000000|dts=0|dts_time=0.000000|duration=N/A|duration_time=N/A|convergence_duration=N/A|convergence_duration_time=N/A|size=323|pos=11594|flags=K
packet|codec_type=video|stream_index=0|pts=512|pts_time=0.040000|dts=0|dts_time=0.000000|duration=512|duration_time=0.040000|convergence_duration=N/A|convergence_duration_time=N/A|size=1328|pos=11894|flags=_
packet|codec_type=audio|stream_index=1|pts=0|pts_time=0.000000|dts=0|dts_time=0.000000|duration=1024|duration_time=0.021333|convergence_duration=N/A|convergence_duration_time=N/A|size=261|pos=13222|flags=K

if you look at the audio stream 2 packets have the same pts, that
looks wrong, previously the first packet had -1024

[...]
diff mbox

Patch

From 4f91db6f34070f0e02ce224badaffa8c4d69d900 Mon Sep 17 00:00:00 2001
From: Sasi Inguva <isasi@google.com>
Date: Wed, 24 Aug 2016 18:30:01 -0700
Subject: [PATCH 2/4] avformat/avframe.h: Add a flag in AVIndexEntry to discard
 frame after decoding.

Signed-off-by: Sasi Inguva <isasi@google.com>
---
 libavformat/avformat.h | 3 +++
 libavformat/version.h  | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/libavformat/avformat.h b/libavformat/avformat.h
index 3ee7051..d943ae1 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -814,6 +814,9 @@  typedef struct AVIndexEntry {
                                * is known
                                */
 #define AVINDEX_KEYFRAME 0x0001
+#define AVINDEX_DISCARD_FRAME  0x0002    /**
+                                          * Flag is used to indicate which frame should be discarded after decoding.
+                                          */
     int flags:2;
     int size:30; //Yeah, trying to keep the size of this small to reduce memory requirements (it is 24 vs. 32 bytes due to possible 8-byte alignment).
     int min_distance;         /**< Minimum distance between this and the previous keyframe, used to avoid unneeded searching. */
diff --git a/libavformat/version.h b/libavformat/version.h
index 88fd4cc..34226ca 100644
--- a/libavformat/version.h
+++ b/libavformat/version.h
@@ -32,7 +32,7 @@ 
 // Major bumping may affect Ticket5467, 5421, 5451(compatibility with Chromium)
 // Also please add any ticket numbers that you believe might be affected here
 #define LIBAVFORMAT_VERSION_MAJOR  57
-#define LIBAVFORMAT_VERSION_MINOR  48
+#define LIBAVFORMAT_VERSION_MINOR  49
 #define LIBAVFORMAT_VERSION_MICRO 100
 
 #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
-- 
2.8.0.rc3.226.g39d4020