[FFmpeg-devel] [PATCH] lavc/vvc: Validate subpartitioning structure
Frank Plowman
post at frankplowman.com
Mon Oct 14 23:18:05 EEST 2024
Thank you for your reply.
On 14/10/2024 16:16, Nuo Mi wrote:
> 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
I'm sorry I'm not sure I follow exactly. What can we not determine
until decoding slice headers? pps_single_slice_per_subpic is a PPS
flag. Also, in the cases I have which have issues with invalid
subpic/slice structures, we start hitting bugs and crashes in
frame_setup, for example in pps_slice_map, before ff_vvc_decode_sh has
been called even.
> The task_init_parse function will ensure that a single CTU does not belong
> to two slices.
Looking at the implementation of task_init_parse, I see it checks a
single task object is not initialised multiple times, but the location
of this CTU is simply supplied by the caller and already depends on the
slice structure (ep->ctu_{start,end}, ctb_addr_in_curr_slice), which may
be invalid. As far as I can tell there is nothing here which checks
slices don't overlap. Am I missing something?
> 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).
I don't think this is possible currently as the way we get tasks is by
iterating over those in a SliceContext. If there were CTUs not covered
by any slice, the loops in ff_vvc_frame_submit would not see them.
>
>>
>>>
>>> + 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
>>>>
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list