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

[Frank Plowman]

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.

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