[FFmpeg-devel] [PATCH 2/2] lavc/vaapi_decode: recreate hw_frames_ctx without destroy va_context
Yan Wang
yan.wang at linux.intel.com
Mon Jul 8 10:54:15 EEST 2019
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.
Thanks.
Yan Wang
>
> So the new va_context cannot find the corresponding surface based on
> this surface ID.
>
> IMHO, one possible solution is to create one the VA surfaces including
> VP9Context.s.refs[] data which is AVFrame in fact and pass them into
> libva when re-creating new va_context.
>
> Thanks.
>
> Yan Wang
>
>>
>>>> As libva allows re-create surface separately without changing the
>>>> context, this issue could be handled by only recreating hw_frames_ctx.
>>>>
>>>> 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 | 8 ++++----
>>>> libavcodec/vaapi_decode.c | 40
>>>> ++++++++++++++++++++++----------------
>>> --
>>>> 2 files changed, 26 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
>>>> index 0863b82..a81b857 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.
>>>> @@ -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)
>>>> @@ -1397,8 +1396,9 @@ int ff_get_format(AVCodecContext *avctx, const
>>> enum AVPixelFormat *fmt)
>>>> for (;;) {
>>>> // Remove the previous hwaccel, if there was one.
>>>> - hwaccel_uninit(avctx);
>>>> -
>>>> + // VAAPI allows reinit hw_frames_ctx only
>>>> + if (choices[0] != AV_PIX_FMT_VAAPI_VLD)
>>> Including codec-specific conditions in the generic code suggests that
>>> something very shady is going on.
>> Yes, will try to avoid this.
>>
>>>> + hwaccel_uninit(avctx);> user_choice = avctx-
>>>> get_format(avctx, choices);
>>>> if (user_choice == AV_PIX_FMT_NONE) {
>>>> // Explicitly chose nothing, give up.
>>>> diff --git a/libavcodec/vaapi_decode.c b/libavcodec/vaapi_decode.c
>>>> index 69512e1..13f0cf0 100644
>>>> --- a/libavcodec/vaapi_decode.c
>>>> +++ b/libavcodec/vaapi_decode.c
>>>> @@ -613,8 +613,10 @@ int ff_vaapi_decode_init(AVCodecContext *avctx)
>>>> VAStatus vas;
>>>> int err;
>>>>
>>>> - ctx->va_config = VA_INVALID_ID;
>>>> - ctx->va_context = VA_INVALID_ID;
>>>> + if (!ctx->va_config && !ctx->va_context){
>>>> + ctx->va_config = VA_INVALID_ID;
>>>> + ctx->va_context = VA_INVALID_ID;
>>>> + }
>>>>
>>>> #if FF_API_STRUCT_VAAPI_CONTEXT
>>>> if (avctx->hwaccel_context) {
>>>> @@ -642,7 +644,6 @@ int ff_vaapi_decode_init(AVCodecContext *avctx)
>>>> // present, so set it here to avoid the behaviour changing.
>>>> ctx->hwctx->driver_quirks =
>>>> AV_VAAPI_DRIVER_QUIRK_RENDER_PARAM_BUFFERS;
>>>> -
>>>> }
>>>> #endif
>>>>
>>>> @@ -655,7 +656,8 @@ int ff_vaapi_decode_init(AVCodecContext *avctx)
>>>> "context: %#x/%#x.\n", ctx->va_config,
>>>> ctx->va_context);
>>>> } else {
>>>> #endif
>>>> -
>>>> + // Get a new hw_frames_ctx even if there is already one
>>>> + // recreate surface without reconstuct va_context
>>>> err = ff_decode_get_hw_frames_ctx(avctx,
>>> AV_HWDEVICE_TYPE_VAAPI);
>>>> if (err < 0)
>>>> goto fail;
>>>> @@ -670,21 +672,23 @@ int ff_vaapi_decode_init(AVCodecContext *avctx)
>>>> if (err)
>>>> goto fail;
>>>>
>>>> - vas = vaCreateContext(ctx->hwctx->display, ctx->va_config,
>>>> - avctx->coded_width, avctx->coded_height,
>>>> - VA_PROGRESSIVE,
>>>> - ctx->hwfc->surface_ids,
>>>> - ctx->hwfc->nb_surfaces,
>>>> - &ctx->va_context);
>>>> - if (vas != VA_STATUS_SUCCESS) {
>>>> - av_log(avctx, AV_LOG_ERROR, "Failed to create decode "
>>>> - "context: %d (%s).\n", vas, vaErrorStr(vas));
>>>> - err = AVERROR(EIO);
>>>> - goto fail;
>>>> - }
>>>> + if (ctx->va_context == VA_INVALID_ID) {
>>>> + vas = vaCreateContext(ctx->hwctx->display, ctx->va_config,
>>>> + avctx->coded_width,
>>>> avctx->coded_height,
>>>> + VA_PROGRESSIVE,
>>>> + ctx->hwfc->surface_ids,
>>>> + ctx->hwfc->nb_surfaces,
>>>> + &ctx->va_context);
>>>> + if (vas != VA_STATUS_SUCCESS) {
>>>> + av_log(avctx, AV_LOG_ERROR, "Failed to create decode "
>>>> + "context: %d (%s).\n", vas, vaErrorStr(vas));
>>>> + err = AVERROR(EIO);
>>>> + goto fail;
>>>> + }
>>>>
>>>> - av_log(avctx, AV_LOG_DEBUG, "Decode context initialised: "
>>>> - "%#x/%#x.\n", ctx->va_config, ctx->va_context);
>>>> + av_log(avctx, AV_LOG_DEBUG, "Decode context initialised: "
>>>> + "%#x/%#x.\n", ctx->va_config, ctx->va_context);
>>>> + }
>>> If I'm reading this correctly, this changes to only creating one
>>> context ever
>>> even when the resolution changes.
>>>
>>> How can that work if the resolution increases? For example you start
>>> decoding at 1280x720 and create a context for that, then the resolution
>>> changes to 3840x2160 and it tries to decode using the 1280x720 context.
>>>
>> Recreating hw_frame_ctx can cope with this.
>> Verified in one case with resolution increasing:
>> Resolution changes from 495x168 -> 328 x 307 -> 395 x376, the
>> pipeline works
>> and the output image is good.
>
> As my understanding, you decoding output should be 495x168 always for
> every frame. Right?
>
> When the resolution of the decoded frame is different with the first
> frame, the scaling is fired.
>
> The scaling should come from media driver VP pipeline because ffmpeg
> use vaGetImage() to get decoded frame.
>
> This is dependent on ffmpeg requirement. If ffmpeg hopes to get
> original decoded frame, this behavior should be changed.
>
> Thanks.
>
> Yan Wang
>
>> It's not increasing on both width and height, but as long as one of
>> them increases,
>> current pipeline will be broken ff we don’t reinit the context(or
>> just don't recreate
>> hw_frame_ctx).
>> (also did experiment as a contrast that we use the same context
>> without recreate
>> hw_frame_ctx, the pipeline will be blocked when mapping surface in
>> resolution increase)
>>
>> ffmpeg -hwaccel vaapi -hwaccel_device /dev/dri/renderD128 -v verbose
>> -i ./test3232.ivf
>> -pix_fmt p010le -f rawvideo -vf scale=w=416:h=168 -vframes 10 -y md5.yuv
>>
>> log is attached.
>>
>> Thanks.
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list