[FFmpeg-devel] [PATCH v2 1/2] lavc/vvc: Use sps_chroma_qp_table return code

[FFmpeg-devel] [PATCH v2 1/2] lavc/vvc: Use sps_chroma_qp_table return code

[Frank Plowman]

Signed-off-by: Frank Plowman <post@frankplowman.com>
---
 libavcodec/vvc/ps.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/libavcodec/vvc/ps.c b/libavcodec/vvc/ps.c
index 1b23675c98..bfc3c121fd 100644
--- a/libavcodec/vvc/ps.c
+++ b/libavcodec/vvc/ps.c
@@ -186,8 +186,11 @@ static int sps_derive(VVCSPS *sps, void *log_ctx)
     sps_inter(sps);
     sps_partition_constraints(sps);
     sps_ladf(sps);
-    if (r->sps_chroma_format_idc != 0)
-        sps_chroma_qp_table(sps);
+    if (r->sps_chroma_format_idc != 0) {
+        ret = sps_chroma_qp_table(sps);
+        if (ret < 0)
+            return ret;
+    }
 
     return 0;
 }
-- 
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".

[FFmpeg-devel] [PATCH v2 2/2] lavc/vvc: Prevent overflow in chroma QP derivation

[Frank Plowman]

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(-)

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

Re: [FFmpeg-devel] [PATCH v2 2/2] lavc/vvc: Prevent overflow in chroma QP derivation

[Nuo Mi]

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

Re: [FFmpeg-devel] [PATCH v2 1/2] lavc/vvc: Use sps_chroma_qp_table return code

[Nuo Mi]

this code is simple, you can combine it with the next one

On Wed, Jun 5, 2024 at 5:24 PM Frank Plowman <post@frankplowman.com> wrote:

> Signed-off-by: Frank Plowman <post@frankplowman.com>
> ---
>  libavcodec/vvc/ps.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/vvc/ps.c b/libavcodec/vvc/ps.c
> index 1b23675c98..bfc3c121fd 100644
> --- a/libavcodec/vvc/ps.c
> +++ b/libavcodec/vvc/ps.c
> @@ -186,8 +186,11 @@ static int sps_derive(VVCSPS *sps, void *log_ctx)
>      sps_inter(sps);
>      sps_partition_constraints(sps);
>      sps_ladf(sps);
> -    if (r->sps_chroma_format_idc != 0)
> -        sps_chroma_qp_table(sps);
> +    if (r->sps_chroma_format_idc != 0) {
> +        ret = sps_chroma_qp_table(sps);
> +        if (ret < 0)
> +            return ret;
> +    }
>
>      return 0;
>  }
> --
> 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".
>
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2 2/2] lavc/vvc: Prevent overflow in chroma QP derivation

[Frank Plowman]

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,
-- 
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-request@ffmpeg.org with subject "unsubscribe".