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

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

Commit Message

Maxym Dmytrychenko July 16, 2018, 1:26 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 | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

Comments

Mark Thompson July 16, 2018, 9:59 p.m. | #1
On 16/07/18 14:26, 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 | 35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)

This makes sense, but it looks like it might need some sort of 'once' construction for thread-safety?

I believe the current API intent is that performing simultaneous transfer operations on different frames in the same frames context should be safe.


> diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> index b3eb4a3ea..3e6c38037 100644
> --- a/libavutil/hwcontext_qsv.c
> +++ b/libavutil/hwcontext_qsv.c
> @@ -56,7 +56,9 @@ typedef struct QSVDeviceContext {
>  
>  typedef struct QSVFramesContext {
>      mfxSession session_download;
> +    int session_download_init;
>      mfxSession session_upload;
> +    int session_upload_init;
>  
>      AVBufferRef *child_frames_ref;
>      mfxFrameSurface1 *surfaces_internal;
> @@ -146,13 +148,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 +539,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;
>  }
> @@ -740,6 +741,14 @@ static int qsv_transfer_data_from(AVHWFramesContext *ctx, AVFrame *dst,
>  
>      mfxSyncPoint sync = NULL;
>      mfxStatus err;
> +    int ret = -1;

The initialisation is redundant?  The -1 confused me, but I don't think it's ever read anywhere.

> +
> +    if (!s->session_download_init) {
> +        s->session_download_init = 1;
> +        ret = qsv_init_internal_session(ctx, &s->session_download, 0);
> +        if (ret < 0)
> +            return ret;
> +    }
>  
>      if (!s->session_download) {
>          if (s->child_frames_ref)
> @@ -787,6 +796,14 @@ static int qsv_transfer_data_to(AVHWFramesContext *ctx, AVFrame *dst,
>  
>      mfxSyncPoint sync = NULL;
>      mfxStatus err;
> +    int ret = -1;
Likewise this one.

> +
> +    if (!s->session_upload_init) {
> +        s->session_upload_init = 1;
> +        ret = qsv_init_internal_session(ctx, &s->session_upload, 1);
> +        if (ret < 0)
> +            return ret;
> +    }
>  
>      if (!s->session_upload) {
>          if (s->child_frames_ref)
> 

Thanks,

- Mark
Maxym Dmytrychenko July 16, 2018, 10:13 p.m. | #2
On Tue, Jul 17, 2018 at 12:00 AM Mark Thompson <sw@jkqxz.net> wrote:

> On 16/07/18 14:26, 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 | 35 ++++++++++++++++++++++++++---------
> >  1 file changed, 26 insertions(+), 9 deletions(-)
>
> This makes sense, but it looks like it might need some sort of 'once'
> construction for thread-safety?
>
> I believe the current API intent is that performing simultaneous transfer
> operations on different frames in the same frames context should be safe.
>
> good point!
So far, I see pretty much always the same thread, so like single threaded
usage, unless I am missing something.
should we consider this implementation ok and to remember: if
multithreading support - to be adjusted?


>
> > diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> > index b3eb4a3ea..3e6c38037 100644
> > --- a/libavutil/hwcontext_qsv.c
> > +++ b/libavutil/hwcontext_qsv.c
> > @@ -56,7 +56,9 @@ typedef struct QSVDeviceContext {
> >
> >  typedef struct QSVFramesContext {
> >      mfxSession session_download;
> > +    int session_download_init;
> >      mfxSession session_upload;
> > +    int session_upload_init;
> >
> >      AVBufferRef *child_frames_ref;
> >      mfxFrameSurface1 *surfaces_internal;
> > @@ -146,13 +148,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 +539,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;
> >  }
> > @@ -740,6 +741,14 @@ static int qsv_transfer_data_from(AVHWFramesContext
> *ctx, AVFrame *dst,
> >
> >      mfxSyncPoint sync = NULL;
> >      mfxStatus err;
> > +    int ret = -1;
>
> The initialisation is redundant?  The -1 confused me, but I don't think
> it's ever read anywhere.
>
> can change this, sure.


> > +
> > +    if (!s->session_download_init) {
> > +        s->session_download_init = 1;
> > +        ret = qsv_init_internal_session(ctx, &s->session_download, 0);
> > +        if (ret < 0)
> > +            return ret;
> > +    }
> >
> >      if (!s->session_download) {
> >          if (s->child_frames_ref)
> > @@ -787,6 +796,14 @@ static int qsv_transfer_data_to(AVHWFramesContext
> *ctx, AVFrame *dst,
> >
> >      mfxSyncPoint sync = NULL;
> >      mfxStatus err;
> > +    int ret = -1;
> Likewise this one.
>
> as above as well - can change.


> > +
> > +    if (!s->session_upload_init) {
> > +        s->session_upload_init = 1;
> > +        ret = qsv_init_internal_session(ctx, &s->session_upload, 1);
> > +        if (ret < 0)
> > +            return ret;
> > +    }
> >
> >      if (!s->session_upload) {
> >          if (s->child_frames_ref)
> >
>
> Thanks,
>
> thank you for the review


> - Mark
> _______________________________________________
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel


Max
Mark Thompson July 16, 2018, 10:53 p.m. | #3
On 16/07/18 23:13, Maxym Dmytrychenko wrote:
> On Tue, Jul 17, 2018 at 12:00 AM Mark Thompson <sw@jkqxz.net> wrote:
> 
>> On 16/07/18 14:26, 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 | 35 ++++++++++++++++++++++++++---------
>>>  1 file changed, 26 insertions(+), 9 deletions(-)
>>
>> This makes sense, but it looks like it might need some sort of 'once'
>> construction for thread-safety?
>>
>> I believe the current API intent is that performing simultaneous transfer
>> operations on different frames in the same frames context should be safe.
>>
>> good point!
> So far, I see pretty much always the same thread, so like single threaded
> usage, unless I am missing something.
> should we consider this implementation ok and to remember: if
> multithreading support - to be adjusted?

Well, the code is currently (I believe) thread-safe and this patch looks like it would change it not to be.  While I don't think avconv ever hits this case, API users certainly can.

- Mark

Patch

diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
index b3eb4a3ea..3e6c38037 100644
--- a/libavutil/hwcontext_qsv.c
+++ b/libavutil/hwcontext_qsv.c
@@ -56,7 +56,9 @@  typedef struct QSVDeviceContext {
 
 typedef struct QSVFramesContext {
     mfxSession session_download;
+    int session_download_init;
     mfxSession session_upload;
+    int session_upload_init;
 
     AVBufferRef *child_frames_ref;
     mfxFrameSurface1 *surfaces_internal;
@@ -146,13 +148,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 +539,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;
 }
@@ -740,6 +741,14 @@  static int qsv_transfer_data_from(AVHWFramesContext *ctx, AVFrame *dst,
 
     mfxSyncPoint sync = NULL;
     mfxStatus err;
+    int ret = -1;
+
+    if (!s->session_download_init) {
+        s->session_download_init = 1;
+        ret = qsv_init_internal_session(ctx, &s->session_download, 0);
+        if (ret < 0)
+            return ret;
+    }
 
     if (!s->session_download) {
         if (s->child_frames_ref)
@@ -787,6 +796,14 @@  static int qsv_transfer_data_to(AVHWFramesContext *ctx, AVFrame *dst,
 
     mfxSyncPoint sync = NULL;
     mfxStatus err;
+    int ret = -1;
+
+    if (!s->session_upload_init) {
+        s->session_upload_init = 1;
+        ret = qsv_init_internal_session(ctx, &s->session_upload, 1);
+        if (ret < 0)
+            return ret;
+    }
 
     if (!s->session_upload) {
         if (s->child_frames_ref)