On 6 February 2018 at 03:16, Rostislav Pehlivanov <atomnu...@gmail.com>
wrote:
> On 6 February 2018 at 06:56, Josh Allmann <joshua.allm...@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@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
