diff mbox series

[FFmpeg-devel,v0,02/14] avcodec: move AVCodecInternal allocation to avcodec_alloc_context3

Message ID 20230320233408.134255-3-jeebjp@gmail.com
State New
Headers show
Series encoder AVCodecContext configuration side data | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Jan Ekström March 20, 2023, 11:33 p.m. UTC 
This allows for private values to be stored before the {de,en}coder
has been opened and initialized.

Add a new unsigned boolean entry to specifically note that a
context has been opened instead of just depending on the internal
pointer.
---
 libavcodec/avcodec.c              | 18 +++++++++++-------
 libavcodec/frame_thread_encoder.c |  7 ++++---
 libavcodec/internal.h             |  5 +++++
 libavcodec/options.c              | 20 +++++++++++++++++++-
 4 files changed, 39 insertions(+), 11 deletions(-)

Comments

Anton Khirnov March 24, 2023, 10:41 a.m. UTC | #1 
Quoting Jan Ekström (2023-03-21 00:33:56)
> This allows for private values to be stored before the {de,en}coder
> has been opened and initialized.
> 
> Add a new unsigned boolean entry to specifically note that a
> context has been opened instead of just depending on the internal
> pointer.
> ---
>  libavcodec/avcodec.c              | 18 +++++++++++-------
>  libavcodec/frame_thread_encoder.c |  7 ++++---
>  libavcodec/internal.h             |  5 +++++
>  libavcodec/options.c              | 20 +++++++++++++++++++-
>  4 files changed, 39 insertions(+), 11 deletions(-)
> 
> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
> index fb1362290f..c110b19e08 100644
> --- a/libavcodec/avcodec.c
> +++ b/libavcodec/avcodec.c
> @@ -115,7 +115,7 @@ static int64_t get_bit_rate(AVCodecContext *ctx)
>  int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *codec, AVDictionary **options)
>  {
>      int ret = 0;
> -    AVCodecInternal *avci;
> +    AVCodecInternal *avci = NULL;

Just initialize it to avctx->avci.

>      const FFCodec *codec2;
>  
>      if (avcodec_is_open(avctx))
> @@ -147,12 +147,13 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code
>      if (avctx->extradata_size < 0 || avctx->extradata_size >= FF_MAX_EXTRADATA_SIZE)
>          return AVERROR(EINVAL);
>  
> -    avci = av_mallocz(sizeof(*avci));
> +    avci = avctx->internal;
>      if (!avci) {
> -        ret = AVERROR(ENOMEM);
> -        goto end;
> +        av_log(avctx, AV_LOG_ERROR,
> +               "This AVCodecContext was not properly allocated! Please utilize "
> +               "avcodec_alloc_context3!\n");
> +        return AVERROR(EINVAL);

This should be an assert.

> @@ -147,7 +149,21 @@ FF_ENABLE_DEPRECATION_WARNINGS
>              d++;
>          }
>      }
> +
> +    avci = av_mallocz(sizeof(*avci));
> +    if (!avci)
> +        goto alloc_fail;
> +
> +    s->internal = avci;
> +
>      return 0;
> +
> +alloc_fail:
> +    av_freep(&s->internal);
> +
> +    av_freep(&s->priv_data);
> +
> +    return AVERROR(ENOMEM);

A bit overdoing it with empty lines.

