Re: [FFmpeg-devel] [PATCH v3] avformat/mp3dec: Subtract known padding from duration
Just noticed that there is a ticket https://trac.ffmpeg.org/ticket/10163 about this, that I believe is solved with this patch. Den ons 23 aug. 2023 kl 20:33 skrev Ulrik Mikaelsson : > > When an Info-tag is present, marking initial and trailing samples as > padding, those samples should not be included in the calculation of track > duration. > > This solves a surprising user experience where converting a WAV->MP3->WAV, > ffprobe will show the duration of the mp3 as slightly longer than both the > input and the output. > > As a result, the estimated duration and imprecise seek-results of some > FATE-tests have been updated. > --- > libavformat/mp3dec.c | 19 +-- > tests/ref/fate/gapless-mp3-side-data | 4 ++-- > tests/ref/seek/extra-mp3 | 8 > 3 files changed, 19 insertions(+), 12 deletions(-) > > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c > index 05c13228bc..24ec4dae06 100644 > --- a/libavformat/mp3dec.c > +++ b/libavformat/mp3dec.c > @@ -51,6 +51,7 @@ typedef struct { > int usetoc; > unsigned frames; /* Total number of frames in file */ > unsigned header_filesize; /* Total number of bytes in the stream */ > +unsigned frame_duration; /* Frame duration in st->time_base */ > int is_cbr; > } MP3DecContext; > > @@ -339,6 +340,7 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, > AVStream *st, int64_t base) > > mp3->frames = 0; > mp3->header_filesize = 0; > +mp3->frame_duration = av_rescale_q(spf, (AVRational){1, c.sample_rate}, > st->time_base); > > mp3_parse_info_tag(s, st, , spf); > mp3_parse_vbri_tag(s, st, base); > @@ -349,11 +351,17 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, > AVStream *st, int64_t base) > /* Skip the vbr tag frame */ > avio_seek(s->pb, base + vbrtag_size, SEEK_SET); > > -if (mp3->frames) > -st->duration = av_rescale_q(mp3->frames, (AVRational){spf, > c.sample_rate}, > +if (mp3->frames) { > +int64_t full_duration; > + > +full_duration = mp3->frames * (int64_t)spf; > +st->duration = av_rescale_q(full_duration - mp3->start_pad - > mp3->end_pad, > +(AVRational){1, c.sample_rate}, > st->time_base); > -if (mp3->header_filesize && mp3->frames && !mp3->is_cbr) > -st->codecpar->bit_rate = av_rescale(mp3->header_filesize, 8 * > c.sample_rate, mp3->frames * (int64_t)spf); > + > +if (mp3->header_filesize && !mp3->is_cbr) > +st->codecpar->bit_rate = av_rescale(mp3->header_filesize, 8 * > c.sample_rate, full_duration); > +} > > return 0; > } > @@ -589,8 +597,7 @@ static int mp3_seek(AVFormatContext *s, int stream_index, > int64_t timestamp, > return best_pos; > > if (mp3->is_cbr && ie == && mp3->frames) { > -int frame_duration = av_rescale(st->duration, 1, mp3->frames); > -ie1.timestamp = frame_duration * av_rescale(best_pos - > si->data_offset, mp3->frames, mp3->header_filesize); > +ie1.timestamp = mp3->frame_duration * av_rescale(best_pos - > si->data_offset, mp3->frames, mp3->header_filesize); > } > > avpriv_update_cur_dts(s, st, ie->timestamp); > diff --git a/tests/ref/fate/gapless-mp3-side-data > b/tests/ref/fate/gapless-mp3-side-data > index caf42068dc..495a5bb865 100644 > --- a/tests/ref/fate/gapless-mp3-side-data > +++ b/tests/ref/fate/gapless-mp3-side-data > @@ -596,5 +596,5 @@ > packet|codec_type=audio|stream_index=0|pts=218603520|pts_time=15.490612|dts=2186 > > > packet|codec_type=audio|stream_index=0|pts=218972160|pts_time=15.516735|dts=218972160|dts_time=15.516735|duration=368640|duration_time=0.026122|size=418|pos=249718|flags=K__|data_hash=CRC32:3789f3cf|side_data|side_data_type=Skip > Samples|skip_samples=0|discard_padding=1152|skip_reason=0|discard_reason=0 > > -stream|index=0|codec_name=mp3|profile=unknown|codec_type=audio|codec_tag_string=[0][0][0][0]|codec_tag=0x|sample_fmt=fltp|sample_rate=44100|channels=2|channel_layout=stereo|bits_per_sample=0|initial_padding=0|id=N/A|r_frame_rate=0/0|avg_frame_rate=0/0|time_base=1/14112000|start_pts=353600|start_time=0.025057|duration_ts=219340800|duration=15.542857|bit_rate=128000|max_bit_rate=N/A|bits_per_raw_sample=N/A|nb_frames=N/A|nb_read_frames=N/A|nb_read_packets=595|disposition:default=0|disposition:dub=0|disposition:original=0|disposition:comment=0|disposition:lyrics=0|disposition:karaoke=0|disposition:forced=0|disposition:hearing_im
Re: [FFmpeg-devel] [PATCH v2] avformat/mp3dec: Subtract known padding from duration
> This still fails fate-seek-extra-mp3 I noticed. Apologies for the noise, I'm unfamiliar with the FATE suite and failed to realize "make fate" won't run the _whole_ thing. I have just submitted a 3d revision of this patch, fixing the review comments and fully passing `make fate SAMPLES=./fate-sui te`. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH v3] avformat/mp3dec: Subtract known padding from duration
When an Info-tag is present, marking initial and trailing samples as padding, those samples should not be included in the calculation of track duration. This solves a surprising user experience where converting a WAV->MP3->WAV, ffprobe will show the duration of the mp3 as slightly longer than both the input and the output. As a result, the estimated duration and imprecise seek-results of some FATE-tests have been updated. --- libavformat/mp3dec.c | 19 +-- tests/ref/fate/gapless-mp3-side-data | 4 ++-- tests/ref/seek/extra-mp3 | 8 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index 05c13228bc..24ec4dae06 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -51,6 +51,7 @@ typedef struct { int usetoc; unsigned frames; /* Total number of frames in file */ unsigned header_filesize; /* Total number of bytes in the stream */ +unsigned frame_duration; /* Frame duration in st->time_base */ int is_cbr; } MP3DecContext; @@ -339,6 +340,7 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base) mp3->frames = 0; mp3->header_filesize = 0; +mp3->frame_duration = av_rescale_q(spf, (AVRational){1, c.sample_rate}, st->time_base); mp3_parse_info_tag(s, st, , spf); mp3_parse_vbri_tag(s, st, base); @@ -349,11 +351,17 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base) /* Skip the vbr tag frame */ avio_seek(s->pb, base + vbrtag_size, SEEK_SET); -if (mp3->frames) -st->duration = av_rescale_q(mp3->frames, (AVRational){spf, c.sample_rate}, +if (mp3->frames) { +int64_t full_duration; + +full_duration = mp3->frames * (int64_t)spf; +st->duration = av_rescale_q(full_duration - mp3->start_pad - mp3->end_pad, +(AVRational){1, c.sample_rate}, st->time_base); -if (mp3->header_filesize && mp3->frames && !mp3->is_cbr) -st->codecpar->bit_rate = av_rescale(mp3->header_filesize, 8 * c.sample_rate, mp3->frames * (int64_t)spf); + +if (mp3->header_filesize && !mp3->is_cbr) +st->codecpar->bit_rate = av_rescale(mp3->header_filesize, 8 * c.sample_rate, full_duration); +} return 0; } @@ -589,8 +597,7 @@ static int mp3_seek(AVFormatContext *s, int stream_index, int64_t timestamp, return best_pos; if (mp3->is_cbr && ie == && mp3->frames) { -int frame_duration = av_rescale(st->duration, 1, mp3->frames); -ie1.timestamp = frame_duration * av_rescale(best_pos - si->data_offset, mp3->frames, mp3->header_filesize); +ie1.timestamp = mp3->frame_duration * av_rescale(best_pos - si->data_offset, mp3->frames, mp3->header_filesize); } avpriv_update_cur_dts(s, st, ie->timestamp); diff --git a/tests/ref/fate/gapless-mp3-side-data b/tests/ref/fate/gapless-mp3-side-data index caf42068dc..495a5bb865 100644 --- a/tests/ref/fate/gapless-mp3-side-data +++ b/tests/ref/fate/gapless-mp3-side-data @@ -596,5 +596,5 @@ packet|codec_type=audio|stream_index=0|pts=218603520|pts_time=15.490612|dts=2186 packet|codec_type=audio|stream_index=0|pts=218972160|pts_time=15.516735|dts=218972160|dts_time=15.516735|duration=368640|duration_time=0.026122|size=418|pos=249718|flags=K__|data_hash=CRC32:3789f3cf|side_data|side_data_type=Skip Samples|skip_samples=0|discard_padding=1152|skip_reason=0|discard_reason=0 -stream|index=0|codec_name=mp3|profile=unknown|codec_type=audio|codec_tag_string=[0][0][0][0]|codec_tag=0x|sample_fmt=fltp|sample_rate=44100|channels=2|channel_layout=stereo|bits_per_sample=0|initial_padding=0|id=N/A|r_frame_rate=0/0|avg_frame_rate=0/0|time_base=1/14112000|start_pts=353600|start_time=0.025057|duration_ts=219340800|duration=15.542857|bit_rate=128000|max_bit_rate=N/A|bits_per_raw_sample=N/A|nb_frames=N/A|nb_read_frames=N/A|nb_read_packets=595|disposition:default=0|disposition:dub=0|disposition:original=0|disposition:comment=0|disposition:lyrics=0|disposition:karaoke=0|disposition:forced=0|disposition:hearing_impaired=0|disposition:visual_impaired=0|disposition:clean_effects=0|disposition:attached_pic=0|disposition:timed_thumbnails=0|disposition:captions=0|disposition:descriptions=0|disposition:metadata=0|disposition:dependent=0|disposition:still_image=0|tag:encoder=LAME3.93 -format|filename=gapless.mp3|nb_streams=1|nb_programs=0|format_name=mp3|start_time=0.025057|duration=15.542857|size=250264|bit_rate=128812|probe_score=51|tag:title=test
[FFmpeg-devel] [PATCH v2] avformat/mp3dec: Subtract known padding from duration
When an Info-tag is present, marking initial and trailing samples as padding, those samples should not be included in the calculation of track duration. This solves a surprising user experience where converting a WAV->MP3->WAV, ffprobe will show the duration of the mp3 as slightly longer than both the input and the output. --- libavformat/mp3dec.c | 3 ++- tests/ref/fate/gapless-mp3-side-data | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index 05c13228bc..54947f521e 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -350,7 +350,8 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base) avio_seek(s->pb, base + vbrtag_size, SEEK_SET); if (mp3->frames) -st->duration = av_rescale_q(mp3->frames, (AVRational){spf, c.sample_rate}, +st->duration = av_rescale_q(((uint64_t)mp3->frames * (uint64_t)spf) - mp3->start_pad - mp3->end_pad, +(AVRational){1, c.sample_rate}, st->time_base); if (mp3->header_filesize && mp3->frames && !mp3->is_cbr) st->codecpar->bit_rate = av_rescale(mp3->header_filesize, 8 * c.sample_rate, mp3->frames * (int64_t)spf); diff --git a/tests/ref/fate/gapless-mp3-side-data b/tests/ref/fate/gapless-mp3-side-data index caf42068dc..495a5bb865 100644 --- a/tests/ref/fate/gapless-mp3-side-data +++ b/tests/ref/fate/gapless-mp3-side-data @@ -596,5 +596,5 @@ packet|codec_type=audio|stream_index=0|pts=218603520|pts_time=15.490612|dts=2186 packet|codec_type=audio|stream_index=0|pts=218972160|pts_time=15.516735|dts=218972160|dts_time=15.516735|duration=368640|duration_time=0.026122|size=418|pos=249718|flags=K__|data_hash=CRC32:3789f3cf|side_data|side_data_type=Skip Samples|skip_samples=0|discard_padding=1152|skip_reason=0|discard_reason=0 -stream|index=0|codec_name=mp3|profile=unknown|codec_type=audio|codec_tag_string=[0][0][0][0]|codec_tag=0x|sample_fmt=fltp|sample_rate=44100|channels=2|channel_layout=stereo|bits_per_sample=0|initial_padding=0|id=N/A|r_frame_rate=0/0|avg_frame_rate=0/0|time_base=1/14112000|start_pts=353600|start_time=0.025057|duration_ts=219340800|duration=15.542857|bit_rate=128000|max_bit_rate=N/A|bits_per_raw_sample=N/A|nb_frames=N/A|nb_read_frames=N/A|nb_read_packets=595|disposition:default=0|disposition:dub=0|disposition:original=0|disposition:comment=0|disposition:lyrics=0|disposition:karaoke=0|disposition:forced=0|disposition:hearing_impaired=0|disposition:visual_impaired=0|disposition:clean_effects=0|disposition:attached_pic=0|disposition:timed_thumbnails=0|disposition:captions=0|disposition:descriptions=0|disposition:metadata=0|disposition:dependent=0|disposition:still_image=0|tag:encoder=LAME3.93 -format|filename=gapless.mp3|nb_streams=1|nb_programs=0|format_name=mp3|start_time=0.025057|duration=15.542857|size=250264|bit_rate=128812|probe_score=51|tag:title=test +stream|index=0|codec_name=mp3|profile=unknown|codec_type=audio|codec_tag_string=[0][0][0][0]|codec_tag=0x|sample_fmt=fltp|sample_rate=44100|channels=2|channel_layout=stereo|bits_per_sample=0|initial_padding=0|id=N/A|r_frame_rate=0/0|avg_frame_rate=0/0|time_base=1/14112000|start_pts=353600|start_time=0.025057|duration_ts=218521600|duration=15.484807|bit_rate=128000|max_bit_rate=N/A|bits_per_raw_sample=N/A|nb_frames=N/A|nb_read_frames=N/A|nb_read_packets=595|disposition:default=0|disposition:dub=0|disposition:original=0|disposition:comment=0|disposition:lyrics=0|disposition:karaoke=0|disposition:forced=0|disposition:hearing_impaired=0|disposition:visual_impaired=0|disposition:clean_effects=0|disposition:attached_pic=0|disposition:timed_thumbnails=0|disposition:captions=0|disposition:descriptions=0|disposition:metadata=0|disposition:dependent=0|disposition:still_image=0|tag:encoder=LAME3.93 +format|filename=gapless.mp3|nb_streams=1|nb_programs=0|format_name=mp3|start_time=0.025057|duration=15.484807|size=250264|bit_rate=129295|probe_score=51|tag:title=test -- 2.39.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avformat/mp3dec:Subtract known padding from duration
> > --- > > libavformat/mp3dec.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c > > index 05c13228bc..c8c3b298ab 100644 > > --- a/libavformat/mp3dec.c > > +++ b/libavformat/mp3dec.c > > @@ -350,7 +350,8 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, > > AVStream *st, int64_t base) > > avio_seek(s->pb, base + vbrtag_size, SEEK_SET); > > > > if (mp3->frames) > > -st->duration = av_rescale_q(mp3->frames, (AVRational){spf, > > c.sample_rate}, > > +st->duration = av_rescale_q((mp3->frames * spf) - mp3- > > >start_pad - mp3->end_pad, > > This can overflow. Casting to int64_t before multiplying should be > enough to fix it. Good catch! I also noticed a fate-test failed (due to it including padding-samples in the reference output for duration), and a formatting error in the patch itself. Will fix and resubmit. Den ons 23 aug. 2023 kl 11:18 skrev Tomas Härdin : > > tis 2023-08-22 klockan 14:03 +0200 skrev Ulrik Mikaelsson: > > When an Info-tag is present, marking initial and trailing samples as > > padding, those samples should not be included in the calculation of > > track > > duration. > > > > This solves a surprising user experience where converting a WAV->MP3- > > >WAV, > > ffprobe will show the duration of the mp3 as slightly longer than > > both the > > input and the output. > > I've had similar issues with burning gapless CDs. Hopefully this fixes > some downstream projects. > > > --- > > libavformat/mp3dec.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c > > index 05c13228bc..c8c3b298ab 100644 > > --- a/libavformat/mp3dec.c > > +++ b/libavformat/mp3dec.c > > @@ -350,7 +350,8 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, > > AVStream *st, int64_t base) > > avio_seek(s->pb, base + vbrtag_size, SEEK_SET); > > > > if (mp3->frames) > > -st->duration = av_rescale_q(mp3->frames, (AVRational){spf, > > c.sample_rate}, > > +st->duration = av_rescale_q((mp3->frames * spf) - mp3- > > >start_pad - mp3->end_pad, > > This can overflow. Casting to int64_t before multiplying should be > enough to fix it. > > /Tomas > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH] avformat/mp3dec:Subtract known padding from duration
When an Info-tag is present, marking initial and trailing samples as padding, those samples should not be included in the calculation of track duration. This solves a surprising user experience where converting a WAV->MP3->WAV, ffprobe will show the duration of the mp3 as slightly longer than both the input and the output. --- libavformat/mp3dec.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index 05c13228bc..c8c3b298ab 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -350,7 +350,8 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base) avio_seek(s->pb, base + vbrtag_size, SEEK_SET); if (mp3->frames) -st->duration = av_rescale_q(mp3->frames, (AVRational){spf, c.sample_rate}, +st->duration = av_rescale_q((mp3->frames * spf) - mp3->start_pad - mp3->end_pad, +(AVRational){1, c.sample_rate}, st->time_base); if (mp3->header_filesize && mp3->frames && !mp3->is_cbr) st->codecpar->bit_rate = av_rescale(mp3->header_filesize, 8 * c.sample_rate, mp3->frames * (int64_t)spf); -- 2.39.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".