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.
>>
>>>
>>> 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".
