[FFmpeg-devel] [PATCH, v2 2/2] lavc/vaapi_decode: recreate hw_frames_ctx for vp9 without destroy va_context

[Fu, Linjie]

> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Fu, Linjie
> Sent: Friday, August 9, 2019 19:47
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH, v2 2/2] lavc/vaapi_decode: recreate
> hw_frames_ctx for vp9 without destroy va_context
> 
> > -----Original Message-----
> > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On
> Behalf
> > Of Hendrik Leppkes
> > Sent: Friday, August 9, 2019 17:40
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel at ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH, v2 2/2] lavc/vaapi_decode: recreate
> > hw_frames_ctx for vp9 without destroy va_context
> >
> > On Tue, Aug 6, 2019 at 10:55 AM Fu, Linjie <linjie.fu at intel.com> wrote:
> > >
> > > > -----Original Message-----
> > > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On
> > Behalf
> > > > Of Hendrik Leppkes
> > > > Sent: Tuesday, August 6, 2019 16:27
> > > > To: FFmpeg development discussions and patches <ffmpeg-
> > > > devel at ffmpeg.org>
> > > > Subject: Re: [FFmpeg-devel] [PATCH, v2 2/2] lavc/vaapi_decode:
> > recreate
> > > > hw_frames_ctx for vp9 without destroy va_context
> > > >
> > > > On Thu, Jul 11, 2019 at 10:20 AM Linjie Fu <linjie.fu at intel.com> wrote:
> > > > >
> > > > > VP9 allows resolution changes per frame. Currently in VAAPI,
> resolution
> > > > > changes leads to va context destroy and reinit. This will cause
> > > > > reference frame surface lost and produce garbage.
> > > > >
> > > > > Though refs surface id could be passed to media driver and found in
> > > > > RTtbl, vp9RefList[] in hal layer has already been destroyed. Thus the
> > > > > new created VaContext could only got an empty RefList.
> > > > >
> > > > > As libva allows re-create surface separately without changing the
> > > > > context, this issue could be handled by only recreating
> hw_frames_ctx.
> > > > >
> > > > > Set hwaccel_priv_data_keeping flag for vp9 to only recreating
> > > > > hw_frame_ctx when dynamic resolution changing happens.
> > > > >
> > > > > Could be verified by:
> > > > > ffmpeg -hwaccel vaapi -hwaccel_device /dev/dri/renderD128 -i
> > > > >   ./resolutions.ivf -pix_fmt p010le -f rawvideo -vframes 20 -y vaapi.yuv
> > > > >
> > > > > Signed-off-by: Linjie Fu <linjie.fu at intel.com>
> > > > > ---
> > > > >  libavcodec/decode.c        | 10 +++++-----
> > > > >  libavcodec/internal.h      |  1 +
> > > > >  libavcodec/pthread_frame.c |  2 ++
> > > > >  libavcodec/vaapi_decode.c  | 40 ++++++++++++++++++++++---------
> --
> > -----
> > > > --
> > > > >  libavcodec/vaapi_vp9.c     |  4 ++++
> > > > >  5 files changed, 34 insertions(+), 23 deletions(-)
> > > > >
> > > > > diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> > > > > index 0863b82..7b15fa5 100644
> > > > > --- a/libavcodec/decode.c
> > > > > +++ b/libavcodec/decode.c
> > > > > @@ -1254,7 +1254,6 @@ int
> > > > ff_decode_get_hw_frames_ctx(AVCodecContext *avctx,
> > > > >
> > > > >      frames_ctx = (AVHWFramesContext*)avctx->hw_frames_ctx-
> >data;
> > > > >
> > > > > -
> > > > >      if (frames_ctx->initial_pool_size) {
> > > > >          // We guarantee 4 base work surfaces. The function above
> > guarantees
> > > > 1
> > > > >          // (the absolute minimum), so add the missing count.
> > > >
> > > > Unrelated whitespace change
> > >
> > > There is  a redundant whitespace here, so I removed it within this patch.
> > >
> > > > > @@ -1333,7 +1332,7 @@ static int hwaccel_init(AVCodecContext
> > *avctx,
> > > > >          return AVERROR_PATCHWELCOME;
> > > > >      }
> > > > >
> > > > > -    if (hwaccel->priv_data_size) {
> > > > > +    if (hwaccel->priv_data_size && !avctx->internal-
> > >hwaccel_priv_data) {
> > > > >          avctx->internal->hwaccel_priv_data =
> > > > >              av_mallocz(hwaccel->priv_data_size);
> > > > >          if (!avctx->internal->hwaccel_priv_data)
> > > > > @@ -1396,9 +1395,10 @@ int ff_get_format(AVCodecContext *avctx,
> > > > const enum AVPixelFormat *fmt)
> > > > >      memcpy(choices, fmt, (n + 1) * sizeof(*choices));
> > > > >
> > > > >      for (;;) {
> > > > > -        // Remove the previous hwaccel, if there was one.
> > > > > -        hwaccel_uninit(avctx);
> > > > > -
> > > > > +        // Remove the previous hwaccel, if there was one,
> > > > > +        // and no need for keeping.
> > > > > +        if (!avctx->internal->hwaccel_priv_data_keeping)
> > > > > +            hwaccel_uninit(avctx);
> > > > >          user_choice = avctx->get_format(avctx, choices);
> > > > >          if (user_choice == AV_PIX_FMT_NONE) {
> > > > >              // Explicitly chose nothing, give up.
> > > >
> > > > There could be a dozen special cases how stuff can go wrong here.
> What
> > > > if get_format actually returns a different format then the one
> > > > currently in use? Or a software format?
> > > > Just removing this alone is not safe.
> > >
> > > Didn't quite get your point.
> > > IMHO,  avctx->internal->hwaccel_priv_data_keeping won't be set in
> other
> > cases
> > > other than vaapi_vp9, so this patch won't break the default pipeline, and
> > > hwaccel_uninit(avctx) will always be called.
> > >
> >
> > The point is that you cannot rely on get_format to return the same
> > format that it previously did. It could return a software format, or
> > in some cases possibly even a different hardware format. And you just
> > don't handle that.
> 
> Got it. Thanks for the explanation, it should be reconsidered in case it
> happens.
> 
> > The entire approach here smells a bit of hack. Lets try to think this
> > through and do it properly. One idea that comes to mind is a new
> > hwaccel callback that checks if a in-place re-init is possible without
> > destroying everything. This could be used for a multitude of different
> > situations, and not just this one special case.
> 
> Sounds great, and just FYI, this similar issue is reproduced with nvdec/dxva2
> as well. Clips and some details are provided on trac #8068 in case you and
> other developers may be interested in or need to verify your solution.
> http://trac.ffmpeg.org/ticket/8068

Any step-further progress for the hwaccel callback methods or something I can
help to fix this gap?

- linjie