diff mbox series

[FFmpeg-devel] avformat/mp3dec:Subtract known padding from duration

Message ID 20230822120304.396910-1-ulrik.mikaelsson@gmail.com
State New
Headers show
Series [FFmpeg-devel] avformat/mp3dec:Subtract known padding from duration | expand

Checks

Context Check Description
yinshiyou/commit_msg_loongarch64 warning The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ".
andriy/commit_msg_x86 warning The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ".
andriy/make_x86 success Make finished
andriy/make_fate_x86 fail Make fate failed

Commit Message

Ulrik Mikaelsson Aug. 22, 2023, 12:03 p.m. UTC 
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(-)

Comments

Tomas Härdin Aug. 23, 2023, 9:18 a.m. UTC | #1 
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
Ulrik Mikaelsson Aug. 23, 2023, 9:25 a.m. UTC | #2 
> > ---
> >  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 <git@haerdin.se>:
>
> 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-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

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);