Re: [libav-devel] [PATCH v6] qsvvpp: Fix to perform full init only when needed
On Thu, Jul 26, 2018 at 8:36 PM Rogozhkin, Dmitry V <
dmitry.v.rogozh...@intel.com> wrote:
> On Thu, 2018-07-26 at 15:48 +0200, Joe Olivas wrote:
> > 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
> Please, add:
> Cc: Maxym Dmytrychenko
> Cc: Dmitry Rogozhkin
>
> To keep us in the review loop.
> > ---
> > 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
> > +#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(>session_lock);
> > +pthread_cond_destroy(>session_cond);
> > +#endif
> >
> > av_freep(>mem_ids);
> > av_freep(>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, >session_download, 0);
> > -if (ret < 0)
> > -return ret;
> > +s->session_download = NULL;
> > +s->session_upload = NULL;
> >
> > -ret = qsv_init_internal_session(ctx, >session_upload, 1);
> > -if (ret < 0)
> > -return ret;
> > +s->session_download_init = 0;
> > +s->session_upload_init = 0;
> > +
> > +#if HAVE_PTHREADS
> > +pthread_mutex_init(>session_lock, NULL);
> > +pthread_cond_init(>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
> > +#if HAVE_PTHREADS
> > + if (pthread_mutex_trylock(>session_lock) == 0) {
> > +#endif
> > +if (!s->session_download_init) {
> > +qsv_init_internal_session(ctx, >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
> > +if (s->session_download)
> > +s->session_download_init = 1;
> > +}
> > +#if HAVE_PTHREADS
> > +pthread_mutex_unlock(>session_lock);
> > +pthread_cond_signal(>session_cond);
> > +}
> > + else {
> > +pthread_mutex_lock(>session_lock);
> > +while (!s->session_download_init && !s-
> > >session_download) {
> > +pthread_cond_wait(>session_cond,
> > >session_lock);
> > +}
> > +pthread_mutex_unlock(>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.
> > 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.
>
> > +#if HAVE_PTHREADS
> > + if (pthread_mutex_trylock(>session_lock) == 0) {
> >
Re: [libav-devel] [PATCH v6] qsvvpp: Fix to perform full init only when needed
On Thu, 2018-07-26 at 15:48 +0200, Joe Olivas wrote:
> 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
Please, add:
Cc: Maxym Dmytrychenko
Cc: Dmitry Rogozhkin
To keep us in the review loop.
> ---
> 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
> +#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(>session_lock);
> +pthread_cond_destroy(>session_cond);
> +#endif
>
> av_freep(>mem_ids);
> av_freep(>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, >session_download, 0);
> -if (ret < 0)
> -return ret;
> +s->session_download = NULL;
> +s->session_upload = NULL;
>
> -ret = qsv_init_internal_session(ctx, >session_upload, 1);
> -if (ret < 0)
> -return ret;
> +s->session_download_init = 0;
> +s->session_upload_init = 0;
> +
> +#if HAVE_PTHREADS
> +pthread_mutex_init(>session_lock, NULL);
> +pthread_cond_init(>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.
> +#if HAVE_PTHREADS
> + if (pthread_mutex_trylock(>session_lock) == 0) {
> +#endif
> +if (!s->session_download_init) {
> +qsv_init_internal_session(ctx, >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?
> +if (s->session_download)
> +s->session_download_init = 1;
> +}
> +#if HAVE_PTHREADS
> +pthread_mutex_unlock(>session_lock);
> +pthread_cond_signal(>session_cond);
> +}
> + else {
> +pthread_mutex_lock(>session_lock);
> +while (!s->session_download_init && !s-
> >session_download) {
> +pthread_cond_wait(>session_cond,
> >session_lock);
> +}
> +pthread_mutex_unlock(>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.
> 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.
> +#if HAVE_PTHREADS
> + if (pthread_mutex_trylock(>session_lock) == 0) {
> +#endif
> +if (!s->session_upload_init) {
> +qsv_init_internal_session(ctx, >session_upload,
> 1);
> +if (s->session_upload)
> +s->session_upload_init = 1;
> +}
> +#if HAVE_PTHREADS
> +pthread_mutex_unlock(>session_lock);
> +pthread_cond_signal(>session_cond);
> +}
> + else {
> +pthread_mutex_lock(>session_lock);
>
[libav-devel] [PATCH v6] qsvvpp: Fix to perform full init only when needed
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
---
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
+#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(>session_lock);
+pthread_cond_destroy(>session_cond);
+#endif
av_freep(>mem_ids);
av_freep(>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, >session_download, 0);
-if (ret < 0)
-return ret;
+s->session_download = NULL;
+s->session_upload = NULL;
-ret = qsv_init_internal_session(ctx, >session_upload, 1);
-if (ret < 0)
-return ret;
+s->session_download_init = 0;
+s->session_upload_init = 0;
+
+#if HAVE_PTHREADS
+pthread_mutex_init(>session_lock, NULL);
+pthread_cond_init(>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(>session_lock) == 0) {
+#endif
+if (!s->session_download_init) {
+qsv_init_internal_session(ctx, >session_download, 0);
+if (s->session_download)
+s->session_download_init = 1;
+}
+#if HAVE_PTHREADS
+pthread_mutex_unlock(>session_lock);
+pthread_cond_signal(>session_cond);
+}
+ else {
+pthread_mutex_lock(>session_lock);
+while (!s->session_download_init && !s->session_download) {
+pthread_cond_wait(>session_cond, >session_lock);
+}
+pthread_mutex_unlock(>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(>session_lock) == 0) {
+#endif
+if (!s->session_upload_init) {
+qsv_init_internal_session(ctx, >session_upload, 1);
+if (s->session_upload)
+s->session_upload_init = 1;
+}
+#if HAVE_PTHREADS
+pthread_mutex_unlock(>session_lock);
+pthread_cond_signal(>session_cond);
+}
+ else {
+pthread_mutex_lock(>session_lock);
+while (!s->session_upload_init && !s->session_upload) {
+pthread_cond_wait(>session_cond, >session_lock);
+}
+pthread_mutex_unlock(>session_lock);
+}
+#endif
+}
+
if (!s->session_upload) {
if (s->child_frames_ref)
return qsv_transfer_data_child(ctx, dst, src);
--
2.18.0
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
