[FFmpeg-devel] [PATCH] libavutil/hwcontext_qsv: Make qsv hardware transfers thread safe
Xiang, Haihao
haihao.xiang at intel.com
Wed Apr 17 11:15:41 EEST 2024
On Sa, 2024-04-13 at 07:57 -0500, Mark Samuelson wrote:
> The QSV hardware context currently uses pthreads to lock initilization,
> which is not available on windows builds. Instead, use the AVMutex
> object. Also lock uses of the realigned_upload_frame and
> realigned_download_frame objects, so multiple threads do not attempt
> to write to them at the same time.
> ---
> libavutil/hwcontext_qsv.c | 75 ++++++++++++++++++++++++---------------
> 1 file changed, 46 insertions(+), 29 deletions(-)
>
> diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> index c7c7878644..92bab134e4 100644
> --- a/libavutil/hwcontext_qsv.c
> +++ b/libavutil/hwcontext_qsv.c
> @@ -23,10 +23,7 @@
> #include <mfxvideo.h>
>
> #include "config.h"
> -
> -#if HAVE_PTHREADS
> -#include <pthread.h>
> -#endif
> +#include "thread.h"
>
> #define COBJMACROS
> #if CONFIG_VAAPI
> @@ -98,9 +95,7 @@ typedef struct QSVFramesContext {
> atomic_int session_download_init;
> mfxSession session_upload;
> atomic_int session_upload_init;
> -#if HAVE_PTHREADS
> - pthread_mutex_t session_lock;
> -#endif
> + AVMutex session_lock;
>
> AVBufferRef *child_frames_ref;
> mfxFrameSurface1 *surfaces_internal;
> @@ -354,9 +349,7 @@ static void qsv_frames_uninit(AVHWFramesContext *ctx)
> s->session_upload = NULL;
> s->session_upload_init = 0;
>
> -#if HAVE_PTHREADS
> - pthread_mutex_destroy(&s->session_lock);
> -#endif
> + ff_mutex_destroy(&s->session_lock);
>
> av_freep(&s->mem_ids);
> #if QSV_HAVE_OPAQUE
> @@ -1302,9 +1295,7 @@ static int qsv_frames_init(AVHWFramesContext *ctx)
> s->session_download_init = 0;
> s->session_upload_init = 0;
>
> -#if HAVE_PTHREADS
> - pthread_mutex_init(&s->session_lock, NULL);
> -#endif
> + ff_mutex_init(&s->session_lock, NULL);
>
> return 0;
> }
> @@ -1629,24 +1620,20 @@ static int
> qsv_internal_session_check_init(AVHWFramesContext *ctx, int upload)
> if (atomic_load(inited))
> return 0;
>
> -#if HAVE_PTHREADS
> - pthread_mutex_lock(&s->session_lock);
> -#endif
> + ff_mutex_lock(&s->session_lock);
ff_mutex_lock is called twice in the same thread when realigned is 1, the
current thread is blocked. You may run the command below to reproduce this
issue:
./ffmpeg -init_hw_device qsv -f lavfi -i testsrc=size=352x280,format=nv12 -vf
"hwupload=extra_hw_frames=16" -f null -
>
> if (!atomic_load(inited)) {
> ret = qsv_init_internal_session(ctx, session, upload);
> atomic_store(inited, 1);
> }
>
> -#if HAVE_PTHREADS
> - pthread_mutex_unlock(&s->session_lock);
> -#endif
> + ff_mutex_unlock(&s->session_lock);
>
> return ret;
> }
>
> -static int qsv_transfer_data_from(AVHWFramesContext *ctx, AVFrame *dst,
> - const AVFrame *src)
> +static int qsv_transfer_data_from_internal(AVHWFramesContext *ctx, AVFrame
> *dst,
> + const AVFrame *src, int realigned)
> {
> QSVFramesContext *s = ctx->hwctx;
> mfxFrameSurface1 out = {{ 0 }};
> @@ -1658,7 +1645,6 @@ static int qsv_transfer_data_from(AVHWFramesContext
> *ctx, AVFrame *dst,
> /* download to temp frame if the output is not padded as libmfx requires
> */
> AVFrame *tmp_frame = &s->realigned_download_frame;
> AVFrame *dst_frame;
> - int realigned = 0;
>
> ret = qsv_internal_session_check_init(ctx, 0);
> if (ret < 0)
> @@ -1667,8 +1653,7 @@ static int qsv_transfer_data_from(AVHWFramesContext
> *ctx, AVFrame *dst,
> /* According to MSDK spec for mfxframeinfo, "Width must be a multiple of
> 16.
> * Height must be a multiple of 16 for progressive frame sequence and a
> * multiple of 32 otherwise.", so allign all frames to 16 before
> downloading. */
> - if (dst->height & 15 || dst->linesize[0] & 15) {
> - realigned = 1;
> + if (realigned) {
> if (tmp_frame->format != dst->format ||
> tmp_frame->width != FFALIGN(dst->linesize[0], 16) ||
> tmp_frame->height != FFALIGN(dst->height, 16)) {
> @@ -1728,8 +1713,25 @@ static int qsv_transfer_data_from(AVHWFramesContext
> *ctx, AVFrame *dst,
> return 0;
> }
>
> -static int qsv_transfer_data_to(AVHWFramesContext *ctx, AVFrame *dst,
> - const AVFrame *src)
> +static int qsv_transfer_data_from(AVHWFramesContext *ctx, AVFrame *dst,
> + const AVFrame *src)
> +{
> + QSVFramesContext *s = ctx->hwctx;
Please do not use tab for indentation.
> + int realigned = 0;
> + if (dst->height & 15 || dst->linesize[0] & 15)
> + {
{ and if statement should be on the same line to keep the consistent code style.
> + realigned = 1;
> + ff_mutex_lock(&s->session_lock);
> + }
> + int ret = qsv_transfer_data_from_internal(ctx, dst, src, realigned);
Please do not mix declaration and code.
> + if (realigned)
> + ff_mutex_unlock(&s->session_lock);
> +
> + return ret;
> +}
> +
> +static int qsv_transfer_data_to_internal(AVHWFramesContext *ctx, AVFrame
> *dst,
> + const AVFrame *src, int realigned)
> {
> QSVFramesContext *s = ctx->hwctx;
> mfxFrameSurface1 in = {{ 0 }};
> @@ -1742,7 +1744,6 @@ static int qsv_transfer_data_to(AVHWFramesContext *ctx,
> AVFrame *dst,
> /* make a copy if the input is not padded as libmfx requires */
> AVFrame *tmp_frame = &s->realigned_upload_frame;
> const AVFrame *src_frame;
> - int realigned = 0;
>
> ret = qsv_internal_session_check_init(ctx, 1);
> if (ret < 0)
> @@ -1751,8 +1752,7 @@ static int qsv_transfer_data_to(AVHWFramesContext *ctx,
> AVFrame *dst,
> /* According to MSDK spec for mfxframeinfo, "Width must be a multiple of
> 16.
> * Height must be a multiple of 16 for progressive frame sequence and a
> * multiple of 32 otherwise.", so allign all frames to 16 before
> uploading. */
> - if (src->height & 15 || src->linesize[0] & 15) {
> - realigned = 1;
> + if (realigned) {
> if (tmp_frame->format != src->format ||
> tmp_frame->width != FFALIGN(src->width, 16) ||
> tmp_frame->height != FFALIGN(src->height, 16)) {
> @@ -1821,6 +1821,23 @@ static int qsv_transfer_data_to(AVHWFramesContext *ctx,
> AVFrame *dst,
> return 0;
> }
>
> +static int qsv_transfer_data_to(AVHWFramesContext *ctx, AVFrame *dst,
> + const AVFrame *src)
> +{
> + QSVFramesContext *s = ctx->hwctx;
Please do not use tab for indentation too.
> + int realigned = 0;
> + if (src->height & 15 || src->linesize[0] & 15)
> + {
{ and if statement should be on the same line
> + realigned = 1;
> + ff_mutex_lock(&s->session_lock);
> + }
> + int ret = qsv_transfer_data_to_internal(ctx, dst, src, realigned);
Please do not mix declaration and code.
Thanks
Haihao
> + if (realigned)
> + ff_mutex_unlock(&s->session_lock);
> +
> + return ret;
> +}
> +
> static int qsv_frames_derive_to(AVHWFramesContext *dst_ctx,
> AVHWFramesContext *src_ctx, int flags)
> {
More information about the ffmpeg-devel
mailing list