[FFmpeg-devel] Don't adjust start time for MP3 files; packets are not adjusted.

This is a patch Chromium has carried for a while, we forgot to send it
upstream. 7546ac2fee4 made it so that the start_time for mp3 files is
adjusted for skip_samples. However, this appears incorrect because
subsequent packet timestamps are not adjusted and skip_samples are
applied by deleting data from a packet without changing the timestamp.

E.g., we are told the start_time is ~25ms and we get a packet with a
timestamp of 0 that has had the skip_samples discarded from it. As such
rendering engines may incorrectly discard everything prior to the
25ms thinking that is where playback should officially start. Since the
samples were deleted without adjusting timestamps though, the true
start_time is still 0.

Other formats like MP4 with edit lists will adjust both the start
time and the timestamps of subsequent packets to avoid this issue.

Signed-off-by: Dale Curtis <dalecurtis@chromium.org>

Ping for this patch. Thanks

- dale

On Thu, Apr 23, 2020 at 4:33 PM Dale Curtis <dalecurtis@chromium.org> wrote:

> This is a patch Chromium has carried for a while, we forgot to send it
> upstream. 7546ac2fee4 made it so that the start_time for mp3 files is
> adjusted for skip_samples. However, this appears incorrect because
> subsequent packet timestamps are not adjusted and skip_samples are
> applied by deleting data from a packet without changing the timestamp.
>
> E.g., we are told the start_time is ~25ms and we get a packet with a
> timestamp of 0 that has had the skip_samples discarded from it. As such
> rendering engines may incorrectly discard everything prior to the
> 25ms thinking that is where playback should officially start. Since the
> samples were deleted without adjusting timestamps though, the true
> start_time is still 0.
>
> Other formats like MP4 with edit lists will adjust both the start
> time and the timestamps of subsequent packets to avoid this issue.
>
> Signed-off-by: Dale Curtis <dalecurtis@chromium.org>
>

Any comments on this? Thanks.

- dale

On Thu, Apr 30, 2020 at 12:42 PM Dale Curtis <dalecurtis@chromium.org>
wrote:

> Ping for this patch. Thanks
>
> - dale
>
> On Thu, Apr 23, 2020 at 4:33 PM Dale Curtis <dalecurtis@chromium.org>
> wrote:
>
>> This is a patch Chromium has carried for a while, we forgot to send it
>> upstream. 7546ac2fee4 made it so that the start_time for mp3 files is
>> adjusted for skip_samples. However, this appears incorrect because
>> subsequent packet timestamps are not adjusted and skip_samples are
>> applied by deleting data from a packet without changing the timestamp.
>>
>> E.g., we are told the start_time is ~25ms and we get a packet with a
>> timestamp of 0 that has had the skip_samples discarded from it. As such
>> rendering engines may incorrectly discard everything prior to the
>> 25ms thinking that is where playback should officially start. Since the
>> samples were deleted without adjusting timestamps though, the true
>> start_time is still 0.
>>
>> Other formats like MP4 with edit lists will adjust both the start
>> time and the timestamps of subsequent packets to avoid this issue.
>>
>> Signed-off-by: Dale Curtis <dalecurtis@chromium.org>
>>
>

Bump again for this one too. Thanks. The Arch Linux distro is awaiting
resolution of this issue.

- dale

On Mon, May 4, 2020 at 11:10 AM Dale Curtis <dalecurtis@chromium.org> wrote:

