diff mbox

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

Message ID CAGD_KHdrJiDMtKrNZMY1WZC2bQz46w-x6N1MLqyh_hrBMSBa_g@mail.gmail.com
State Superseded
Headers show

Commit Message

Sasi Inguva Aug. 26, 2016, 7:49 p.m. UTC
I think there is some bug in mp3 decoder which is making skip
samples -1431655766 for ~/tickets/5528/fire.mp3 . For now I have removed
the assert from the 3rd commit.
For the file one.mov , I think the audio has edit list with start time
correspending to the second sample - (which should be media time 1024 if I
guess correctly). This indicates that the first sample be used for encoder
delay.
 Previously without edit  list parsing , we used to offset the start_dts by
-1024 to make the second sample timestamp start from zero.
             sc->time_offset = start_time - empty_duration;
-            current_dts = -sc->time_offset;
             if (sc->ctts_count>0 && sc->stts_count>0 &&

But now edit list parsing handles the rebasing of timestamps to zero,
because it will  assign increasing timestamps  starting from zero, to
samples present in the edit list. Because the first sample is not in the
edit list, we mark it as DISCARD, which flag av_decode_audio4 will look at
and decode-and-discard that frame. So it wouldn't matter what the first
sample timestamp should be assigned because it is anyway discarded after
decode.

I am sure when you do ./ffprobe -show_frames -print_format compact one.mov
, you won't get the first audio sample at all and second audio sample will
start from zero timestamp. If I can get a link to the file, I can get a
deeper look but I am almost sure that this is what is happening.

Attaching the modified 3rd patch again.


On Thu, Aug 25, 2016 at 3:01 PM, Michael Niedermayer <michael@niedermayer.cc
> wrote:

> 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
>
> [...]
>
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> I do not agree with what you have to say, but I'll defend to the death your
> right to say it. -- Voltaire
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>

Comments

Michael Niedermayer Aug. 27, 2016, 12:55 a.m. UTC | #1
On Fri, Aug 26, 2016 at 12:49:19PM -0700, Sasi Inguva wrote:
> I think there is some bug in mp3 decoder which is making skip
> samples -1431655766 for ~/tickets/5528/fire.mp3 . For now I have removed
> the assert from the 3rd commit.
> For the file one.mov , I think the audio has edit list with start time
> correspending to the second sample - (which should be media time 1024 if I
> guess correctly). This indicates that the first sample be used for encoder
> delay.
>  Previously without edit  list parsing , we used to offset the start_dts by
> -1024 to make the second sample timestamp start from zero.
>              sc->time_offset = start_time - empty_duration;
> -            current_dts = -sc->time_offset;
>              if (sc->ctts_count>0 && sc->stts_count>0 &&
> 
> But now edit list parsing handles the rebasing of timestamps to zero,
> because it will  assign increasing timestamps  starting from zero, to
> samples present in the edit list.

> Because the first sample is not in the
> edit list, we mark it as DISCARD, which flag av_decode_audio4 will look at
> and decode-and-discard that frame. So it wouldn't matter what the first
> sample timestamp should be assigned because it is anyway discarded after
> decode.

current applications using libavformat have no knowledge of the
discard flag you can add the DISCARD flag, you can set it on packets but
applications written or built for libavformat 57 dont have to know
this flag and can treat the packets like any other packet.

Adding this feature without a major version bump requires things to
still work reasonable with the old semantics that apps are build
around. That should be possible unless iam missing something


> 
> I am sure when you do ./ffprobe -show_frames -print_format compact one.mov
> , you won't get the first audio sample at all and second audio sample will
> start from zero timestamp. If I can get a link to the file, I can get a
> deeper look but I am almost sure that this is what is happening.

heres a direct link
http://trac.ffmpeg.org/raw-attachment/ticket/5528/fire.mp3

[...]
Sasi Inguva Aug. 27, 2016, 10:30 p.m. UTC | #2
On Fri, Aug 26, 2016 at 5:55 PM, Michael Niedermayer <michael@niedermayer.cc
> wrote:

> On Fri, Aug 26, 2016 at 12:49:19PM -0700, Sasi Inguva wrote:
> > I think there is some bug in mp3 decoder which is making skip
> > samples -1431655766 for ~/tickets/5528/fire.mp3 . For now I have removed
> > the assert from the 3rd commit.
> > For the file one.mov , I think the audio has edit list with start time
> > correspending to the second sample - (which should be media time 1024 if
> I
> > guess correctly). This indicates that the first sample be used for
> encoder
> > delay.
> >  Previously without edit  list parsing , we used to offset the start_dts
> by
> > -1024 to make the second sample timestamp start from zero.
> >              sc->time_offset = start_time - empty_duration;
> > -            current_dts = -sc->time_offset;
> >              if (sc->ctts_count>0 && sc->stts_count>0 &&
> >
> > But now edit list parsing handles the rebasing of timestamps to zero,
> > because it will  assign increasing timestamps  starting from zero, to
> > samples present in the edit list.
>
> > Because the first sample is not in the
> > edit list, we mark it as DISCARD, which flag av_decode_audio4 will look
> at
> > and decode-and-discard that frame. So it wouldn't matter what the first
> > sample timestamp should be assigned because it is anyway discarded after
> > decode.
>
> current applications using libavformat have no knowledge of the
> discard flag you can add the DISCARD flag, you can set it on packets but
> applications written or built for libavformat 57 dont have to know
> this flag and can treat the packets like any other packet.
>

Yes. they can treat the packet like any other packet. They don't have to
know about the discard flag.

Adding this feature without a major version bump requires things to
> still work reasonable with the old semantics that apps are build
> around. That should be possible unless iam missing something


As I have pointed out earlier this code will change the timestamps of the
packets. In the case of multiple edit lists, the code will also repeat some
packets, and might make the timestamps non-monotonous. I don't know if the
last behavior is not  an acceptable expectation from av_read_frame.
However as I've pointed out, we are already showing the wrong packets for
videos with multiple edit lists by not parsing the edit lists currently,
which will introduce AVSync issues. So this patch wont make things any
worse for those cases in my view.

I've tested remuxing (-c copy) the MOV files with edit lists to other
containers. I see that ffmpeg actually modifies DTS to be monotonous in
case of non monotonic DTS
http://git.videolan.org/?p=ffmpeg.git;a=blob;f=ffmpeg.c;h=bad311d57561e8ce2e790d733ae0fc81809ce585;hb=HEAD#l729
 , so the remuxing operation never fails at least.


>
> >
> > I am sure when you do ./ffprobe -show_frames -print_format compact
> one.mov
> > , you won't get the first audio sample at all and second audio sample
> will
> > start from zero timestamp. If I can get a link to the file, I can get a
> > deeper look but I am almost sure that this is what is happening.
>
> heres a direct link
> http://trac.ffmpeg.org/raw-attachment/ticket/5528/fire.mp3
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> It is dangerous to be right in matters on which the established authorities
> are wrong. -- Voltaire
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
Michael Niedermayer Aug. 28, 2016, 10:10 a.m. UTC | #3
On Sat, Aug 27, 2016 at 03:30:24PM -0700, Sasi Inguva wrote:
> On Fri, Aug 26, 2016 at 5:55 PM, Michael Niedermayer <michael@niedermayer.cc
> > wrote:
> 
> > On Fri, Aug 26, 2016 at 12:49:19PM -0700, Sasi Inguva wrote:
> > > I think there is some bug in mp3 decoder which is making skip
> > > samples -1431655766 for ~/tickets/5528/fire.mp3 . For now I have removed
> > > the assert from the 3rd commit.
> > > For the file one.mov , I think the audio has edit list with start time
> > > correspending to the second sample - (which should be media time 1024 if
> > I
> > > guess correctly). This indicates that the first sample be used for
> > encoder
> > > delay.
> > >  Previously without edit  list parsing , we used to offset the start_dts
> > by
> > > -1024 to make the second sample timestamp start from zero.
> > >              sc->time_offset = start_time - empty_duration;
> > > -            current_dts = -sc->time_offset;
> > >              if (sc->ctts_count>0 && sc->stts_count>0 &&
> > >
> > > But now edit list parsing handles the rebasing of timestamps to zero,
> > > because it will  assign increasing timestamps  starting from zero, to
> > > samples present in the edit list.
> >
> > > Because the first sample is not in the
> > > edit list, we mark it as DISCARD, which flag av_decode_audio4 will look
> > at
> > > and decode-and-discard that frame. So it wouldn't matter what the first
> > > sample timestamp should be assigned because it is anyway discarded after
> > > decode.
> >
> > current applications using libavformat have no knowledge of the
> > discard flag you can add the DISCARD flag, you can set it on packets but
> > applications written or built for libavformat 57 dont have to know
> > this flag and can treat the packets like any other packet.
> >
> 
> Yes. they can treat the packet like any other packet. They don't have to
> know about the discard flag.
> 
> Adding this feature without a major version bump requires things to
> > still work reasonable with the old semantics that apps are build
> > around. That should be possible unless iam missing something
> 
> 
> As I have pointed out earlier this code will change the timestamps of the
> packets. In the case of multiple edit lists, the code will also repeat some
> packets, and might make the timestamps non-monotonous. I don't know if the
> last behavior is not  an acceptable expectation from av_read_frame.

if timestamps repeat then it will not be possible to seek in the file
by timestamp (to all positions) and i suspect also not by byte position.
How would one seek then ?
or do i misunderstand ?


> However as I've pointed out, we are already showing the wrong packets for
> videos with multiple edit lists by not parsing the edit lists currently,
> which will introduce AVSync issues. So this patch wont make things any
> worse for those cases in my view.

Is it difficult to adjust the timestamps of discarded packets to avoid
timestamp discontinuities ? (for the cases where this is possible of
course only)
the timestamps for discarded packets i would assume are meaningless in
the new semenatics but they matter for the old semantics 
again, please correct me if iam wrong

[...]
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