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