On Sun, May 18, 2025 at 9:16 PM Frank Plowman <p...@frankplowman.com> wrote:
> On 29/04/2025 14:24, Nuo Mi wrote:
> > Hi Frank,
> > Thank you for the detail.
> >
> > On Mon, Apr 28, 2025 at 10:35 PM Frank Plowman <p...@frankplowman.com>
> > wrote:
> >
> >> On 28/04/2025 14:33, Nuo Mi wrote:
> >>> Hi Frank,
> >>> Thank you for the v2.
> >>> Could we remove all asserts?
> >>> Asserts can cause the application to crash at runtime.
> >>
> >> Hi,
> >>
> >> I think av_assert2s are the right thing to use here. In case it was not
> >> clear, these asserts should never be triggered by any bitstream (legal
> >> or illegal). The alternatives, as I see it, are both less favourable:
> >>
> >> * Don't check the return value at all. If the assumption above that
> >> pps_add_ctus shouldn't fail in these cases is incorrect, then all of a
> >> sudden there is a rather unscrutable error arising from subtracting a
> >> value from off, which might be rather difficult to debug. An assertion
> >> is better because it makes the issue obvious by crashing, and
> >> immediately points to the location in the code which is problematic.
> >>
> > FFmpeg will run on multiple computers, so detecting and returning an
> error
> > is better than crashing the program — even if it's a rare occurrence.
> >
> >>
> >> * Add a runtime check for these cases. If the assumption above is
> >> correct, then we're incurring needless runtime penalty checking for
> >> things which are always true. An av_assert2 is better because it is
> >> only enabled in debug builds, and not where performance is essential.
> >>
> > The entire process happens at the PPS level, and usually, we only have
> one
> > per stream, so the performance loss should be minimal.
> >
>
> Ok, I've replaced the asserts with runtime errors in v3.
>
Thank you, Frank,
Applied v3.
>
> >>
> >>>
> >>> On Sun, Apr 27, 2025 at 4:48 PM Frank Plowman <p...@frankplowman.com>
> >> wrote:
> >>>
> >>>> In d5dbcc00d889fb17948b025a468b00ddbea9e058, it was hoped that
> detection
> >>>> of subpicture overlaps could be performed at the tile level, so as to
> >>>> avoid introducing per-CTU checks. Unfortunately since that patch,
> >>>> fuzzing has indicated there are some structures involving
> >>>> pps_subpic_one_or_more_tiles_slice where tile-level checking is not
> >>>> sufficient. Performing the check at the CTU level should (touch wood)
> >>>> be the be-all and and-all of this, as CTUs are the lowest common
> >>>> denominator of the picture partitioning.
> >>>>
> >>>> Signed-off-by: Frank Plowman <p...@frankplowman.com>
> >>>> ---
> >>>> Changes since v1:
> >>>> * Merge pps_add_ctus and pps_add_ctus_check
> >>>> * Change if/else for early-exit where possible
> >>>>
> >>>> ---
> >>>> libavcodec/vvc/ps.c | 71
> ++++++++++++++++++++-------------------------
> >>>> 1 file changed, 31 insertions(+), 40 deletions(-)
> >>>>
> >>>> diff --git a/libavcodec/vvc/ps.c b/libavcodec/vvc/ps.c
> >>>> index e8c312d8ac..ed96268bae 100644
> >>>> --- a/libavcodec/vvc/ps.c
> >>>> +++ b/libavcodec/vvc/ps.c
> >>>> @@ -408,6 +408,8 @@ static int pps_add_ctus(VVCPPS *pps, int *off,
> const
> >>>> int rx, const int ry,
> >>>> int start = *off;
> >>>> for (int y = 0; y < h; y++) {
> >>>> for (int x = 0; x < w; x++) {
> >>>> + if (*off >= pps->ctb_count)
> >>>> + return AVERROR_INVALIDDATA;
> >>>> pps->ctb_addr_in_slice[*off] = ctu_rs(rx + x, ry + y,
> pps);
> >>>> (*off)++;
> >>>> }
> >>>> @@ -420,9 +422,11 @@ static void pps_single_slice_picture(VVCPPS *pps,
> >> int
> >>>> *off)
> >>>> pps->num_ctus_in_slice[0] = 0;
> >>>> for (int j = 0; j < pps->r->num_tile_rows; j++) {
> >>>> for (int i = 0; i < pps->r->num_tile_columns; i++) {
> >>>> - pps->num_ctus_in_slice[0] += pps_add_ctus(pps, off,
> >>>> + const int ret = pps_add_ctus(pps, off,
> >>>> pps->col_bd[i], pps->row_bd[j],
> >>>> pps->r->col_width_val[i], pps->r->row_height_val[j]);
> >>>> + av_assert2(ret >= 0);
> >>>> + pps->num_ctus_in_slice[0] += ret;
> >>>> }
> >>>> }
> >>>> }
> >>>> @@ -451,50 +455,36 @@ static void subpic_tiles(int *tile_x, int
> *tile_y,
> >>>> int *tile_x_end, int *tile_y_
> >>>> (*tile_y_end)++;
> >>>> }
> >>>>
> >>>> -static bool mark_tile_as_used(bool *tile_in_subpic, const int tx,
> const
> >>>> int ty, const int tile_columns)
> >>>> -{
> >>>> - const size_t tile_idx = ty * tile_columns + tx;
> >>>> - if (tile_in_subpic[tile_idx]) {
> >>>> - /* the tile is covered by other subpictures */
> >>>> - return false;
> >>>> - }
> >>>> - tile_in_subpic[tile_idx] = true;
> >>>> - return true;
> >>>> -}
> >>>> -
> >>>> -static int pps_subpic_less_than_one_tile_slice(VVCPPS *pps, const
> >> VVCSPS
> >>>> *sps, const int i, const int tx, const int ty, int *off, bool
> >>>> *tile_in_subpic)
> >>>> +static int pps_subpic_less_than_one_tile_slice(VVCPPS *pps, const
> >> VVCSPS
> >>>> *sps, const int i, const int tx, const int ty, int *off)
> >>>> {
> >>>> - const int subpic_bottom = sps->r->sps_subpic_ctu_top_left_y[i] +
> >>>> sps->r->sps_subpic_height_minus1[i];
> >>>> - const int tile_bottom = pps->row_bd[ty] +
> >> pps->r->row_height_val[ty]
> >>>> - 1;
> >>>> - const bool is_final_subpic_in_tile = subpic_bottom ==
> tile_bottom;
> >>>> -
> >>>> - if (is_final_subpic_in_tile && !mark_tile_as_used(tile_in_subpic,
> >> tx,
> >>>> ty, pps->r->num_tile_columns))
> >>>> - return AVERROR_INVALIDDATA;
> >>>> -
> >>>> - pps->num_ctus_in_slice[i] = pps_add_ctus(pps, off,
> >>>> + const int ret = pps_add_ctus(pps, off,
> >>>> sps->r->sps_subpic_ctu_top_left_x[i],
> >>>> sps->r->sps_subpic_ctu_top_left_y[i],
> >>>> sps->r->sps_subpic_width_minus1[i] + 1,
> >>>> sps->r->sps_subpic_height_minus1[i] + 1);
> >>>> + if (ret < 0)
> >>>> + return ret;
> >>>>
> >>>> + pps->num_ctus_in_slice[i] = ret;
> >>>> return 0;
> >>>> }
> >>>>
> >>>> static int pps_subpic_one_or_more_tiles_slice(VVCPPS *pps, const int
> >>>> tile_x, const int tile_y, const int x_end, const int y_end,
> >>>> - const int i, int *off, bool *tile_in_subpic)
> >>>> + const int i, int *off)
> >>>> {
> >>>> for (int ty = tile_y; ty < y_end; ty++) {
> >>>> for (int tx = tile_x; tx < x_end; tx++) {
> >>>> - if (!mark_tile_as_used(tile_in_subpic, tx, ty,
> >>>> pps->r->num_tile_columns))
> >>>> - return AVERROR_INVALIDDATA;
> >>>> -
> >>>> - pps->num_ctus_in_slice[i] += pps_add_ctus(pps, off,
> >>>> + const int ret = pps_add_ctus(pps, off,
> >>>> pps->col_bd[tx], pps->row_bd[ty],
> >>>> pps->r->col_width_val[tx],
> pps->r->row_height_val[ty]);
> >>>> + if (ret < 0)
> >>>> + return ret;
> >>>> +
> >>>> + pps->num_ctus_in_slice[i] += ret;
> >>>> }
> >>>> }
> >>>> return 0;
> >>>> }
> >>>>
> >>>> -static int pps_subpic_slice(VVCPPS *pps, const VVCSPS *sps, const int
> >> i,
> >>>> int *off, bool *tile_in_subpic)
> >>>> +static int pps_subpic_slice(VVCPPS *pps, const VVCSPS *sps, const int
> >> i,
> >>>> int *off)
> >>>> {
> >>>> int tx, ty, x_end, y_end;
> >>>>
> >>>> @@ -503,9 +493,9 @@ static int pps_subpic_slice(VVCPPS *pps, const
> >> VVCSPS
> >>>> *sps, const int i, int *of
> >>>>
> >>>> subpic_tiles(&tx, &ty, &x_end, &y_end, sps, pps, i);
> >>>> if (ty + 1 == y_end && sps->r->sps_subpic_height_minus1[i] + 1 <
> >>>> pps->r->row_height_val[ty])
> >>>> - return pps_subpic_less_than_one_tile_slice(pps, sps, i, tx,
> ty,
> >>>> off, tile_in_subpic);
> >>>> + return pps_subpic_less_than_one_tile_slice(pps, sps, i, tx,
> ty,
> >>>> off);
> >>>> else
> >>>> - return pps_subpic_one_or_more_tiles_slice(pps, tx, ty, x_end,
> >>>> y_end, i, off, tile_in_subpic);
> >>>> + return pps_subpic_one_or_more_tiles_slice(pps, tx, ty, x_end,
> >>>> y_end, i, off);
> >>>> }
> >>>>
> >>>> static int pps_single_slice_per_subpic(VVCPPS *pps, const VVCSPS
> *sps,
> >>>> int *off)
> >>>> @@ -513,18 +503,11 @@ static int pps_single_slice_per_subpic(VVCPPS
> >> *pps,
> >>>> const VVCSPS *sps, int *off)
> >>>> if (!sps->r->sps_subpic_info_present_flag) {
> >>>> pps_single_slice_picture(pps, off);
> >>>> } else {
> >>>> - bool tile_in_subpic[VVC_MAX_TILES_PER_AU] = {0};
> >>>> for (int i = 0; i < pps->r->pps_num_slices_in_pic_minus1 + 1;
> >>>> i++) {
> >>>> - const int ret = pps_subpic_slice(pps, sps, i, off,
> >>>> tile_in_subpic);
> >>>> + const int ret = pps_subpic_slice(pps, sps, i, off);
> >>>> if (ret < 0)
> >>>> return ret;
> >>>> }
> >>>> -
> >>>> - // We only use tile_in_subpic to check that the subpictures
> >> don't
> >>>> overlap
> >>>> - // here; we don't use tile_in_subpic to check that the
> >>>> subpictures cover
> >>>> - // every tile. It is possible to avoid doing this work here
> >>>> because the
> >>>> - // covering property of subpictures is already guaranteed by
> >> the
> >>>> mechanisms
> >>>> - // which check every CTU belongs to a slice.
> >>>> }
> >>>> return 0;
> >>>> }
> >>>> @@ -538,9 +521,12 @@ static int pps_one_tile_slices(VVCPPS *pps, const
> >> int
> >>>> tile_idx, int i, int *off)
> >>>> ctu_xy(&rx, &ry, tile_x, tile_y, pps);
> >>>> ctu_y_end = ry + r->row_height_val[tile_y];
> >>>> while (ry < ctu_y_end) {
> >>>> + int ret;
> >>>> pps->slice_start_offset[i] = *off;
> >>>> - pps->num_ctus_in_slice[i] = pps_add_ctus(pps, off, rx, ry,
> >>>> + ret = pps_add_ctus(pps, off, rx, ry,
> >>>> r->col_width_val[tile_x], r->slice_height_in_ctus[i]);
> >>>> + av_assert2(ret >= 0);
> >>>> + pps->num_ctus_in_slice[i] = ret;
> >>>> ry += r->slice_height_in_ctus[i++];
> >>>> }
> >>>> i--;
> >>>> @@ -557,13 +543,16 @@ static int pps_multi_tiles_slice(VVCPPS *pps,
> >> const
> >>>> int tile_idx, const int i, i
> >>>> pps->num_ctus_in_slice[i] = 0;
> >>>> for (int ty = tile_y; ty <= tile_y +
> >>>> r->pps_slice_height_in_tiles_minus1[i]; ty++) {
> >>>> for (int tx = tile_x; tx <= tile_x +
> >>>> r->pps_slice_width_in_tiles_minus1[i]; tx++) {
> >>>> + int ret;
> >>>> const int idx = ty * r->num_tile_columns + tx;
> >>>> if (tile_in_slice[idx])
> >>>> return AVERROR_INVALIDDATA;
> >>>> tile_in_slice[idx] = true;
> >>>> ctu_xy(&rx, &ry, tx, ty, pps);
> >>>> - pps->num_ctus_in_slice[i] += pps_add_ctus(pps, off, rx,
> ry,
> >>>> + ret = pps_add_ctus(pps, off, rx, ry,
> >>>> r->col_width_val[tx], r->row_height_val[ty]);
> >>>> + av_assert2(ret >= 0);
> >>>> + pps->num_ctus_in_slice[i] += ret;
> >>>> }
> >>>> }
> >>>>
> >>>> @@ -610,8 +599,10 @@ static void pps_no_rect_slice(VVCPPS* pps)
> >>>>
> >>>> for (int tile_y = 0; tile_y < r->num_tile_rows; tile_y++) {
> >>>> for (int tile_x = 0; tile_x < r->num_tile_columns; tile_x++)
> {
> >>>> + int ret;
> >>>> ctu_xy(&rx, &ry, tile_x, tile_y, pps);
> >>>> - pps_add_ctus(pps, &off, rx, ry, r->col_width_val[tile_x],
> >>>> r->row_height_val[tile_y]);
> >>>> + ret = pps_add_ctus(pps, &off, rx, ry,
> >>>> r->col_width_val[tile_x], r->row_height_val[tile_y]);
> >>>> + av_assert2(ret >= 0);
> >>>> }
> >>>> }
> >>>> }
> >>>> --
> >>>> 2.47.0
> >>>>
> >>>>
> >>> _______________________________________________
> >>> 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".
>
>
> _______________________________________________
> 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".