> Any comments on this? Thanks.
>
> - dale
>
> On Thu, Apr 30, 2020 at 12:42 PM Dale Curtis <dalecurtis@chromium.org>
> wrote:
>
>> Ping for this patch. Thanks
>>
>> - dale
>>
>> On Thu, Apr 23, 2020 at 4:33 PM Dale Curtis <dalecurtis@chromium.org>
>> wrote:
>>
>>> This is a patch Chromium has carried for a while, we forgot to send it
>>> upstream. 7546ac2fee4 made it so that the start_time for mp3 files is
>>> adjusted for skip_samples. However, this appears incorrect because
>>> subsequent packet timestamps are not adjusted and skip_samples are
>>> applied by deleting data from a packet without changing the timestamp.
>>>
>>> E.g., we are told the start_time is ~25ms and we get a packet with a
>>> timestamp of 0 that has had the skip_samples discarded from it. As such
>>> rendering engines may incorrectly discard everything prior to the
>>> 25ms thinking that is where playback should officially start. Since the
>>> samples were deleted without adjusting timestamps though, the true
>>> start_time is still 0.
>>>
>>> Other formats like MP4 with edit lists will adjust both the start
>>> time and the timestamps of subsequent packets to avoid this issue.
>>>
>>> Signed-off-by: Dale Curtis <dalecurtis@chromium.org>
>>>
>>

Quoting Dale Curtis (2020-04-24 01:33:21)
> This is a patch Chromium has carried for a while, we forgot to send it
> upstream. 7546ac2fee4 made it so that the start_time for mp3 files is
> adjusted for skip_samples. However, this appears incorrect because
> subsequent packet timestamps are not adjusted and skip_samples are
> applied by deleting data from a packet without changing the timestamp.
> 
> E.g., we are told the start_time is ~25ms and we get a packet with a
> timestamp of 0 that has had the skip_samples discarded from it. As such
> rendering engines may incorrectly discard everything prior to the
> 25ms thinking that is where playback should officially start. Since the
> samples were deleted without adjusting timestamps though, the true
> start_time is still 0.
> 
> Other formats like MP4 with edit lists will adjust both the start
> time and the timestamps of subsequent packets to avoid this issue.
> 
> Signed-off-by: Dale Curtis <dalecurtis@chromium.org>

Looks reasonable, will push.

On Wed, May 20, 2020 at 5:22 AM Anton Khirnov <anton@khirnov.net> wrote:

>
> Looks reasonable, will push.
>

Thanks! I don't see this in the commit log. Did it end up getting pushed?

- dale

On Thu, Apr 23, 2020 at 04:33:21PM -0700, Dale Curtis wrote:
> This is a patch Chromium has carried for a while, we forgot to send it
> upstream. 7546ac2fee4 made it so that the start_time for mp3 files is
> adjusted for skip_samples. However, this appears incorrect because
> subsequent packet timestamps are not adjusted and skip_samples are
> applied by deleting data from a packet without changing the timestamp.
> 
> E.g., we are told the start_time is ~25ms and we get a packet with a
> timestamp of 0 that has had the skip_samples discarded from it. As such
> rendering engines may incorrectly discard everything prior to the
> 25ms thinking that is where playback should officially start. Since the
> samples were deleted without adjusting timestamps though, the true
> start_time is still 0.
> 
> Other formats like MP4 with edit lists will adjust both the start
> time and the timestamps of subsequent packets to avoid this issue.
> 
> Signed-off-by: Dale Curtis <dalecurtis@chromium.org>

> 173f45ece85db7ccb5f242043b1d029bfbdbd28b  no_start_time_adjust_v0.patch
> From 1f551502a2e271da044f7e4be4be7ddca7f30bc1 Mon Sep 17 00:00:00 2001
> From: Dale Curtis <dalecurtis@chromium.org>
> Date: Thu, 23 Apr 2020 16:18:18 -0700
> Subject: [PATCH] Don't adjust start time for MP3 files; packets are not
>  adjusted.
> 
> This is a patch Chromium has carried for a while, we forgot to send it
> upstream. 7546ac2fee4 made it so that the start_time for mp3 files is
> adjusted for skip_samples. However, this appears incorrect because
> subsequent packet timestamps are not adjusted and skip_samples are
> applied by deleting data from a packet without changing the timestamp.
> 
> E.g., we are told the start_time is ~25ms and we get a packet with a
> timestamp of 0 that has had the skip_samples discarded from it. As such
> rendering engines may incorrectly discard everything prior to the
> 25ms thinking that is where playback should officially start. Since the
> samples were deleted without adjusting timestamps though, the true
> start_time is still 0.
> 
> Other formats like MP4 with edit lists will adjust both the start
> time and the timestamps of subsequent packets to avoid this issue.
> 
> Signed-off-by: Dale Curtis <dalecurtis@chromium.org>
> ---
>  libavformat/mp3dec.c       | 4 ----
>  tests/ref/fate/gapless-mp3 | 2 +-
>  2 files changed, 1 insertion(+), 5 deletions(-)

