> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf > Of Fu, Linjie > Sent: Wednesday, September 11, 2019 00:02 > To: ffmpeg-devel@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-boun...@ffmpeg.org] On > Behalf > > Of Mark Thompson > > Sent: Tuesday, September 10, 2019 08:02 > > To: ffmpeg-devel@ffmpeg.org > > Subject: Re: [FFmpeg-devel] [PATCH, v2 2/2] lavc/vaapi_decode: recreate > > hw_frames_ctx for vp9 without destroy va_context > > > > On 09/09/2019 16:40, Fu, Linjie wrote: > > >> -----Original Message----- > > >> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On > > Behalf > > >> Of Fu, Linjie > > >> Sent: Friday, August 30, 2019 16:05 > > >> To: FFmpeg development discussions and patches <ffmpeg- > > >> de...@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-boun...@ffmpeg.org] On > > >> Behalf > > >>> Of Fu, Linjie > > >>> Sent: Friday, August 9, 2019 19:47 > > >>> To: FFmpeg development discussions and patches <ffmpeg- > > >>> de...@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-boun...@ffmpeg.org] On > > >>> Behalf > > >>>> Of Hendrik Leppkes > > >>>> Sent: Friday, August 9, 2019 17:40 > > >>>> To: FFmpeg development discussions and patches <ffmpeg- > > >>>> de...@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...@intel.com> > wrote: > > >>>>> > > >>>>>> -----Original Message----- > > >>>>>> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] > > On > > >>>> Behalf > > >>>>>> Of Hendrik Leppkes > > >>>>>> Sent: Tuesday, August 6, 2019 16:27 > > >>>>>> To: FFmpeg development discussions and patches <ffmpeg- > > >>>>>> de...@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...@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...@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? > > >> > > > > > > Ping? > > > A general solution works for multitude situation is great to me, and how > > about having > > > one solution specific for vp9 which introduces no regression as the first > > step, > > > since there are lots of cases(1400 +) failed/blocked and could be fixed by > > this patch. > > > > > > This blocked quite a lot, please comment what I can do to get this step > > further. > > > > I still don't understand how the error here can be in FFmpeg - it looks more > > like a driver problem to me. > > > > The sequence with VAAPI using HW_CONFIG_METHOD_HW_DEVICE_CTX > on > > your lena_resolution_change_on_inter_frame.ivf should be as follows: > > > > 1. The header of frame 0 is read, it's a key frame with resolution is > > 352x288. > > 2. The user-supplied get_format() is called, they pick AV_PIX_FMT_VAAPI > > and supply a VAAPI device. > > 3. Output/reference surfaces are created in a new frames context at > > 352x288. > > 4. A decoder context is created for VP9 at 352x288, rendering to the > surfaces > > created in the previous step. > > 5. Frame 0 is decoded and placed in all reference slots. > > 6. Frames 1-49 are decoded normally, they overwrite slots 0 and 1 only. > > 7. The header of frame 50 is read, it's an inter frame but with a new > > resolution of 240x196. > > 8. The old decoder context is discarded, since it has the wrong resolution > and > > is bound to the wrong render targets. > > 9. The old frames context is unreferenced, but references remain to its > > frames in slots 2-7 so the actual frames themselves stay around. > > 10. The user-supplied get_format() is called, they pick AV_PIX_FMT_VAAPI > > again. > > 11. Output/reference surfaces are created in a new frames context at > > 240x196. > > 12. A new decoder context is created for VP9 at 240x196, rendering to the > > new surfaces. > > 13. Frame 50 is decoded with reference to frame slots 0, 1 and 2 (those are > all > > in the old frames context and have the old resolution); the result is placed > in > > slots 0 and 1. > > 14. Frames 51-100 are all decoded with reference to slots 0, 1 and 2, > > overwriting slots 0 and/or 1 only (in every case slot 2 still contains the > original > > key frame). > > > > (Using HW_CONFIG_METHOD_HW_FRAMES_CTX the main difference is > that > > steps 3 and 11 would be removed, replaced by user action in the > get_format() > > callback in the steps immediately preceding them.) > > > > The iHD driver indeed returns "internal error" immediately on step 13. > > However, looking at traces made with libva everything about that render > call > > looks correct - the decoder context is the new one which matches the > > resolution and render target, and the right surfaces are provided as > > reference frames (in the old frames context, but definitely haven't been > > destroyed). > > > > So, can you explain more about what is going wrong? > > > > Hi Mark, > > Thanks for the detailed response. If I got it correctly, the concern is mainly > about > "since the reference surface is definitely not destroyed, destroy/recreate > context > shouldn't have blocked the decode for vp9" > > Actually, there are some intermedia dependencies in driver beside reference > frame itself. > For example, > 1. Motion vector dependency. > > As chapter 5.13 (motion vector prediction )in VP9 spec has said: > "When the spatial neighbors do not provide sufficient information, it can fall > back to using the > Motion vectors from the previous decoded frames". > > MV buffer is the dependency across DPB, driver have to allocate the internal > buffer to store the > MV information associated to each frame on DPB. > We need previous frame’s mv, and we hold mv in decoder context. > Destroying the context would > cause the loss of mv information. > > 2.Besides, segmentation map buffer is the another dependency. > > It is allocated in driver as well, and could be updated by every frame and > impact next frame. > The segmentation map is coded differentially across frames in order to > minimize the bit overheads. > > As a consequence, context destroying would block the decode for clips with > resolution changing on > inter frames. > A friendly ping in case this message is missed.
- linjie _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".