[v5] qsvvpp: Fix to perform full init only when needed

Message ID 20180718120717.49994-1-maxim.d33@gmail.com
State New
Headers show
Series
  • [v5] qsvvpp: Fix to perform full init only when needed
Related show

Commit Message

Maxym Dmytrychenko July 18, 2018, 12:07 p.m.
Not used VPP sessions, like for hwupload/hwdownload handling,
can increase CPU utilization and this patch fixes it.
thank you,Joe, for the contribution.

Signed-off-by: Maxym Dmytrychenko <maxim.d33@gmail.com>
---
 libavutil/hwcontext_qsv.c | 72 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 64 insertions(+), 8 deletions(-)

Comments

Rogozhkin, Dmitry V July 25, 2018, 12:56 a.m. | #1
On Wed, 2018-07-18 at 14:07 +0200, Maxym Dmytrychenko wrote:
> Not used VPP sessions, like for hwupload/hwdownload handling,
> can increase CPU utilization and this patch fixes it.
> thank you,Joe, for the contribution.
> 
> Signed-off-by: Maxym Dmytrychenko <maxim.d33@gmail.com>
> ---
>  libavutil/hwcontext_qsv.c | 72
> +++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 64 insertions(+), 8 deletions(-)
> 
> diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> index b3eb4a3ea..6bc2a38ff 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;
> @@ -146,13 +156,20 @@ static void qsv_frames_uninit(AVHWFramesContext
> *ctx)
>          MFXVideoVPP_Close(s->session_download);
>          MFXClose(s->session_download);
>      }
> -    s->session_download = NULL;
> +    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      = 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,24 @@ 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
> +            qsv_init_internal_session(ctx, &s->session_download, 0);
> +            s->session_download_init = 1;
> +#if HAVE_PTHREADS
> +            pthread_cond_signal(&s->session_cond);
You don't need to signal under the mutex. More efficiently is to do
that without.
> +            pthread_mutex_unlock(&s->session_lock);
> +        }
> +        else {
> +            pthread_mutex_lock(&s->session_lock);
> +            pthread_cond_wait(&s->session_cond, &s->session_lock);
This is incorrect usage of condition variables. Look into example in
'man 3 pthread_cond_wait': you should check predicate under the mutex.
> +            pthread_mutex_unlock(&s->session_lock);
> +        }
> +#endif
> +    }
> +
As I said the above code is an example of incorrect usage of condition
variables. I believe this should be written in this way:

#if HAVE_PTHREADS
    if (pthread_mutex_trylock(&s->session_lock) == 0) {
#endif
        qsv_init_internal_session(ctx, &s->session_download, 0);
        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 +826,24 @@ 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
> +            qsv_init_internal_session(ctx, &s->session_upload, 1);
> +            s->session_upload_init = 1;
> +#if HAVE_PTHREADS
> +            pthread_cond_signal(&s->session_cond);
> +            pthread_mutex_unlock(&s->session_lock);
> +        }
> +        else {
> +            pthread_mutex_lock(&s->session_lock);
> +            pthread_cond_wait(&s->session_cond, &s->session_lock);
> +            pthread_mutex_unlock(&s->session_lock);
> +        }
> +#endif
> +    }
> +
Same comment as above. Correct code would be:

#if HAVE_PTHREADS
    if (pthread_mutex_trylock(&s->session_lock) == 0) {
#endif
        qsv_init_internal_session(ctx, &s->session_upload, 0);
        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);