this causes
./ffmpeg -i tickets//5205/example-with-error_cut.mp3 -t 0.11 -f framecrc before
to change this way:

--- before	2020-05-27 17:16:08.282813662 +0200
+++ after	2020-05-27 17:14:40.654886720 +0200
@@ -11,9 +11,8 @@
 #channel_layout 1: 3
 #channel_layout_name 1: stereo
 0,          0,          0,        0,  3000000, 0x68775127
-1,          0,          0,       47,      188, 0x05b53e6d
-1,         47,         47,     1152,     4608, 0xaf804111
-1,       1199,       1199,     1152,     4608, 0xded29ae0
-1,       2351,       2351,     1152,     4608, 0x51b68940
-1,       3503,       3503,     1152,     4608, 0xa63e5ef7
-1,       4655,       4655,      196,      784, 0x9fd3a615
+1,       1105,       1105,       47,      188, 0x05b53e6d
+1,       1152,       1152,     1152,     4608, 0xaf804111
+1,       2304,       2304,     1152,     4608, 0xded29ae0
+1,       3456,       3456,     1152,     4608, 0x51b68940
+1,       4608,       4608,     1152,     4608, 0xa63e5ef7

what id like to point out here is that the audio stream no
longer starts at the same time as the video
also the duration from adding the durations before is
exact but not afterwards

i sadly didnt had the time to look into this before
it was applied. So ATM iam just reporting this change, i didnt look
at what exactly is going on

I assume the file is here:
https://trac.ffmpeg.org/raw-attachment/ticket/5205/example-with-error_cut.mp3

also i seem to have more files that appear to behave the same

thx

[...]

On Wed, May 27, 2020 at 8:29 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> what id like to point out here is that the audio stream no
> longer starts at the same time as the video
> also the duration from adding the durations before is
> exact but not afterwards
>

I'm not sure about the duration issue, I'd assume it's just an accounting
error since the change only affects start_time. As noted in the change
description, if you want to keep start_time adjusted for skip samples all
packet timestamps need to be changed too. Doing that broke many test cases
at the time, so we opted to just revert the patch. I.e., basically every
mp3 changes to having its first PTS be ~26ms due to the common 1152 skip
sample amount.

- dale

On Wed, May 27, 2020 at 11:57:13AM -0700, Dale Curtis wrote:
> On Wed, May 27, 2020 at 8:29 AM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > what id like to point out here is that the audio stream no
> > longer starts at the same time as the video
> > also the duration from adding the durations before is
> > exact but not afterwards
> >
> 
> I'm not sure about the duration issue, I'd assume it's just an accounting
> error since the change only affects start_time. As noted in the change
> description, if you want to keep start_time adjusted for skip samples all
> packet timestamps need to be changed too. Doing that broke many test cases
> at the time, so we opted to just revert the patch. I.e., basically every
> mp3 changes to having its first PTS be ~26ms due to the common 1152 skip
> sample amount.

I dont really have an oppinion about start_time, its more the change in
timestamps & duratiion on the output side which is whats user vissible
and that looked worse for the testcase

thx

[...]

On Thu, May 28, 2020 at 1:33 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> I dont really have an oppinion about start_time, its more the change in
> timestamps & duratiion on the output side which is whats user vissible
> and that looked worse for the testcase
>

The difference is due to the decode stage applying the skip samples to pts:
https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/decode.c#L421

That makes my statement in the commit "skip_samples are applied by deleting
data from a packet without changing the timestamp" incorrect -  sorry for
getting that wrong. Since we use decoders other than ffmpeg, Chromium's
implementation deletes skip samples indicated by the side data without
changing the packet timestamps so audio and video line up properly. ffmpeg
seems to do the same thing but in a more roundabout fashion that I don't
fully understand.

