Re: [FFmpeg-devel] [PATCH v3 08/12] lavc/frame_thread_encoder: avoid assigning a whole AVCodecContext

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

Anton Khirnov:
> It is highly unsafe, as AVCodecContext contains many allocated fields.
> Everything needed by worked threads should be covered by
> * routing through AVCodecParameters
> * av_opt_copy()
> * copying quantisation matrices manually
> 
> avcodec_free_context() can now be used for per-thread contexts.

So now we have to maintain a list of stuff to be copied just to avoid
this "unsafe" copying. Sounds worse to me.
There is another usecase you are breaking: The execute-callbacks. The
user may have used custom ones that use a thread pool which would allow
using both slice- and frame-threading together.
And you also forgot to copy get_encode_buffer.

> ---
>  libavcodec/frame_thread_encoder.c | 48 ++++++++++++++++++++++++-------
>  1 file changed, 38 insertions(+), 10 deletions(-)
> 
> diff --git a/libavcodec/frame_thread_encoder.c b/libavcodec/frame_thread_encoder.c
> index cda5158117..d34fe022a5 100644
> --- a/libavcodec/frame_thread_encoder.c
> +++ b/libavcodec/frame_thread_encoder.c
> @@ -28,6 +28,7 @@
>  #include "libavutil/thread.h"
>  #include "avcodec.h"
>  #include "avcodec_internal.h"
> +#include "codec_par.h"
>  #include "encode.h"
>  #include "internal.h"
>  #include "pthread_internal.h"
> @@ -111,8 +112,7 @@ static void * attribute_align_arg worker(void *v){
>          pthread_mutex_unlock(&c->finished_task_mutex);
>      }
>  end:
> -    ff_codec_close(avctx);
> -    av_freep(&avctx);
> +    avcodec_free_context(&avctx);
>      return NULL;
>  }
>  
> @@ -121,6 +121,7 @@ av_cold int ff_frame_thread_encoder_init(AVCodecContext *avctx)
>      int i=0;
>      ThreadContext *c;
>      AVCodecContext *thread_avctx = NULL;
> +    AVCodecParameters *par = NULL;
>      int ret;
>  
>      if(   !(avctx->thread_type & FF_THREAD_FRAME)
> @@ -194,18 +195,27 @@ av_cold int ff_frame_thread_encoder_init(AVCodecContext *avctx)
>          }
>      }
>  
> +    par = avcodec_parameters_alloc();
> +    if (!par) {
> +        ret = AVERROR(ENOMEM);
> +        goto fail;
> +    }
> +
> +    ret = avcodec_parameters_from_context(par, avctx);
> +    if (ret < 0)
> +        goto fail;
> +
>      for(i=0; i<avctx->thread_count ; i++){
> -        void *tmpv;
>          thread_avctx = avcodec_alloc_context3(avctx->codec);
>          if (!thread_avctx) {
>              ret = AVERROR(ENOMEM);
>              goto fail;
>          }
> -        tmpv = thread_avctx->priv_data;
> -        *thread_avctx = *avctx;
> -        thread_avctx->priv_data = tmpv;
> -        thread_avctx->internal = NULL;
> -        thread_avctx->hw_frames_ctx = NULL;
> +
> +        ret = avcodec_parameters_to_context(thread_avctx, par);
> +        if (ret < 0)
> +            goto fail;
> +
>          ret = av_opt_copy(thread_avctx, avctx);
>          if (ret < 0)
>              goto fail;
> @@ -217,6 +227,22 @@ av_cold int ff_frame_thread_encoder_init(AVCodecContext *avctx)
>          thread_avctx->thread_count = 1;
>          thread_avctx->active_thread_type &= ~FF_THREAD_FRAME;
>  
> +#define DUP_MATRIX(m)                                                       \
> +        if (avctx->m) {                                                     \
> +            thread_avctx->m = av_memdup(avctx->m, 64 * sizeof(*avctx->m));  \
> +            if (!thread_avctx->m) {                                         \
> +                ret = AVERROR(ENOMEM);                                      \
> +                goto fail;                                                  \
> +            }                                                               \
> +        }
> +        DUP_MATRIX(intra_matrix);
> +        DUP_MATRIX(chroma_intra_matrix);
> +        DUP_MATRIX(inter_matrix);
> +
> +#undef DUP_MATRIX
> +
> +        thread_avctx->stats_in = avctx->stats_in;
> +
>          if ((ret = avcodec_open2(thread_avctx, avctx->codec, NULL)) < 0)
>              goto fail;
>          av_assert0(!thread_avctx->internal->frame_thread_encoder);
> @@ -227,12 +253,14 @@ av_cold int ff_frame_thread_encoder_init(AVCodecContext *avctx)
>          }
>      }
>  
> +    avcodec_parameters_free(&par);
> +
>      avctx->active_thread_type = FF_THREAD_FRAME;
>  
>      return 0;
>  fail:
> -    ff_codec_close(thread_avctx);
> -    av_freep(&thread_avctx);
> +    avcodec_parameters_free(&par);
> +    avcodec_free_context(&thread_avctx);
>      avctx->thread_count = i;
>      av_log(avctx, AV_LOG_ERROR, "ff_frame_thread_encoder_init failed\n");
>      ff_frame_thread_encoder_free(avctx);

_______________________________________________
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".