On Tue, 6 Feb 2018 08:40:20 -0800 Josh Allmann <joshua.allm...@gmail.com> wrote:
> 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. 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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel