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

Maxym Dmytrychenko maxim.d33 at gmail.com
Wed Jul 18 01:25:59 CEST 2018


On Wed, Jul 18, 2018 at 12:48 AM Mark Thompson <sw at jkqxz.net> wrote:

> On 17/07/18 17:07, 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 at gmail.com>
> > ---
> >  libavutil/hwcontext_qsv.c | 38 +++++++++++++++++++++++++++++---------
> >  1 file changed, 29 insertions(+), 9 deletions(-)
> >
> > diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> > index b3eb4a3ea..390c3aac4 100644
> > --- a/libavutil/hwcontext_qsv.c
> > +++ b/libavutil/hwcontext_qsv.c
> > @@ -18,6 +18,7 @@
> >
> >  #include <stdint.h>
> >  #include <string.h>
> > +#include <stdatomic.h>
> >
> >  #include <mfx/mfxvideo.h>
> >
> > @@ -56,7 +57,9 @@ typedef struct QSVDeviceContext {
> >
> >  typedef struct QSVFramesContext {
> >      mfxSession session_download;
> > +    atomic_int session_download_init;
> >      mfxSession session_upload;
> > +    atomic_int session_upload_init;
> >
> >      AVBufferRef *child_frames_ref;
> >      mfxFrameSurface1 *surfaces_internal;
> > @@ -146,13 +149,15 @@ 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;
> >
> >      av_freep(&s->mem_ids);
> >      av_freep(&s->surface_ptrs);
> > @@ -535,13 +540,10 @@ 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;
> > -
> > -    ret = qsv_init_internal_session(ctx, &s->session_upload, 1);
> > -    if (ret < 0)
> > -        return ret;
> > +    s->session_download      = NULL;
> > +    s->session_upload        = NULL;
> > +    s->session_download_init = 0;
> > +    s->session_upload_init   = 0;
> >
> >      return 0;
> >  }
> > @@ -741,6 +743,15 @@ static int qsv_transfer_data_from(AVHWFramesContext
> *ctx, AVFrame *dst,
> >      mfxSyncPoint sync = NULL;
> >      mfxStatus err;
> >
> > +    while (!s->session_download_init && !s->session_download) {
> > +     if (atomic_fetch_add(&s->session_download_init, 1) == 0) {
> > +            qsv_init_internal_session(ctx, &s->session_download, 0);
> > +        }
> > +     else {
> > +            av_usleep(1);
>
> This races - consider what happens if the other thread is preempted for
> more than 1┬Ás, or if the initialisation itself takes more than that long.
>
> You need to actually do some synchronisation here (e.g. with a 'once'
> variable) - with only atomic flags there is no way to guarantee that the
> other thread has finished unless you spin, which isn't acceptable.
>
> pthread_once was considered but we need to pass  s->session_download
adding mutex - can be overkill for each call of   qsv_transfer_data_*

what do you mean by 'once' variable?
dont really see it currently implemented somewhere...


> > +        }
> > +    }
> > +
> >      if (!s->session_download) {
> >          if (s->child_frames_ref)
> >              return qsv_transfer_data_child(ctx, dst, src);
> > @@ -788,6 +799,15 @@ static int qsv_transfer_data_to(AVHWFramesContext
> *ctx, AVFrame *dst,
> >      mfxSyncPoint sync = NULL;
> >      mfxStatus err;
> >
> > +    while (!s->session_upload_init && !s->session_upload) {
> > +     if (atomic_fetch_add(&s->session_upload_init, 1) == 0) {
> > +            qsv_init_internal_session(ctx, &s->session_upload, 1);
> > +        }
> > +     else {
> > +            av_usleep(1);
> > +        }
> > +    }
> > +
> >      if (!s->session_upload) {
> >          if (s->child_frames_ref)
> >              return qsv_transfer_data_child(ctx, dst, src);
> >
>
> Thanks,
>
> - Mark
> _______________________________________________
> libav-devel mailing list
> libav-devel at libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel


regards
Max


More information about the libav-devel mailing list