[FFmpeg-devel] [PATCH] lavc/vvc: Validate subpartitioning structure

[Nuo Mi]

On Mon, Oct 14, 2024 at 3:14 AM Frank Plowman <post at frankplowman.com> wrote:

> On 13/10/2024 05:43, Nuo Mi wrote:
> > On Sun, Oct 6, 2024 at 6:49 AM Frank Plowman <post at frankplowman.com>
> wrote:
> >
> >> H.266 (V3) section 6.3.3 dictates that the division of the picture into
> >> subpictures must be exhaustive and mutually exclusive, i.e. that each
> >> CTU "belongs to" one and only one subpicture.  In most cases this is
> >> guaranteed by the syntax, but in the case sps_subpic_same_size_flag=0,
> >> we must check this is true ourselves.
> >>
> >> Signed-off-by: Frank Plowman <post at frankplowman.com>
> >> ---
> >>  libavcodec/cbs_h266_syntax_template.c | 46 +++++++++++++++++++++++++--
> >>  1 file changed, 44 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/libavcodec/cbs_h266_syntax_template.c
> >> b/libavcodec/cbs_h266_syntax_template.c
> >> index b4165b43b3..822ee26f46 100644
> >> --- a/libavcodec/cbs_h266_syntax_template.c
> >> +++ b/libavcodec/cbs_h266_syntax_template.c
> >> @@ -1191,7 +1191,7 @@ static int FUNC(sps)(CodedBitstreamContext *ctx,
> >> RWContext *rw,
> >>                             win_left_edge_ctus >
> >> current->sps_subpic_ctu_top_left_x[i]
> >>                                 ? win_left_edge_ctus -
> >> current->sps_subpic_ctu_top_left_x[i]
> >>                                 : 0,
> >> -                           MAX_UINT_BITS(wlen), 1, i);
> >> +                           tmp_width_val -
> >> current->sps_subpic_ctu_top_left_x[i] - 1, 1, i);
> >>                      } else {
> >>                          infer(sps_subpic_width_minus1[i],
> >>                                tmp_width_val -
> >> @@ -1208,7 +1208,7 @@ static int FUNC(sps)(CodedBitstreamContext *ctx,
> >> RWContext *rw,
> >>                             win_top_edge_ctus >
> >> current->sps_subpic_ctu_top_left_y[i]
> >>                                 ? win_top_edge_ctus -
> >> current->sps_subpic_ctu_top_left_y[i]
> >>                                 : 0,
> >> -                           MAX_UINT_BITS(hlen), 1, i);
> >> +                           tmp_height_val -
> >> current->sps_subpic_ctu_top_left_y[i] - 1, 1, i);
> >>                      } else {
> >>                          infer(sps_subpic_height_minus1[i],
> >>                                tmp_height_val -
> >> @@ -1242,6 +1242,48 @@ static int FUNC(sps)(CodedBitstreamContext *ctx,
> >> RWContext *rw,
> >>
> infer(sps_loop_filter_across_subpic_enabled_flag[i],
> >> 0);
> >>                  }
> >>              }
> >> +            // If the subpic partitioning structure is signalled
> >> explicitly,
> >> +            // validate it constitutes an exhaustive and mutually
> >> exclusive
> >> +            // coverage of the picture, per 6.3.3.  If the partitioning
> >> is not
> >> +            // provided explicitly, then it is ensured by the syntax
> and
> >> we need
> >> +            // not check.
> >> +            if (!current->sps_subpic_same_size_flag) {
> >> +                char *ctu_in_subpic = av_mallocz(tmp_width_val *
> >> tmp_height_val);
> >>
> > Thank you for the patch.
> > The slices will cover the entire subpicture without any overlap, and the
> > CTUs will cover the entire slice without any overlap.
> > We will check num_slices_in_subpic[] in FUNC(pps). How about summing all
> > the values in num_slices_in_subpic[] and verifying if it equals
> > sps_num_subpics_minus1 + 1?
>
> This is not sufficient in the case pps_single_slice_per_subpic flag is
> 1.  When this flag is 1, the slice layout is the same as the subpicture
> layout and so your suggested condition is always satisfied.  In this
> case, we have no guarantees that the slice layout is valid however.
>
We can only determine this after decoding all slice headers. see
https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/vvc/ps.c#L1218
The task_init_parse function will ensure that a single CTU does not belong
to two slices.
It might be helpful to add a check in ff_vvc_frame_submit to confirm that
each task (CTU) points to a valid slice (i.e., t->sc is not NULL).

>
> >
> > +                if (!ctu_in_subpic)
> >> +                    return AVERROR(ENOMEM);
> >> +                for (i = 0; i <= current->sps_num_subpics_minus1; i++)
> {
> >> +                    const unsigned x0 =
> >> current->sps_subpic_ctu_top_left_x[i];
> >> +                    const unsigned y0 =
> >> current->sps_subpic_ctu_top_left_y[i];
> >> +                    const unsigned w =
> >> current->sps_subpic_width_minus1[i] + 1;
> >> +                    const unsigned h =
> >> current->sps_subpic_height_minus1[i] + 1;
> >> +                    av_assert0(x0 + w - 1 < tmp_width_val);
> >> +                    av_assert0(y0 + h - 1 < tmp_height_val);
> >> +                    for (unsigned x = x0; x < x0 + w; x++) {
> >> +                        for (unsigned y = y0; y < y0 + h; y++) {
> >> +                            const unsigned idx = y * tmp_width_val + x;
> >> +                            if (ctu_in_subpic[idx]) {
> >> +                                av_log(ctx->log_ctx, AV_LOG_ERROR,
> >> +                                       "Subpictures overlap.\n");
> >> +                                av_freep(&ctu_in_subpic);
> >> +                                return AVERROR_INVALIDDATA;
> >> +                            }
> >> +                            ctu_in_subpic[idx] = 1;
> >> +                        }
> >> +                    }
> >> +                }
> >> +                for (unsigned x = 0; x < tmp_width_val; x++) {
> >> +                    for (unsigned y = 0; y < tmp_height_val; y++) {
> >> +                        const unsigned idx = y * tmp_width_val + x;
> >> +                        if (!ctu_in_subpic[idx]) {
> >> +                            av_log(ctx->log_ctx, AV_LOG_ERROR,
> >> +                                   "Subpictures do not cover the entire
> >> picture.\n");
> >> +                            av_freep(&ctu_in_subpic);
> >> +                            return AVERROR_INVALIDDATA;
> >> +                        }
> >> +                    }
> >> +                }
> >> +                av_freep(&ctu_in_subpic);
> >> +            }
> >>          } else {
> >>              infer(sps_subpic_ctu_top_left_x[0], 0);
> >>              infer(sps_subpic_ctu_top_left_y[0], 0);
> >> --
> >> 2.46.2
> >>
>