[FFmpeg-devel] [PATCH 3/3] lavf/mp3dec: read encoder delay/padding from Info tag

[Jon Toohill]

On Wed, Oct 5, 2016 at 11:03 AM, wm4 <nfxjfg at googlemail.com> wrote:

> On Wed, 5 Oct 2016 10:42:13 -0700
> Jon Toohill <jtoohill-at-google.com at ffmpeg.org> wrote:
>
> > On Wed, Oct 5, 2016 at 10:40 AM, Jon Toohill <jtoohill at google.com>
> wrote:
> >
> > > On Tue, Oct 4, 2016 at 7:19 AM, wm4 <nfxjfg at googlemail.com> wrote:
> > >
> > >> On Mon,  3 Oct 2016 17:45:08 -0700
> > >> Jon Toohill <jtoohill-at-google.com at 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).

_______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>