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

[Maxym Dmytrychenko]

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 <***@gmail.com>
---
libavutil/hwcontext_qsv.c | 35 ++++++++++++++++++++++++++---------
1 file changed, 26 insertions(+), 9 deletions(-)

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)

[Mark Thompson]

Post by Maxym Dmytrychenko
Not used VPP sessions, like for hwupload/hwdownload handling,
can increase CPU utilization and this patch fixes it.
thank you,Joe, for the contribution.
---
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.

Post by Maxym Dmytrychenko
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.

Post by Maxym Dmytrychenko
+
+ 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.

Post by Maxym Dmytrychenko
+
+ 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]

Post by Mark Thompson

Post by Maxym Dmytrychenko
Not used VPP sessions, like for hwupload/hwdownload handling,
can increase CPU utilization and this patch fixes it.
thank you,Joe, for the contribution.
---
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?

Post by Mark Thompson

Post by Maxym Dmytrychenko
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)

Post by Maxym Dmytrychenko
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,

Post by Maxym Dmytrychenko
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.

Post by Maxym Dmytrychenko
+
+ 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,

Post by Maxym Dmytrychenko
mfxSyncPoint sync = NULL;
mfxStatus err;
+ int ret = -1;

Likewise this one.
as above as well - can change.

Post by Maxym Dmytrychenko
+
+ 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
https://lists.libav.org/mailman/listinfo/libav-devel

Max

[Mark Thompson]

Post by Maxym Dmytrychenko

Post by Mark Thompson

Post by Maxym Dmytrychenko
Not used VPP sessions, like for hwupload/hwdownload handling,
can increase CPU utilization and this patch fixes it.
thank you,Joe, for the contribution.
---
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