Re: [FFmpeg-devel] [PATCH 1/3] lavf/mp3dec: pass Xing gapless metadata to AVCodecParameters

[Michael Niedermayer]

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

[Jon Toohill]

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 Niedermayer  wrote:

> 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

[Michael Niedermayer]

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

[Jon Toohill]

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