[FFmpeg-devel] [PATCH, v2 2/2] lavc/vaapi_decode: recreate hw_frames_ctx for vp9 without destroy va_context
Fu, Linjie
linjie.fu at intel.com
Tue Aug 6 11:55:09 EEST 2019
> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Hendrik Leppkes
> Sent: Tuesday, August 6, 2019 16:27
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at 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.fu at 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.fu at 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.
> > diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> > index 5096ffa..7adef08 100644
> > --- a/libavcodec/internal.h
> > +++ b/libavcodec/internal.h
> > @@ -188,6 +188,7 @@ typedef struct AVCodecInternal {
> > * hwaccel-specific private data
> > */
> > void *hwaccel_priv_data;
> > + int hwaccel_priv_data_keeping;
> >
> > /**
> > * checks API usage: after codec draining, flush is required to resume
> operation
> > diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> > index 36ac0ac..6032818 100644
> > --- a/libavcodec/pthread_frame.c
> > +++ b/libavcodec/pthread_frame.c
> > @@ -283,6 +283,7 @@ static int
> update_context_from_thread(AVCodecContext *dst, AVCodecContext *src,
> > dst->sample_fmt = src->sample_fmt;
> > dst->channel_layout = src->channel_layout;
> > dst->internal->hwaccel_priv_data = src->internal->hwaccel_priv_data;
> > + dst->internal->hwaccel_priv_data_keeping = src->internal-
> >hwaccel_priv_data_keeping;
> >
> > if (!!dst->hw_frames_ctx != !!src->hw_frames_ctx ||
> > (dst->hw_frames_ctx && dst->hw_frames_ctx->data != src-
> >hw_frames_ctx->data)) {
> > @@ -340,6 +341,7 @@ static int
> update_context_from_user(AVCodecContext *dst, AVCodecContext *src)
> > dst->frame_number = src->frame_number;
> > dst->reordered_opaque = src->reordered_opaque;
> > dst->thread_safe_callbacks = src->thread_safe_callbacks;
> > + dst->internal->hwaccel_priv_data_keeping = src->internal-
> >hwaccel_priv_data_keeping;
> >
> > if (src->slice_count && src->slice_offset) {
> > if (dst->slice_count < src->slice_count) {
> > 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
> >
>
> More unrelated whitespace
Same, another redundant whitespace.
A separate patch will be more acceptable?
- linjie
More information about the ffmpeg-devel
mailing list