[FFmpeg-devel] [PATCH] aacenc: Free any extradata before re-allocating.

[wm4]

On Tue, 6 Feb 2018 08:40:20 -0800
Josh Allmann <joshua.allmann at gmail.com> wrote:

> On 6 February 2018 at 03:16, Rostislav Pehlivanov <atomnuker at gmail.com>
> wrote:
> 
> > On 6 February 2018 at 06:56, Josh Allmann <joshua.allmann at gmail.com>
> > wrote:
> >  
> > > Fixes a leak that occurs if avctx->extradata contains any data
> > > prior to opening the codec, eg left over from an initialization
> > > call to avcodec_parameters_from_context.
> > > ---
> > >  libavcodec/aacenc.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/libavcodec/aacenc.c b/libavcodec/aacenc.c
> > > index 6d94c76905..f8fbe69d87 100644
> > > --- a/libavcodec/aacenc.c
> > > +++ b/libavcodec/aacenc.c
> > > @@ -98,6 +98,10 @@ static int put_audio_specific_config(AVCodecContext
> > > *avctx)
> > >      int channels = (!s->needs_pce)*(s->channels - (s->channels == 8 ? 1  
> > :  
> > > 0));
> > >      const int max_size = 32;
> > >
> > > +    if (avctx->extradata) {
> > > +        av_freep(&avctx->extradata);
> > > +        avctx->extradata_size = 0;
> > > +    }
> > >      avctx->extradata = av_mallocz(max_size);
> > >      if (!avctx->extradata)
> > >          return AVERROR(ENOMEM);
> > > --
> > > 2.14.2
> > >
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel at ffmpeg.org
> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >  
> >
> > No, its not up to the encoder to free up the extradata. Its up to the API
> > user to close the avctx for the encoder which will free the extradata, even
> > if encoder init fails. Besides, if you don't, you'll have a dirty context
> > from the previous encoder since they don't have to set the same avctx
> > fields.
> >  
> 
> While many (all?) other encoders share the pattern of allocating extradata
> without checking for previous allocations, the abstraction seems... leaky?
> (Pun fully intended.) If the encoder has its avctx fields set by
> avcodec_parameters_to_context, then the extradata is deep-copied. Even when
> deep copying, we do take care to deallocate any existing avctx extradata,
> to avoid introducing the same type of leak. Without this patch, the API
> user would have to explicitly undo the work that
> avcodec_parameters_to_context is trying to help with. Hence, the 'right'
> workflow when using avcodec_parameters_to_context would be:
> 
> 0. Allocate codec context
> 1. Copy codec params to context with avcodec_parameters_to_context
> 2. Check if extradata exists in context and deallocate from context if so.
> 3. Open codec.
> ...
> 4. Close codec.
> 
> These semantics don't seem clean to me, and I think we should strive to
> avoid making the user deal with corner cases like these explicitly. If not,
> we should at least document it.

I'd say the encoder isn't supposed to get these codec params, and the
function you mentioned can't be called on the encoder.

Yes, that's a result of all those public fields on a big struct, and the
awkward sharing of the codec context struct type between encoders and
decoders.