[FFmpeg-devel] [PATCH 1/2] lavfi/qsvpp: fix after 85c938fa28

[FFmpeg-devel] [PATCH 1/2] lavfi/qsvpp: fix after 85c938fa28

[Anton Khirnov]

---
 libavfilter/qsvvpp.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/libavfilter/qsvvpp.c b/libavfilter/qsvvpp.c
index 35769dfd60..954f882637 100644
--- a/libavfilter/qsvvpp.c
+++ b/libavfilter/qsvvpp.c
@@ -796,7 +796,7 @@ int ff_qsvvpp_filter_frame(QSVVPPContext *s, AVFilterLink *inlink, AVFrame *picr
     AVFilterLink     *outlink = ctx->outputs[0];
     QSVAsyncFrame     aframe;
     mfxSyncPoint      sync;
-    QSVFrame         *in_frame, *out_frame, *tmp;
+    QSVFrame         *in_frame, *out_frame;
     int               ret, filter_ret;
 
     while (s->eof && av_fifo_read(s->async_fifo, &aframe, 1) >= 0) {
@@ -857,15 +857,15 @@ int ff_qsvvpp_filter_frame(QSVVPPContext *s, AVFilterLink *inlink, AVFrame *picr
                 ret = MFXVideoCORE_SyncOperation(s->session, aframe.sync, 1000);
             } while (ret == MFX_WRN_IN_EXECUTION);
 
-            filter_ret = s->filter_frame(outlink, tmp->frame);
+            filter_ret = s->filter_frame(outlink, aframe.frame->frame);
             if (filter_ret < 0) {
-                av_frame_free(&tmp->frame);
+                av_frame_free(&aframe.frame->frame);
                 return filter_ret;
             }
 
-            tmp->queued--;
+            aframe.frame->queued--;
             s->got_frame = 1;
-            tmp->frame = NULL;
+            aframe.frame->frame = NULL;
         }
     } while(ret == MFX_ERR_MORE_SURFACE);
 
-- 
2.34.1

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

[FFmpeg-devel] [PATCH 2/2] lavu/hwcontext_qsv: fix a potential infinite loop

[Anton Khirnov]

Current code will loop forever if MFXVideoVPP_Init() fails.
Also, simplify the code.
---
 libavutil/hwcontext_qsv.c | 80 +++++++++++++++------------------------
 1 file changed, 31 insertions(+), 49 deletions(-)

diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
index d3d8f42c99..95f8071abe 100644
--- a/libavutil/hwcontext_qsv.c
+++ b/libavutil/hwcontext_qsv.c
@@ -16,6 +16,7 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
+#include <stdatomic.h>
 #include <stdint.h>
 #include <string.h>
 
