Re: [libav-devel] [PATCH v3] qsvvpp: Fix to perform full init only when needed
On 16/07/18 23:13, Maxym Dmytrychenko wrote: > On Tue, Jul 17, 2018 at 12:00 AM Mark Thompson 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 >>> --- >>> 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 ___ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH v3] qsvvpp: Fix to perform full init only when needed
On Tue, Jul 17, 2018 at 12:00 AM Mark Thompson 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
> > ---
> > 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(>mem_ids);
> > av_freep(>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, >session_download, 0);
> > -if (ret < 0)
> > -return ret;
> > -
> > -ret = qsv_init_internal_session(ctx, >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, >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, >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
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH v3] qsvvpp: Fix to perform full init only when needed
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
> ---
> 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(>mem_ids);
> av_freep(>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, >session_download, 0);
> -if (ret < 0)
> -return ret;
> -
> -ret = qsv_init_internal_session(ctx, >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, >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, >session_upload, 1);
> +if (ret < 0)
> +return ret;
> +}
>
> if (!s->session_upload) {
> if (s->child_frames_ref)
>
Thanks,
- Mark
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
[libav-devel] [PATCH v3] qsvvpp: Fix to perform full init only when needed
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
---
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(>mem_ids);
av_freep(>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, >session_download, 0);
-if (ret < 0)
-return ret;
-
-ret = qsv_init_internal_session(ctx, >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, >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, >session_upload, 1);
+if (ret < 0)
+return ret;
+}
if (!s->session_upload) {
if (s->child_frames_ref)
--
2.15.2 (Apple Git-101.1)
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
