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

---
 libavformat/mp3dec.c | 2 ++
 1 file changed, 2 insertions(+)

On Mon, 26 Sep 2016 10:13:39 -0700
Jon Toohill <jtoohill-at-google.com@ffmpeg.org> wrote:

> ---
>  libavformat/mp3dec.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> index 56c7f8c..9cc85a3 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 + 528 + 1;
> +        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;

I'm somewhat suspicious about this, because mp3dec.c uses
AV_PKT_DATA_SKIP_SAMPLES to communicate delay/padding
(libavformat/utils.c turns the start_skip_samples field into side
data). So I'm not quite convinced is this mess of FFmpeg and Libav API
mixture is healthy. Opinions welcome.

A similar concern was raised in a previous related patch:
http://ffmpeg.org/pipermail/ffmpeg-devel/2016-May/194690.html
I think the resolution at the time was to go ahead with using both, since
both are used widely and serve slightly different purposes, although I do
agree that it's confusing.

I think I could use AV_PKT_DATA_SKIP_SAMPLES here, and change mp3enc to use
that to fill out the Info tag. But that only seems worthwhile to me if
there is a general consensus to move to just AV_PKT_DATA_SKIP_SAMPLES (and
deprecate/remove initial_padding and trailing_padding). If there's a
concrete case where knowing trailing_padding at the start of a stream is
necessary, then that's not possible, but I'm pretty new to this and don't
know of one. Thoughts?


Jon Toohill |  Google Play Music |  jtoohill@google.com |  (650) 215-0770

On Mon, Sep 26, 2016 at 11:30 AM, wm4 <nfxjfg@googlemail.com> wrote:

> On Mon, 26 Sep 2016 10:13:39 -0700
> Jon Toohill <jtoohill-at-google.com@ffmpeg.org> wrote:
>
> > ---
> >  libavformat/mp3dec.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> > index 56c7f8c..9cc85a3 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 + 528 + 1;
> > +        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;
>
> I'm somewhat suspicious about this, because mp3dec.c uses
> AV_PKT_DATA_SKIP_SAMPLES to communicate delay/padding
> (libavformat/utils.c turns the start_skip_samples field into side
> data). So I'm not quite convinced is this mess of FFmpeg and Libav API
> mixture is healthy. Opinions welcome.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

On Mon, 26 Sep 2016 16:04:26 -0700
Jon Toohill <jtoohill-at-google.com@ffmpeg.org> wrote:

> A similar concern was raised in a previous related patch:
> http://ffmpeg.org/pipermail/ffmpeg-devel/2016-May/194690.html
> I think the resolution at the time was to go ahead with using both, since
> both are used widely and serve slightly different purposes, although I do
> agree that it's confusing.
> 
> I think I could use AV_PKT_DATA_SKIP_SAMPLES here, and change mp3enc to use
> that to fill out the Info tag. But that only seems worthwhile to me if
> there is a general consensus to move to just AV_PKT_DATA_SKIP_SAMPLES (and
> deprecate/remove initial_padding and trailing_padding). If there's a
> concrete case where knowing trailing_padding at the start of a stream is
> necessary, then that's not possible, but I'm pretty new to this and don't
> know of one. Thoughts?

AV_PKT_DATA_SKIP_SAMPLES is FFmpeg's own "recent" solution for handling
gapless, the codecpar padding fields are Libav's newer solution for the
same problem. Libav doesn't have AV_PKT_DATA_SKIP_SAMPLES. There's also
AVCodecContext.delay, which is an older solution for the initial
padding. We're entering a real mess here. So we should make a conscious
decision about what mechanism should be used in the future, rather than
making an ad-hoc decision for a single patch. (And then probably
changing our opinion later, all contributing to furthering the mess.)

So we need to decide a few things:
- which mechanism should be used?
- should the older mechanism(s) be deprecated?
- do they conflict? how should compatibility be kept?
- should decoding (i.e. in AVCodecContext) use the newer mechanism to
  change output data, like with AV_PKT_DATA_SKIP_SAMPLES?
- should this patch be held back until a decision was made?

(Unless I'm missing something, which is possible.)

And of course we need to know how to make a decision, since we
apparently don't have a process for this in this project.