[FFmpeg-devel] [PATCH] lavc/vvc: Validate subpartitioning structure
Frank Plowman
post at frankplowman.com
Sun Oct 20 12:34:34 EEST 2024
On 16/10/2024 13:17, Nuo Mi wrote:
> On Tue, Oct 15, 2024 at 7:54 AM Frank Plowman <post at frankplowman.com> wrote:
>
>> 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
>>
> Could you please share the clip with me?
>
>> frame_setup, for example in pps_slice_map, before ff_vvc_decode_sh has
>> been called even.
>>
> Sorry for the confussion.
> We can only know how many CTUs in a slice after we decode the slice header.
>
>
>>> 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?
>>
> Each CTU has a task structure. If a CTU is covered by two slices, the task
> structure will be initialized twice, causing task_init_parse to return an
> error.
In the case of an invalid slice structure derived from an invalid
subpicture structure explicitly provided in the bitstream
(sps_subpic_same_size_flag=0 and pps_single_slice_per_subpic_flag=1),
CTUs which belong to multiple slices do not manifest in the same Task
being initialised multiple times. Instead they manifest in multiple
Tasks with the same rx and ry which are then initialised separately, a
scenario which is not caught by these checks.
>
>>
>>> 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 a CTU is not covered by any slice, the Task->sc will point to NULL. We
> can return an error for this.
What I am trying to say here is that I don't think there is any code
path which can result in Task->sc being null at the moment. If a CTU is
not covered by any slice, we will simply have no Task for it. We could
add such a check, and it might be a good idea for ensuring robustness to
changes to the surrounding code in the future, but as it stands it would
be redundant.
>
> Thank you.
>
>>
>>>
>>>>
>>>>>
>>>>> + 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".
>> _______________________________________________
>> 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".
>>
> _______________________________________________
> 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