diff mbox

[FFmpeg-devel,1/2] avcodec/options: factorize avcodec_copy_context cleanup code

Message ID 20170422172609.5292-1-jamrial@gmail.com
State Accepted
Commit 54a4c9b4e9a1524b1ac5d2be97c8042272402d0a
Headers show

Commit Message

James Almer April 22, 2017, 5:26 p.m. UTC
Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/options.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

Comments

Aaron Levinson April 23, 2017, 5:12 a.m. UTC | #1
On 4/22/2017 10:26 AM, James Almer wrote:
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/options.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/libavcodec/options.c b/libavcodec/options.c
> index 7bdb0be5af..b98da9378a 100644
> --- a/libavcodec/options.c
> +++ b/libavcodec/options.c
> @@ -188,6 +188,19 @@ void avcodec_free_context(AVCodecContext **pavctx)
>  }
>
>  #if FF_API_COPY_CONTEXT
> +static void copy_context_reset(AVCodecContext *avctx)
> +{
> +    av_opt_free(avctx);
> +    av_freep(&avctx->rc_override);
> +    av_freep(&avctx->intra_matrix);
> +    av_freep(&avctx->inter_matrix);
> +    av_freep(&avctx->extradata);
> +    av_freep(&avctx->subtitle_header);
> +    av_buffer_unref(&avctx->hw_frames_ctx);
> +    avctx->subtitle_header_size = 0;
> +    avctx->extradata_size = 0;
> +}
> +
>  int avcodec_copy_context(AVCodecContext *dest, const AVCodecContext *src)
>  {
>      const AVCodec *orig_codec = dest->codec;
> @@ -200,12 +213,7 @@ int avcodec_copy_context(AVCodecContext *dest, const AVCodecContext *src)
>          return AVERROR(EINVAL);
>      }
>
> -    av_opt_free(dest);
> -    av_freep(&dest->rc_override);
> -    av_freep(&dest->intra_matrix);
> -    av_freep(&dest->inter_matrix);
> -    av_freep(&dest->extradata);
> -    av_freep(&dest->subtitle_header);
> +    copy_context_reset(dest);
>
>      memcpy(dest, src, sizeof(*dest));
>      av_opt_copy(dest, src);
> @@ -264,15 +272,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>      return 0;
>
>  fail:
> -    av_freep(&dest->subtitle_header);
> -    av_freep(&dest->rc_override);
> -    av_freep(&dest->intra_matrix);
> -    av_freep(&dest->inter_matrix);
> -    av_freep(&dest->extradata);
> -    av_buffer_unref(&dest->hw_frames_ctx);
> -    dest->subtitle_header_size = 0;
> -    dest->extradata_size = 0;
> -    av_opt_free(dest);
> +    copy_context_reset(dest);
>      return AVERROR(ENOMEM);
>  }
>  #endif
>

There is one real difference in the behavior--it will call 
av_buffer_unref(&dest->hw_frames_ctx) the first time around, and this 
wasn't done before.  The initialization of subtitle_header_size and 
extradata_size to 0 doesn't really matter the first time around, since 
they will be overwritten anyway as a result of using memcpy(), but the 
use of av_buffer_unref() does technically change the behavior compared 
to what it used to do.  It is likely correct to do that the first time 
through, but I wonder, why not also do 
av_buffer_unref(&avctx->hw_device_ctx) and a bunch of the other things 
that avcodec_close() already does?  The call to memcpy() will basically 
overwrite dest with the values in src, and besides the copies made of 
dest->codec and dest->priv_data earlier in the function, anything else 
that is overwritten could in theory leak.  But, I guess that would have 
been a preexisting issue with this function and not really relevant for 
this patch.  And this function is deprecated anyway.

If there is no issue with the additional call to av_buffer_unref(), as 
mentioned above, this patch looks good.

Aaron Levinson
diff mbox

Patch

diff --git a/libavcodec/options.c b/libavcodec/options.c
index 7bdb0be5af..b98da9378a 100644
--- a/libavcodec/options.c
+++ b/libavcodec/options.c
@@ -188,6 +188,19 @@  void avcodec_free_context(AVCodecContext **pavctx)
 }
 
 #if FF_API_COPY_CONTEXT
+static void copy_context_reset(AVCodecContext *avctx)
+{
+    av_opt_free(avctx);
+    av_freep(&avctx->rc_override);
+    av_freep(&avctx->intra_matrix);
+    av_freep(&avctx->inter_matrix);
+    av_freep(&avctx->extradata);
+    av_freep(&avctx->subtitle_header);
+    av_buffer_unref(&avctx->hw_frames_ctx);
+    avctx->subtitle_header_size = 0;
+    avctx->extradata_size = 0;
+}
+
 int avcodec_copy_context(AVCodecContext *dest, const AVCodecContext *src)
 {
     const AVCodec *orig_codec = dest->codec;
@@ -200,12 +213,7 @@  int avcodec_copy_context(AVCodecContext *dest, const AVCodecContext *src)
         return AVERROR(EINVAL);
     }
 
-    av_opt_free(dest);
-    av_freep(&dest->rc_override);
-    av_freep(&dest->intra_matrix);
-    av_freep(&dest->inter_matrix);
-    av_freep(&dest->extradata);
-    av_freep(&dest->subtitle_header);
+    copy_context_reset(dest);
 
     memcpy(dest, src, sizeof(*dest));
     av_opt_copy(dest, src);
@@ -264,15 +272,7 @@  FF_ENABLE_DEPRECATION_WARNINGS
     return 0;
 
 fail:
-    av_freep(&dest->subtitle_header);
-    av_freep(&dest->rc_override);
-    av_freep(&dest->intra_matrix);
-    av_freep(&dest->inter_matrix);
-    av_freep(&dest->extradata);
-    av_buffer_unref(&dest->hw_frames_ctx);
-    dest->subtitle_header_size = 0;
-    dest->extradata_size = 0;
-    av_opt_free(dest);
+    copy_context_reset(dest);
     return AVERROR(ENOMEM);
 }
 #endif