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

["Xiang, Haihao" <haihao.xiang-at-intel.com@ffmpeg.org>]

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

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