Re: [FFmpeg-devel] [PATCH 1/3] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters
On Tue, Jul 12, 2016 at 03:41:38PM -0700, Jon Toohill wrote: > I'm having a hard time finding software that uses this via simple GitHub > searches. FFmpeg itself uses it > > I think this should be considered a bugfix, since the struct field states > that it only represents encoder delay, not decoder delay. I'll send out an > updated patchset that splits this into more patches, including > documentation and another minor version bump. heres a case that this patch breaks: ./ffmpeg -i ~/videos/matrixbench_mpeg2.mpg -acodec mp3 -vn -t 1 file.mp4 ./ffprobe -show_packets -print_format compact file.mp4 this seems to require libmp3lame which is why it doesnt show up in fate --- 3-old 2016-07-13 01:19:50.338105803 +0200 +++ 3-new 2016-07-13 01:18:59.354104729 +0200 @@ -1,43 +1,43 @@ -packet|codec_type=audio|stream_index=0|pts=-1105|pts_time=-0.023021|dts=-1105|dts_time=-0.023021|duration=1152|duration_time=0.024000|convergence_duration=N/A|convergence_duration_time=N/A|size=384|pos=44|flags=K -packet|codec_type=audio|stream_index=0|pts=47|pts_time=0.000979|dts=47|dts_time=0.000979|duration=1152|duration_time=0.024000|convergence_duration=N/A|convergence_duration_time=N/A|size=384|pos=428|flags=K -packet|codec_type=audio|stream_index=0|pts=1199|pts_time=0.024979|dts=1199|dts_time=0.024979|duration=1152|duration_time=0.024000|convergence_duration=N/A|convergence_duration_time=N/A|size=384|pos=812|flags=K -packet|codec_type=audio|stream_index=0|pts=2351|pts_time=0.048979|dts=2351|dts_time=0.048979|duration=1152|duration_time=0.024000|convergence_duration=N/A|convergence_duration_time=N/A|size=384|pos=1196|flags=K -packet|codec_type=audio|stream_index=0|pts=3503|pts_time=0.072979|dts=3503|dts_time=0.072979|duration=1152|duration_time=0.024000|convergence_duration=N/A|convergence_duration_time=N/A|size=384|pos=1580|flags=K -packet|codec_type=audio|stream_index=0|pts=4655|pts_time=0.096979|dts=4655|dts_time=0.096979|duration=1152|duration_time=0.024000|convergence_duration=N/A|convergence_duration_time=N/A|size=384|pos=1964|flags=K -packet|codec_type=audio|stream_index=0|pts=5807|pts_time=0.120979|dts=5807|dts_time=0.120979|duration=1152|duration_time=0.024000|convergence_duration=N/A|convergence_duration_time=N/A|size=384|pos=2348|flags=K -packet|codec_type=audio|stream_index=0|pts=6959|pts_time=0.144979|dts=6959|dts_time=0.144979|duration=1152|duration_time=0.024000|convergence_duration=N/A|convergence_duration_time=N/A|size=384|pos=2732|flags=K -packet|codec_type=audio|stream_index=0|pts=8111|pts_time=0.168979|dts=8111|dts_time=0.168979|duration=1152|duration_time=0.024000|convergence_duration=N/A|convergence_duration_time=N/A|size=384|pos=3116|flags=K -packet|codec_type=audio|stream_index=0|pts=9263|pts_time=0.192979|dts=9263|dts_time=0.192979|duration=1152|duration_time=0.024000|convergence_duration=N/A|convergence_duration_time=N/A|size=384|pos=3500|flags=K -packet|codec_type=audio|stream_index=0|pts=10415|pts_time=0.216979|dts=10415|dts_time=0.216979|duration=1152|duration_time=0.024000|convergence_duration=N/A|convergence_duration_time=N/A|size=384|pos=3884|flags=K -packet|codec_type=audio|stream_index=0|pts=11567|pts_time=0.240979|dts=11567|dts_time=0.240979|duration=1152|duration_time=0.024000|convergence_duration=N/A|convergence_duration_time=N/A|size=384|pos=4268|flags=K -packet|codec_type=audio|stream_index=0|pts=12719|pts_time=0.264979|dts=12719|dts_time=0.264979|duration=1152|duration_time=0.024000|convergence_duration=N/A|convergence_duration_time=N/A|size=384|pos=4652|flags=K -packet|codec_type=audio|stream_index=0|pts=13871|pts_time=0.288979|dts=13871|dts_time=0.288979|duration=1152|duration_time=0.024000|convergence_duration=N/A|convergence_duration_time=N/A|size=384|pos=5036|flags=K -packet|codec_type=audio|stream_index=0|pts=15023|pts_time=0.312979|dts=15023|dts_time=0.312979|duration=1152|duration_time=0.024000|convergence_duration=N/A|convergence_duration_time=N/A|size=384|pos=5420|flags=K -packet|codec_type=audio|stream_index=0|pts=16175|pts_time=0.336979|dts=16175|dts_time=0.336979|duration=1152|duration_time=0.024000|convergence_duration=N/A|convergence_duration_time=N/A|size=384|pos=5804|flags=K -packet|codec_type=audio|stream_index=0|pts=17327|pts_time=0.360979|dts=17327|dts_time=0.360979|duration=1152|duration_time=0.024000|convergence_duration=N/A|convergence_duration_time=N/A|size=384|pos=6188|flags=K -packet|codec_type=audio|stream_index=0|pts=18479|pts_time=0.384979|dts=18479|dts_time=0.384979|duration=1152|duration_time=0.024000|convergence_duration=N/A|convergence_duration_time=N/A|size=384|pos=6572|flags=K -packet|codec_type=audio|stream_index=0|pts=19631|pts_time=0.408979|dts=19631|dts_time=0.408979|duration=1152|duration_time=0.024000|convergence_duration=N/A|convergence_duration_time=N/A|size=384|pos=6956|flags=K
Re: [FFmpeg-devel] [PATCH 1/3] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters
I'm having a hard time finding software that uses this via simple GitHub searches. I think this should be considered a bugfix, since the struct field states that it only represents encoder delay, not decoder delay. I'll send out an updated patchset that splits this into more patches, including documentation and another minor version bump. Jon Toohill | Google Play Music | jtooh...@google.com | (650) 215-0770 On Fri, Jun 17, 2016 at 5:32 PM, Michael Niedermayerwrote: > On Thu, Jun 16, 2016 at 11:16:05AM -0700, Jon Toohill wrote: > > Also removes decoder delay compensation from libmp3lame and mp3enc. > > initial_padding specifies only encoder delay, decoder delay is > > handled by start_skip_samples. > > --- > > libavcodec/libmp3lame.c | 2 +- > > libavformat/mp3dec.c| 2 ++ > > libavformat/mp3enc.c| 9 ++--- > > 3 files changed, 9 insertions(+), 4 deletions(-) > > > > diff --git a/libavcodec/libmp3lame.c b/libavcodec/libmp3lame.c > > index 5642264..198ac94 100644 > > --- a/libavcodec/libmp3lame.c > > +++ b/libavcodec/libmp3lame.c > > @@ -137,7 +137,7 @@ static av_cold int > mp3lame_encode_init(AVCodecContext *avctx) > > } > > > > /* get encoder delay */ > > -avctx->initial_padding = lame_get_encoder_delay(s->gfp) + 528 + 1; > > +avctx->initial_padding = lame_get_encoder_delay(s->gfp); > > ff_af_queue_init(avctx, >afq); > > > > avctx->frame_size = lame_get_framesize(s->gfp); > > you are changing a field of the public API > changing public API without major version bumps is tricky, we dont want > to break applications linkng to a newer lib > > is there software that uses this? > software that would break if this is applied ? (or maybe it wuld fix > some software usig it) > > If this is a bugfix it should be documented in APIChanges with > minor version bumps, any available references to specifications > should be added too > > Such bugfix should also be seperate of other unrelated changes > > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > I have often repented speaking, but never of holding my tongue. > -- Xenocrates > > ___ > 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/3] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters
On Thu, Jun 16, 2016 at 11:16:05AM -0700, Jon Toohill wrote: > Also removes decoder delay compensation from libmp3lame and mp3enc. > initial_padding specifies only encoder delay, decoder delay is > handled by start_skip_samples. > --- > libavcodec/libmp3lame.c | 2 +- > libavformat/mp3dec.c| 2 ++ > libavformat/mp3enc.c| 9 ++--- > 3 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/libavcodec/libmp3lame.c b/libavcodec/libmp3lame.c > index 5642264..198ac94 100644 > --- a/libavcodec/libmp3lame.c > +++ b/libavcodec/libmp3lame.c > @@ -137,7 +137,7 @@ static av_cold int mp3lame_encode_init(AVCodecContext > *avctx) > } > > /* get encoder delay */ > -avctx->initial_padding = lame_get_encoder_delay(s->gfp) + 528 + 1; > +avctx->initial_padding = lame_get_encoder_delay(s->gfp); > ff_af_queue_init(avctx, >afq); > > avctx->frame_size = lame_get_framesize(s->gfp); you are changing a field of the public API changing public API without major version bumps is tricky, we dont want to break applications linkng to a newer lib is there software that uses this? software that would break if this is applied ? (or maybe it wuld fix some software usig it) If this is a bugfix it should be documented in APIChanges with minor version bumps, any available references to specifications should be added too Such bugfix should also be seperate of other unrelated changes [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I have often repented speaking, but never of holding my tongue. -- Xenocrates signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/3] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters
Also removes decoder delay compensation from libmp3lame and mp3enc. initial_padding specifies only encoder delay, decoder delay is handled by start_skip_samples. --- libavcodec/libmp3lame.c | 2 +- libavformat/mp3dec.c| 2 ++ libavformat/mp3enc.c| 9 ++--- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/libavcodec/libmp3lame.c b/libavcodec/libmp3lame.c index 5642264..198ac94 100644 --- a/libavcodec/libmp3lame.c +++ b/libavcodec/libmp3lame.c @@ -137,7 +137,7 @@ static av_cold int mp3lame_encode_init(AVCodecContext *avctx) } /* get encoder delay */ -avctx->initial_padding = lame_get_encoder_delay(s->gfp) + 528 + 1; +avctx->initial_padding = lame_get_encoder_delay(s->gfp); ff_af_queue_init(avctx, >afq); avctx->frame_size = lame_get_framesize(s->gfp); diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index 56c7f8c..345fa88 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -239,6 +239,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; diff --git a/libavformat/mp3enc.c b/libavformat/mp3enc.c index de63401..da70d13 100644 --- a/libavformat/mp3enc.c +++ b/libavformat/mp3enc.c @@ -248,11 +248,14 @@ static int mp3_write_xing(AVFormatContext *s) avio_w8(dyn_ctx, 0); // unknown encoding flags avio_w8(dyn_ctx, 0); // unknown abr/minimal bitrate -// encoder delay -if (par->initial_padding - 528 - 1 >= 1 << 12) { +// encoder delay/padding +if (par->initial_padding >= 1 << 12) { 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); +if (par->trailing_padding >= 1 << 12) { +av_log(s, AV_LOG_WARNING, "Too many samples of trailing padding.\n"); +} +avio_wb24(dyn_ctx, (par->initial_padding << 12) | (par->trailing_padding & 0xFFF)); avio_w8(dyn_ctx, 0); // misc avio_w8(dyn_ctx, 0); // mp3gain -- 2.8.0.rc3.226.g39d4020 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel