[FFmpeg-devel] [PATCH] libavcodec/options.c: handle hw_frames_ctx where necessary

[wm4]

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.