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

2024-04-23 Thread 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 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 
>  
>  #include "config.h"
> -
> -#if HAVE_PTHREADS
> -#include 
> -#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(>session_lock);
> -#endif
> +    ff_mutex_destroy(>session_lock);
>  
>  av_freep(>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(>session_lock, NULL);
> -#endif
> +    ff_mutex_init(>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(>session_lock);
> -#endif
> +    ff_mutex_lock(>session_lock);
>  
>  if (!atomic_load(inited)) {
>  ret = qsv_init_internal_session(ctx, session, upload);
>  atomic_store(inited, 1);
>  }
>  
> -#if HAVE_PTHREADS
> -    pthread_mutex_unlock(>session_lock);
> -#endif
> +    ff_mutex_unlock(>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 = >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(>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(>session_lock);
> +
> +    return ret;
> +}
> +
> +static int

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

2024-04-17 Thread Mark Samuelson 
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 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 
 
 #include "config.h"
-
-#if HAVE_PTHREADS
-#include 
-#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(>session_lock);
-#endif
+ff_mutex_destroy(>session_lock);
 
 av_freep(>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(>session_lock, NULL);
-#endif
+ff_mutex_init(>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(>session_lock);
-#endif
+ff_mutex_lock(>session_lock);
 
 if (!atomic_load(inited)) {
 ret = qsv_init_internal_session(ctx, session, upload);
 atomic_store(inited, 1);
 }
 
-#if HAVE_PTHREADS
-pthread_mutex_unlock(>session_lock);
-#endif
+ff_mutex_unlock(>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 = >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(>session_lock);
+}
+ret = qsv_transfer_data_from_internal(ctx, dst, src, realigned);
+if (realigned)
+ff_mutex_unlock(>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 = >realigned_upload_frame;
 const AVFrame *src_frame;
-int realigned = 0;

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

2024-04-17 Thread Xiang, Haihao 
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 
>  
>  #include "config.h"
> -
> -#if HAVE_PTHREADS
> -#include 
> -#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(>session_lock);
> -#endif
> +    ff_mutex_destroy(>session_lock);
>  
>  av_freep(>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(>session_lock, NULL);
> -#endif
> +    ff_mutex_init(>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(>session_lock);
> -#endif
> +    ff_mutex_lock(>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(>session_lock);
> -#endif
> +    ff_mutex_unlock(>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 = >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(>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(>session_lock);
> +
> +    return

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

2024-04-13 Thread Mark Samuelson 
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 
 
 #include "config.h"
-
-#if HAVE_PTHREADS
-#include 
-#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(>session_lock);
-#endif
+ff_mutex_destroy(>session_lock);
 
 av_freep(>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(>session_lock, NULL);
-#endif
+ff_mutex_init(>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(>session_lock);
-#endif
+ff_mutex_lock(>session_lock);
 
 if (!atomic_load(inited)) {
 ret = qsv_init_internal_session(ctx, session, upload);
 atomic_store(inited, 1);
 }
 
-#if HAVE_PTHREADS
-pthread_mutex_unlock(>session_lock);
-#endif
+ff_mutex_unlock(>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 = >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;
+int realigned = 0;
+if (dst->height & 15 || dst->linesize[0] & 15)
+{
+realigned = 1;
+ff_mutex_lock(>session_lock);
+}
+int ret = qsv_transfer_data_from_internal(ctx, dst, src, realigned);
+if (realigned)
+ff_mutex_unlock(>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 = >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.