Otherwise looks ok.
Andreas Rheinhardt March 24, 2023, 12:07 p.m. UTC | #2 
Jan Ekström:
> This allows for private values to be stored before the {de,en}coder
> has been opened and initialized.
> 
> Add a new unsigned boolean entry to specifically note that a
> context has been opened instead of just depending on the internal
> pointer.
> ---
>  libavcodec/avcodec.c              | 18 +++++++++++-------
>  libavcodec/frame_thread_encoder.c |  7 ++++---
>  libavcodec/internal.h             |  5 +++++
>  libavcodec/options.c              | 20 +++++++++++++++++++-
>  4 files changed, 39 insertions(+), 11 deletions(-)
> 
> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
> index fb1362290f..c110b19e08 100644
> --- a/libavcodec/avcodec.c
> +++ b/libavcodec/avcodec.c
> @@ -115,7 +115,7 @@ static int64_t get_bit_rate(AVCodecContext *ctx)
>  int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *codec, AVDictionary **options)
>  {
>      int ret = 0;
> -    AVCodecInternal *avci;
> +    AVCodecInternal *avci = NULL;
>      const FFCodec *codec2;
>  
>      if (avcodec_is_open(avctx))
> @@ -147,12 +147,13 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code
>      if (avctx->extradata_size < 0 || avctx->extradata_size >= FF_MAX_EXTRADATA_SIZE)
>          return AVERROR(EINVAL);
>  
> -    avci = av_mallocz(sizeof(*avci));
> +    avci = avctx->internal;
>      if (!avci) {
> -        ret = AVERROR(ENOMEM);
> -        goto end;
> +        av_log(avctx, AV_LOG_ERROR,
> +               "This AVCodecContext was not properly allocated! Please utilize "
> +               "avcodec_alloc_context3!\n");
> +        return AVERROR(EINVAL);
>      }
> -    avctx->internal = avci;
>  
>      avci->buffer_frame = av_frame_alloc();
>      avci->buffer_pkt = av_packet_alloc();
> @@ -360,6 +361,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
>  
>  end:
>  
> +    if (ret >= 0 && avci)
> +        avci->ctx_opened = 1;

This should be moved before end: in order to save the checks.

> +
>      return ret;
>  free_and_end:
>      avcodec_close(avctx);
> @@ -470,7 +474,7 @@ av_cold int avcodec_close(AVCodecContext *avctx)
>          ff_icc_context_uninit(&avci->icc);
>  #endif
>  
> -        av_freep(&avctx->internal);
> +        avci->ctx_opened = 0;
>      }
>  
>      for (i = 0; i < avctx->nb_coded_side_data; i++)
> @@ -703,7 +707,7 @@ void avcodec_string(char *buf, int buf_size, AVCodecContext *enc, int encode)
>  
>  int avcodec_is_open(AVCodecContext *s)
>  {
> -    return !!s->internal;
> +    return s->internal && s->internal->ctx_opened;
>  }
>  
>  int attribute_align_arg avcodec_receive_frame(AVCodecContext *avctx, AVFrame *frame)
> diff --git a/libavcodec/frame_thread_encoder.c b/libavcodec/frame_thread_encoder.c
> index 62d9580ad4..683ba52608 100644
> --- a/libavcodec/frame_thread_encoder.c
> +++ b/libavcodec/frame_thread_encoder.c
> @@ -110,8 +110,7 @@ static void * attribute_align_arg worker(void *v){
>          pthread_mutex_unlock(&c->finished_task_mutex);
>      }
>  end:
> -    avcodec_close(avctx);
> -    av_freep(&avctx);
> +    avcodec_free_context(&avctx);

This will also free the other stuff in avcodec_free_context() and might
therefore cause double-frees.

