Message ID | CAPUDrwd7Sht+GfshJEPpxudzH930aiw2Bfb7dbrGNjMWQcyE2g@mail.gmail.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel] Don't adjust start time for MP3 files; packets are not adjusted. | expand |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
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 [...]
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(-) diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index b044679c02..efbf836bcc 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -260,10 +260,6 @@ static void mp3_parse_info_tag(AVFormatContext *s, AVStream *st, st->first_discard_sample = -mp3->end_pad + 528 + 1 + mp3->frames * (int64_t)spf; st->last_discard_sample = mp3->frames * (int64_t)spf; } - if (!st->start_time) - st->start_time = av_rescale_q(st->start_skip_samples, - (AVRational){1, c->sample_rate}, - st->time_base); av_log(s, AV_LOG_DEBUG, "pad %d %d\n", mp3->start_pad, mp3-> end_pad); } diff --git a/tests/ref/fate/gapless-mp3 b/tests/ref/fate/gapless-mp3 index ab4f1a0456..e784391153 100644 --- a/tests/ref/fate/gapless-mp3 +++ b/tests/ref/fate/gapless-mp3 @@ -1,4 +1,4 @@ -44b42cc3a898b45507d856d0813f4f26 *tests/data/fate/gapless-mp3.out-1 +ec876434ed65e338e07234e54d136caf *tests/data/fate/gapless-mp3.out-1 c96c3ae7bd3300fd2f4debac222de5b7 ec876434ed65e338e07234e54d136caf *tests/data/fate/gapless-mp3.out-2 c96c3ae7bd3300fd2f4debac222de5b7 -- 2.26.2.303.gf8c07b1a785-goog
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>