[libav-devel] [PATCH v6] qsvvpp: Fix to perform full init only when needed

Rogozhkin, Dmitry V dmitry.v.rogozhkin at intel.com
Thu Jul 26 20:35:50 CEST 2018


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 <joseph.k.olivas at intel.com>
Please, add:
Cc: Maxym Dmytrychenko <maxim.d33 at gmail.com>
Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin at intel.com>

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

> +#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?

> +                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) {
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(&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) {

Same comment as above.

>          if (s->child_frames_ref)
>              return qsv_transfer_data_child(ctx, dst, src);


More information about the libav-devel mailing list