| Message ID | 20190321183726.9760-1-jamrial@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 21/03/2019 18:37, James Almer wrote: > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/cbs_av1_syntax_template.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c > index 48f4fab514..93bba1e5ad 100644 > --- a/libavcodec/cbs_av1_syntax_template.c > +++ b/libavcodec/cbs_av1_syntax_template.c > @@ -1637,15 +1637,15 @@ static int FUNC(metadata_hdr_mdcv)(CodedBitstreamContext *ctx, RWContext *rw, > int err, i; > > for (i = 0; i < 3; i++) { > - fcs(16, primary_chromaticity_x[i], 0, 50000, 1, i); > - fcs(16, primary_chromaticity_y[i], 0, 50000, 1, i); > + fbs(16, primary_chromaticity_x[i], 1, i); > + fbs(16, primary_chromaticity_y[i], 1, i); > } > > - fc(16, white_point_chromaticity_x, 0, 50000); > - fc(16, white_point_chromaticity_y, 0, 50000); > + fb(16, white_point_chromaticity_x); > + fb(16, white_point_chromaticity_y); > > fc(32, luminance_max, 1, MAX_UINT_BITS(32)); > - fc(32, luminance_min, 0, current->luminance_max >> 6); > + fc(32, luminance_min, 0, (current->luminance_max << 6) - 1); << 6 in uint32_t can overflow. Given the confusion in the previous value, maybe a comment to explain where the upper bound for min is coming from too? > > return 0; > } > LGTM with that. Thanks, - Mark
On 3/23/2019 9:46 AM, Mark Thompson wrote: > On 21/03/2019 18:37, James Almer wrote: >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/cbs_av1_syntax_template.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c >> index 48f4fab514..93bba1e5ad 100644 >> --- a/libavcodec/cbs_av1_syntax_template.c >> +++ b/libavcodec/cbs_av1_syntax_template.c >> @@ -1637,15 +1637,15 @@ static int FUNC(metadata_hdr_mdcv)(CodedBitstreamContext *ctx, RWContext *rw, >> int err, i; >> >> for (i = 0; i < 3; i++) { >> - fcs(16, primary_chromaticity_x[i], 0, 50000, 1, i); >> - fcs(16, primary_chromaticity_y[i], 0, 50000, 1, i); >> + fbs(16, primary_chromaticity_x[i], 1, i); >> + fbs(16, primary_chromaticity_y[i], 1, i); >> } >> >> - fc(16, white_point_chromaticity_x, 0, 50000); >> - fc(16, white_point_chromaticity_y, 0, 50000); >> + fb(16, white_point_chromaticity_x); >> + fb(16, white_point_chromaticity_y); >> >> fc(32, luminance_max, 1, MAX_UINT_BITS(32)); >> - fc(32, luminance_min, 0, current->luminance_max >> 6); >> + fc(32, luminance_min, 0, (current->luminance_max << 6) - 1); > > << 6 in uint32_t can overflow. What do you suggest? Casting to uint64_t will get the value clipped when passed to ff_cbs_read_unsigned() and potentially result in a failed range check. > > Given the confusion in the previous value, maybe a comment to explain where the upper bound for min is coming from too? > >> >> return 0; >> } >> > LGTM with that. > > Thanks, > > - Mark > _______________________________________________ > 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". >
On 23/03/2019 14:48, James Almer wrote: > On 3/23/2019 9:46 AM, Mark Thompson wrote: >> On 21/03/2019 18:37, James Almer wrote: >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> libavcodec/cbs_av1_syntax_template.c | 10 +++++----- >>> 1 file changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c >>> index 48f4fab514..93bba1e5ad 100644 >>> --- a/libavcodec/cbs_av1_syntax_template.c >>> +++ b/libavcodec/cbs_av1_syntax_template.c >>> @@ -1637,15 +1637,15 @@ static int FUNC(metadata_hdr_mdcv)(CodedBitstreamContext *ctx, RWContext *rw, >>> int err, i; >>> >>> for (i = 0; i < 3; i++) { >>> - fcs(16, primary_chromaticity_x[i], 0, 50000, 1, i); >>> - fcs(16, primary_chromaticity_y[i], 0, 50000, 1, i); >>> + fbs(16, primary_chromaticity_x[i], 1, i); >>> + fbs(16, primary_chromaticity_y[i], 1, i); >>> } >>> >>> - fc(16, white_point_chromaticity_x, 0, 50000); >>> - fc(16, white_point_chromaticity_y, 0, 50000); >>> + fb(16, white_point_chromaticity_x); >>> + fb(16, white_point_chromaticity_y); >>> >>> fc(32, luminance_max, 1, MAX_UINT_BITS(32)); >>> - fc(32, luminance_min, 0, current->luminance_max >> 6); >>> + fc(32, luminance_min, 0, (current->luminance_max << 6) - 1); >> >> << 6 in uint32_t can overflow. > > What do you suggest? Casting to uint64_t will get the value clipped when > passed to ff_cbs_read_unsigned() and potentially result in a failed > range check. Does something like FFMIN(((uint64_t)current->luminance_max << 6) - 1, MAX_UINT_BITS(32)) do the right thing? >> >> Given the confusion in the previous value, maybe a comment to explain where the upper bound for min is coming from too? >> >>> >>> return 0; >>> } >>> >> LGTM with that. >>
diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c index 48f4fab514..93bba1e5ad 100644 --- a/libavcodec/cbs_av1_syntax_template.c +++ b/libavcodec/cbs_av1_syntax_template.c @@ -1637,15 +1637,15 @@ static int FUNC(metadata_hdr_mdcv)(CodedBitstreamContext *ctx, RWContext *rw, int err, i; for (i = 0; i < 3; i++) { - fcs(16, primary_chromaticity_x[i], 0, 50000, 1, i); - fcs(16, primary_chromaticity_y[i], 0, 50000, 1, i); + fbs(16, primary_chromaticity_x[i], 1, i); + fbs(16, primary_chromaticity_y[i], 1, i); } - fc(16, white_point_chromaticity_x, 0, 50000); - fc(16, white_point_chromaticity_y, 0, 50000); + fb(16, white_point_chromaticity_x); + fb(16, white_point_chromaticity_y); fc(32, luminance_max, 1, MAX_UINT_BITS(32)); - fc(32, luminance_min, 0, current->luminance_max >> 6); + fc(32, luminance_min, 0, (current->luminance_max << 6) - 1); return 0; }
Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/cbs_av1_syntax_template.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)