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 <p...@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 <p...@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,
--
Frank
_______________________________________________
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".
