[FFmpeg-devel] [PATCH] avcodec/utils: initialize delay in avcodec_parameters_to_context()

[Hendrik Leppkes]

On Sat, Jun 4, 2016 at 7:30 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Sat, Jun 4, 2016 at 1:09 PM, Michael Niedermayer <michael at niedermayer.cc>
> wrote:
>
>> On Sat, Jun 04, 2016 at 09:16:09AM -0400, Ronald S. Bultje wrote:
>> > Hi,
>> >
>> > On Sat, Jun 4, 2016 at 7:27 AM, Michael Niedermayer
>> <michael at niedermayer.cc>
>> > wrote:
>> >
>> > > On Sat, Jun 04, 2016 at 12:15:50PM +0200, Hendrik Leppkes wrote:
>> > > > On Sat, Jun 4, 2016 at 12:09 PM, Michael Niedermayer
>> > > > <michael at niedermayer.cc> wrote:
>> > > > > On Sat, Jun 04, 2016 at 09:47:47AM +0200, Hendrik Leppkes wrote:
>> > > > >> On Sat, Jun 4, 2016 at 4:55 AM, Michael Niedermayer
>> > > > >> <michael at niedermayer.cc> wrote:
>> > > > >> > Fixes lost codec delayy
>> > > > >> > Should fix Ticket5509
>> > > > >> >
>> > > > >> > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>> > > > >> > ---
>> > > > >> >  libavcodec/utils.c |    1 +
>> > > > >> >  1 file changed, 1 insertion(+)
>> > > > >> >
>> > > > >> > diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>> > > > >> > index 7b99526..4016583 100644
>> > > > >> > --- a/libavcodec/utils.c
>> > > > >> > +++ b/libavcodec/utils.c
>> > > > >> > @@ -4157,6 +4157,7 @@ int
>> > > avcodec_parameters_to_context(AVCodecContext *codec,
>> > > > >> >          codec->sample_rate     = par->sample_rate;
>> > > > >> >          codec->block_align     = par->block_align;
>> > > > >> >          codec->frame_size      = par->frame_size;
>> > > > >> > +        codec->delay           =
>> > > > >> >          codec->initial_padding = par->initial_padding;
>> > > > >> >          codec->seek_preroll    = par->seek_preroll;
>> > > > >> >          break;
>> > > > >> > --
>> > > > >> > 1.7.9.5
>> > > > >> >
>> > > > >>
>> > > > >> Its probably fine to set it, but delay is not the correct field
>> for
>> > > > >> ffmpeg.c and/or whichever muxer is involved to be using then.
>> > > > >
>> > > > > delay must be set for the demuxer (required by API and used by
>> > > > > applications), avcodec_parameters_to_context()
>> > > > > has no knowledge about working on demuxer or muxer side
>> > > > > contexts unless i miss something. (i tried avctx->codec but its
>> never
>> > > > > set)
>> > > > > I can add a parameter to avcodec_parameters_to_context() that
>> > > > > indicates if its for the demuxer or muxer or add a
>> > > > > avcodec_parameters_to_context2() or a
>> > > > > avcodec_parameters_to_context_demuxer()
>> > > > > ff_parameters_to_context_demuxer()
>> > > > > do you agree to add such function ?
>> > > > > (iam asking as changes to AVCodecParameters API tend to receive
>> > > > >  a lot of opposition)
>> > > >
>> > > > Adding more weird API sounds silly, this patch is likely the better
>> > > option.
>> > >
>> > > applied
>> > >
>> > >
>> > > > Once ffmpeg.c is updated to use codecpar properly, this whole
>> > > > double-field issue goes away anyway.
>> > >
>> > > i dont think it does as the API requires delay to be set
>> >
>> >
>> > We can change API on the next major version bump.
>>
>> The field is not deprecated
>> we can mark the API as deprecated before the next version bump and
>> remove it on the following API bump at the earliest
>>
>> my oppinion was and is that maintaining long term API compatibility
>> is better than frequent API changes though
>
>
> Well, partially. We now have two fields that are set to the same codecpar
> value. The question is: is there ever a case where these two act
> meaningfully concurrent and have distinctly different meanings?
>

"delay" probably has a clearer meaning for video, at least its
documented to be used there for both encoding and decoding.
For audio, delay is specifically documented to be unused for encoding,
and initial_padding unused for decoding.

- Hendrik