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