| Message ID | 20240605092344.83464-2-post@frankplowman.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v2,1/2] lavc/vvc: Use sps_chroma_qp_table return code | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Hi Frank, Thank you for the patch On Wed, Jun 5, 2024 at 5:24 PM Frank Plowman <post@frankplowman.com> wrote: > On the top of p. 112 in VVC (09/2023): > > It is a requirement of bitstream conformance that the values of > qpInVal[ i ][ j ] and qpOutVal[ i ][ j ] shall be in the range > of −QpBdOffset to 63, inclusive for i in the range of 0 to > Then, why do we not check −QpBdOffset? > numQpTables − 1, inclusive, and j in the range of 0 to > sps_num_points_in_qp_table_minus1[ i ] + 1, inclusive. > > Signed-off-by: Frank Plowman <post@frankplowman.com> > --- > libavcodec/vvc/ps.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/vvc/ps.c b/libavcodec/vvc/ps.c > index bfc3c121fd..c4f64d5da7 100644 > --- a/libavcodec/vvc/ps.c > +++ b/libavcodec/vvc/ps.c > @@ -101,9 +101,14 @@ static int sps_chroma_qp_table(VVCSPS *sps) > > qp_out[0] = qp_in[0] = r->sps_qp_table_start_minus26[i] + 26; > for (int j = 0; j < num_points_in_qp_table; j++ ) { > + const uint8_t delta_qp_out = > (r->sps_delta_qp_in_val_minus1[i][j] ^ r->sps_delta_qp_diff_val[i][j]); > delta_qp_in[j] = r->sps_delta_qp_in_val_minus1[i][j] + 1; > + if (qp_in[j] + delta_qp_in[j] > 63) > + return AVERROR(EINVAL); > qp_in[j+1] = qp_in[j] + delta_qp_in[j]; > - qp_out[j+1] = qp_out[j] + > (r->sps_delta_qp_in_val_minus1[i][j] ^ r->sps_delta_qp_diff_val[i][j]); > + if (qp_out[j] + delta_qp_out > 63) > + return AVERROR(EINVAL); > + qp_out[j+1] = qp_out[j] + delta_qp_out; > Instead of changing so many lines, we can add 2 lines here if (qp_in[j+1] < 63 || qp_out[j+1] < 63) return AVERROR(EINVAL); > } > sps->chroma_qp_table[i][qp_in[0] + off] = qp_out[0]; > for (int k = qp_in[0] - 1 + off; k >= 0; k--) > -- > 2.45.1 > > _______________________________________________ > 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". >
Hi, Thanks for your review. On 05/06/2024 14:50, Nuo Mi wrote: > Hi Frank, > Thank you for the patch > > On Wed, Jun 5, 2024 at 5:24 PM Frank Plowman <post@frankplowman.com> wrote: > >> On the top of p. 112 in VVC (09/2023): >> >> It is a requirement of bitstream conformance that the values of >> qpInVal[ i ][ j ] and qpOutVal[ i ][ j ] shall be in the range >> of −QpBdOffset to 63, inclusive for i in the range of 0 to >> > Then, why do we not check −QpBdOffset? sps_delta_qp_in_val_minus1 is unsigned, therefore we would only need to check the first elements qp{In,Out}Val[i][0], both of which are set to sps_qp_table_start_minus26[i] + 26. sps_qp_table_start_minus26[i] is already constrained to the range [-26-QpBdOffset..36] (see VVC (09/2023) p. 111 and libavcodec/cbs_h266_syntax_template.c:1387). I don't get why the standard reiterates the constraint here, it seems redundant. > >> numQpTables − 1, inclusive, and j in the range of 0 to >> sps_num_points_in_qp_table_minus1[ i ] + 1, inclusive. >> >> Signed-off-by: Frank Plowman <post@frankplowman.com> >> --- >> libavcodec/vvc/ps.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/libavcodec/vvc/ps.c b/libavcodec/vvc/ps.c >> index bfc3c121fd..c4f64d5da7 100644 >> --- a/libavcodec/vvc/ps.c >> +++ b/libavcodec/vvc/ps.c >> @@ -101,9 +101,14 @@ static int sps_chroma_qp_table(VVCSPS *sps) >> >> qp_out[0] = qp_in[0] = r->sps_qp_table_start_minus26[i] + 26; >> for (int j = 0; j < num_points_in_qp_table; j++ ) { >> + const uint8_t delta_qp_out = >> (r->sps_delta_qp_in_val_minus1[i][j] ^ r->sps_delta_qp_diff_val[i][j]); >> delta_qp_in[j] = r->sps_delta_qp_in_val_minus1[i][j] + 1; >> + if (qp_in[j] + delta_qp_in[j] > 63) >> + return AVERROR(EINVAL); >> qp_in[j+1] = qp_in[j] + delta_qp_in[j]; >> - qp_out[j+1] = qp_out[j] + >> (r->sps_delta_qp_in_val_minus1[i][j] ^ r->sps_delta_qp_diff_val[i][j]); >> + if (qp_out[j] + delta_qp_out > 63) >> + return AVERROR(EINVAL); >> + qp_out[j+1] = qp_out[j] + delta_qp_out; >> > Instead of changing so many lines, we can add 2 lines here > if (qp_in[j+1] < 63 || qp_out[j+1] < 63) > return AVERROR(EINVAL); v3 sent with this tweak & squashing the other patch. > >> } >> sps->chroma_qp_table[i][qp_in[0] + off] = qp_out[0]; >> for (int k = qp_in[0] - 1 + off; k >= 0; k--) >> -- >> 2.45.1 >> Cheers,
diff --git a/libavcodec/vvc/ps.c b/libavcodec/vvc/ps.c index bfc3c121fd..c4f64d5da7 100644 --- a/libavcodec/vvc/ps.c +++ b/libavcodec/vvc/ps.c @@ -101,9 +101,14 @@ static int sps_chroma_qp_table(VVCSPS *sps) qp_out[0] = qp_in[0] = r->sps_qp_table_start_minus26[i] + 26; for (int j = 0; j < num_points_in_qp_table; j++ ) { + const uint8_t delta_qp_out = (r->sps_delta_qp_in_val_minus1[i][j] ^ r->sps_delta_qp_diff_val[i][j]); delta_qp_in[j] = r->sps_delta_qp_in_val_minus1[i][j] + 1; + if (qp_in[j] + delta_qp_in[j] > 63) + return AVERROR(EINVAL); qp_in[j+1] = qp_in[j] + delta_qp_in[j]; - qp_out[j+1] = qp_out[j] + (r->sps_delta_qp_in_val_minus1[i][j] ^ r->sps_delta_qp_diff_val[i][j]); + if (qp_out[j] + delta_qp_out > 63) + return AVERROR(EINVAL); + qp_out[j+1] = qp_out[j] + delta_qp_out; } sps->chroma_qp_table[i][qp_in[0] + off] = qp_out[0]; for (int k = qp_in[0] - 1 + off; k >= 0; k--)
On the top of p. 112 in VVC (09/2023): It is a requirement of bitstream conformance that the values of qpInVal[ i ][ j ] and qpOutVal[ i ][ j ] shall be in the range of −QpBdOffset to 63, inclusive for i in the range of 0 to numQpTables − 1, inclusive, and j in the range of 0 to sps_num_points_in_qp_table_minus1[ i ] + 1, inclusive. Signed-off-by: Frank Plowman <post@frankplowman.com> --- libavcodec/vvc/ps.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)