| Message ID | 20200213150733.2515-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,v2] avcodec/cbs_av1: add missing value constrains to point_y_value, point_cb_value and point_cr_value | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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
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.
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
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 --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); } }
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(-)