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

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

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);

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".