> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf > Of Fu, Linjie > Sent: Sunday, July 7, 2019 21:49 > To: FFmpeg development discussions and patches <ffmpeg- > de...@ffmpeg.org> > Cc: Mark Thompson <s...@jkqxz.net> > 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-boun...@ffmpeg.org] On > Behalf > > Of Mark Thompson > > Sent: Sunday, July 7, 2019 19:51 > > To: ffmpeg-devel@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. > > > > > > 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...@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. > 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.
Update the log, sorry for the inconvenience. - Linjie
ffmpeg-20190707-210559.log
Description: ffmpeg-20190707-210559.log
_______________________________________________ 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".