Re: [FFmpeg-devel] [PATCH 3/3] lavf/mp3dec: read encoder delay/padding from Info tag
On Mon, 3 Oct 2016 17:45:08 -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; 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] lavf/mp3dec: read encoder delay/padding from Info tag
On Tue, Oct 4, 2016 at 2:45 AM, 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 > 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] lavf/mp3dec: read encoder delay/padding from Info tag
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 GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Never trust a computer, one day, it may think you are the virus. -- Compn signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] lavf/mp3dec: read encoder delay/padding from Info tag
On 10/4/2016 12:52 PM, Hendrik Leppkes wrote: > On Tue, Oct 4, 2016 at 2:45 AM, 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 >> 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 Changing the test to output the framecrc directly instead of doing a md5sum of the output file shows this TESTgapless-mp3 --- /ffmpeg/src/tests/ref/fate/gapless-mp3 2016-10-04 13:08:51.271126400 -0300 +++ tests/data/fate/gapless-mp3 2016-10-04 13:09:26.519070600 -0300 @@ -596,7 +596,7 @@ 0, 217143996, 217143996, 368640, 418, 0xb260c6a6 0, 217512636, 217512636, 368640, 418, 0xe448c368 0, 217881276, 217881276, 368640, 418, 0xb229c63f -0, 218249916, 218249916, 368640, 418, 0x53de9611, S=1, 10, 0x011f0030 +0, 218249916, 218249916, 368640, 418, 0x53de9611, S=1, 10, 0x018f0043 0, 218618556, 218618556, 368640, 418, 0xe12f8514, S=1, 10, 0x03140084 #tb 0: 1/44100 #media_type 0: audio @@ -1196,7 +1196,7 @@ 0, 678575, 678575, 1152, 4608, 0x5fd9abc4 0, 679727, 679727, 1152, 4608, 0xc7ccda46 0, 680879, 680879, 1152, 4608, 0x96c68e2f -0, 682031, 682031, 849, 3396, 0x4fe14cc5 +0, 682031, 682031, 320, 1280, 0xb3535bc6 #tb 0: 1/14112000 #media_type 0: audio #codec_id 0: mp3 @@ -1795,7 +1795,7 @@ 0, 217497600, 217497600, 368640, 418, 0xb260c6a6 0, 217866240, 217866240, 368640, 418, 0xe448c368 0, 218234880, 218234880, 368640, 418, 0xb229c63f -0, 218603520, 218603520, 368640, 418, 0x53de9611, S=1, 10, 0x011f0030 +0, 218603520, 218603520, 368640, 418, 0x53de9611, S=1, 10, 0x018f0043 0, 218972160, 218972160, 368640, 418, 0xe12f8514, S=1, 10, 0x03140084 #tb 0: 1/44100 #media_type 0: audio @@ -2395,7 +2395,7 @@ 0, 679680, 679680, 1152, 4608, 0x5fd9abc4 0, 680832, 680832, 1152, 4608, 0xc7ccda46 0, 681984, 681984, 1152, 4608, 0x96c68e2f -0, 683136, 683136, 849, 3396, 0x4fe14cc5 +0, 683136, 683136, 320, 1280, 0xb3535bc6 #tb 0: 1/14112000 #media_type 0: audio #codec_id 0: mp3 @@ -2803,5 +2803,5 @@ 0, 146937600, 146937600, 368640,
Re: [FFmpeg-devel] [PATCH 3/3] lavf/mp3dec: read encoder delay/padding from Info tag
On Tue, Oct 4, 2016 at 7:19 AM, wm4 wrote: > On Mon, 3 Oct 2016 17:45:08 -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; > > 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 > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] lavf/mp3dec: read encoder delay/padding from Info tag
On Tue, Oct 4, 2016 at 9:10 AM, Michael Niedermayer 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 > > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] lavf/mp3dec: read encoder delay/padding from Info tag
On Wed, Oct 5, 2016 at 10:40 AM, Jon Toohill wrote: > On Tue, Oct 4, 2016 at 7:19 AM, wm4 wrote: > >> On Mon, 3 Oct 2016 17:45:08 -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; >> >> 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 >> > > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] lavf/mp3dec: read encoder delay/padding from Info tag
On Wed, 5 Oct 2016 10:42:13 -0700 Jon Toohill wrote: > On Wed, Oct 5, 2016 at 10:40 AM, Jon Toohill wrote: > > > On Tue, Oct 4, 2016 at 7:19 AM, wm4 wrote: > > > >> On Mon, 3 Oct 2016 17:45:08 -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; > >> > >> 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. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] lavf/mp3dec: read encoder delay/padding from Info tag
On Wed, Oct 5, 2016 at 11:03 AM, wm4 wrote: > On Wed, 5 Oct 2016 10:42:13 -0700 > Jon Toohill wrote: > > > On Wed, Oct 5, 2016 at 10:40 AM, Jon Toohill > wrote: > > > > > On Tue, Oct 4, 2016 at 7:19 AM, wm4 wrote: > > > > > >> On Mon, 3 Oct 2016 17:45:08 -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; > > >> > > >> 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). ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel