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

[Nuo Mi]

On Fri, Apr 18, 2025 at 10:40 PM Frank Plowman <post at 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.

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

>
> Signed-off-by: Frank Plowman <post at 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

> +            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.

>  }
>
>  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.

>          }
>      }
>      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
>
>