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