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

Josh Allmann joshua.allmann at gmail.com
Tue Feb 6 18:40:20 EET 2018


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.

Josh



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


More information about the ffmpeg-devel mailing list