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 Fu, Linjie
> Sent: Monday, July 8, 2019 16:11
> To: FFmpeg development discussions and patches de...@ffmpeg.org>
> 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 Yan Wang
> > Sent: Monday, July 8, 2019 15:54
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH 2/2] lavc/vaapi_decode: recreate
> > hw_frames_ctx without destroy va_context
> >
> >
> > 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-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.
> > >
> > > 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.
> >
>
> Yes, tracing the code and could find that:
>
> 1. surface is not destroyed until the decode finishes and calls
> avcodec_close;
> 2. refs[i] is passed to driver, however in DdiMediaBase::GetRenderTargetID,
> driver failed to
> find the expected surface in the re-created m_ddiDecodeCtx->RTtbl, and
> returns invalid.
> if(rtTbl->pRT[i] == surface) {
> return i;
> }
> return DDI_CODEC_INVALID_FRAME_INDEX;
>
> One possible way is to register the refs[] surfaces in the new created context
> ->RTtbl and make it
> findable.
Attempt to register refs[] surface when re-creating context, and observed that
refs[] surface
has been registered into RTtbl and is able to accessed in driver.
https://github.com/fulinjie/ffmpeg/commit/9ed1a309786d2e395753ed327b511e6d9779986f
However, the decode still failed to get the correct image.
Working together with Wang Yan to investigate more in driver.
And Yan found iHD driver will maintain some internal variables and reference
link list
(CodeclhalDecodeVp9::m_vp9RefList) in Codec HAL layer which will be destroyed
when destroy VAAPI context.
CodechalDecodeVp9:SetFrameStates() will set resRefPic/dwFrameWidth/dwFrameHeight
of CodeclhalDecodeVp9::m_vp9RefList. And CodechalDecodeVp9::~
CodechalDecodeVp9()
will destroy it when VAAPI context is destroyed. So the new VAAPI context will
onl
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 Yan Wang
> Sent: Monday, July 8, 2019 15:54
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] lavc/vaapi_decode: recreate
> hw_frames_ctx without destroy va_context
>
>
> 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-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.
> >
> > 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.
>
Yes, tracing the code and could find that:
1. surface is not destroyed until the decode finishes and calls avcodec_close;
2. refs[i] is passed to driver, however in DdiMediaBase::GetRenderTargetID,
driver failed to
find the expected surface in the re-created m_ddiDecodeCtx->RTtbl, and returns
invalid.
if(rtTbl->pRT[i] == surface) {
return i;
}
return DDI_CODEC_INVALID_FRAME_INDEX;
One possible way is to register the refs[] surfaces in the new created context
->RTtbl and make it
findable.
Thanks for reviews and all comments.
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".
Re: [FFmpeg-devel] [PATCH 2/2] lavc/vaapi_decode: recreate hw_frames_ctx without destroy va_context
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-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.
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
---
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_R
Re: [FFmpeg-devel] [PATCH 2/2] lavc/vaapi_decode: recreate hw_frames_ctx without destroy va_context
On 7/7/2019 9:49 PM, Fu, Linjie wrote:
-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.
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.
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
---
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
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 Fu, Linjie
> Sent: Sunday, July 7, 2019 21:49
> To: FFmpeg development discussions and patches de...@ffmpeg.org>
> Cc: Mark Thompson
> 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
> > > ---
> > > 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
> > > --
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 Hendrik Leppkes
> Sent: Sunday, July 7, 2019 15:39
> To: FFmpeg development discussions and patches de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] lavc/vaapi_decode: recreate
> hw_frames_ctx without destroy va_context
>
> On Sun, Jul 7, 2019 at 6:41 AM Linjie Fu 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.
> >
> > 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
> > ---
> > 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)
> > +hwaccel_uninit(avctx);
> > user_choice = avctx->get_format(avctx, choices);
>
> Having VAAPI specific code in this generic code path (or any hwaccel
> specific code) seems like a design flaw, and should perhaps be
> adressed by a generic improvement, instead of VAAPI specific hacks.
>
Yes, a better solution(maybe a flag set by reinit in vaapi) should be applied.
Thanks for your response.
___
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".
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
> > ---
> > 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;
>
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.
> 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.
> 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
> ---
> 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.
> +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,
> - >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,
> +
Re: [FFmpeg-devel] [PATCH 2/2] lavc/vaapi_decode: recreate hw_frames_ctx without destroy va_context
On Sun, Jul 7, 2019 at 6:41 AM Linjie Fu 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.
>
> 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
> ---
> 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)
> +hwaccel_uninit(avctx);
> user_choice = avctx->get_format(avctx, choices);
Having VAAPI specific code in this generic code path (or any hwaccel
specific code) seems like a design flaw, and should perhaps be
adressed by a generic improvement, instead of VAAPI specific hacks.
- Hendrik
___
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".
[FFmpeg-devel] [PATCH 2/2] lavc/vaapi_decode: recreate hw_frames_ctx without destroy va_context
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.
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
---
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)
+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,
- >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,
+ >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 FF_API_STRUCT_VAAPI_CONTEXT
}
#endif
--
2.7.4
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
