Re: [FFmpeg-devel] [PATCH 1/2] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters
On Wed, Jun 08, 2016 at 10:46:49AM -0700, Jon Toohill wrote: > Michael et al., is this good to merge as-is? I just tested and a round trip > with ffmpeg from wav -> mp3 -> wav retains the correct number of samples. There seems to be a inconsistency try the patch below with your patch, and write a mp3 file with ffmpeg and the read it the initial_padding is not what was written or try this: /ffmpeg -i test.mp3 -codec copy test2.mp3 read padding 576 written padding 576 ./ffmpeg -i test2.mp3 -codec copy test3.mp3 read padding 47 written padding 47 diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index 345fa88..b063ad9 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -240,6 +240,7 @@ 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; +av_log(0,0, "read padding %d\n", st->codecpar->initial_padding ); st->codecpar->trailing_padding = mp3->end_pad; st->start_skip_samples = mp3->start_pad + 528 + 1; if (mp3->frames) { diff --git a/libavformat/mp3enc.c b/libavformat/mp3enc.c index de63401..a971136 100644 --- a/libavformat/mp3enc.c +++ b/libavformat/mp3enc.c @@ -253,7 +253,7 @@ static int mp3_write_xing(AVFormatContext *s) av_log(s, AV_LOG_WARNING, "Too many samples of initial padding.\n"); } avio_wb24(dyn_ctx, FFMAX(par->initial_padding - 528 - 1, 0)<<12); - +av_log(0,0, "written padding %d\n", par->initial_padding ); avio_w8(dyn_ctx, 0); // misc avio_w8(dyn_ctx, 0); // mp3gain avio_wb16(dyn_ctx, 0); // preset [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB During times of universal deceit, telling the truth becomes a revolutionary act. -- George Orwell signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters
Michael et al., is this good to merge as-is? I just tested and a round trip with ffmpeg from wav -> mp3 -> wav retains the correct number of samples. Jon Toohill | Google Play Music | jtooh...@google.com | (650) 215-0770 On Wed, Jun 1, 2016 at 5:58 PM, Jon Toohillwrote: > Based on my understanding of [1], these values in the Info tag specify > only the encoder delay/padding, which matches the documentation for these > fields. It looks like other formats are using the fields that way as well. > > I think the extra 528 + 1 samples are the decoder delay [2]. It looks like > libmp3lame adds the 528 + 1 only to have mp3dec subtract it, so I'm not > sure why that's done. IIUC start_skip_samples is the mechanism that > actually accounts for the extra delay when decoding. > > [1]: http://gabriel.mp3-tech.org/mp3infotag.html#delays > [2]: http://lame.sourceforge.net/tech-FAQ.txt > > > > Jon Toohill | Google Play Music | jtooh...@google.com | (650) 215-0770 > > On Thu, May 26, 2016 at 7:51 PM, Michael Niedermayer < > mich...@niedermayer.cc> wrote: > >> On Wed, May 25, 2016 at 09:56:59AM -0700, Jon Toohill wrote: >> > --- >> > libavformat/mp3dec.c | 2 ++ >> > 1 file changed, 2 insertions(+) >> > >> > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c >> > index 3725d67..192f5ef 100644 >> > --- a/libavformat/mp3dec.c >> > +++ b/libavformat/mp3dec.c >> > @@ -234,6 +234,8 @@ 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; >> > +st->codecpar->trailing_padding = mp3->end_pad; >> > 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; >> >> is the 528 + 1 difference intended to >> start_skip_samples/first_discard_sample >> ? >> mp3enc stores par->initial_padding - 528 - 1 >> >> [...] >> >> -- >> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB >> >> If you drop bombs on a foreign country and kill a hundred thousand >> innocent people, expect your government to call the consequence >> "unprovoked inhuman terrorist attacks" and use it to justify dropping >> more bombs and killing more people. The technology changed, the idea is >> old. >> >> ___ >> 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 1/2] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters
Based on my understanding of [1], these values in the Info tag specify only the encoder delay/padding, which matches the documentation for these fields. It looks like other formats are using the fields that way as well. I think the extra 528 + 1 samples are the decoder delay [2]. It looks like libmp3lame adds the 528 + 1 only to have mp3dec subtract it, so I'm not sure why that's done. IIUC start_skip_samples is the mechanism that actually accounts for the extra delay when decoding. [1]: http://gabriel.mp3-tech.org/mp3infotag.html#delays [2]: http://lame.sourceforge.net/tech-FAQ.txt Jon Toohill | Google Play Music | jtooh...@google.com | (650) 215-0770 On Thu, May 26, 2016 at 7:51 PM, Michael Niedermayerwrote: > On Wed, May 25, 2016 at 09:56:59AM -0700, Jon Toohill wrote: > > --- > > libavformat/mp3dec.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c > > index 3725d67..192f5ef 100644 > > --- a/libavformat/mp3dec.c > > +++ b/libavformat/mp3dec.c > > @@ -234,6 +234,8 @@ 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; > > +st->codecpar->trailing_padding = mp3->end_pad; > > 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; > > is the 528 + 1 difference intended to > start_skip_samples/first_discard_sample > ? > mp3enc stores par->initial_padding - 528 - 1 > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > If you drop bombs on a foreign country and kill a hundred thousand > innocent people, expect your government to call the consequence > "unprovoked inhuman terrorist attacks" and use it to justify dropping > more bombs and killing more people. The technology changed, the idea is > old. > > ___ > 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 1/2] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters
On Wed, May 25, 2016 at 09:56:59AM -0700, Jon Toohill wrote: > --- > libavformat/mp3dec.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c > index 3725d67..192f5ef 100644 > --- a/libavformat/mp3dec.c > +++ b/libavformat/mp3dec.c > @@ -234,6 +234,8 @@ 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; > +st->codecpar->trailing_padding = mp3->end_pad; > 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; is the 528 + 1 difference intended to start_skip_samples/first_discard_sample ? mp3enc stores par->initial_padding - 528 - 1 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If you drop bombs on a foreign country and kill a hundred thousand innocent people, expect your government to call the consequence "unprovoked inhuman terrorist attacks" and use it to justify dropping more bombs and killing more people. The technology changed, the idea is old. signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters
On 5/26/2016 11:05 PM, Michael Niedermayer wrote: > On Wed, May 25, 2016 at 02:18:38PM -0400, Ronald S. Bultje wrote: >> Hi, >> >> On Wed, May 25, 2016 at 1:24 PM, James Almerwrote: >> >>> On 5/25/2016 1:56 PM, Jon Toohill wrote: --- libavformat/mp3dec.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index 3725d67..192f5ef 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -234,6 +234,8 @@ 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; +st->codecpar->trailing_padding = mp3->end_pad; >>> >>> Every other format so far is using the AV_PKT_DATA_SKIP_SAMPLES side data >>> to >>> discard samples from the last packet/frame. See matroska and ogg demuxers, >>> currently used for Opus only. >>> >>> The trailing_padding AVCodecParameters element was added after the above >>> was >>> designed. To be honest i can't say if we should replace one with the other >>> or find a way to keep both, and seeing how AVCodecParameters hasn't made it >>> to a release yet, we're still on time to choose. >> >> >> I agree having 1 is better than having 2. I can't technically comment on >> which one is better/easier/*. > > mp3 supports AV_PKT_DATA_SKIP_SAMPLES since FFmpeg 2.5 > ogg opus sets codecpar->initial_padding like this patch would > so does dtshddec and matroskadec > > AV_PKT_DATA_SKIP_SAMPLES is not a substitute for setting > trailing_padding because with AV_PKT_DATA_SKIP_SAMPLES the > trailing_padding only becomes available at the end of the stream > > also theres nothing wrong with AV_PKT_DATA_SKIP_SAMPLES, its local > information for the current packet and there may indeed be cases > like with concatenated streams where there are packets in the middle > with discarding. So the 2 AVCodecParameters fields are not a substitute > for AV_PKT_DATA_SKIP_SAMPLES > Fine with me, then. I assumed both both were pretty much made for the same purpose and to cover the same use cases. > also for encoding AV_PKT_DATA_SKIP_SAMPLES does not work with some > formats the trailing_padding is needed to be known when writing the > header The one format i know should be using AV_PKT_DATA_SKIP_SAMPLES the same way as ogg and matroska is mpegts. Probably also mp4. At least for Opus the mpegts muxer reads the value from the last packet the encoder or demuxer sends its way and writes it to the output file, but the mpegts demuxer is apparently none the wiser because the opus parser just discards this information. > > the patch does look like a reasonable step to me but i might be > missing something ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters
On Wed, May 25, 2016 at 02:18:38PM -0400, Ronald S. Bultje wrote: > Hi, > > On Wed, May 25, 2016 at 1:24 PM, James Almerwrote: > > > On 5/25/2016 1:56 PM, Jon Toohill wrote: > > > --- > > > libavformat/mp3dec.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c > > > index 3725d67..192f5ef 100644 > > > --- a/libavformat/mp3dec.c > > > +++ b/libavformat/mp3dec.c > > > @@ -234,6 +234,8 @@ 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; > > > +st->codecpar->trailing_padding = mp3->end_pad; > > > > Every other format so far is using the AV_PKT_DATA_SKIP_SAMPLES side data > > to > > discard samples from the last packet/frame. See matroska and ogg demuxers, > > currently used for Opus only. > > > > The trailing_padding AVCodecParameters element was added after the above > > was > > designed. To be honest i can't say if we should replace one with the other > > or find a way to keep both, and seeing how AVCodecParameters hasn't made it > > to a release yet, we're still on time to choose. > > > I agree having 1 is better than having 2. I can't technically comment on > which one is better/easier/*. mp3 supports AV_PKT_DATA_SKIP_SAMPLES since FFmpeg 2.5 ogg opus sets codecpar->initial_padding like this patch would so does dtshddec and matroskadec AV_PKT_DATA_SKIP_SAMPLES is not a substitute for setting trailing_padding because with AV_PKT_DATA_SKIP_SAMPLES the trailing_padding only becomes available at the end of the stream also theres nothing wrong with AV_PKT_DATA_SKIP_SAMPLES, its local information for the current packet and there may indeed be cases like with concatenated streams where there are packets in the middle with discarding. So the 2 AVCodecParameters fields are not a substitute for AV_PKT_DATA_SKIP_SAMPLES also for encoding AV_PKT_DATA_SKIP_SAMPLES does not work with some formats the trailing_padding is needed to be known when writing the header the patch does look like a reasonable step to me but i might be missing something [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I am the wisest man alive, for I know one thing, and that is that I know nothing. -- Socrates signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters
Hi, On Wed, May 25, 2016 at 1:24 PM, James Almerwrote: > On 5/25/2016 1:56 PM, Jon Toohill wrote: > > --- > > libavformat/mp3dec.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c > > index 3725d67..192f5ef 100644 > > --- a/libavformat/mp3dec.c > > +++ b/libavformat/mp3dec.c > > @@ -234,6 +234,8 @@ 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; > > +st->codecpar->trailing_padding = mp3->end_pad; > > Every other format so far is using the AV_PKT_DATA_SKIP_SAMPLES side data > to > discard samples from the last packet/frame. See matroska and ogg demuxers, > currently used for Opus only. > > The trailing_padding AVCodecParameters element was added after the above > was > designed. To be honest i can't say if we should replace one with the other > or find a way to keep both, and seeing how AVCodecParameters hasn't made it > to a release yet, we're still on time to choose. I agree having 1 is better than having 2. I can't technically comment on which one is better/easier/*. Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters
On 5/25/2016 1:56 PM, Jon Toohill wrote: > --- > libavformat/mp3dec.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c > index 3725d67..192f5ef 100644 > --- a/libavformat/mp3dec.c > +++ b/libavformat/mp3dec.c > @@ -234,6 +234,8 @@ 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; > +st->codecpar->trailing_padding = mp3->end_pad; Every other format so far is using the AV_PKT_DATA_SKIP_SAMPLES side data to discard samples from the last packet/frame. See matroska and ogg demuxers, currently used for Opus only. The trailing_padding AVCodecParameters element was added after the above was designed. To be honest i can't say if we should replace one with the other or find a way to keep both, and seeing how AVCodecParameters hasn't made it to a release yet, we're still on time to choose. > 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; > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/2] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters
--- libavformat/mp3dec.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index 3725d67..192f5ef 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -234,6 +234,8 @@ 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; +st->codecpar->trailing_padding = mp3->end_pad; 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; -- 2.8.0.rc3.226.g39d4020 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/2] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters
From: Jon Toohill--- libavformat/mp3dec.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index 3725d67..192f5ef 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -234,6 +234,8 @@ 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; +st->codecpar->trailing_padding = mp3->end_pad; 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; -- 2.8.0.rc3.226.g39d4020 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel