diff mbox series

[FFmpeg-devel,v2] avcodec/cbs_av1: add missing value constrains to point_y_value, point_cb_value and point_cr_value

Message ID 20200213150733.2515-1-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel,v2] avcodec/cbs_av1: add missing value constrains to point_y_value, point_cb_value and point_cr_value
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

James Almer Feb. 13, 2020, 3:07 p.m. UTC
If i is greater than 0, it is a requirement of bitstream conformance that point_y_value[ i ] is greater than point_y_value[ i - 1 ].
If i is greater than 0, it is a requirement of bitstream conformance that point_cb_value[ i ] is greater than point_cb_value[ i - 1 ].
If i is greater than 0, it is a requirement of bitstream conformance that point_cr_value[ i ] is greater than point_cr_value[ i - 1 ].

Signed-off-by: James Almer <jamrial@gmail.com>
---
Better version. Now it can't overflow the min value, and will also constrain
the max value to ensure v[i] > v[i-1] is always true.

 libavcodec/cbs_av1_syntax_template.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Andreas Rheinhardt Feb. 13, 2020, 5:45 p.m. UTC | #1
On Thu, Feb 13, 2020 at 4:08 PM James Almer <jamrial@gmail.com> wrote:

> If i is greater than 0, it is a requirement of bitstream conformance that
> point_y_value[ i ] is greater than point_y_value[ i - 1 ].
> If i is greater than 0, it is a requirement of bitstream conformance that
> point_cb_value[ i ] is greater than point_cb_value[ i - 1 ].
> If i is greater than 0, it is a requirement of bitstream conformance that
> point_cr_value[ i ] is greater than point_cr_value[ i - 1 ].
>
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> Better version. Now it can't overflow the min value, and will also
> constrain
> the max value to ensure v[i] > v[i-1] is always true.
>

How could the min value overflow at all? After all, the addition v[i - 1] +
1 is performed after promoting v[i - 1] to int. This is then losslessly
converted to uint32_t. So your new version will not detect any more errors
than the old version. It might error out earlier sometimes, but to do so it
always computes the max.
(With the old version it can happen that the min value is bigger than the
max value which leads to the desired error (and an error message that might
be confusing; avoiding this seems to be the only real advantage this new
version has).)

- Andreas
James Almer Feb. 13, 2020, 6:21 p.m. UTC | #2
On 2/13/2020 2:45 PM, Andreas Rheinhardt wrote:
> On Thu, Feb 13, 2020 at 4:08 PM James Almer <jamrial@gmail.com> wrote:
> 
>> If i is greater than 0, it is a requirement of bitstream conformance that
>> point_y_value[ i ] is greater than point_y_value[ i - 1 ].
>> If i is greater than 0, it is a requirement of bitstream conformance that
>> point_cb_value[ i ] is greater than point_cb_value[ i - 1 ].
>> If i is greater than 0, it is a requirement of bitstream conformance that
>> point_cr_value[ i ] is greater than point_cr_value[ i - 1 ].
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> Better version. Now it can't overflow the min value, and will also
>> constrain
>> the max value to ensure v[i] > v[i-1] is always true.
>>
> 
> How could the min value overflow at all? After all, the addition v[i - 1] +
> 1 is performed after promoting v[i - 1] to int. This is then losslessly
> converted to uint32_t. So your new version will not detect any more errors
> than the old version. It might error out earlier sometimes, but to do so it
> always computes the max.
> (With the old version it can happen that the min value is bigger than the
> max value which leads to the desired error (and an error message that might
> be confusing; avoiding this seems to be the only real advantage this new
> version has).)
> 
> - Andreas

I wasn't sure if the sum would be promoted or not, but in any case, the
core change is ensuring a given max value is constrained to always be
smaller than the next min allowed value.
Mark Thompson Feb. 13, 2020, 11:08 p.m. UTC | #3
On 13/02/2020 18:21, James Almer wrote:
> On 2/13/2020 2:45 PM, Andreas Rheinhardt wrote:
>> On Thu, Feb 13, 2020 at 4:08 PM James Almer <jamrial@gmail.com> wrote:
>>
>>> If i is greater than 0, it is a requirement of bitstream conformance that
>>> point_y_value[ i ] is greater than point_y_value[ i - 1 ].
>>> If i is greater than 0, it is a requirement of bitstream conformance that
>>> point_cb_value[ i ] is greater than point_cb_value[ i - 1 ].
>>> If i is greater than 0, it is a requirement of bitstream conformance that
>>> point_cr_value[ i ] is greater than point_cr_value[ i - 1 ].
>>>
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>> Better version. Now it can't overflow the min value, and will also
>>> constrain
>>> the max value to ensure v[i] > v[i-1] is always true.
>>>
>>
>> How could the min value overflow at all? After all, the addition v[i - 1] +
>> 1 is performed after promoting v[i - 1] to int. This is then losslessly
>> converted to uint32_t. So your new version will not detect any more errors
>> than the old version. It might error out earlier sometimes, but to do so it
>> always computes the max.
>> (With the old version it can happen that the min value is bigger than the
>> max value which leads to the desired error (and an error message that might
>> be confusing; avoiding this seems to be the only real advantage this new
>> version has).)
>>
>> - Andreas
> 
> I wasn't sure if the sum would be promoted or not, but in any case, the
> core change is ensuring a given max value is constrained to always be
> smaller than the next min allowed value.

Yeah.  With your change the error will be precise and diagnosed as early as possible (e.g. { 10, 30, 20, 40, 50 } -> ~"value[2] out of range: 20, but must be in [31,253]"), so it looks good to me.

Thanks,

- Mark
James Almer Feb. 13, 2020, 11:30 p.m. UTC | #4
On 2/13/2020 8:08 PM, Mark Thompson wrote:
> On 13/02/2020 18:21, James Almer wrote:
>> On 2/13/2020 2:45 PM, Andreas Rheinhardt wrote:
>>> On Thu, Feb 13, 2020 at 4:08 PM James Almer <jamrial@gmail.com> wrote:
>>>
>>>> If i is greater than 0, it is a requirement of bitstream conformance that
>>>> point_y_value[ i ] is greater than point_y_value[ i - 1 ].
>>>> If i is greater than 0, it is a requirement of bitstream conformance that
>>>> point_cb_value[ i ] is greater than point_cb_value[ i - 1 ].
>>>> If i is greater than 0, it is a requirement of bitstream conformance that
>>>> point_cr_value[ i ] is greater than point_cr_value[ i - 1 ].
>>>>
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>> Better version. Now it can't overflow the min value, and will also
>>>> constrain
>>>> the max value to ensure v[i] > v[i-1] is always true.
>>>>
>>>
>>> How could the min value overflow at all? After all, the addition v[i - 1] +
>>> 1 is performed after promoting v[i - 1] to int. This is then losslessly
>>> converted to uint32_t. So your new version will not detect any more errors
>>> than the old version. It might error out earlier sometimes, but to do so it
>>> always computes the max.
>>> (With the old version it can happen that the min value is bigger than the
>>> max value which leads to the desired error (and an error message that might
>>> be confusing; avoiding this seems to be the only real advantage this new
>>> version has).)
>>>
>>> - Andreas
>>
>> I wasn't sure if the sum would be promoted or not, but in any case, the
>> core change is ensuring a given max value is constrained to always be
>> smaller than the next min allowed value.
> 
> Yeah.  With your change the error will be precise and diagnosed as early as possible (e.g. { 10, 30, 20, 40, 50 } -> ~"value[2] out of range: 20, but must be in [31,253]"), so it looks good to me.
> 
> Thanks,
> 
> - Mark

Applied, thanks.
diff mbox series

Patch

diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c
index b62e07fb11..0855f7306f 100644
--- a/libavcodec/cbs_av1_syntax_template.c
+++ b/libavcodec/cbs_av1_syntax_template.c
@@ -1156,7 +1156,10 @@  static int FUNC(film_grain_params)(CodedBitstreamContext *ctx, RWContext *rw,
 
     fc(4, num_y_points, 0, 14);
     for (i = 0; i < current->num_y_points; i++) {
-        fbs(8, point_y_value[i],   1, i);
+        fcs(8, point_y_value[i],
+            i ? current->point_y_value[i - 1] + 1 : 0,
+            MAX_UINT_BITS(8) - (current->num_y_points - i - 1),
+            1, i);
         fbs(8, point_y_scaling[i], 1, i);
     }
 
@@ -1175,12 +1178,18 @@  static int FUNC(film_grain_params)(CodedBitstreamContext *ctx, RWContext *rw,
     } else {
         fc(4, num_cb_points, 0, 10);
         for (i = 0; i < current->num_cb_points; i++) {
-            fbs(8, point_cb_value[i],   1, i);
+            fcs(8, point_cb_value[i],
+                i ? current->point_cb_value[i - 1] + 1 : 0,
+                MAX_UINT_BITS(8) - (current->num_cb_points - i - 1),
+                1, i);
             fbs(8, point_cb_scaling[i], 1, i);
         }
         fc(4, num_cr_points, 0, 10);
         for (i = 0; i < current->num_cr_points; i++) {
-            fbs(8, point_cr_value[i],   1, i);
+            fcs(8, point_cr_value[i],
+                i ? current->point_cr_value[i - 1] + 1 : 0,
+                MAX_UINT_BITS(8) - (current->num_cr_points - i - 1),
+                1, i);
             fbs(8, point_cr_scaling[i], 1, i);
         }
     }