| Message ID | 1517900195-1879-1-git-send-email-joshua.allmann@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 6 February 2018 at 06:56, Josh Allmann <joshua.allmann@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.
On 6 February 2018 at 03:16, Rostislav Pehlivanov <atomnuker@gmail.com> wrote: > On 6 February 2018 at 06:56, Josh Allmann <joshua.allmann@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 >
On Tue, Feb 6, 2018 at 5:40 PM, Josh Allmann <joshua.allmann@gmail.com> wrote: > If the encoder has its avctx fields set by > avcodec_parameters_to_context, then the extradata is deep-copied. But it shouldn't have. Thats not a common pattern. What source codec parameters would you use for an encoder? What if an encoder doesn't actually generate any extradata (in audio, very few actually do), you could end up writing entirely unrelated extradata (say aac extradata with an ac3 stream), which results in all sorts of weird files. This could be repeated for all sorts of odd fields which are not necessarily used by every encoder. For encoding, you should allocate a context, and set the relevant properties manually (for audio, mostly sample rate and channel count). Everything else should remain empty. Its not intended for another context to be copied into an encoder context. Every field is documented who gets to write to it, and for extradata for encoding its the encoder, not the user, so if you write into it for an encoding context, you are using the API wrong. - Hendrik
On Tue, 6 Feb 2018 08:40:20 -0800 Josh Allmann <joshua.allmann@gmail.com> wrote: > On 6 February 2018 at 03:16, Rostislav Pehlivanov <atomnuker@gmail.com> > wrote: > > > On 6 February 2018 at 06:56, Josh Allmann <joshua.allmann@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.
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);