Discussion:
[PATCH v6] qsvvpp: Fix to perform full init only when needed
(too old to reply)
Joe Olivas
2018-07-26 13:48:21 UTC
Permalink
Removing unused VPP sessions by initializing only when used in order to help reduce CPU utilization. Thanks to Maxym for the guidance.

Signed-off-by: Joe Olivas <***@intel.com>
---
libavutil/hwcontext_qsv.c | 78 ++++++++++++++++++++++++++++++++++++---
1 file changed, 72 insertions(+), 6 deletions(-)

diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
index 250091c4e8..fc4a4127e3 100644
--- a/libavutil/hwcontext_qsv.c
+++ b/libavutil/hwcontext_qsv.c
@@ -23,6 +23,10 @@

#include "config.h"

+#if HAVE_PTHREADS
+#include <pthread.h>
+#endif
+
#if CONFIG_VAAPI
#include "hwcontext_vaapi.h"
#endif
@@ -56,7 +60,13 @@ typedef struct QSVDeviceContext {

typedef struct QSVFramesContext {
mfxSession session_download;
+ int session_download_init;
mfxSession session_upload;
+ int session_upload_init;
+#if HAVE_PTHREADS
+ pthread_mutex_t session_lock;
+ pthread_cond_t session_cond;
+#endif

AVBufferRef *child_frames_ref;
mfxFrameSurface1 *surfaces_internal;
@@ -147,12 +157,19 @@ static void qsv_frames_uninit(AVHWFramesContext *ctx)
MFXClose(s->session_download);
}
s->session_download = NULL;
+ s->session_download_init = 0;

if (s->session_upload) {
MFXVideoVPP_Close(s->session_upload);
MFXClose(s->session_upload);
}
s->session_upload = NULL;
+ s->session_upload_init = 0;
+
+#if HAVE_PTHREADS
+ pthread_mutex_destroy(&s->session_lock);
+ pthread_cond_destroy(&s->session_cond);
+#endif

av_freep(&s->mem_ids);
av_freep(&s->surface_ptrs);
@@ -535,13 +552,16 @@ static int qsv_frames_init(AVHWFramesContext *ctx)
s->mem_ids[i] = frames_hwctx->surfaces[i].Data.MemId;
}

- ret = qsv_init_internal_session(ctx, &s->session_download, 0);
- if (ret < 0)
- return ret;
+ s->session_download = NULL;
+ s->session_upload = NULL;

- ret = qsv_init_internal_session(ctx, &s->session_upload, 1);
- if (ret < 0)
- return ret;
+ s->session_download_init = 0;
+ s->session_upload_init = 0;
+
+#if HAVE_PTHREADS
+ pthread_mutex_init(&s->session_lock, NULL);
+ pthread_cond_init(&s->session_cond, NULL);
+#endif

