On Tue, Oct 15, 2024 at 7:54 AM Frank Plowman <p...@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 <p...@frankplowman.com>
> wrote:
> >
> >> On 13/10/2024 05:43, Nuo Mi wrote:
> >>> On Sun, Oct 6, 2024 at 6:49 AM Frank Plowman <p...@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 <p...@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.
>
> > 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.
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@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-requ...@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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
