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

[Xiang, Haihao]

On Wo, 2024-04-17 at 09:46 -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.
> ---
>  
> Here is a new patch addressing your comments
> Fixed the nested calls to ff_mutex_lock
> Fixed the two accidental tabs
> Fixed the two violations of K&R style
> Fixed the two incidents of mixing declaration and code
> 
> 
>  libavutil/hwcontext_qsv.c | 93 +++++++++++++++++++++++----------------
>  1 file changed, 56 insertions(+), 37 deletions(-)
> 
> diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> index c7c7878644..ed462d440a 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);
>  
>      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,17 +1645,11 @@ 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)
> -        return ret;
>  
>      /* 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 +1709,30 @@ 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;
> +    int realigned = 0;
> +    int ret = 0;
> +
> +    ret = qsv_internal_session_check_init(ctx, 0);
> +    if (ret < 0)
> +        return ret;
> +
> +    if (dst->height & 15 || dst->linesize[0] & 15) {
> +        realigned = 1;
> +        ff_mutex_lock(&s->session_lock);

Needn't lock the use if changing realigned_upload_frame and
realigned_download_frame to local variables.

Thanks
Haihao

> +    }
> +    ret = qsv_transfer_data_from_internal(ctx, dst, src, realigned);
> +    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,17 +1745,11 @@ 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)
> -        return ret;
>  
>      /* 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 +1818,28 @@ 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;
> +    int realigned = 0;
> +    int ret = 0;
> +
> +    ret = qsv_internal_session_check_init(ctx, 1);
> +    if (ret < 0)
> +        return ret;
> +
> +    if (src->height & 15 || src->linesize[0] & 15) {
> +        realigned = 1;
> +        ff_mutex_lock(&s->session_lock);
> +    }
> +    ret = qsv_transfer_data_to_internal(ctx, dst, src, realigned);
> +    if (realigned)
> +        ff_mutex_unlock(&s->session_lock);
> +
> +    return ret;
> +}
> +
>  static int qsv_frames_derive_to(AVHWFramesContext *dst_ctx,
>                                  AVHWFramesContext *src_ctx, int flags)
>  {