Re: [FFmpeg-devel] [PATCH] lavc/vvc: Detect subpic overlaps at CTU level

[Frank Plowman <post@frankplowman.com>]

On 27/04/2025 03:42, Nuo Mi wrote:
> On Fri, Apr 18, 2025 at 10:40 PM Frank Plowman <post@frankplowman.com>
> wrote:
> 
>> In d5dbcc00d889fb17948b025a468b00ddbea9e058, it was hoped that detection
>> of subpicture overlaps could be performed at the tile level, so as to
>> avoid introducing per-CTU checks. Unfortunately since that patch,
>> fuzzing has indicated there are some structures involving
>> pps_subpic_one_or_more_tiles_slice where tile-level checking is not
>> sufficient.  Performing the check at the CTU level should (touch wood)
>> be the be-all and and-all of this, as CTUs are the lowest common
>> denominator of the picture partitioning.
>>
> Hi Frank,
> Thank you for the patch.

Thank you for your review.

> 
> Before this patch, we could detect the 'CTU A added twice + CTU B never
> added' case, but the new implementation cannot, right?
> 

In ff_vvc_frame_submit we check that every CTU of the picture belongs to
a slice, which catches cases like this.  This patch does mean that
certain invalid structures like you describe are not caught until a
little later, in ff_vvc_frame_submit.  In my tests this does not appear
to cause issues.

Alternatively, we could take an approach similar to what existed
previously but at the CTU level, where we allocate a ctb_width x
ctb_height array and mark each cell used/unused in pps_add_ctus.  That
array could be quite large though and would require a dynamic allocation.

