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

[Fu, Linjie]

> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Fu, Linjie
> Sent: Monday, July 8, 2019 16:11
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] lavc/vaapi_decode: recreate
> hw_frames_ctx without destroy va_context
> 
> > -----Original Message-----
> > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On
> Behalf
> > Of Yan Wang
> > Sent: Monday, July 8, 2019 15:54
> > To: ffmpeg-devel at ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH 2/2] lavc/vaapi_decode: recreate
> > hw_frames_ctx without destroy va_context
> >
> >
> > On 7/8/2019 2:45 PM, Yan Wang wrote:
> > >
> > > On 7/7/2019 9:49 PM, Fu, Linjie wrote:
> > >>> -----Original Message-----
> > >>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On
> > Behalf
> > >>> Of Mark Thompson
> > >>> Sent: Sunday, July 7, 2019 19:51
> > >>> To: ffmpeg-devel at ffmpeg.org
> > >>> Subject: Re: [FFmpeg-devel] [PATCH 2/2] lavc/vaapi_decode: recreate
> > >>> hw_frames_ctx without destroy va_context
> > >>>
> > >>> On 07/07/2019 17:38, Linjie Fu wrote:
> > >>>> VP9 allows resolution changes per frame. Currently in VAAPI,
> > >>>> resolution
> > >>>> changes leads to va context destroy and reinit.
> > >>> Which is correct - it needs to remake the context because the old
> > >>> one is for
> > >>> the wrong resolution.
> > >> It seems that we don't need to remake context, remaking the surface
> > >> is enough
> > >> for resolution changing.
> > >> Currently in libva, surface is able to be recreated separately
> > >> without remaking context.
> > >> And this way is used in libyami to cope with resolution changing cases.
> > >>
> > >>>> This will cause
> > >>>> reference frame surface lost and produce garbage.
> > >>> This isn't right - the reference frame surfaces aren't lost. See
> > >>> VP9Context.s.refs[], which holds references to the frames (including
> > >>> their
> > >>> hardware frame contexts) until the stream indicates that they can be
> > >>> discarded by overwriting their reference frame slots.
> > >> I'm not quite sure about this, at least the result shows they are not
> > >> used correctly
> > >> in current way.
> > >> Will look deeper into it.
> > >
> > > In vaapi_vp9_start_frame() of libavcodec/vaapi_vp9.c, it only passes
> > > VASurfaceID into pic_param.reference_frames[i].
> > >
> > > But when destroy va_context, the surface/render target based on this
> > > VASurfaceID has been destroyed.
> >
> > Update: the surface isn't destroyed when destroy va_context. But every
> > va_context maintains one independent map table: m_ddiDecodeCtx-
> >RTtbl.
> >
> > So the new context cannot find this surface in its map table.
> >
> > My previous suggested solution should be available still.
> >
> 
> Yes, tracing the code and could find that:
> 
> 1.  surface is not destroyed until the decode finishes and calls avcodec_close;
> 2. refs[i] is passed to driver, however in DdiMediaBase::GetRenderTargetID,
> driver failed to
> find the expected surface in the re-created m_ddiDecodeCtx->RTtbl, and
> returns invalid.
> 		if(rtTbl->pRT[i] == surface) {
> 			return i;
> 		}
>  		return DDI_CODEC_INVALID_FRAME_INDEX;
> 
> One possible way is to register the refs[] surfaces in the new created context
> ->RTtbl and make it
> findable.

Attempt  to register refs[] surface when re-creating context, and observed that refs[] surface
has been registered into RTtbl and is able to accessed in driver.
https://github.com/fulinjie/ffmpeg/commit/9ed1a309786d2e395753ed327b511e6d9779986f

However, the decode still failed to get the correct image.

Working together with  Wang Yan to investigate more in driver.

And Yan found iHD driver will maintain some internal variables and reference link list
(CodeclhalDecodeVp9::m_vp9RefList) in Codec HAL layer which will be destroyed
when destroy VAAPI context.

CodechalDecodeVp9:SetFrameStates() will set resRefPic/dwFrameWidth/dwFrameHeight
of CodeclhalDecodeVp9::m_vp9RefList. And CodechalDecodeVp9::~ CodechalDecodeVp9()
will destroy it when VAAPI context is destroyed. So the new VAAPI context will only include
an empty CodeclhalDecodeVp9::m_vp9RefList.

It seems that passing surface ids to driver when recreate context could not fix the dynamic
resolution changing issue simply.

Refined a new patch to be less hacky.
And it fixes more than 1000 dynamic resolution changing cases during my test.

It's obvious that currently context in driver is not designed for remap internal variable
from previous context. 
If driver could not cope with this easily, I think we may make it more robust in ffmpeg vaapi vp9.

- linjie