On Thu, Jul 26, 2018 at 8:36 PM Rogozhkin, Dmitry V <
dmitry.v.rogozh...@intel.com> wrote:
> On Thu, 2018-07-26 at 15:48 +0200, Joe Olivas wrote:
> > Removing unused VPP sessions by initializing only when used in order
> > to help reduce CPU utilization. Thanks to Maxym for the guidance.
> >
> > Signed-off-by: Joe Olivas <joseph.k.oli...@intel.com>
> Please, add:
> Cc: Maxym Dmytrychenko <maxim....@gmail.com>
> Cc: Dmitry Rogozhkin <dmitry.v.rogozh...@intel.com>
>
> To keep us in the review loop.
> > ---
> > libavutil/hwcontext_qsv.c | 78 ++++++++++++++++++++++++++++++++++++-
> > --
> > 1 file changed, 72 insertions(+), 6 deletions(-)
> >
> > diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> > index 250091c4e8..fc4a4127e3 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;
> > @@ -147,12 +157,19 @@ static void qsv_frames_uninit(AVHWFramesContext
> > *ctx)
> > MFXClose(s->session_download);
> > }
> > 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_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,29 @@ static int
> > qsv_transfer_data_from(AVHWFramesContext *ctx, AVFrame *dst,
> > mfxSyncPoint sync = NULL;
> > mfxStatus err;
> >
> > + while (!s->session_download_init && !s->session_download) {
>
> I don't think you need this while at all.
>
>
we should not leave this part unless init'ed
> > +#if HAVE_PTHREADS
> > + if (pthread_mutex_trylock(&s->session_lock) == 0) {
> > +#endif
> > + if (!s->session_download_init) {
> > + qsv_init_internal_session(ctx, &s->session_download,
> > 0);
>
> I can imagine the only one situation why you need the while above: this
> function fails and you need to try again, again and again. Is that
> possible or if function failed it failed permanently?
>
>
as above
> > + 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) {
> This condition starts to be _very_ strange especially if you will
> motivate why you need a while above:). If you don't need the while
> above (and I think you don't), then this condition is somewhat like an
> error handling in the case when for some reason you failed to
> initialize session_download. Unfortunately I don't know whether you can
> or can not meet such a situation.
>
>
sanity check.
> > if (s->child_frames_ref)
> > return qsv_transfer_data_child(ctx, dst, src);
> > @@ -788,6 +831,29 @@ static int
> > qsv_transfer_data_to(AVHWFramesContext *ctx, AVFrame *dst,
> > mfxSyncPoint sync = NULL;
> > mfxStatus err;
> >
> > + while (!s->session_upload_init && !s->session_upload) {
>
> Same comment as above.
>
> > +#if HAVE_PTHREADS
> > + if (pthread_mutex_trylock(&s->session_lock) == 0) {
> > +#endif
> > + if (!s->session_upload_init) {
> > + qsv_init_internal_session(ctx, &s->session_upload,
> > 1);
> > + 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) {
>
> Same comment as above.
>
> > 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
I would propose to close the topic if no more big open points
and move forward
regards
Max
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