>>
>> Signed-off-by: Frank Plowman <post@frankplowman.com>
>> ---
>>  libavcodec/vvc/ps.c | 81 +++++++++++++++++++++++----------------------
>>  1 file changed, 42 insertions(+), 39 deletions(-)
>>
>> diff --git a/libavcodec/vvc/ps.c b/libavcodec/vvc/ps.c
>> index e8c312d8ac..4edfe408c0 100644
>> --- a/libavcodec/vvc/ps.c
>> +++ b/libavcodec/vvc/ps.c
>> @@ -402,14 +402,35 @@ static int ctu_rs(const int rx, const int ry, const
>> VVCPPS *pps)
>>      return pps->ctb_width * ry + rx;
>>  }
>>
>> +static void pps_add_ctu(VVCPPS *pps, int *off, const int x, const int y)
>> +{
>> +    pps->ctb_addr_in_slice[*off] = ctu_rs(x, y, pps);
>> +    (*off)++;
>> +}
>> +
>>  static int pps_add_ctus(VVCPPS *pps, int *off, const int rx, const int ry,
>>      const int w, const int h)
>>  {
>>      int start = *off;
>>      for (int y = 0; y < h; y++) {
>>          for (int x = 0; x < w; x++) {
>> -            pps->ctb_addr_in_slice[*off] = ctu_rs(rx + x, ry + y, pps);
>> -            (*off)++;
>> +            pps_add_ctu(pps, off, rx + x, ry + y);
>> +        }
>> +    }
>> +    return *off - start;
>> +}
>> +
>> +// Similar to pps_add_ctus, but with a check to ensure a given CTU isn't
>> used
>> +// multiple times, to be used with some of the more complex partitioning
>> mechanisms.
>> +static int pps_add_ctus_check(VVCPPS *pps, int *off, const int rx, const
>> int ry,
>> +    const int w, const int h)
>> +{
>> +    int start = *off;
>> +    for (int y = 0; y < h; y++) {
>> +        for (int x = 0; x < w; x++) {
>> +            if (*off >= pps->ctb_count)
>> +                return AVERROR_INVALIDDATA;
>>
> This can be combined to pss_add_ctu.  So we remvoe pps_add_ctus_check and
> keep pps_add_ctus only
> 

This check can only fail in the case that
pps_single_slice_per_subpic_flag is 1, so I was concerned changing
pps_add_ctus directly would incur the cost of this check unnecessarily
when pps_single_slice_per_subpic_flag is 0.  That being said, I just
tested and any performance impact seems negligible and I think the
compiler may be able to optimise much of this away, so I've done as you
said and merged them in v2.

>> +            pps_add_ctu(pps, off, rx + x, ry + y);
>>          }
>>      }
>>      return *off - start;
>> @@ -451,50 +472,39 @@ static void subpic_tiles(int *tile_x, int *tile_y,
>> int *tile_x_end, int *tile_y_
>>          (*tile_y_end)++;
>>  }
>>
>> -static bool mark_tile_as_used(bool *tile_in_subpic, const int tx, const
>> int ty, const int tile_columns)
>> -{
>> -    const size_t tile_idx = ty * tile_columns + tx;
>> -    if (tile_in_subpic[tile_idx]) {
>> -        /* the tile is covered by other subpictures */
>> -        return false;
>> -    }
>> -    tile_in_subpic[tile_idx] = true;
>> -    return true;
>> -}
>> -
>> -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)
>> +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)
>>  {
>> -    const int subpic_bottom = sps->r->sps_subpic_ctu_top_left_y[i] +
>> sps->r->sps_subpic_height_minus1[i];
>> -    const int tile_bottom = pps->row_bd[ty] + pps->r->row_height_val[ty]
>> - 1;
>> -    const bool is_final_subpic_in_tile = subpic_bottom == tile_bottom;
>> -
>> -    if (is_final_subpic_in_tile && !mark_tile_as_used(tile_in_subpic, tx,
>> ty, pps->r->num_tile_columns))
>> -        return AVERROR_INVALIDDATA;
>> -
>> -    pps->num_ctus_in_slice[i] = pps_add_ctus(pps, off,
>> +    const int ret = pps_add_ctus_check(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;
>> +    if (ret < 0)
>> +        return ret;
>> +    else {
>> +        pps->num_ctus_in_slice[i] = ret;
>> +        return 0;
>> +    }
>>
> The else is not needed; if the condition is true, the function returns.
> 

Changed in v2.

>>  }
>>
>>  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)
>> +    const int i, int *off)
>>  {
>>      for (int ty = tile_y; ty < y_end; ty++) {
>>          for (int tx = tile_x; tx < x_end; tx++) {
>> -            if (!mark_tile_as_used(tile_in_subpic, tx, ty,
>> pps->r->num_tile_columns))
>> -                return AVERROR_INVALIDDATA;
>> -
>> -            pps->num_ctus_in_slice[i] += pps_add_ctus(pps, off,
>> +            const int ret = pps_add_ctus_check(pps, off,
>>                  pps->col_bd[tx], pps->row_bd[ty],
>>                  pps->r->col_width_val[tx], pps->r->row_height_val[ty]);
>> +
>> +            if (ret < 0)
>> +                return ret;
>> +            else
>> +                pps->num_ctus_in_slice[i] += ret;
>>
> else is not needed too.
> 

Same as above.

>>          }
>>      }
>>      return 0;
>>  }
>>
>> -static int pps_subpic_slice(VVCPPS *pps, const VVCSPS *sps, const int i,
>> int *off, bool *tile_in_subpic)
>> +static int pps_subpic_slice(VVCPPS *pps, const VVCSPS *sps, const int i,
>> int *off)
>>  {
>>      int tx, ty, x_end, y_end;
>>
>> @@ -503,9 +513,9 @@ static int pps_subpic_slice(VVCPPS *pps, const VVCSPS
>> *sps, const int i, int *of
>>
>>      subpic_tiles(&tx, &ty, &x_end, &y_end, sps, pps, i);
>>      if (ty + 1 == y_end && sps->r->sps_subpic_height_minus1[i] + 1 <
>> pps->r->row_height_val[ty])
>> -        return pps_subpic_less_than_one_tile_slice(pps, sps, i, tx, ty,
>> off, tile_in_subpic);
>> +        return pps_subpic_less_than_one_tile_slice(pps, sps, i, tx, ty,
>> off);
>>      else
>> -        return pps_subpic_one_or_more_tiles_slice(pps, tx, ty, x_end,
>> y_end, i, off, tile_in_subpic);
>> +        return pps_subpic_one_or_more_tiles_slice(pps, tx, ty, x_end,
>> y_end, i, off);
>>  }
>>
>>  static int pps_single_slice_per_subpic(VVCPPS *pps, const VVCSPS *sps,
>> int *off)
>> @@ -513,18 +523,11 @@ 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 {
>> -        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);
>> +            const int ret = pps_subpic_slice(pps, sps, i, off);
>>              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;
>>  }
>> --
>> 2.47.0
>>
>>

-- 
Frank

_______________________________________________
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".