Prior to my patch, avformat_find_stream_info() indicated a non-zero start
time, av_read_frame() returned packets from time zero, but with skip
samples marked as side data. Later avcodec_receive_frame() adjusts the pts,
dts, and duration. Finally some other piece of code is adjusting the pts by
the start time before it reaches the framecrc muxer. I haven't been able to
figure out where that happens though.

I'm not sure what the right fix is here. As an API user, we've always
expected start_time to refer to the pts of the first packet returned by
av_read_frame(). Indeed avformat.h says "pts of the first frame of the
stream in presentation order". If that's not the case, then it'd be helpful
to have some guidance on how seeking works (e.g., even ffmpeg.c seeks to
start_time, but that skips the preroll if it's non-zero) and what should be
done with frames before the start time.

I'm less sure about this last point, but adjusting the pts during the
decode stage seems incompatible with how edit lists are applied for mp4
during the demuxing stage. E.g., IIUC, if an edit list slices out the time
range [0,6] across two keyframe packets p0={pts=0, dur=5} and p1={pts=5,
dur=5} the mov demuxer would discard p0 and p1 would be become {pts=7,
dur=5, skip_samples=2}.

In any case, it seems incorrect that ffmpeg no longer has a timestamp of
zero for the first decoded mp3 frame when skip samples are present. So at
the very least that does seem to need a fix. Either by reverting this patch
or another mechanism.

- dale

On Fri, May 29, 2020 at 03:36:47PM -0700, Dale Curtis wrote:
> On Thu, May 28, 2020 at 1:33 PM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > I dont really have an oppinion about start_time, its more the change in
> > timestamps & duratiion on the output side which is whats user vissible
> > and that looked worse for the testcase
> >
> 
> The difference is due to the decode stage applying the skip samples to pts:
> https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/decode.c#L421
> 
> That makes my statement in the commit "skip_samples are applied by deleting
> data from a packet without changing the timestamp" incorrect -  sorry for
> getting that wrong. Since we use decoders other than ffmpeg, Chromium's
> implementation deletes skip samples indicated by the side data without
> changing the packet timestamps so audio and video line up properly. ffmpeg
> seems to do the same thing but in a more roundabout fashion that I don't
> fully understand.
> 
> Prior to my patch, avformat_find_stream_info() indicated a non-zero start
> time, av_read_frame() returned packets from time zero, but with skip
> samples marked as side data. Later avcodec_receive_frame() adjusts the pts,
> dts, and duration. Finally some other piece of code is adjusting the pts by
> the start time before it reaches the framecrc muxer. I haven't been able to
> figure out where that happens though.
> 
> I'm not sure what the right fix is here. As an API user, we've always
> expected start_time to refer to the pts of the first packet returned by
> av_read_frame(). Indeed avformat.h says "pts of the first frame of the
> stream in presentation order". If that's not the case, then it'd be helpful
> to have some guidance on how seeking works (e.g., even ffmpeg.c seeks to
> start_time, but that skips the preroll if it's non-zero) and what should be
> done with frames before the start time.
> 
> I'm less sure about this last point, but adjusting the pts during the
> decode stage seems incompatible with how edit lists are applied for mp4
> during the demuxing stage. E.g., IIUC, if an edit list slices out the time
> range [0,6] across two keyframe packets p0={pts=0, dur=5} and p1={pts=5,
> dur=5} the mov demuxer would discard p0 and p1 would be become {pts=7,
> dur=5, skip_samples=2}.
> 

> In any case, it seems incorrect that ffmpeg no longer has a timestamp of
> zero for the first decoded mp3 frame when skip samples are present. So at
> the very least that does seem to need a fix. Either by reverting this patch
> or another mechanism.

hmm, ill revert for 4.3, so we can look into this with more time available
and avoid creating a release that has a new but also somewhat wrong behavior

thx

[...]