[FFmpeg-devel] [PATCH 2/2] lavc/vaapi_decode: recreate hw_frames_ctx without destroy va_context
Fu, Linjie
linjie.fu at intel.com
Sun Jul 7 17:15:23 EEST 2019
> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Fu, Linjie
> Sent: Sunday, July 7, 2019 21:49
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Cc: Mark Thompson <sw at 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-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.
>
> >
> > > 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.
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpeg-20190707-210559.log
Type: application/octet-stream
Size: 27862 bytes
Desc: ffmpeg-20190707-210559.log
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190707/05069279/attachment.obj>
More information about the ffmpeg-devel
mailing list