Rogozhkin, Dmitry V July 25, 2018, 4:14 p.m. | #2
On Tue, 2018-07-24 at 09:53 -0700, Dmitry Rogozhkin wrote:
> On Wed, 2018-07-18 at 14:07 +0200, Maxym Dmytrychenko wrote:
> > Not used VPP sessions, like for hwupload/hwdownload handling,
> > can increase CPU utilization and this patch fixes it.
> > thank you,Joe, for the contribution.
> > 
> > Signed-off-by: Maxym Dmytrychenko <maxim.d33@gmail.com>
> > ---
> >  libavutil/hwcontext_qsv.c | 72
> > +++++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 64 insertions(+), 8 deletions(-)
> > 
> > diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> > index b3eb4a3ea..6bc2a38ff 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;
> > @@ -146,13 +156,20 @@ static void
> > qsv_frames_uninit(AVHWFramesContext
> > *ctx)
> >          MFXVideoVPP_Close(s->session_download);
> >          MFXClose(s->session_download);
> >      }
> > -    s->session_download = NULL;
> > +    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      = 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,24 @@ 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
> > +            qsv_init_internal_session(ctx, &s->session_download,
> > 0);
> > +            s->session_download_init = 1;
+#if HAVE_PTHREADS
> > +            pthread_cond_signal(&s->session_cond);
> 
> You don't need to signal under the mutex. More efficiently is to do
> that without.
> > +            pthread_mutex_unlock(&s->session_lock);
> > +        }
> > +        else {
> > +            pthread_mutex_lock(&s->session_lock);
> > +            pthread_cond_wait(&s->session_cond, &s->session_lock);
> 
> This is incorrect usage of condition variables. Look into example in
> 'man 3 pthread_cond_wait': you should check predicate under the
> mutex.
> > +            pthread_mutex_unlock(&s->session_lock);
> > +        }
> > +#endif
> > +    }
> > +
> 
> As I said the above code is an example of incorrect usage of
> condition
> variables. I believe this should be written in this way:
> 
> #if HAVE_PTHREADS
>     if (pthread_mutex_trylock(&s->session_lock) == 0) {
> #endif
>         qsv_init_internal_session(ctx, &s->session_download, 0);
>         s->session_download_init = 1;
Never do something 2 mins before the meeting. This should probably be:        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 +826,24 @@ 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
> > +            qsv_init_internal_session(ctx, &s->session_upload, 1);
> > +            s->session_upload_init = 1;
> > +#if HAVE_PTHREADS
> > +            pthread_cond_signal(&s->session_cond);
> > +            pthread_mutex_unlock(&s->session_lock);
> > +        }
> > +        else {
> > +            pthread_mutex_lock(&s->session_lock);
> > +            pthread_cond_wait(&s->session_cond, &s->session_lock);
> > +            pthread_mutex_unlock(&s->session_lock);
> > +        }
> > +#endif
> > +    }
> > +
> 
> Same comment as above. Correct code would be:
> 
> #if HAVE_PTHREADS
>     if (pthread_mutex_trylock(&s->session_lock) == 0) {
> #endif
>         qsv_init_internal_session(ctx, &s->session_upload, 0);
>         s->session_upload_init = 1;
And here:
        if (!s->session_upload_init) {
            qsv_init_internal_session(ctx, &s->session_upload, 0);
            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);
Maxym Dmytrychenko Aug. 23, 2018, 8:35 p.m. | #3
On Wed, Jul 18, 2018 at 2:07 PM Maxym Dmytrychenko <maxim.d33@gmail.com>
wrote:

> Not used VPP sessions, like for hwupload/hwdownload handling,
> can increase CPU utilization and this patch fixes it.
> thank you,Joe, for the contribution.
>
> Signed-off-by: Maxym Dmytrychenko <maxim.d33@gmail.com>
> ---
>  libavutil/hwcontext_qsv.c | 72
> +++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 64 insertions(+), 8 deletions(-)
>
> diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> index b3eb4a3ea..6bc2a38ff 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;
> @@ -146,13 +156,20 @@ static void qsv_frames_uninit(AVHWFramesContext *ctx)
>          MFXVideoVPP_Close(s->session_download);
>          MFXClose(s->session_download);
>      }
> -    s->session_download = NULL;
> +    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      = 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,24 @@ 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
> +            qsv_init_internal_session(ctx, &s->session_download, 0);
> +            s->session_download_init = 1;
> +#if HAVE_PTHREADS
> +            pthread_cond_signal(&s->session_cond);
> +            pthread_mutex_unlock(&s->session_lock);
> +        }
> +        else {
> +            pthread_mutex_lock(&s->session_lock);
> +            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 +826,24 @@ 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
> +            qsv_init_internal_session(ctx, &s->session_upload, 1);
> +            s->session_upload_init = 1;
> +#if HAVE_PTHREADS
> +            pthread_cond_signal(&s->session_cond);
> +            pthread_mutex_unlock(&s->session_lock);
> +        }
> +        else {
> +            pthread_mutex_lock(&s->session_lock);
> +            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.15.2 (Apple Git-101.1)
>
>
ping if any other comments

thanks
Max
Rogozhkin, Dmitry V Aug. 23, 2018, 8:51 p.m. | #4
On Wed, 2018-07-25 at 16:14 +0000, Rogozhkin, Dmitry V wrote:
> On Tue, 2018-07-24 at 09:53 -0700, Dmitry Rogozhkin wrote:
> > On Wed, 2018-07-18 at 14:07 +0200, Maxym Dmytrychenko wrote:
> > > Not used VPP sessions, like for hwupload/hwdownload handling,
> > > can increase CPU utilization and this patch fixes it.
> > > thank you,Joe, for the contribution.
> > > 
> > > Signed-off-by: Maxym Dmytrychenko <maxim.d33@gmail.com>
> > > ---
> > >  libavutil/hwcontext_qsv.c | 72
> > > +++++++++++++++++++++++++++++++++++++++++------
> > >  1 file changed, 64 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/libavutil/hwcontext_qsv.c
> > > b/libavutil/hwcontext_qsv.c
> > > index b3eb4a3ea..6bc2a38ff 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;
> > > @@ -146,13 +156,20 @@ static void
> > > qsv_frames_uninit(AVHWFramesContext
> > > *ctx)
> > >          MFXVideoVPP_Close(s->session_download);
> > >          MFXClose(s->session_download);
> > >      }
> > > -    s->session_download = NULL;
> > > +    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      = 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,24 @@ 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
> > > +            qsv_init_internal_session(ctx, &s->session_download,
> > > 0);
> > > +            s->session_download_init = 1;
> 
> +#if HAVE_PTHREADS
> > > +            pthread_cond_signal(&s->session_cond);
> > 
> > You don't need to signal under the mutex. More efficiently is to do
> > that without.
> > > +            pthread_mutex_unlock(&s->session_lock);
> > > +        }
> > > +        else {
> > > +            pthread_mutex_lock(&s->session_lock);
> > > +            pthread_cond_wait(&s->session_cond, &s-
> > > >session_lock);
> > 
> > This is incorrect usage of condition variables. Look into example
> > in
> > 'man 3 pthread_cond_wait': you should check predicate under the
> > mutex.
> > > +            pthread_mutex_unlock(&s->session_lock);
> > > +        }
> > > +#endif
> > > +    }
> > > +
> > 
> > As I said the above code is an example of incorrect usage of
> > condition
> > variables. I believe this should be written in this way:
> > 
> > #if HAVE_PTHREADS
> >     if (pthread_mutex_trylock(&s->session_lock) == 0) {
> > #endif
> >         qsv_init_internal_session(ctx, &s->session_download, 0);
> >         s->session_download_init = 1;
> 
> Never do something 2 mins before the meeting. This should probably
> be:        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 +826,24 @@ 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
> > > +            qsv_init_internal_session(ctx, &s->session_upload,
> > > 1);
> > > +            s->session_upload_init = 1;
> > > +#if HAVE_PTHREADS
> > > +            pthread_cond_signal(&s->session_cond);
> > > +            pthread_mutex_unlock(&s->session_lock);
> > > +        }
> > > +        else {
> > > +            pthread_mutex_lock(&s->session_lock);
> > > +            pthread_cond_wait(&s->session_cond, &s-
> > > >session_lock);
> > > +            pthread_mutex_unlock(&s->session_lock);
> > > +        }
> > > +#endif
> > > +    }
> > > +
> > 
> > Same comment as above. Correct code would be:
> > 
> > #if HAVE_PTHREADS
> >     if (pthread_mutex_trylock(&s->session_lock) == 0) {
> > #endif
> >         qsv_init_internal_session(ctx, &s->session_upload, 0);
> >         s->session_upload_init = 1;
> 
> And here:
>         if (!s->session_upload_init) {
>             qsv_init_internal_session(ctx, &s->session_upload, 0);
>             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);
> 
> _______________________________________________
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel

Maxim, did you miss these comments? or I missed a reply?

Patch

diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
index b3eb4a3ea..6bc2a38ff 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;
@@ -146,13 +156,20 @@  static void qsv_frames_uninit(AVHWFramesContext *ctx)
         MFXVideoVPP_Close(s->session_download);
         MFXClose(s->session_download);
     }
-    s->session_download = NULL;
+    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      = 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,24 @@  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
+            qsv_init_internal_session(ctx, &s->session_download, 0);
+            s->session_download_init = 1;
+#if HAVE_PTHREADS
+            pthread_cond_signal(&s->session_cond);
+            pthread_mutex_unlock(&s->session_lock);
+        }
+        else {
+            pthread_mutex_lock(&s->session_lock);
+            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 +826,24 @@  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
+            qsv_init_internal_session(ctx, &s->session_upload, 1);
+            s->session_upload_init = 1;
+#if HAVE_PTHREADS
+            pthread_cond_signal(&s->session_cond);
+            pthread_mutex_unlock(&s->session_lock);
+        }
+        else {
+            pthread_mutex_lock(&s->session_lock);
+            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);