[FFmpeg-devel] libavutil/hwcontext_qsv: Make qsv hardware transfers thread safe

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

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