| Message ID | 20241005223955.54158-1-post@frankplowman.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] lavc/vvc: Validate subpartitioning structure | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
On Sun, Oct 6, 2024 at 6:49 AM Frank Plowman <post@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@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? + 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-request@ffmpeg.org with subject "unsubscribe". >
On 13/10/2024 05:43, Nuo Mi wrote: > On Sun, Oct 6, 2024 at 6:49 AM Frank Plowman <post@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@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 >>
On Mon, Oct 14, 2024 at 3:14 AM Frank Plowman <post@frankplowman.com> wrote: > On 13/10/2024 05:43, Nuo Mi wrote: > > On Sun, Oct 6, 2024 at 6:49 AM Frank Plowman <post@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@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 > >> >
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@frankplowman.com> wrote: > >> On 13/10/2024 05:43, Nuo Mi wrote: >>> On Sun, Oct 6, 2024 at 6:49 AM Frank Plowman <post@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@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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Tue, Oct 15, 2024 at 7:54 AM Frank Plowman <post@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@frankplowman.com> > wrote: > > > >> On 13/10/2024 05:43, Nuo Mi wrote: > >>> On Sun, Oct 6, 2024 at 6:49 AM Frank Plowman <post@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@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-request@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-request@ffmpeg.org with subject "unsubscribe". >
On 16/10/2024 13:17, Nuo Mi wrote: > On Tue, Oct 15, 2024 at 7:54 AM Frank Plowman <post@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@frankplowman.com> >> wrote: >>> >>>> On 13/10/2024 05:43, Nuo Mi wrote: >>>>> On Sun, Oct 6, 2024 at 6:49 AM Frank Plowman <post@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@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@ffmpeg.org >>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>> >>> To unsubscribe, visit link above, or email >>> ffmpeg-devel-request@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-request@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-request@ffmpeg.org with subject "unsubscribe".
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); + 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);
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@frankplowman.com> --- libavcodec/cbs_h266_syntax_template.c | 46 +++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 2 deletions(-)