| Message ID | 20240324092917.12874-2-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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);
Quoting Andreas Rheinhardt (2024-03-24 12:25:53) > 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. We have to maintain a list of stuff either way, the difference is that with my patch forgetting an item may result in some features not working, while with current code forgetting an item will typically cause a double free.
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);