[FFmpeg-devel] [PATCH] libavcodec/options.c: handle hw_frames_ctx where necessary
wm4
nfxjfg at googlemail.com
Fri May 13 12:06:58 CEST 2016
On Fri, 13 May 2016 10:58:02 +0100
Mark Thompson <sw at jkqxz.net> wrote:
> On 13/05/16 10:42, wm4 wrote:
> > On Fri, 13 May 2016 10:54:17 +0300
> > Andrey Turkin <andrey.turkin at gmail.com> wrote:
> >
> >> 2016-05-13 10:35 GMT+03:00 wm4 <nfxjfg at googlemail.com>:
> >>
> >>> On Thu, 12 May 2016 22:35:48 +0300
> >>> Andrey Turkin <andrey.turkin at gmail.com> wrote:
> >>>
> >>>> Few functions didn't handle hw_frames_ctx references causing resources
> >>> leaks and even crashes.
> >>>> ---
> >>>> libavcodec/options.c | 10 ++++++++++
> >>>> 1 file changed, 10 insertions(+)
> >>>>
> >>>> diff --git a/libavcodec/options.c b/libavcodec/options.c
> >>>> index ea2563b..8682262 100644
> >>>> --- a/libavcodec/options.c
> >>>> +++ b/libavcodec/options.c
> >>>> @@ -175,6 +175,7 @@ void avcodec_free_context(AVCodecContext **pavctx)
> >>>> av_freep(&avctx->intra_matrix);
> >>>> av_freep(&avctx->inter_matrix);
> >>>> av_freep(&avctx->rc_override);
> >>>> + av_buffer_unref(&avctx->hw_frames_ctx);
> >>>>
> >>>> av_freep(pavctx);
> >>>> }
> >>>
> >>> I would have thought this is the responsibility of the API user?
> >>>
> >>>
> >> AVCodecContext documentation says it is set by a user but then managed and
> >> owned by libavcodec (which is a logical thing to do for any shared
> >> reference).
> >
> > Even so it's a breaking API change and should be treated as such. An
> > API user could for example have a separate variable with the same
> > buffer ref somewhere, which would lead to a double-free. Even more so
> > because an API user might have noticed the leak, and concluded the ref
> > must be released manually.
> >
> > Since this looks like an unintentional bug and there's no release with
> > it included yet, we can probably skip major jumps. But it should still
> > be mentioned in APIchanges, and be sent to Libav too.
>
> No: this part of the patch does nothing because it's already unreffed in the
> avcodec_close() called immediately above. Maybe the unref should actually be in
> avcodec_free_context() only (if treating it like rc_override and those fields),
> but it shouldn't be in both.
Oh I see. You could argue that avodec_open() is the thing which takes
over ownership. But the real sketchy thing is that avcodec_close()
unrefs it even if the context was not opened.
This is all such an inconsistent mess.
More information about the ffmpeg-devel
mailing list