[FFmpeg-devel,3/3] lavf/mp3dec: read encoder delay/padding from Info tag

Submitted by Jon Toohill on Oct. 4, 2016, 12:45 a.m.

Details

Message ID 1475541908-21260-4-git-send-email-jtoohill@google.com
State New
Headers show

Commit Message

Jon Toohill Oct. 4, 2016, 12:45 a.m.
Muxers can check AVCodecParameters.initial_padding for the
encoder+decoder delay, and read the AV_PKT_DATA_SKIP_SAMPLES
side data from the last packet for the encoder padding.

This change also fixes the first_discard_sample calculation
which erroneously included the decoder delay. Decoder delay
is already accounted for in st->skip_samples. The affected
FATE tests have been updated accordingly.
---
 libavformat/mp3dec.c                 |  3 ++-
 tests/ref/fate/audiomatch-square-mp3 |  2 +-
 tests/ref/fate/gapless-mp3           | 10 +++++-----
 3 files changed, 8 insertions(+), 7 deletions(-)

Comments

wm4 Oct. 4, 2016, 2:19 p.m.
On Mon,  3 Oct 2016 17:45:08 -0700
Jon Toohill <jtoohill-at-google.com@ffmpeg.org> wrote:

> Muxers can check AVCodecParameters.initial_padding for the
> encoder+decoder delay, and read the AV_PKT_DATA_SKIP_SAMPLES
> side data from the last packet for the encoder padding.
> 
> This change also fixes the first_discard_sample calculation
> which erroneously included the decoder delay. Decoder delay
> is already accounted for in st->skip_samples. The affected
> FATE tests have been updated accordingly.
> ---
>  libavformat/mp3dec.c                 |  3 ++-
>  tests/ref/fate/audiomatch-square-mp3 |  2 +-
>  tests/ref/fate/gapless-mp3           | 10 +++++-----
>  3 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> index 56c7f8c..e8b2428 100644
> --- a/libavformat/mp3dec.c
> +++ b/libavformat/mp3dec.c
> @@ -239,9 +239,10 @@ static void mp3_parse_info_tag(AVFormatContext *s, AVStream *st,
>  
>          mp3->start_pad = v>>12;
>          mp3->  end_pad = v&4095;
> +        st->codecpar->initial_padding = mp3->start_pad + 528 + 1;
>          st->start_skip_samples = mp3->start_pad + 528 + 1;
>          if (mp3->frames) {
> -            st->first_discard_sample = -mp3->end_pad + 528 + 1 + mp3->frames * (int64_t)spf;
> +            st->first_discard_sample = -mp3->end_pad + mp3->frames * (int64_t)spf;

How does mixing these even make sense?

>              st->last_discard_sample = mp3->frames * (int64_t)spf;
>          }
>          if (!st->start_time)
> diff --git a/tests/ref/fate/audiomatch-square-mp3 b/tests/ref/fate/audiomatch-square-mp3
> index 8de55c2..05176a0 100644
> --- a/tests/ref/fate/audiomatch-square-mp3
> +++ b/tests/ref/fate/audiomatch-square-mp3
> @@ -1 +1 @@
> -presig: 0 postsig:0 c: 0.9447 lenerr:0
> +presig: 0 postsig:-529 c: 0.9334 lenerr:-529
> diff --git a/tests/ref/fate/gapless-mp3 b/tests/ref/fate/gapless-mp3
> index ebe7bfa..8b80bfc 100644
> --- a/tests/ref/fate/gapless-mp3
> +++ b/tests/ref/fate/gapless-mp3
> @@ -1,5 +1,5 @@
> -37534a3bcc3ef306e8c5ebfcfedfc41c *tests/data/fate/gapless-mp3.out-1
> -c96c3ae7bd3300fd2f4debac222de5b7
> -0cd1cdbcfd5cdbf6270cd98219bf31cd *tests/data/fate/gapless-mp3.out-2
> -c96c3ae7bd3300fd2f4debac222de5b7
> -9d3d8ba8a61b534f2d02ee648d6a8229 *tests/data/fate/gapless-mp3.out-3
> +81695be427d45e8be4d527a6b2af2a85 *tests/data/fate/gapless-mp3.out-1
> +c7879a827ab017364774069268d9a267
> +62d074296f8c84a5f86a6afdd7bab459 *tests/data/fate/gapless-mp3.out-2
> +c7879a827ab017364774069268d9a267
> +e931f3fe1ba25e0d5eece4977c4061a9 *tests/data/fate/gapless-mp3.out-3
Hendrik Leppkes Oct. 4, 2016, 3:52 p.m.
On Tue, Oct 4, 2016 at 2:45 AM, Jon Toohill
<jtoohill-at-google.com@ffmpeg.org> wrote:
> Muxers can check AVCodecParameters.initial_padding for the
> encoder+decoder delay, and read the AV_PKT_DATA_SKIP_SAMPLES
> side data from the last packet for the encoder padding.
>
> This change also fixes the first_discard_sample calculation
> which erroneously included the decoder delay. Decoder delay
> is already accounted for in st->skip_samples. The affected
> FATE tests have been updated accordingly.
> ---
>  libavformat/mp3dec.c                 |  3 ++-
>  tests/ref/fate/audiomatch-square-mp3 |  2 +-
>  tests/ref/fate/gapless-mp3           | 10 +++++-----
>  3 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> index 56c7f8c..e8b2428 100644
> --- a/libavformat/mp3dec.c
> +++ b/libavformat/mp3dec.c
> @@ -239,9 +239,10 @@ static void mp3_parse_info_tag(AVFormatContext *s, AVStream *st,
>
>          mp3->start_pad = v>>12;
>          mp3->  end_pad = v&4095;
> +        st->codecpar->initial_padding = mp3->start_pad + 528 + 1;
>          st->start_skip_samples = mp3->start_pad + 528 + 1;
>          if (mp3->frames) {
> -            st->first_discard_sample = -mp3->end_pad + 528 + 1 + mp3->frames * (int64_t)spf;
> +            st->first_discard_sample = -mp3->end_pad + mp3->frames * (int64_t)spf;
>              st->last_discard_sample = mp3->frames * (int64_t)spf;
>          }
>          if (!st->start_time)
> diff --git a/tests/ref/fate/audiomatch-square-mp3 b/tests/ref/fate/audiomatch-square-mp3
> index 8de55c2..05176a0 100644
> --- a/tests/ref/fate/audiomatch-square-mp3
> +++ b/tests/ref/fate/audiomatch-square-mp3
> @@ -1 +1 @@
> -presig: 0 postsig:0 c: 0.9447 lenerr:0
> +presig: 0 postsig:-529 c: 0.9334 lenerr:-529
> diff --git a/tests/ref/fate/gapless-mp3 b/tests/ref/fate/gapless-mp3
> index ebe7bfa..8b80bfc 100644
> --- a/tests/ref/fate/gapless-mp3
> +++ b/tests/ref/fate/gapless-mp3
> @@ -1,5 +1,5 @@
> -37534a3bcc3ef306e8c5ebfcfedfc41c *tests/data/fate/gapless-mp3.out-1
> -c96c3ae7bd3300fd2f4debac222de5b7
> -0cd1cdbcfd5cdbf6270cd98219bf31cd *tests/data/fate/gapless-mp3.out-2
> -c96c3ae7bd3300fd2f4debac222de5b7
> -9d3d8ba8a61b534f2d02ee648d6a8229 *tests/data/fate/gapless-mp3.out-3
> +81695be427d45e8be4d527a6b2af2a85 *tests/data/fate/gapless-mp3.out-1
> +c7879a827ab017364774069268d9a267
> +62d074296f8c84a5f86a6afdd7bab459 *tests/data/fate/gapless-mp3.out-2
> +c7879a827ab017364774069268d9a267
> +e931f3fe1ba25e0d5eece4977c4061a9 *tests/data/fate/gapless-mp3.out-3
> --

Presumably when these tests were setup, someone verified that their
output was sane and proper, and gapless.
So when these are changed, I have to ask - what exactly changes in
this output? The hashes unfortunately don't tell us that.

- Hendrik
Michael Niedermayer Oct. 4, 2016, 4:10 p.m.
On Mon, Oct 03, 2016 at 05:45:08PM -0700, Jon Toohill wrote:
> Muxers can check AVCodecParameters.initial_padding for the
> encoder+decoder delay, and read the AV_PKT_DATA_SKIP_SAMPLES
> side data from the last packet for the encoder padding.
> 
> This change also fixes the first_discard_sample calculation
> which erroneously included the decoder delay. Decoder delay
> is already accounted for in st->skip_samples. The affected
> FATE tests have been updated accordingly.
> ---
>  libavformat/mp3dec.c                 |  3 ++-
>  tests/ref/fate/audiomatch-square-mp3 |  2 +-
>  tests/ref/fate/gapless-mp3           | 10 +++++-----
>  3 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> index 56c7f8c..e8b2428 100644
> --- a/libavformat/mp3dec.c
> +++ b/libavformat/mp3dec.c
> @@ -239,9 +239,10 @@ static void mp3_parse_info_tag(AVFormatContext *s, AVStream *st,
>  
>          mp3->start_pad = v>>12;
>          mp3->  end_pad = v&4095;
> +        st->codecpar->initial_padding = mp3->start_pad + 528 + 1;
>          st->start_skip_samples = mp3->start_pad + 528 + 1;
>          if (mp3->frames) {
> -            st->first_discard_sample = -mp3->end_pad + 528 + 1 + mp3->frames * (int64_t)spf;
> +            st->first_discard_sample = -mp3->end_pad + mp3->frames * (int64_t)spf;
>              st->last_discard_sample = mp3->frames * (int64_t)spf;
>          }
>          if (!st->start_time)
> diff --git a/tests/ref/fate/audiomatch-square-mp3 b/tests/ref/fate/audiomatch-square-mp3
> index 8de55c2..05176a0 100644
> --- a/tests/ref/fate/audiomatch-square-mp3
> +++ b/tests/ref/fate/audiomatch-square-mp3
> @@ -1 +1 @@
> -presig: 0 postsig:0 c: 0.9447 lenerr:0
> +presig: 0 postsig:-529 c: 0.9334 lenerr:-529

isnt this exactly correct before and wrong after this change ?

zero signal before and zero signal after the original is what one would
expect and equal length and high correlation

every number that changes gets worse

[...]
Jon Toohill Oct. 5, 2016, 5:38 p.m.
On Tue, Oct 4, 2016 at 9:10 AM, Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Mon, Oct 03, 2016 at 05:45:08PM -0700, Jon Toohill wrote:
> > Muxers can check AVCodecParameters.initial_padding for the
> > encoder+decoder delay, and read the AV_PKT_DATA_SKIP_SAMPLES
> > side data from the last packet for the encoder padding.
> >
> > This change also fixes the first_discard_sample calculation
> > which erroneously included the decoder delay. Decoder delay
> > is already accounted for in st->skip_samples. The affected
> > FATE tests have been updated accordingly.
> > ---
> >  libavformat/mp3dec.c                 |  3 ++-
> >  tests/ref/fate/audiomatch-square-mp3 |  2 +-
> >  tests/ref/fate/gapless-mp3           | 10 +++++-----
> >  3 files changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> > index 56c7f8c..e8b2428 100644
> > --- a/libavformat/mp3dec.c
> > +++ b/libavformat/mp3dec.c
> > @@ -239,9 +239,10 @@ static void mp3_parse_info_tag(AVFormatContext *s,
> AVStream *st,
> >
> >          mp3->start_pad = v>>12;
> >          mp3->  end_pad = v&4095;
> > +        st->codecpar->initial_padding = mp3->start_pad + 528 + 1;
> >          st->start_skip_samples = mp3->start_pad + 528 + 1;
> >          if (mp3->frames) {
> > -            st->first_discard_sample = -mp3->end_pad + 528 + 1 +
> mp3->frames * (int64_t)spf;
> > +            st->first_discard_sample = -mp3->end_pad + mp3->frames *
> (int64_t)spf;
> >              st->last_discard_sample = mp3->frames * (int64_t)spf;
> >          }
> >          if (!st->start_time)
> > diff --git a/tests/ref/fate/audiomatch-square-mp3
> b/tests/ref/fate/audiomatch-square-mp3
> > index 8de55c2..05176a0 100644
> > --- a/tests/ref/fate/audiomatch-square-mp3
> > +++ b/tests/ref/fate/audiomatch-square-mp3
> > @@ -1 +1 @@
> > -presig: 0 postsig:0 c: 0.9447 lenerr:0
> > +presig: 0 postsig:-529 c: 0.9334 lenerr:-529
>
> isnt this exactly correct before and wrong after this change ?
>
> zero signal before and zero signal after the original is what one would
> expect and equal length and high correlation
>
> every number that changes gets worse
>

Michael - you're right, this patch is incorrect currently. I had mistakenly
convinced myself that mp3dec.c was overcompensating for the decoder delay.

I also found that LAME itself writes the encoder padding + decoder delay
into the trailing padding field (e.g. for an input where 468 samples of
padding are added by the encoder, the Info tag contains the value 997).
I'll send a revised patch set.


> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Never trust a computer, one day, it may think you are the virus. -- Compn
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
Jon Toohill Oct. 5, 2016, 5:40 p.m.
On Tue, Oct 4, 2016 at 7:19 AM, wm4 <nfxjfg@googlemail.com> wrote:

> On Mon,  3 Oct 2016 17:45:08 -0700
> Jon Toohill <jtoohill-at-google.com@ffmpeg.org> wrote:
>
> > Muxers can check AVCodecParameters.initial_padding for the
> > encoder+decoder delay, and read the AV_PKT_DATA_SKIP_SAMPLES
> > side data from the last packet for the encoder padding.
> >
> > This change also fixes the first_discard_sample calculation
> > which erroneously included the decoder delay. Decoder delay
> > is already accounted for in st->skip_samples. The affected
> > FATE tests have been updated accordingly.
> > ---
> >  libavformat/mp3dec.c                 |  3 ++-
> >  tests/ref/fate/audiomatch-square-mp3 |  2 +-
> >  tests/ref/fate/gapless-mp3           | 10 +++++-----
> >  3 files changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> > index 56c7f8c..e8b2428 100644
> > --- a/libavformat/mp3dec.c
> > +++ b/libavformat/mp3dec.c
> > @@ -239,9 +239,10 @@ static void mp3_parse_info_tag(AVFormatContext *s,
> AVStream *st,
> >
> >          mp3->start_pad = v>>12;
> >          mp3->  end_pad = v&4095;
> > +        st->codecpar->initial_padding = mp3->start_pad + 528 + 1;
> >          st->start_skip_samples = mp3->start_pad + 528 + 1;
> >          if (mp3->frames) {
> > -            st->first_discard_sample = -mp3->end_pad + 528 + 1 +
> mp3->frames * (int64_t)spf;
> > +            st->first_discard_sample = -mp3->end_pad + mp3->frames *
> (int64_t)spf;
>
> How does mixing these even make sense?
>

mp3enc.c already uses initial_padding for the encoder delay, and as you
previously pointed out, mp3dec.c already uses AV_PKT_START_SKIP_SAMPLES for
the encoder delay. I'm not attempting to solve the inconsistency in this
patch set.


> >              st->last_discard_sample = mp3->frames * (int64_t)spf;
> >          }
> >          if (!st->start_time)
> > diff --git a/tests/ref/fate/audiomatch-square-mp3
> b/tests/ref/fate/audiomatch-square-mp3
> > index 8de55c2..05176a0 100644
> > --- a/tests/ref/fate/audiomatch-square-mp3
> > +++ b/tests/ref/fate/audiomatch-square-mp3
> > @@ -1 +1 @@
> > -presig: 0 postsig:0 c: 0.9447 lenerr:0
> > +presig: 0 postsig:-529 c: 0.9334 lenerr:-529
> > diff --git a/tests/ref/fate/gapless-mp3 b/tests/ref/fate/gapless-mp3
> > index ebe7bfa..8b80bfc 100644
> > --- a/tests/ref/fate/gapless-mp3
> > +++ b/tests/ref/fate/gapless-mp3
> > @@ -1,5 +1,5 @@
> > -37534a3bcc3ef306e8c5ebfcfedfc41c *tests/data/fate/gapless-mp3.out-1
> > -c96c3ae7bd3300fd2f4debac222de5b7
> > -0cd1cdbcfd5cdbf6270cd98219bf31cd *tests/data/fate/gapless-mp3.out-2
> > -c96c3ae7bd3300fd2f4debac222de5b7
> > -9d3d8ba8a61b534f2d02ee648d6a8229 *tests/data/fate/gapless-mp3.out-3
> > +81695be427d45e8be4d527a6b2af2a85 *tests/data/fate/gapless-mp3.out-1
> > +c7879a827ab017364774069268d9a267
> > +62d074296f8c84a5f86a6afdd7bab459 *tests/data/fate/gapless-mp3.out-2
> > +c7879a827ab017364774069268d9a267
> > +e931f3fe1ba25e0d5eece4977c4061a9 *tests/data/fate/gapless-mp3.out-3
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Jon Toohill Oct. 5, 2016, 5:42 p.m.
On Wed, Oct 5, 2016 at 10:40 AM, Jon Toohill <jtoohill@google.com> wrote:

> On Tue, Oct 4, 2016 at 7:19 AM, wm4 <nfxjfg@googlemail.com> wrote:
>
>> On Mon,  3 Oct 2016 17:45:08 -0700
>> Jon Toohill <jtoohill-at-google.com@ffmpeg.org> wrote:
>>
>> > Muxers can check AVCodecParameters.initial_padding for the
>> > encoder+decoder delay, and read the AV_PKT_DATA_SKIP_SAMPLES
>> > side data from the last packet for the encoder padding.
>> >
>> > This change also fixes the first_discard_sample calculation
>> > which erroneously included the decoder delay. Decoder delay
>> > is already accounted for in st->skip_samples. The affected
>> > FATE tests have been updated accordingly.
>> > ---
>> >  libavformat/mp3dec.c                 |  3 ++-
>> >  tests/ref/fate/audiomatch-square-mp3 |  2 +-
>> >  tests/ref/fate/gapless-mp3           | 10 +++++-----
>> >  3 files changed, 8 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
>> > index 56c7f8c..e8b2428 100644
>> > --- a/libavformat/mp3dec.c
>> > +++ b/libavformat/mp3dec.c
>> > @@ -239,9 +239,10 @@ static void mp3_parse_info_tag(AVFormatContext
>> *s, AVStream *st,
>> >
>> >          mp3->start_pad = v>>12;
>> >          mp3->  end_pad = v&4095;
>> > +        st->codecpar->initial_padding = mp3->start_pad + 528 + 1;
>> >          st->start_skip_samples = mp3->start_pad + 528 + 1;
>> >          if (mp3->frames) {
>> > -            st->first_discard_sample = -mp3->end_pad + 528 + 1 +
>> mp3->frames * (int64_t)spf;
>> > +            st->first_discard_sample = -mp3->end_pad + mp3->frames *
>> (int64_t)spf;
>>
>> How does mixing these even make sense?
>>
>
> mp3enc.c already uses initial_padding for the encoder delay, and as you
> previously pointed out, mp3dec.c already uses AV_PKT_START_SKIP_SAMPLES for
> the encoder delay. I'm not attempting to solve the inconsistency in this
> patch set.
>

err, *mp3dec.c already uses AV_PKT_DATA_SKIP_SAMPLES for the encoder
padding. Sorry for the confusion.


>
>
>> >              st->last_discard_sample = mp3->frames * (int64_t)spf;
>> >          }
>> >          if (!st->start_time)
>> > diff --git a/tests/ref/fate/audiomatch-square-mp3
>> b/tests/ref/fate/audiomatch-square-mp3
>> > index 8de55c2..05176a0 100644
>> > --- a/tests/ref/fate/audiomatch-square-mp3
>> > +++ b/tests/ref/fate/audiomatch-square-mp3
>> > @@ -1 +1 @@
>> > -presig: 0 postsig:0 c: 0.9447 lenerr:0
>> > +presig: 0 postsig:-529 c: 0.9334 lenerr:-529
>> > diff --git a/tests/ref/fate/gapless-mp3 b/tests/ref/fate/gapless-mp3
>> > index ebe7bfa..8b80bfc 100644
>> > --- a/tests/ref/fate/gapless-mp3
>> > +++ b/tests/ref/fate/gapless-mp3
>> > @@ -1,5 +1,5 @@
>> > -37534a3bcc3ef306e8c5ebfcfedfc41c *tests/data/fate/gapless-mp3.out-1
>> > -c96c3ae7bd3300fd2f4debac222de5b7
>> > -0cd1cdbcfd5cdbf6270cd98219bf31cd *tests/data/fate/gapless-mp3.out-2
>> > -c96c3ae7bd3300fd2f4debac222de5b7
>> > -9d3d8ba8a61b534f2d02ee648d6a8229 *tests/data/fate/gapless-mp3.out-3
>> > +81695be427d45e8be4d527a6b2af2a85 *tests/data/fate/gapless-mp3.out-1
>> > +c7879a827ab017364774069268d9a267
>> > +62d074296f8c84a5f86a6afdd7bab459 *tests/data/fate/gapless-mp3.out-2
>> > +c7879a827ab017364774069268d9a267
>> > +e931f3fe1ba25e0d5eece4977c4061a9 *tests/data/fate/gapless-mp3.out-3
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>
>
wm4 Oct. 5, 2016, 6:03 p.m.
On Wed, 5 Oct 2016 10:42:13 -0700
Jon Toohill <jtoohill-at-google.com@ffmpeg.org> wrote:

> On Wed, Oct 5, 2016 at 10:40 AM, Jon Toohill <jtoohill@google.com> wrote:
> 
> > On Tue, Oct 4, 2016 at 7:19 AM, wm4 <nfxjfg@googlemail.com> wrote:
> >  
> >> On Mon,  3 Oct 2016 17:45:08 -0700
> >> Jon Toohill <jtoohill-at-google.com@ffmpeg.org> wrote:
> >>  
> >> > Muxers can check AVCodecParameters.initial_padding for the
> >> > encoder+decoder delay, and read the AV_PKT_DATA_SKIP_SAMPLES
> >> > side data from the last packet for the encoder padding.
> >> >
> >> > This change also fixes the first_discard_sample calculation
> >> > which erroneously included the decoder delay. Decoder delay
> >> > is already accounted for in st->skip_samples. The affected
> >> > FATE tests have been updated accordingly.
> >> > ---
> >> >  libavformat/mp3dec.c                 |  3 ++-
> >> >  tests/ref/fate/audiomatch-square-mp3 |  2 +-
> >> >  tests/ref/fate/gapless-mp3           | 10 +++++-----
> >> >  3 files changed, 8 insertions(+), 7 deletions(-)
> >> >
> >> > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> >> > index 56c7f8c..e8b2428 100644
> >> > --- a/libavformat/mp3dec.c
> >> > +++ b/libavformat/mp3dec.c
> >> > @@ -239,9 +239,10 @@ static void mp3_parse_info_tag(AVFormatContext  
> >> *s, AVStream *st,  
> >> >
> >> >          mp3->start_pad = v>>12;
> >> >          mp3->  end_pad = v&4095;
> >> > +        st->codecpar->initial_padding = mp3->start_pad + 528 + 1;
> >> >          st->start_skip_samples = mp3->start_pad + 528 + 1;
> >> >          if (mp3->frames) {
> >> > -            st->first_discard_sample = -mp3->end_pad + 528 + 1 +  
> >> mp3->frames * (int64_t)spf;  
> >> > +            st->first_discard_sample = -mp3->end_pad + mp3->frames *  
> >> (int64_t)spf;
> >>
> >> How does mixing these even make sense?
> >>  
> >
> > mp3enc.c already uses initial_padding for the encoder delay, and as you
> > previously pointed out, mp3dec.c already uses AV_PKT_START_SKIP_SAMPLES for
> > the encoder delay. I'm not attempting to solve the inconsistency in this
> > patch set.
> >  
> 
> err, *mp3dec.c already uses AV_PKT_DATA_SKIP_SAMPLES for the encoder
> padding. Sorry for the confusion.
> 

Not solving the inconsistency is problematic - but the worse issue is
that you seem to introduce inconsistencies. In my current opinion, the
packet side data and the initial_padding fields do the same (i.e.
duplicated API), so only one of them can or should be used. Your patch
seems to require the decoder to use them both, though.
Jon Toohill Oct. 5, 2016, 6:57 p.m.
On Wed, Oct 5, 2016 at 11:03 AM, wm4 <nfxjfg@googlemail.com> wrote:

> On Wed, 5 Oct 2016 10:42:13 -0700
> Jon Toohill <jtoohill-at-google.com@ffmpeg.org> wrote:
>
> > On Wed, Oct 5, 2016 at 10:40 AM, Jon Toohill <jtoohill@google.com>
> wrote:
> >
> > > On Tue, Oct 4, 2016 at 7:19 AM, wm4 <nfxjfg@googlemail.com> wrote:
> > >
> > >> On Mon,  3 Oct 2016 17:45:08 -0700
> > >> Jon Toohill <jtoohill-at-google.com@ffmpeg.org> wrote:
> > >>
> > >> > Muxers can check AVCodecParameters.initial_padding for the
> > >> > encoder+decoder delay, and read the AV_PKT_DATA_SKIP_SAMPLES
> > >> > side data from the last packet for the encoder padding.
> > >> >
> > >> > This change also fixes the first_discard_sample calculation
> > >> > which erroneously included the decoder delay. Decoder delay
> > >> > is already accounted for in st->skip_samples. The affected
> > >> > FATE tests have been updated accordingly.
> > >> > ---
> > >> >  libavformat/mp3dec.c                 |  3 ++-
> > >> >  tests/ref/fate/audiomatch-square-mp3 |  2 +-
> > >> >  tests/ref/fate/gapless-mp3           | 10 +++++-----
> > >> >  3 files changed, 8 insertions(+), 7 deletions(-)
> > >> >
> > >> > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> > >> > index 56c7f8c..e8b2428 100644
> > >> > --- a/libavformat/mp3dec.c
> > >> > +++ b/libavformat/mp3dec.c
> > >> > @@ -239,9 +239,10 @@ static void mp3_parse_info_tag(AVFormatContext
> > >> *s, AVStream *st,
> > >> >
> > >> >          mp3->start_pad = v>>12;
> > >> >          mp3->  end_pad = v&4095;
> > >> > +        st->codecpar->initial_padding = mp3->start_pad + 528 + 1;
> > >> >          st->start_skip_samples = mp3->start_pad + 528 + 1;
> > >> >          if (mp3->frames) {
> > >> > -            st->first_discard_sample = -mp3->end_pad + 528 + 1 +
> > >> mp3->frames * (int64_t)spf;
> > >> > +            st->first_discard_sample = -mp3->end_pad + mp3->frames
> *
> > >> (int64_t)spf;
> > >>
> > >> How does mixing these even make sense?
> > >>
> > >
> > > mp3enc.c already uses initial_padding for the encoder delay, and as you
> > > previously pointed out, mp3dec.c already uses
> AV_PKT_START_SKIP_SAMPLES for
> > > the encoder delay. I'm not attempting to solve the inconsistency in
> this
> > > patch set.
> > >
> >
> > err, *mp3dec.c already uses AV_PKT_DATA_SKIP_SAMPLES for the encoder
> > padding. Sorry for the confusion.
> >
>
> Not solving the inconsistency is problematic - but the worse issue is
> that you seem to introduce inconsistencies. In my current opinion, the
> packet side data and the initial_padding fields do the same (i.e.
> duplicated API), so only one of them can or should be used. Your patch
> seems to require the decoder to use them both, though.
>

That's a fair point; I'll send a revised patch set that doesn't change
mp3dec.c.

Note that I don't think I can get away from having libmp3lame.c set
AVCodecContext.initial_padding, since that field is used by the
AudioFrameQueue (and other encoders rely on it as well).

Patch hide | download patch | download mbox

diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
index 56c7f8c..e8b2428 100644
--- a/libavformat/mp3dec.c
+++ b/libavformat/mp3dec.c
@@ -239,9 +239,10 @@  static void mp3_parse_info_tag(AVFormatContext *s, AVStream *st,
 
         mp3->start_pad = v>>12;
         mp3->  end_pad = v&4095;
+        st->codecpar->initial_padding = mp3->start_pad + 528 + 1;
         st->start_skip_samples = mp3->start_pad + 528 + 1;
         if (mp3->frames) {
-            st->first_discard_sample = -mp3->end_pad + 528 + 1 + mp3->frames * (int64_t)spf;
+            st->first_discard_sample = -mp3->end_pad + mp3->frames * (int64_t)spf;
             st->last_discard_sample = mp3->frames * (int64_t)spf;
         }
         if (!st->start_time)
diff --git a/tests/ref/fate/audiomatch-square-mp3 b/tests/ref/fate/audiomatch-square-mp3
index 8de55c2..05176a0 100644
--- a/tests/ref/fate/audiomatch-square-mp3
+++ b/tests/ref/fate/audiomatch-square-mp3
@@ -1 +1 @@ 
-presig: 0 postsig:0 c: 0.9447 lenerr:0
+presig: 0 postsig:-529 c: 0.9334 lenerr:-529
diff --git a/tests/ref/fate/gapless-mp3 b/tests/ref/fate/gapless-mp3
index ebe7bfa..8b80bfc 100644
--- a/tests/ref/fate/gapless-mp3
+++ b/tests/ref/fate/gapless-mp3
@@ -1,5 +1,5 @@ 
-37534a3bcc3ef306e8c5ebfcfedfc41c *tests/data/fate/gapless-mp3.out-1
-c96c3ae7bd3300fd2f4debac222de5b7
-0cd1cdbcfd5cdbf6270cd98219bf31cd *tests/data/fate/gapless-mp3.out-2
-c96c3ae7bd3300fd2f4debac222de5b7
-9d3d8ba8a61b534f2d02ee648d6a8229 *tests/data/fate/gapless-mp3.out-3
+81695be427d45e8be4d527a6b2af2a85 *tests/data/fate/gapless-mp3.out-1
+c7879a827ab017364774069268d9a267
+62d074296f8c84a5f86a6afdd7bab459 *tests/data/fate/gapless-mp3.out-2
+c7879a827ab017364774069268d9a267
+e931f3fe1ba25e0d5eece4977c4061a9 *tests/data/fate/gapless-mp3.out-3