@@ -71,12 +72,11 @@ typedef struct QSVDeviceContext {
 
 typedef struct QSVFramesContext {
     mfxSession session_download;
-    int session_download_init;
+    atomic_int session_download_init;
     mfxSession session_upload;
-    int session_upload_init;
+    atomic_int session_upload_init;
 #if HAVE_PTHREADS
     pthread_mutex_t session_lock;
-    pthread_cond_t session_cond;
 #endif
 
     AVBufferRef *child_frames_ref;
@@ -297,7 +297,6 @@ static void qsv_frames_uninit(AVHWFramesContext *ctx)
 
 #if HAVE_PTHREADS
     pthread_mutex_destroy(&s->session_lock);
-    pthread_cond_destroy(&s->session_cond);
 #endif
 
     av_freep(&s->mem_ids);
@@ -744,7 +743,6 @@ static int qsv_frames_init(AVHWFramesContext *ctx)
 
 #if HAVE_PTHREADS
     pthread_mutex_init(&s->session_lock, NULL);
-    pthread_cond_init(&s->session_cond, NULL);
 #endif
 
     return 0;
@@ -1024,6 +1022,32 @@ static int map_frame_to_surface(const AVFrame *frame, mfxFrameSurface1 *surface)
     return 0;
 }
 
+static int qsv_internal_session_check_init(AVHWFramesContext *ctx, int upload)
+{
+    QSVFramesContext *s = ctx->internal->priv;
+    atomic_int *inited  = upload ? &s->session_upload_init : &s->session_download_init;
+    mfxSession *session = upload ? &s->session_upload      : &s->session_download;
+    int ret = 0;
+
+    if (atomic_load(inited))
+        return 0;
+
+#if HAVE_PTHREADS
+    pthread_mutex_lock(&s->session_lock);
+#endif
+
+    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
+
+    return ret;
+}
+
 static int qsv_transfer_data_from(AVHWFramesContext *ctx, AVFrame *dst,
                                   const AVFrame *src)
 {
@@ -1035,28 +1059,7 @@ static int qsv_transfer_data_from(AVHWFramesContext *ctx, AVFrame *dst,
     mfxStatus err;
     int ret = 0;
 
-    while (!s->session_download_init && !s->session_download && !ret) {
-#if HAVE_PTHREADS
-        if (pthread_mutex_trylock(&s->session_lock) == 0) {
-#endif
-            if (!s->session_download_init) {
-                ret = qsv_init_internal_session(ctx, &s->session_download, 0);
-                if (s->session_download)
-                    s->session_download_init = 1;
-            }
-#if HAVE_PTHREADS
-            pthread_mutex_unlock(&s->session_lock);
-            pthread_cond_signal(&s->session_cond);
-        } else {
-            pthread_mutex_lock(&s->session_lock);
-            while (!s->session_download_init && !s->session_download) {
-                pthread_cond_wait(&s->session_cond, &s->session_lock);
-            }
-            pthread_mutex_unlock(&s->session_lock);
-        }
-#endif
-    }
-
+    ret = qsv_internal_session_check_init(ctx, 0);
     if (ret < 0)
         return ret;
 
@@ -1109,28 +1112,7 @@ static int qsv_transfer_data_to(AVHWFramesContext *ctx, AVFrame *dst,
     const AVFrame *src_frame;
     int realigned = 0;
 
-
-    while (!s->session_upload_init && !s->session_upload && !ret) {
-#if HAVE_PTHREADS
-        if (pthread_mutex_trylock(&s->session_lock) == 0) {
-#endif
-            if (!s->session_upload_init) {
-                ret = qsv_init_internal_session(ctx, &s->session_upload, 1);
-                if (s->session_upload)
-                    s->session_upload_init = 1;
-            }
-#if HAVE_PTHREADS
-            pthread_mutex_unlock(&s->session_lock);
-            pthread_cond_signal(&s->session_cond);
-        } else {
-            pthread_mutex_lock(&s->session_lock);
-            while (!s->session_upload_init && !s->session_upload) {
-                pthread_cond_wait(&s->session_cond, &s->session_lock);
-            }
-            pthread_mutex_unlock(&s->session_lock);
-        }
-#endif
-    }
+    ret = qsv_internal_session_check_init(ctx, 1);
     if (ret < 0)
         return ret;
 
-- 
2.34.1

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

Re: [FFmpeg-devel] [PATCH 2/2] lavu/hwcontext_qsv: fix a potential infinite loop

[James Almer]

On 2/8/2022 3:26 PM, Anton Khirnov wrote:
> Current code will loop forever if MFXVideoVPP_Init() fails.
> Also, simplify the code.
> ---
>   libavutil/hwcontext_qsv.c | 80 +++++++++++++++------------------------
>   1 file changed, 31 insertions(+), 49 deletions(-)
> 
> diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> index d3d8f42c99..95f8071abe 100644
> --- a/libavutil/hwcontext_qsv.c
> +++ b/libavutil/hwcontext_qsv.c
> @@ -16,6 +16,7 @@
>    * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>    */
>   
> +#include <stdatomic.h>
>   #include <stdint.h>
>   #include <string.h>
>   
> @@ -71,12 +72,11 @@ typedef struct QSVDeviceContext {
>   
>   typedef struct QSVFramesContext {
>       mfxSession session_download;
> -    int session_download_init;
> +    atomic_int session_download_init;
>       mfxSession session_upload;
> -    int session_upload_init;
> +    atomic_int session_upload_init;
>   #if HAVE_PTHREADS
>       pthread_mutex_t session_lock;
> -    pthread_cond_t session_cond;
>   #endif
>   
>       AVBufferRef *child_frames_ref;
> @@ -297,7 +297,6 @@ static void qsv_frames_uninit(AVHWFramesContext *ctx)
>   
>   #if HAVE_PTHREADS
>       pthread_mutex_destroy(&s->session_lock);
> -    pthread_cond_destroy(&s->session_cond);
>   #endif
>   
>       av_freep(&s->mem_ids);
> @@ -744,7 +743,6 @@ static int qsv_frames_init(AVHWFramesContext *ctx)
>   
>   #if HAVE_PTHREADS
>       pthread_mutex_init(&s->session_lock, NULL);
> -    pthread_cond_init(&s->session_cond, NULL);
>   #endif
>   
>       return 0;
> @@ -1024,6 +1022,32 @@ static int map_frame_to_surface(const AVFrame *frame, mfxFrameSurface1 *surface)
>       return 0;
>   }
>   
> +static int qsv_internal_session_check_init(AVHWFramesContext *ctx, int upload)
> +{
> +    QSVFramesContext *s = ctx->internal->priv;
> +    atomic_int *inited  = upload ? &s->session_upload_init : &s->session_download_init;
> +    mfxSession *session = upload ? &s->session_upload      : &s->session_download;
> +    int ret = 0;
> +
> +    if (atomic_load(inited))
> +        return 0;
> +
> +#if HAVE_PTHREADS
> +    pthread_mutex_lock(&s->session_lock);
> +#endif
> +
> +    if (!atomic_load(inited)) {
> +        ret = qsv_init_internal_session(ctx, session, upload);
> +        atomic_store(inited, 1);

Nit: How about

if (atomic_compare_exchange_strong(inited, &ret, 1))
     ret = qsv_init_internal_session(ctx, session, upload);

Or using some other variable for the second argument (It must be 0 
before the call, and will become 1 if *inited was already 1).

Since atomic_compare_exchange_strong() is not used anywhere in the tree, 
it would let us know if any of the atomics emulation implementations are 
faulty (win32, pthreads, old gcc).

> +    }
> +
> +#if HAVE_PTHREADS
> +    pthread_mutex_unlock(&s->session_lock);
> +#endif
> +
> +    return ret;
> +}
> +
>   static int qsv_transfer_data_from(AVHWFramesContext *ctx, AVFrame *dst,
>                                     const AVFrame *src)
>   {
> @@ -1035,28 +1059,7 @@ static int qsv_transfer_data_from(AVHWFramesContext *ctx, AVFrame *dst,
>       mfxStatus err;
>       int ret = 0;
>   
> -    while (!s->session_download_init && !s->session_download && !ret) {
> -#if HAVE_PTHREADS
> -        if (pthread_mutex_trylock(&s->session_lock) == 0) {
> -#endif
> -            if (!s->session_download_init) {
> -                ret = qsv_init_internal_session(ctx, &s->session_download, 0);
> -                if (s->session_download)
> -                    s->session_download_init = 1;
> -            }
> -#if HAVE_PTHREADS
> -            pthread_mutex_unlock(&s->session_lock);
> -            pthread_cond_signal(&s->session_cond);
> -        } else {
> -            pthread_mutex_lock(&s->session_lock);
> -            while (!s->session_download_init && !s->session_download) {
> -                pthread_cond_wait(&s->session_cond, &s->session_lock);
> -            }
> -            pthread_mutex_unlock(&s->session_lock);
> -        }
> -#endif
> -    }
> -
> +    ret = qsv_internal_session_check_init(ctx, 0);
>       if (ret < 0)
>           return ret;
>   
> @@ -1109,28 +1112,7 @@ static int qsv_transfer_data_to(AVHWFramesContext *ctx, AVFrame *dst,
>       const AVFrame *src_frame;
>       int realigned = 0;
>   
> -
> -    while (!s->session_upload_init && !s->session_upload && !ret) {
> -#if HAVE_PTHREADS
> -        if (pthread_mutex_trylock(&s->session_lock) == 0) {
> -#endif
> -            if (!s->session_upload_init) {
> -                ret = qsv_init_internal_session(ctx, &s->session_upload, 1);
> -                if (s->session_upload)
> -                    s->session_upload_init = 1;
> -            }
> -#if HAVE_PTHREADS
> -            pthread_mutex_unlock(&s->session_lock);
> -            pthread_cond_signal(&s->session_cond);
> -        } else {
> -            pthread_mutex_lock(&s->session_lock);
> -            while (!s->session_upload_init && !s->session_upload) {
> -                pthread_cond_wait(&s->session_cond, &s->session_lock);
> -            }
> -            pthread_mutex_unlock(&s->session_lock);
> -        }
> -#endif
> -    }
> +    ret = qsv_internal_session_check_init(ctx, 1);
>       if (ret < 0)
>           return ret;
>   
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/2] lavfi/qsvpp: fix after 85c938fa28

[Eoff, Ullysses A]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton Khirnov
> Sent: Tuesday, February 8, 2022 10:27 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH 1/2] lavfi/qsvpp: fix after 85c938fa28
> 
> ---
>  libavfilter/qsvvpp.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/libavfilter/qsvvpp.c b/libavfilter/qsvvpp.c
> index 35769dfd60..954f882637 100644
> --- a/libavfilter/qsvvpp.c
> +++ b/libavfilter/qsvvpp.c
> @@ -796,7 +796,7 @@ int ff_qsvvpp_filter_frame(QSVVPPContext *s, AVFilterLink *inlink, AVFrame *picr
>      AVFilterLink     *outlink = ctx->outputs[0];
>      QSVAsyncFrame     aframe;
>      mfxSyncPoint      sync;
> -    QSVFrame         *in_frame, *out_frame, *tmp;
> +    QSVFrame         *in_frame, *out_frame;
>      int               ret, filter_ret;
> 
>      while (s->eof && av_fifo_read(s->async_fifo, &aframe, 1) >= 0) {
> @@ -857,15 +857,15 @@ int ff_qsvvpp_filter_frame(QSVVPPContext *s, AVFilterLink *inlink, AVFrame *picr
>                  ret = MFXVideoCORE_SyncOperation(s->session, aframe.sync, 1000);
>              } while (ret == MFX_WRN_IN_EXECUTION);
> 
> -            filter_ret = s->filter_frame(outlink, tmp->frame);
> +            filter_ret = s->filter_frame(outlink, aframe.frame->frame);
>              if (filter_ret < 0) {
> -                av_frame_free(&tmp->frame);
> +                av_frame_free(&aframe.frame->frame);
>                  return filter_ret;
>              }
> 
> -            tmp->queued--;
> +            aframe.frame->queued--;
>              s->got_frame = 1;
> -            tmp->frame = NULL;
> +            aframe.frame->frame = NULL;
>          }
>      } while(ret == MFX_ERR_MORE_SURFACE);
> 
> --
> 2.34.1
> 

Fixes https://trac.ffmpeg.org/ticket/9629 for me.
LGTM

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