diff mbox series

[FFmpeg-devel,v1] avcodec/vaapi_av1: correct data size when create slice data buffer

Message ID 20210517015036.22156-1-fei.w.wang@intel.com
State New
Headers show
Series [FFmpeg-devel,v1] avcodec/vaapi_av1: correct data size when create slice data buffer
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Wang, Fei W May 17, 2021, 1:50 a.m. UTC
Set all tiles size to create slice data buffer, hardware will use
slice_data_offset/slice_data_size in slice parameter buffer to get
each tile's data.

This change will let it success to decode clip which has multi
tiles data inside one OBU.

Signed-off-by: Fei Wang <fei.w.wang@intel.com>
---
 libavcodec/vaapi_av1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Xiang, Haihao May 17, 2021, 2:09 a.m. UTC | #1
On Mon, 2021-05-17 at 09:50 +0800, Fei Wang wrote:
> Set all tiles size to create slice data buffer, hardware will use
> slice_data_offset/slice_data_size in slice parameter buffer to get
> each tile's data.
> 
> This change will let it success to decode clip which has multi
> tiles data inside one OBU.
> 
> Signed-off-by: Fei Wang <fei.w.wang@intel.com>
> ---
>  libavcodec/vaapi_av1.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/vaapi_av1.c b/libavcodec/vaapi_av1.c
> index 1809b485aa..16b7e35747 100644
> --- a/libavcodec/vaapi_av1.c
> +++ b/libavcodec/vaapi_av1.c
> @@ -292,7 +292,7 @@ static int vaapi_av1_decode_slice(AVCodecContext *avctx,
>          err = ff_vaapi_decode_make_slice_buffer(avctx, pic, &slice_param,
>                                                  sizeof(VASliceParameterBuffer
> AV1),
>                                                  buffer,
> -                                                s-
> >tile_group_info[i].tile_size);
> +                                                size);
>          if (err) {
>              ff_vaapi_decode_cancel(avctx, pic);
>              return err;

LGTM

Thanks
Haihao
Wolfgang Haupt May 17, 2021, 7:28 a.m. UTC | #2
On 17.05.21 03:50, Fei Wang wrote:
> Set all tiles size to create slice data buffer, hardware will use
> slice_data_offset/slice_data_size in slice parameter buffer to get
> each tile's data.
>
> This change will let it success to decode clip which has multi
> tiles data inside one OBU.
>
> Signed-off-by: Fei Wang <fei.w.wang@intel.com>
> ---
>  libavcodec/vaapi_av1.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/vaapi_av1.c b/libavcodec/vaapi_av1.c
> index 1809b485aa..16b7e35747 100644
> --- a/libavcodec/vaapi_av1.c
> +++ b/libavcodec/vaapi_av1.c
> @@ -292,7 +292,7 @@ static int vaapi_av1_decode_slice(AVCodecContext *avctx,
>          err = ff_vaapi_decode_make_slice_buffer(avctx, pic, &slice_param,
>                                                  sizeof(VASliceParameterBufferAV1),
>                                                  buffer,
> -                                                s->tile_group_info[i].tile_size);
> +                                                size);
>          if (err) {
>              ff_vaapi_decode_cancel(avctx, pic);
>              return err;

+1, I can confirm that this fix works using mpv/kodi.
Jan Ekström May 17, 2021, 8:59 a.m. UTC | #3
On Mon, May 17, 2021 at 4:50 AM Fei Wang <fei.w.wang@intel.com> wrote:
>
> Set all tiles size to create slice data buffer, hardware will use
> slice_data_offset/slice_data_size in slice parameter buffer to get
> each tile's data.
>
> This change will let it success to decode clip which has multi
> tiles data inside one OBU.
>
> Signed-off-by: Fei Wang <fei.w.wang@intel.com>
> ---
>  libavcodec/vaapi_av1.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/vaapi_av1.c b/libavcodec/vaapi_av1.c
> index 1809b485aa..16b7e35747 100644
> --- a/libavcodec/vaapi_av1.c
> +++ b/libavcodec/vaapi_av1.c
> @@ -292,7 +292,7 @@ static int vaapi_av1_decode_slice(AVCodecContext *avctx,
>          err = ff_vaapi_decode_make_slice_buffer(avctx, pic, &slice_param,
>                                                  sizeof(VASliceParameterBufferAV1),
>                                                  buffer,
> -                                                s->tile_group_info[i].tile_size);
> +                                                size);
>          if (err) {
>              ff_vaapi_decode_cancel(avctx, pic);
>              return err;
> --
> 2.17.1

So basically this fixes setting the size of the passed buffer to the
whole size of the buffer (in which tile offset and tile size should
always be located). As such it makes sense.

It seems like at some point there was an idea to only pass "buffer +
tile_offset", but that never materialized completely (and then setting
VASliceParameterBufferAV1's tile_offset to 0)? Would that lead to
smaller buffers being utilized? Or does libva already extract the
tiles into their own buffers based on the information?

Jan
Xiang, Haihao May 18, 2021, 1:55 a.m. UTC | #4
On Mon, 2021-05-17 at 11:59 +0300, Jan Ekström wrote:
> On Mon, May 17, 2021 at 4:50 AM Fei Wang <fei.w.wang@intel.com> wrote:
> > 
> > Set all tiles size to create slice data buffer, hardware will use
> > slice_data_offset/slice_data_size in slice parameter buffer to get
> > each tile's data.
> > 
> > This change will let it success to decode clip which has multi
> > tiles data inside one OBU.
> > 
> > Signed-off-by: Fei Wang <fei.w.wang@intel.com>
> > ---
> >  libavcodec/vaapi_av1.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/vaapi_av1.c b/libavcodec/vaapi_av1.c
> > index 1809b485aa..16b7e35747 100644
> > --- a/libavcodec/vaapi_av1.c
> > +++ b/libavcodec/vaapi_av1.c
> > @@ -292,7 +292,7 @@ static int vaapi_av1_decode_slice(AVCodecContext *avctx,
> >          err = ff_vaapi_decode_make_slice_buffer(avctx, pic, &slice_param,
> >                                                  sizeof(VASliceParameterBuff
> > erAV1),
> >                                                  buffer,
> > -                                                s-
> > >tile_group_info[i].tile_size);
> > +                                                size);
> >          if (err) {
> >              ff_vaapi_decode_cancel(avctx, pic);
> >              return err;
> > --
> > 2.17.1
> 
> So basically this fixes setting the size of the passed buffer to the
> whole size of the buffer (in which tile offset and tile size should
> always be located). As such it makes sense.
> 
> It seems like at some point there was an idea to only pass "buffer +
> tile_offset", but that never materialized completely (and then setting
> VASliceParameterBufferAV1's tile_offset to 0)? 

Yes, it also works.

> Would that lead to smaller buffers being utilized? Or does libva already
> extract the tiles into their own buffers based on the information?

libva doesn't extract the tiles, it is up to the underlying backend driver.

Thanks
Haihao



> 
> Jan
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Wang, Fei W May 18, 2021, 2:54 a.m. UTC | #5
On Mon, 2021-05-17 at 11:59 +0300, Jan Ekström wrote:
> On Mon, May 17, 2021 at 4:50 AM Fei Wang <fei.w.wang@intel.com>
> wrote:
> > 
> > Set all tiles size to create slice data buffer, hardware will use
> > slice_data_offset/slice_data_size in slice parameter buffer to get
> > each tile's data.
> > 
> > This change will let it success to decode clip which has multi
> > tiles data inside one OBU.
> > 
> > Signed-off-by: Fei Wang <fei.w.wang@intel.com>
> > ---
> >  libavcodec/vaapi_av1.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/vaapi_av1.c b/libavcodec/vaapi_av1.c
> > index 1809b485aa..16b7e35747 100644
> > --- a/libavcodec/vaapi_av1.c
> > +++ b/libavcodec/vaapi_av1.c
> > @@ -292,7 +292,7 @@ static int
> > vaapi_av1_decode_slice(AVCodecContext *avctx,
> >          err = ff_vaapi_decode_make_slice_buffer(avctx, pic,
> > &slice_param,
> >                                                  sizeof(VASlicePara
> > meterBufferAV1),
> >                                                  buffer,
> > -                                                s-
> > >tile_group_info[i].tile_size);
> > +                                                size);
> >          if (err) {
> >              ff_vaapi_decode_cancel(avctx, pic);
> >              return err;
> > --
> > 2.17.1
> 
> So basically this fixes setting the size of the passed buffer to the
> whole size of the buffer (in which tile offset and tile size should
> always be located). As such it makes sense.
> 
> It seems like at some point there was an idea to only pass "buffer +
> tile_offset", but that never materialized completely (and then
> setting
> VASliceParameterBufferAV1's tile_offset to 0)? Would that lead to
> smaller buffers being utilized? Or does libva already extract the
> tiles into their own buffers based on the information?

Buffer+tile_offset can fix the issue too. I chose change the size only
is to try to follow current logic and do the smallest change to fix
this.

With buffer+tile_offset way, the buffers will be smaller, but the
effect can be ignore, since bitstream size is small. For 8K resolution
bitstream, its each frame size is only ~100KB.

Fei
Thanks

> 
> Jan
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Jan Ekström May 18, 2021, 9:26 a.m. UTC | #6
On Tue, May 18, 2021 at 5:55 AM Wang, Fei W <fei.w.wang@intel.com> wrote:
>
> On Mon, 2021-05-17 at 11:59 +0300, Jan Ekström wrote:
> > On Mon, May 17, 2021 at 4:50 AM Fei Wang <fei.w.wang@intel.com>
> > wrote:
> > >
> > > Set all tiles size to create slice data buffer, hardware will use
> > > slice_data_offset/slice_data_size in slice parameter buffer to get
> > > each tile's data.
> > >
> > > This change will let it success to decode clip which has multi
> > > tiles data inside one OBU.
> > >
> > > Signed-off-by: Fei Wang <fei.w.wang@intel.com>
> > > ---
> > >  libavcodec/vaapi_av1.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/libavcodec/vaapi_av1.c b/libavcodec/vaapi_av1.c
> > > index 1809b485aa..16b7e35747 100644
> > > --- a/libavcodec/vaapi_av1.c
> > > +++ b/libavcodec/vaapi_av1.c
> > > @@ -292,7 +292,7 @@ static int
> > > vaapi_av1_decode_slice(AVCodecContext *avctx,
> > >          err = ff_vaapi_decode_make_slice_buffer(avctx, pic,
> > > &slice_param,
> > >                                                  sizeof(VASlicePara
> > > meterBufferAV1),
> > >                                                  buffer,
> > > -                                                s-
> > > >tile_group_info[i].tile_size);
> > > +                                                size);
> > >          if (err) {
> > >              ff_vaapi_decode_cancel(avctx, pic);
> > >              return err;
> > > --
> > > 2.17.1
> >
> > So basically this fixes setting the size of the passed buffer to the
> > whole size of the buffer (in which tile offset and tile size should
> > always be located). As such it makes sense.
> >
> > It seems like at some point there was an idea to only pass "buffer +
> > tile_offset", but that never materialized completely (and then
> > setting
> > VASliceParameterBufferAV1's tile_offset to 0)? Would that lead to
> > smaller buffers being utilized? Or does libva already extract the
> > tiles into their own buffers based on the information?
>
> Buffer+tile_offset can fix the issue too. I chose change the size only
> is to try to follow current logic and do the smallest change to fix
> this.
>

Yes, and as I noted it makes sense. It was just interesting to see how
these two things seemed to be intermixed.

Functionally this patch is LGTM, maybe something a la

"""
avcodec/vaapi_av1: pass full buffer size for each tile

Previously, only the size of a given tile was passed, making the offset
and size marked in VASliceParameterBufferAV1 invalid with multiple tiles.
"""

for the commit message?

This should probably then also be back-ported to release/4.4 as I
think 3308bbf7761a2723f784fcd6bb921dcbadc18c6e (original addition of
the vaapi AV1 hwaccel is included in it).

Thanks.

> With buffer+tile_offset way, the buffers will be smaller, but the
> effect can be ignore, since bitstream size is small. For 8K resolution
> bitstream, its each frame size is only ~100KB.
>
> Fei

Yup. Of course this depends on the bit rate of the stream, but I do
not consider this a required change.

Jan
Wang, Fei W May 19, 2021, 2:19 a.m. UTC | #7
On Tue, 2021-05-18 at 12:26 +0300, Jan Ekström wrote:
> On Tue, May 18, 2021 at 5:55 AM Wang, Fei W <fei.w.wang@intel.com>
> wrote:
> > 
> > On Mon, 2021-05-17 at 11:59 +0300, Jan Ekström wrote:
> > > On Mon, May 17, 2021 at 4:50 AM Fei Wang <fei.w.wang@intel.com>
> > > wrote:
> > > > 
> > > > Set all tiles size to create slice data buffer, hardware will
> > > > use
> > > > slice_data_offset/slice_data_size in slice parameter buffer to
> > > > get
> > > > each tile's data.
> > > > 
> > > > This change will let it success to decode clip which has multi
> > > > tiles data inside one OBU.
> > > > 
> > > > Signed-off-by: Fei Wang <fei.w.wang@intel.com>
> > > > ---
> > > >  libavcodec/vaapi_av1.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/libavcodec/vaapi_av1.c b/libavcodec/vaapi_av1.c
> > > > index 1809b485aa..16b7e35747 100644
> > > > --- a/libavcodec/vaapi_av1.c
> > > > +++ b/libavcodec/vaapi_av1.c
> > > > @@ -292,7 +292,7 @@ static int
> > > > vaapi_av1_decode_slice(AVCodecContext *avctx,
> > > >          err = ff_vaapi_decode_make_slice_buffer(avctx, pic,
> > > > &slice_param,
> > > >                                                  sizeof(VASlice
> > > > Para
> > > > meterBufferAV1),
> > > >                                                  buffer,
> > > > -                                                s-
> > > > > tile_group_info[i].tile_size);
> > > > 
> > > > +                                                size);
> > > >          if (err) {
> > > >              ff_vaapi_decode_cancel(avctx, pic);
> > > >              return err;
> > > > --
> > > > 2.17.1
> > > 
> > > So basically this fixes setting the size of the passed buffer to
> > > the
> > > whole size of the buffer (in which tile offset and tile size
> > > should
> > > always be located). As such it makes sense.
> > > 
> > > It seems like at some point there was an idea to only pass
> > > "buffer +
> > > tile_offset", but that never materialized completely (and then
> > > setting
> > > VASliceParameterBufferAV1's tile_offset to 0)? Would that lead to
> > > smaller buffers being utilized? Or does libva already extract the
> > > tiles into their own buffers based on the information?
> > 
> > Buffer+tile_offset can fix the issue too. I chose change the size
> > only
> > is to try to follow current logic and do the smallest change to fix
> > this.
> > 
> 
> Yes, and as I noted it makes sense. It was just interesting to see
> how
> these two things seemed to be intermixed.
> 
> Functionally this patch is LGTM, maybe something a la
> 
> """
> avcodec/vaapi_av1: pass full buffer size for each tile
> 
> Previously, only the size of a given tile was passed, making the
> offset
> and size marked in VASliceParameterBufferAV1 invalid with multiple
> tiles.
> """
> 
> for the commit message?

Thanks, I can update the commit message like this.

> 
> This should probably then also be back-ported to release/4.4 as I
> think 3308bbf7761a2723f784fcd6bb921dcbadc18c6e (original addition of
> the vaapi AV1 hwaccel is included in it).

Yes, this is a important fix. Appreciate if you who has write access
can help back-port it to release/4.4.(The patch can be cherry-pick to
release/4.4 without any conflict)

Fei
Thanks

> 
> Thanks.
> 
> > With buffer+tile_offset way, the buffers will be smaller, but the
> > effect can be ignore, since bitstream size is small. For 8K
> > resolution
> > bitstream, its each frame size is only ~100KB.
> > 
> > Fei
> 
> Yup. Of course this depends on the bit rate of the stream, but I do
> not consider this a required change.
> 
> Jan
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavcodec/vaapi_av1.c b/libavcodec/vaapi_av1.c
index 1809b485aa..16b7e35747 100644
--- a/libavcodec/vaapi_av1.c
+++ b/libavcodec/vaapi_av1.c
@@ -292,7 +292,7 @@  static int vaapi_av1_decode_slice(AVCodecContext *avctx,
         err = ff_vaapi_decode_make_slice_buffer(avctx, pic, &slice_param,
                                                 sizeof(VASliceParameterBufferAV1),
                                                 buffer,
-                                                s->tile_group_info[i].tile_size);
+                                                size);
         if (err) {
             ff_vaapi_decode_cancel(avctx, pic);
             return err;