return 0;
}
@@ -741,6 +761,29 @@ static int qsv_transfer_data_from(AVHWFramesContext *ctx, AVFrame *dst,
mfxSyncPoint sync = NULL;
mfxStatus err;

+ while (!s->session_download_init && !s->session_download) {
+#if HAVE_PTHREADS
+ if (pthread_mutex_trylock(&s->session_lock) == 0) {
+#endif
+ if (!s->session_download_init) {
+ 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
+ }
+
if (!s->session_download) {
if (s->child_frames_ref)
return qsv_transfer_data_child(ctx, dst, src);
@@ -788,6 +831,29 @@ static int qsv_transfer_data_to(AVHWFramesContext *ctx, AVFrame *dst,
mfxSyncPoint sync = NULL;
mfxStatus err;

+ while (!s->session_upload_init && !s->session_upload) {
+#if HAVE_PTHREADS
+ if (pthread_mutex_trylock(&s->session_lock) == 0) {
+#endif
+ if (!s->session_upload_init) {
+ 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
+ }
+
if (!s->session_upload) {
if (s->child_frames_ref)
return qsv_transfer_data_child(ctx, dst, src);
--
2.18.0
Rogozhkin, Dmitry V
2018-07-26 18:35:50 UTC
Permalink
Post by Joe Olivas
Removing unused VPP sessions by initializing only when used in order
to help reduce CPU utilization. Thanks to Maxym for the guidance.
Please, add:
Cc: Maxym Dmytrychenko <***@gmail.com>
Cc: Dmitry Rogozhkin <***@intel.com>

To keep us in the review loop.
Post by Joe Olivas
---
 libavutil/hwcontext_qsv.c | 78 ++++++++++++++++++++++++++++++++++++-
--
 1 file changed, 72 insertions(+), 6 deletions(-)
diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
index 250091c4e8..fc4a4127e3 100644
--- a/libavutil/hwcontext_qsv.c
+++ b/libavutil/hwcontext_qsv.c
@@ -23,6 +23,10 @@
 
 #include "config.h"
 
+#if HAVE_PTHREADS
+#include <pthread.h>
+#endif
+
 #if CONFIG_VAAPI
 #include "hwcontext_vaapi.h"
 #endif
@@ -56,7 +60,13 @@ typedef struct QSVDeviceContext {
 
 typedef struct QSVFramesContext {
     mfxSession session_download;
+    int session_download_init;
     mfxSession session_upload;
+    int session_upload_init;
+#if HAVE_PTHREADS
+    pthread_mutex_t session_lock;
+    pthread_cond_t session_cond;
+#endif
 
     AVBufferRef *child_frames_ref;
     mfxFrameSurface1 *surfaces_internal;
@@ -147,12 +157,19 @@ static void qsv_frames_uninit(AVHWFramesContext *ctx)
         MFXClose(s->session_download);
     }
     s->session_download = NULL;
+    s->session_download_init = 0;
 
     if (s->session_upload) {
         MFXVideoVPP_Close(s->session_upload);
         MFXClose(s->session_upload);
     }
     s->session_upload = NULL;
+    s->session_upload_init = 0;
+
+#if HAVE_PTHREADS
+    pthread_mutex_destroy(&s->session_lock);
+    pthread_cond_destroy(&s->session_cond);
+#endif
 
     av_freep(&s->mem_ids);
     av_freep(&s->surface_ptrs);
@@ -535,13 +552,16 @@ static int qsv_frames_init(AVHWFramesContext *ctx)
             s->mem_ids[i] = frames_hwctx->surfaces[i].Data.MemId;
     }
 
-    ret = qsv_init_internal_session(ctx, &s->session_download, 0);
-    if (ret < 0)
-        return ret;
+    s->session_download = NULL;
+    s->session_upload   = NULL;
 
-    ret = qsv_init_internal_session(ctx, &s->session_upload, 1);
-    if (ret < 0)
-        return ret;
+    s->session_download_init = 0;
+    s->session_upload_init   = 0;
+
+#if HAVE_PTHREADS
+    pthread_mutex_init(&s->session_lock, NULL);
+    pthread_cond_init(&s->session_cond, NULL);
+#endif
 
     return 0;
 }
@@ -741,6 +761,29 @@ static int
qsv_transfer_data_from(AVHWFramesContext *ctx, AVFrame *dst,
     mfxSyncPoint sync = NULL;
     mfxStatus err;
 
+    while (!s->session_download_init && !s->session_download) {
I don't think you need this while at all.
Post by Joe Olivas
+#if HAVE_PTHREADS
+ if (pthread_mutex_trylock(&s->session_lock) == 0) {
+#endif
+            if (!s->session_download_init) {
+                qsv_init_internal_session(ctx, &s->session_download,
0);
I can imagine the only one situation why you need the while above: this
function fails and you need to try again, again and again. Is that
possible or if function failed it failed permanently?
Post by Joe Olivas
+                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-
Post by Joe Olivas
session_download) {
+                pthread_cond_wait(&s->session_cond, &s-
Post by Joe Olivas
session_lock);
+            }
+            pthread_mutex_unlock(&s->session_lock);
+        }
+#endif
+    }
+
     if (!s->session_download) {
This condition starts to be _very_ strange especially if you will
motivate why you need a while above:). If you don't need the while
above (and I think you don't), then this condition is somewhat like an
error handling in the case when for some reason you failed to
initialize session_download. Unfortunately I don't know whether you can
or can not meet such a situation.
Post by Joe Olivas
         if (s->child_frames_ref)
             return qsv_transfer_data_child(ctx, dst, src);
@@ -788,6 +831,29 @@ static int
qsv_transfer_data_to(AVHWFramesContext *ctx, AVFrame *dst,
     mfxSyncPoint sync = NULL;
     mfxStatus err;
 
+    while (!s->session_upload_init && !s->session_upload) {
Same comment as above.
Post by Joe Olivas
+#if HAVE_PTHREADS
+ if (pthread_mutex_trylock(&s->session_lock) == 0) {
+#endif
+            if (!s->session_upload_init) {
+                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-
Post by Joe Olivas
session_lock);
+            }
+            pthread_mutex_unlock(&s->session_lock);
+        }
+#endif
+    }
+
     if (!s->session_upload) {
Same comment as above.
Post by Joe Olivas
         if (s->child_frames_ref)
             return qsv_transfer_data_child(ctx, dst, src);
Maxym Dmytrychenko
2018-08-24 08:16:43 UTC
Permalink
On Thu, Jul 26, 2018 at 8:36 PM Rogozhkin, Dmitry V <
Post by Rogozhkin, Dmitry V
Post by Joe Olivas
Removing unused VPP sessions by initializing only when used in order
to help reduce CPU utilization. Thanks to Maxym for the guidance.
To keep us in the review loop.
Post by Joe Olivas
---
libavutil/hwcontext_qsv.c | 78 ++++++++++++++++++++++++++++++++++++-
--
1 file changed, 72 insertions(+), 6 deletions(-)
diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
index 250091c4e8..fc4a4127e3 100644
--- a/libavutil/hwcontext_qsv.c
+++ b/libavutil/hwcontext_qsv.c
@@ -23,6 +23,10 @@
#include "config.h"
+#if HAVE_PTHREADS
+#include <pthread.h>
+#endif
+
#if CONFIG_VAAPI
#include "hwcontext_vaapi.h"
#endif
@@ -56,7 +60,13 @@ typedef struct QSVDeviceContext {
typedef struct QSVFramesContext {
mfxSession session_download;
+ int session_download_init;
mfxSession session_upload;
+ int session_upload_init;
+#if HAVE_PTHREADS
+ pthread_mutex_t session_lock;
+ pthread_cond_t session_cond;
+#endif
AVBufferRef *child_frames_ref;
mfxFrameSurface1 *surfaces_internal;
@@ -147,12 +157,19 @@ static void qsv_frames_uninit(AVHWFramesContext *ctx)
MFXClose(s->session_download);
}
s->session_download = NULL;
+ s->session_download_init = 0;
if (s->session_upload) {
MFXVideoVPP_Close(s->session_upload);
MFXClose(s->session_upload);
}
s->session_upload = NULL;
+ s->session_upload_init = 0;
+
+#if HAVE_PTHREADS
+ pthread_mutex_destroy(&s->session_lock);
+ pthread_cond_destroy(&s->session_cond);
+#endif
av_freep(&s->mem_ids);
av_freep(&s->surface_ptrs);
@@ -535,13 +552,16 @@ static int qsv_frames_init(AVHWFramesContext *ctx)
s->mem_ids[i] = frames_hwctx->surfaces[i].Data.MemId;
}
- ret = qsv_init_internal_session(ctx, &s->session_download, 0);
- if (ret < 0)
- return ret;
+ s->session_download = NULL;
+ s->session_upload = NULL;
- ret = qsv_init_internal_session(ctx, &s->session_upload, 1);
- if (ret < 0)
- return ret;
+ s->session_download_init = 0;
+ s->session_upload_init = 0;
+
+#if HAVE_PTHREADS
+ pthread_mutex_init(&s->session_lock, NULL);
+ pthread_cond_init(&s->session_cond, NULL);
+#endif
return 0;
}
@@ -741,6 +761,29 @@ static int
qsv_transfer_data_from(AVHWFramesContext *ctx, AVFrame *dst,
mfxSyncPoint sync = NULL;
mfxStatus err;
+ while (!s->session_download_init && !s->session_download) {
I don't think you need this while at all.
we should not leave this part unless init'ed
Post by Rogozhkin, Dmitry V
Post by Joe Olivas
+#if HAVE_PTHREADS
+ if (pthread_mutex_trylock(&s->session_lock) == 0) {
+#endif
+ if (!s->session_download_init) {
+ qsv_init_internal_session(ctx, &s->session_download, 0);
I can imagine the only one situation why you need the while above: this
function fails and you need to try again, again and again. Is that
possible or if function failed it failed permanently?
as above
Post by Rogozhkin, Dmitry V
Post by Joe Olivas
+ 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-
Post by Joe Olivas
session_download) {
+ pthread_cond_wait(&s->session_cond, &s-
Post by Joe Olivas
session_lock);
+ }
+ pthread_mutex_unlock(&s->session_lock);
+ }
+#endif
+ }
+
if (!s->session_download) {
This condition starts to be _very_ strange especially if you will
motivate why you need a while above:). If you don't need the while
above (and I think you don't), then this condition is somewhat like an
error handling in the case when for some reason you failed to
initialize session_download. Unfortunately I don't know whether you can
or can not meet such a situation.
sanity check.
Post by Rogozhkin, Dmitry V
Post by Joe Olivas
if (s->child_frames_ref)
return qsv_transfer_data_child(ctx, dst, src);
@@ -788,6 +831,29 @@ static int
qsv_transfer_data_to(AVHWFramesContext *ctx, AVFrame *dst,
mfxSyncPoint sync = NULL;
mfxStatus err;
+ while (!s->session_upload_init && !s->session_upload) {
Same comment as above.
Post by Joe Olivas
+#if HAVE_PTHREADS
+ if (pthread_mutex_trylock(&s->session_lock) == 0) {
+#endif
+ if (!s->session_upload_init) {
+ 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-
Post by Joe Olivas
session_lock);
+ }
+ pthread_mutex_unlock(&s->session_lock);
+ }
+#endif
+ }
+
if (!s->session_upload) {
Same comment as above.
Post by Joe Olivas
if (s->child_frames_ref)
return qsv_transfer_data_child(ctx, dst, src);
_______________________________________________
libav-devel mailing list
https://lists.libav.org/mailman/listinfo/libav-devel
I would propose to close the topic if no more big open points
and move forward

regards
Max
Luca Barbato
2018-09-02 11:23:11 UTC
Permalink
---

I'd squash this on top of this patch and push it in a bit.

libavutil/hwcontext_qsv.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
index 36a30c5c35..0cfcc01fe8 100644
--- a/libavutil/hwcontext_qsv.c
+++ b/libavutil/hwcontext_qsv.c
@@ -760,21 +760,21 @@ static int qsv_transfer_data_from(AVHWFramesContext *ctx, AVFrame *dst,

mfxSyncPoint sync = NULL;
mfxStatus err;
+ int ret = 0;

- while (!s->session_download_init && !s->session_download) {
+ 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) {
- qsv_init_internal_session(ctx, &s->session_download, 0);
+ 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 {
+ } else {
pthread_mutex_lock(&s->session_lock);
while (!s->session_download_init && !s->session_download) {
pthread_cond_wait(&s->session_cond, &s->session_lock);
@@ -784,6 +784,9 @@ static int qsv_transfer_data_from(AVHWFramesContext *ctx, AVFrame *dst,
#endif
}

+ if (ret < 0)
+ return ret;
+
if (!s->session_download) {
if (s->child_frames_ref)
return qsv_transfer_data_child(ctx, dst, src);
@@ -830,21 +833,21 @@ static int qsv_transfer_data_to(AVHWFramesContext *ctx, AVFrame *dst,

mfxSyncPoint sync = NULL;
mfxStatus err;
+ int ret;

- while (!s->session_upload_init && !s->session_upload) {
+ 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) {
- qsv_init_internal_session(ctx, &s->session_upload, 1);
+ 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 {
+ } else {
pthread_mutex_lock(&s->session_lock);
while (!s->session_upload_init && !s->session_upload) {
pthread_cond_wait(&s->session_cond, &s->session_lock);
@@ -853,6 +856,8 @@ static int qsv_transfer_data_to(AVHWFramesContext *ctx, AVFrame *dst,
}
#endif
}
+ if (ret < 0)
+ return ret;

if (!s->session_upload) {
if (s->child_frames_ref)
--
2.13.3

Loading...