>      return NULL;
>  }
>  
> @@ -195,15 +194,17 @@ av_cold int ff_frame_thread_encoder_init(AVCodecContext *avctx)
>  
>      for(i=0; i<avctx->thread_count ; i++){
>          void *tmpv;
> +        AVCodecInternal *avci;
>          thread_avctx = avcodec_alloc_context3(avctx->codec);
>          if (!thread_avctx) {
>              ret = AVERROR(ENOMEM);
>              goto fail;
>          }
>          tmpv = thread_avctx->priv_data;
> +        avci = thread_avctx->internal;
>          *thread_avctx = *avctx;
>          thread_avctx->priv_data = tmpv;
> -        thread_avctx->internal = NULL;
> +        thread_avctx->internal = avci;
>          thread_avctx->hw_frames_ctx = NULL;
>          ret = av_opt_copy(thread_avctx, avctx);
>          if (ret < 0)
> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> index a283c52e01..f21101752d 100644
> --- a/libavcodec/internal.h
> +++ b/libavcodec/internal.h
> @@ -163,6 +163,11 @@ typedef struct AVCodecInternal {
>  #if CONFIG_LCMS2
>      FFIccContext icc; /* used to read and write embedded ICC profiles */
>  #endif
> +
> +    /**
> +     * a boolean to describe whether context is opened or not.
> +     */
> +    unsigned int ctx_opened;
>  } AVCodecInternal;
>  
>  /**
> diff --git a/libavcodec/options.c b/libavcodec/options.c
> index a9b35ee1c3..f8fab164fb 100644
> --- a/libavcodec/options.c
> +++ b/libavcodec/options.c
> @@ -28,6 +28,7 @@
>  
>  #include "avcodec.h"
>  #include "codec_internal.h"
> +#include "internal.h"
>  #include "libavutil/avassert.h"
>  #include "libavutil/internal.h"
>  #include "libavutil/mem.h"
> @@ -89,6 +90,7 @@ static const AVClass av_codec_context_class = {
>  static int init_context_defaults(AVCodecContext *s, const AVCodec *codec)
>  {
>      const FFCodec *const codec2 = ffcodec(codec);
> +    AVCodecInternal *avci = NULL;
>      int flags=0;
>      memset(s, 0, sizeof(AVCodecContext));
>  
> @@ -132,7 +134,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>      if(codec && codec2->priv_data_size){
>          s->priv_data = av_mallocz(codec2->priv_data_size);
>          if (!s->priv_data)
> -            return AVERROR(ENOMEM);
> +            goto alloc_fail;
>          if(codec->priv_class){
>              *(const AVClass**)s->priv_data = codec->priv_class;
>              av_opt_set_defaults(s->priv_data);
> @@ -147,7 +149,21 @@ FF_ENABLE_DEPRECATION_WARNINGS
>              d++;
>          }
>      }
> +
> +    avci = av_mallocz(sizeof(*avci));
> +    if (!avci)
> +        goto alloc_fail;
> +
> +    s->internal = avci;
> +
>      return 0;
> +
> +alloc_fail:
> +    av_freep(&s->internal);
> +
> +    av_freep(&s->priv_data);
> +
> +    return AVERROR(ENOMEM);
>  }
>  
>  AVCodecContext *avcodec_alloc_context3(const AVCodec *codec)
> @@ -174,6 +190,8 @@ void avcodec_free_context(AVCodecContext **pavctx)
>  
>      avcodec_close(avctx);
>  
> +    av_freep(&avctx->internal);

Moving this to avcodec_free_context() creates a leak when using
avcodec_close()+av_free(). This can be fixed by allocating the
AVCodecContext and the AVCodecInternal jointly.

> +
>      av_freep(&avctx->extradata);
>      av_freep(&avctx->subtitle_header);
>      av_freep(&avctx->intra_matrix);
James Almer March 24, 2023, 1:02 p.m. UTC | #3 
On 3/24/2023 9:07 AM, Andreas Rheinhardt wrote:
>> @@ -174,6 +190,8 @@ void avcodec_free_context(AVCodecContext **pavctx)
>>   
>>       avcodec_close(avctx);
>>   
>> +    av_freep(&avctx->internal);
> Moving this to avcodec_free_context() creates a leak when using
> avcodec_close()+av_free(). This can be fixed by allocating the
> AVCodecContext and the AVCodecInternal jointly.

Can't we just declare that doing av_free() on a AVCodecContext is not a 
valid API usage? Every other struct with an specific free function is 
very clear about it being the only way to free them.

> 
>> +
>>       av_freep(&avctx->extradata);
>>       av_freep(&avctx->subtitle_header);
>>       av_freep(&avctx->intra_matrix);
Anton Khirnov March 24, 2023, 1:20 p.m. UTC | #4 
Quoting James Almer (2023-03-24 14:02:40)
> On 3/24/2023 9:07 AM, Andreas Rheinhardt wrote:
> >> @@ -174,6 +190,8 @@ void avcodec_free_context(AVCodecContext **pavctx)
> >>   
> >>       avcodec_close(avctx);
> >>   
> >> +    av_freep(&avctx->internal);
> > Moving this to avcodec_free_context() creates a leak when using
> > avcodec_close()+av_free(). This can be fixed by allocating the
> > AVCodecContext and the AVCodecInternal jointly.
> 
> Can't we just declare that doing av_free() on a AVCodecContext is not a 
> valid API usage? Every other struct with an specific free function is 
> very clear about it being the only way to free them.

Sadly I expect many callers still do this, even though
avcodec_free_context() has existed since 2014. The proper solution is to
deprecated avcodec_close(), but that needs a new parser API.
Michael Niedermayer March 24, 2023, 5:23 p.m. UTC | #5 
On Tue, Mar 21, 2023 at 01:33:56AM +0200, Jan Ekström wrote:
> This allows for private values to be stored before the {de,en}coder
> has been opened and initialized.
> 
> Add a new unsigned boolean entry to specifically note that a
> context has been opened instead of just depending on the internal
> pointer.
> ---
>  libavcodec/avcodec.c              | 18 +++++++++++-------
>  libavcodec/frame_thread_encoder.c |  7 ++++---
>  libavcodec/internal.h             |  5 +++++
>  libavcodec/options.c              | 20 +++++++++++++++++++-
>  4 files changed, 39 insertions(+), 11 deletions(-)

this causes memory corruption with mjpeg
./ffmpeg_g -i lena.pnm -qscale 4 -intra_matrix 10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10 -chroma_intra_matrix 400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400 -bitexact file-custommatrix10,400.jpg

i see

video:19kB audio:0kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: unknown
corrupted size vs. prev_size
Aborted (core dumped)

i can provide more details if it isnt reproduceable

thx

[...]
Andreas Rheinhardt March 24, 2023, 5:26 p.m. UTC | #6 
Michael Niedermayer:
> On Tue, Mar 21, 2023 at 01:33:56AM +0200, Jan Ekström wrote:
>> This allows for private values to be stored before the {de,en}coder
>> has been opened and initialized.
>>
>> Add a new unsigned boolean entry to specifically note that a
>> context has been opened instead of just depending on the internal
>> pointer.
>> ---
>>  libavcodec/avcodec.c              | 18 +++++++++++-------
>>  libavcodec/frame_thread_encoder.c |  7 ++++---
>>  libavcodec/internal.h             |  5 +++++
>>  libavcodec/options.c              | 20 +++++++++++++++++++-
>>  4 files changed, 39 insertions(+), 11 deletions(-)
> 
> this causes memory corruption with mjpeg
> ./ffmpeg_g -i lena.pnm -qscale 4 -intra_matrix 10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10 -chroma_intra_matrix 400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400 -bitexact file-custommatrix10,400.jpg
> 
> i see
> 
> video:19kB audio:0kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: unknown
> corrupted size vs. prev_size
> Aborted (core dumped)
> 
> i can provide more details if it isnt reproduceable
> 

This is an example of the double-frees due to using
avcodec_free_context() to free the worker threads that I mentioned in my
reply.

- Andreas
diff mbox series

Patch

diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
index fb1362290f..c110b19e08 100644
--- a/libavcodec/avcodec.c
+++ b/libavcodec/avcodec.c
@@ -115,7 +115,7 @@  static int64_t get_bit_rate(AVCodecContext *ctx)
 int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *codec, AVDictionary **options)
 {
     int ret = 0;
-    AVCodecInternal *avci;
+    AVCodecInternal *avci = NULL;
     const FFCodec *codec2;
 
     if (avcodec_is_open(avctx))
@@ -147,12 +147,13 @@  int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code
     if (avctx->extradata_size < 0 || avctx->extradata_size >= FF_MAX_EXTRADATA_SIZE)
         return AVERROR(EINVAL);
 
-    avci = av_mallocz(sizeof(*avci));
+    avci = avctx->internal;
     if (!avci) {
-        ret = AVERROR(ENOMEM);
-        goto end;
+        av_log(avctx, AV_LOG_ERROR,
+               "This AVCodecContext was not properly allocated! Please utilize "
+               "avcodec_alloc_context3!\n");
+        return AVERROR(EINVAL);
     }
-    avctx->internal = avci;
 
     avci->buffer_frame = av_frame_alloc();
     avci->buffer_pkt = av_packet_alloc();
@@ -360,6 +361,9 @@  FF_ENABLE_DEPRECATION_WARNINGS
 
 end:
 
+    if (ret >= 0 && avci)
+        avci->ctx_opened = 1;
+
     return ret;
 free_and_end:
     avcodec_close(avctx);
@@ -470,7 +474,7 @@  av_cold int avcodec_close(AVCodecContext *avctx)
         ff_icc_context_uninit(&avci->icc);
 #endif
 
-        av_freep(&avctx->internal);
+        avci->ctx_opened = 0;
     }
 
     for (i = 0; i < avctx->nb_coded_side_data; i++)
@@ -703,7 +707,7 @@  void avcodec_string(char *buf, int buf_size, AVCodecContext *enc, int encode)
 
 int avcodec_is_open(AVCodecContext *s)
 {
-    return !!s->internal;
+    return s->internal && s->internal->ctx_opened;
 }
 
 int attribute_align_arg avcodec_receive_frame(AVCodecContext *avctx, AVFrame *frame)
diff --git a/libavcodec/frame_thread_encoder.c b/libavcodec/frame_thread_encoder.c
index 62d9580ad4..683ba52608 100644
--- a/libavcodec/frame_thread_encoder.c
+++ b/libavcodec/frame_thread_encoder.c
@@ -110,8 +110,7 @@  static void * attribute_align_arg worker(void *v){
         pthread_mutex_unlock(&c->finished_task_mutex);
     }
 end:
-    avcodec_close(avctx);
-    av_freep(&avctx);
+    avcodec_free_context(&avctx);
     return NULL;
 }
 
@@ -195,15 +194,17 @@  av_cold int ff_frame_thread_encoder_init(AVCodecContext *avctx)
 
     for(i=0; i<avctx->thread_count ; i++){
         void *tmpv;
+        AVCodecInternal *avci;
         thread_avctx = avcodec_alloc_context3(avctx->codec);
         if (!thread_avctx) {
             ret = AVERROR(ENOMEM);
             goto fail;
         }
         tmpv = thread_avctx->priv_data;
+        avci = thread_avctx->internal;
         *thread_avctx = *avctx;
         thread_avctx->priv_data = tmpv;
-        thread_avctx->internal = NULL;
+        thread_avctx->internal = avci;
         thread_avctx->hw_frames_ctx = NULL;
         ret = av_opt_copy(thread_avctx, avctx);
         if (ret < 0)
diff --git a/libavcodec/internal.h b/libavcodec/internal.h
index a283c52e01..f21101752d 100644
--- a/libavcodec/internal.h
+++ b/libavcodec/internal.h
@@ -163,6 +163,11 @@  typedef struct AVCodecInternal {
 #if CONFIG_LCMS2
     FFIccContext icc; /* used to read and write embedded ICC profiles */
 #endif
+
+    /**
+     * a boolean to describe whether context is opened or not.
+     */
+    unsigned int ctx_opened;
 } AVCodecInternal;
 
 /**
diff --git a/libavcodec/options.c b/libavcodec/options.c
index a9b35ee1c3..f8fab164fb 100644
--- a/libavcodec/options.c
+++ b/libavcodec/options.c
@@ -28,6 +28,7 @@ 
 
 #include "avcodec.h"
 #include "codec_internal.h"
+#include "internal.h"
 #include "libavutil/avassert.h"
 #include "libavutil/internal.h"
 #include "libavutil/mem.h"
@@ -89,6 +90,7 @@  static const AVClass av_codec_context_class = {
 static int init_context_defaults(AVCodecContext *s, const AVCodec *codec)
 {
     const FFCodec *const codec2 = ffcodec(codec);
+    AVCodecInternal *avci = NULL;
     int flags=0;
     memset(s, 0, sizeof(AVCodecContext));
 
@@ -132,7 +134,7 @@  FF_ENABLE_DEPRECATION_WARNINGS
     if(codec && codec2->priv_data_size){
         s->priv_data = av_mallocz(codec2->priv_data_size);
         if (!s->priv_data)
-            return AVERROR(ENOMEM);
+            goto alloc_fail;
         if(codec->priv_class){
             *(const AVClass**)s->priv_data = codec->priv_class;
             av_opt_set_defaults(s->priv_data);
@@ -147,7 +149,21 @@  FF_ENABLE_DEPRECATION_WARNINGS
             d++;
         }
     }
+
+    avci = av_mallocz(sizeof(*avci));
+    if (!avci)
+        goto alloc_fail;
+
+    s->internal = avci;
+
     return 0;
+
+alloc_fail:
+    av_freep(&s->internal);
+
+    av_freep(&s->priv_data);
+
+    return AVERROR(ENOMEM);
 }
 
 AVCodecContext *avcodec_alloc_context3(const AVCodec *codec)
@@ -174,6 +190,8 @@  void avcodec_free_context(AVCodecContext **pavctx)
 
     avcodec_close(avctx);
 
+    av_freep(&avctx->internal);
+
     av_freep(&avctx->extradata);
     av_freep(&avctx->subtitle_header);
     av_freep(&avctx->intra_matrix);