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

2023-08-25 Thread Ulrik Mikaelsson
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_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 v3] avformat/mp3dec: Subtract known padding from duration

2023-08-23 Thread 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_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