| Message ID | 1475541908-21260-4-git-send-email-jtoohill@google.com |
|---|---|
| State | New |
| Headers | show |
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
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
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 [...]
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 > >
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 >
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 >> > >
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.
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).
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