Re: [FFmpeg-devel] [PATCH 2/2] lavc/vvc: Ensure subpictures don't overlap

[Nuo Mi <nuomi2021@gmail.com>]

On Thu, Feb 20, 2025 at 4:29 AM Frank Plowman <post@frankplowman.com> wrote:

> 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@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@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.
>
Okay, Thanks for the explaining. I might be overthinking it.

Made some small changes in v3:
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20250222075155.504540-2-nuomi2021@gmail.com/

1. Refact out mark_tile_as_used to remove duplicate logic.
2.Only check bottom for is_final_subpic_in_tile, as a slice within a tile
always has the same width as the tile.


> >      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@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
>
> _______________________________________________
> 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".
>
_______________________________________________
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".