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

[Andrey Turkin]

> 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.
I missed that, thank you for pointing it out; so there is no memory leak
then. Whether it should be unrefed on codec close is a matter of opinion;
in my opinion avcodec_close shouldn't touch things that aren't created by
avcodec_open.
However I'm inclined to remove that portion of a patch and let someone else
decide if this behaviour should be changed.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[wm4]

On Fri, 13 May 2016 10:58:02 +0100
Mark Thompson  wrote:

> On 13/05/16 10:42, wm4 wrote:
> > On Fri, 13 May 2016 10:54:17 +0300
> > Andrey Turkin  wrote:
> >   
> >> 2016-05-13 10:35 GMT+03:00 wm4 :
> >>  
> >>> On Thu, 12 May 2016 22:35:48 +0300
> >>> Andrey Turkin  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(>intra_matrix);
>   av_freep(>inter_matrix);
>   av_freep(>rc_override);
>  +av_buffer_unref(>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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Andrey Turkin]

2016-05-13 10:35 GMT+03:00 wm4 :

> On Thu, 12 May 2016 22:35:48 +0300
> Andrey Turkin  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(>intra_matrix);
> >  av_freep(>inter_matrix);
> >  av_freep(>rc_override);
> > +av_buffer_unref(>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).
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[wm4]

On Thu, 12 May 2016 22:35:48 +0300
Andrey Turkin  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(>intra_matrix);
>  av_freep(>inter_matrix);
>  av_freep(>rc_override);
> +av_buffer_unref(>hw_frames_ctx);
>  
>  av_freep(pavctx);
>  }

I would have thought this is the responsibility of the API user?

> @@ -197,6 +198,7 @@ int avcodec_copy_context(AVCodecContext *dest, const 
> AVCodecContext *src)
>  av_freep(>inter_matrix);
>  av_freep(>extradata);
>  av_freep(>subtitle_header);
> +av_buffer_unref(>hw_frames_ctx);
>  
>  memcpy(dest, src, sizeof(*dest));
>  av_opt_copy(dest, src);
> @@ -225,6 +227,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>  dest->inter_matrix= NULL;
>  dest->rc_override = NULL;
>  dest->subtitle_header = NULL;
> +dest->hw_frames_ctx   = NULL;
>  
>  #define alloc_and_copy_or_fail(obj, size, pad) \
>  if (src->obj && size > 0) { \
> @@ -245,6 +248,12 @@ FF_ENABLE_DEPRECATION_WARNINGS
>  av_assert0(dest->subtitle_header_size == src->subtitle_header_size);
>  #undef alloc_and_copy_or_fail
>  
> +if (src->hw_frames_ctx) {
> +dest->hw_frames_ctx = av_buffer_ref(src->hw_frames_ctx);
> +if (!dest->hw_frames_ctx)
> +goto fail;
> +}
> +
>  return 0;
>  
>  fail:
> @@ -255,6 +264,7 @@ fail:
>  av_freep(>subtitle_header);
>  dest->subtitle_header_size = 0;
>  dest->extradata_size = 0;
> +av_buffer_unref(>hw_frames_ctx);
>  av_opt_free(dest);
>  return AVERROR(ENOMEM);
>  }

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel