[FFmpeg-devel] [PATCH 2/2] lavc/vvc: Ensure subpictures don't overlap
Frank Plowman
post at frankplowman.com
Wed Feb 19 22:29:08 EET 2025
Hi,
Thanks for your review.
On 16/02/2025 15:19, Nuo Mi wrote:
> Hi Frank,
> Thank you for the patch.
>
> On Sun, Feb 9, 2025 at 11:45 PM Frank Plowman <post at frankplowman.com> wrote:
>
>> This is essentially a re-implementation of
>>
>> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20241005223955.54158-1-post@frankplowman.com/
>>
>> That patch was not applied last time. Instead we opted to identify
>> issues which could be caused by invalid subpicture layouts and remedy
>> those issues where they manifest, either through error detection or code
>> hardening. This was primarily implemented in the set
>> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=13381.
>>
>> This has worked to some degree, however issues with subpicture layouts
>> continue to crop up from the fuzzer and I've fixed a number of bugs
>> related to subpicture layouts since then. I think it's best to return
>> to the initial plan and simply check if the subpicture layout is valid
>> initially.
>>
>> This implementation is also lighter than the first time -- by doing a
>> bit more logic in pps_subpic_less_than_one_tile_slice, we are able to
>> store a tile_in_subpic map rather than a ctu_in_subpic map. This
>> reduces the size of the map to the point it becomes possible to allocate
>> it on the stack. Similar to 8bd66a8c9587af61c7b46558be3c4ee317c1af5a,
>> the layout is also validated in the slice map construction code, rather
>> than in the CBS, which avoids duplicating some logic.
>>
>> Signed-off-by: Frank Plowman <post at frankplowman.com>
>> ---
>> libavcodec/vvc/ps.c | 52 ++++++++++++++++++++++++++++++++++++---------
>> 1 file changed, 42 insertions(+), 10 deletions(-)
>>
>> diff --git a/libavcodec/vvc/ps.c b/libavcodec/vvc/ps.c
>> index 9480540e03..9af5e1250b 100644
>> --- a/libavcodec/vvc/ps.c
>> +++ b/libavcodec/vvc/ps.c
>> @@ -401,25 +401,47 @@ static void subpic_tiles(int *tile_x, int *tile_y,
>> int *tile_x_end, int *tile_y_
>> (*tile_y_end)++;
>> }
>>
>> -static void pps_subpic_less_than_one_tile_slice(VVCPPS *pps, const VVCSPS
>> *sps, const int i, const int tx, const int ty, int *off)
>> +static int pps_subpic_less_than_one_tile_slice(VVCPPS *pps, const VVCSPS
>> *sps, const int i, const int tx, const int ty, int *off, bool
>> *tile_in_subpic)
>> {
>> + const int subpic_right = sps->r->sps_subpic_ctu_top_left_x[i] +
>> sps->r->sps_subpic_width_minus1[i];
>> + const int subpic_bottom = sps->r->sps_subpic_ctu_top_left_y[i] +
>> sps->r->sps_subpic_height_minus1[i];
>> + const int tile_right = pps->col_bd[tx] + pps->r->col_width_val[tx] -
>> 1;
>> + const int tile_bottom = pps->row_bd[ty] + pps->r->row_height_val[ty]
>> - 1;
>> + const bool is_final_subpic_in_tile = subpic_right == tile_right &&
>> subpic_bottom == tile_bottom;
>> +
>> + if (is_final_subpic_in_tile) {
>> + const size_t tile_idx = ty * VVC_MAX_TILE_COLUMNS + tx;
>
> If we have VVC_MAX_TILES_PER_AU rows. this will overwrite.
> How about tile_idx = ty * pps->r->num_tile_columns + tx?
>
Ah, I'd presumed VVC_MAX_TILE_COLUMNS * VVC_MAX_TILE_ROWS =
VVC_MAX_TILES_PER_AU but I see now this is not the case. Fixed in v2.
>>
>
> + if (tile_in_subpic[tile_idx])
>> + return AVERROR_INVALIDDATA;
>> + tile_in_subpic[tile_idx] = true;
>> + }
>> +
>> pps->num_ctus_in_slice[i] = pps_add_ctus(pps, off,
>> sps->r->sps_subpic_ctu_top_left_x[i],
>> sps->r->sps_subpic_ctu_top_left_y[i],
>> sps->r->sps_subpic_width_minus1[i] + 1,
>> sps->r->sps_subpic_height_minus1[i] + 1);
>> +
>> + return 0;
>> }
>>
>> -static void pps_subpic_one_or_more_tiles_slice(VVCPPS *pps, const int
>> tile_x, const int tile_y, const int x_end, const int y_end, const int i,
>> int *off)
>> +static int pps_subpic_one_or_more_tiles_slice(VVCPPS *pps, const int
>> tile_x, const int tile_y, const int x_end, const int y_end,
>> + const int i, int *off, bool *tile_in_subpic)
>> {
>> for (int ty = tile_y; ty < y_end; ty++) {
>> for (int tx = tile_x; tx < x_end; tx++) {
>> + const size_t tile_idx = ty * VVC_MAX_TILE_COLUMNS + tx;
>> + if (tile_in_subpic[tile_idx])
>> + return AVERROR_INVALIDDATA;
>> + tile_in_subpic[tile_idx] = true;
>> +
>> pps->num_ctus_in_slice[i] += pps_add_ctus(pps, off,
>> pps->col_bd[tx], pps->row_bd[ty],
>> pps->r->col_width_val[tx], pps->r->row_height_val[ty]);
>> }
>> }
>> + return 0;
>> }
>>
>> -static void pps_subpic_slice(VVCPPS *pps, const VVCSPS *sps, const int i,
>> int *off)
>> +static int pps_subpic_slice(VVCPPS *pps, const VVCSPS *sps, const int i,
>> int *off, bool *tile_in_subpic)
>> {
>> int tx, ty, x_end, y_end;
>>
>> @@ -428,19 +450,30 @@ static void pps_subpic_slice(VVCPPS *pps, const
>> VVCSPS *sps, const int i, int *o
>>
>> subpic_tiles(&tx, &ty, &x_end, &y_end, sps, pps, i);
>>
> Calculating tiles for each slice smaller than one tile is inefficient.
> Maybe we can move subpic_tiles() to pps_single_slice_per_subpic(); this
> might simplify the patch.
>
I don't think this will be all that straightforward. In
pps_single_slice_per_subpic we are iterating over slices whose locations
and sizes are determined by explicitly-signaled subpictures. Because
their locations are explicitly-signaled and are therefore pretty much
arbitrary, it will be difficult in pps_single_slice_per_subpic to group
together the slices/subpictures which make up a given tile. I think
it's possible, but by the time you've implemented the logic to figure
that out, I don't think there would be any performance benefit over just
calling subpic_tiles for each subpic.
It's also worth noting that I don't think there are any encoders out
there which actually produce subpictures smaller than a tile, at least
I've never seen a bitstream which exercises
pps_subpic_less_than_one_tile_slice. I think changing
pps_single_slice_per_subpic could run the risk of slowing down the much
more common pps_subpic_one_or_more_tiles_slice.
> if (ty + 1 == y_end && sps->r->sps_subpic_height_minus1[i] + 1 <
>> pps->r->row_height_val[ty])
>> - pps_subpic_less_than_one_tile_slice(pps, sps, i, tx, ty, off);
>> + return pps_subpic_less_than_one_tile_slice(pps, sps, i, tx, ty,
>> off, tile_in_subpic);
>> else
>> - pps_subpic_one_or_more_tiles_slice(pps, tx, ty, x_end, y_end, i,
>> off);
>> + return pps_subpic_one_or_more_tiles_slice(pps, tx, ty, x_end,
>> y_end, i, off, tile_in_subpic);
>> }
>>
>> -static void pps_single_slice_per_subpic(VVCPPS *pps, const VVCSPS *sps,
>> int *off)
>> +static int pps_single_slice_per_subpic(VVCPPS *pps, const VVCSPS *sps,
>> int *off)
>> {
>> if (!sps->r->sps_subpic_info_present_flag) {
>> pps_single_slice_picture(pps, off);
>> } else {
>> - for (int i = 0; i < pps->r->pps_num_slices_in_pic_minus1 + 1; i++)
>> - pps_subpic_slice(pps, sps, i, off);
>> + bool tile_in_subpic[VVC_MAX_TILES_PER_AU] = {0};
>> + for (int i = 0; i < pps->r->pps_num_slices_in_pic_minus1 + 1;
>> i++) {
>> + const int ret = pps_subpic_slice(pps, sps, i, off,
>> tile_in_subpic);
>> + if (ret < 0)
>> + return ret;
>> + }
>> +
>> + // We only use tile_in_subpic to check that the subpictures don't
>> overlap
>> + // here; we don't use tile_in_subpic to check that the
>> subpictures cover
>> + // every tile. It is possible to avoid doing this work here
>> because the
>> + // covering property of subpictures is already guaranteed by the
>> mechanisms
>> + // which check every CTU belongs to a slice.
>> }
>> + return 0;
>> }
>>
>> static int pps_one_tile_slices(VVCPPS *pps, const int tile_idx, int i,
>> int *off)
>> @@ -491,8 +524,7 @@ static int pps_rect_slice(VVCPPS *pps, const VVCSPS
>> *sps)
>> int tile_idx = 0, off = 0;
>>
>> if (r->pps_single_slice_per_subpic_flag) {
>> - pps_single_slice_per_subpic(pps, sps, &off);
>> - return 0;
>> + return pps_single_slice_per_subpic(pps, sps, &off);
>> }
>>
>> for (int i = 0; i < r->pps_num_slices_in_pic_minus1 + 1; i++) {
>> --
>> 2.47.0
>>
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list