| 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 | expand |
| 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 |
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
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.
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
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".
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".
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
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 --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;
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(-)