| Message ID | 20190423225515.54955-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 4/23/2019 7:55 PM, Andreas Rheinhardt wrote: > horizontal/vertical_size_value (containing the twelve least significant > bits of the frame size) mustn't be zero according to the specifications; > and the value 0 is forbidden for the colour_description elements. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > The actual function calls after macro expansion are the same as in the > earlier versions with the exception of the extra_bit_slice calls. > libavcodec/cbs_mpeg2.c | 14 ++++++++------ > libavcodec/cbs_mpeg2_syntax_template.c | 20 ++++++++++---------- > 2 files changed, 18 insertions(+), 16 deletions(-) > > diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c > index cdde68ea38..fc21745a51 100644 > --- a/libavcodec/cbs_mpeg2.c > +++ b/libavcodec/cbs_mpeg2.c > @@ -41,20 +41,22 @@ > #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL) > > #define ui(width, name) \ > - xui(width, name, current->name, 0) > + xui(width, name, current->name, 0, MAX_UINT_BITS(width), 0) > +#define uir(width, name, range_min, range_max) \ > + xui(width, name, current->name, range_min, range_max, 0) > #define uis(width, name, subs, ...) \ > - xui(width, name, current->name, subs, __VA_ARGS__) > + xui(width, name, current->name, 0, MAX_UINT_BITS(width), subs, __VA_ARGS__) > > > #define READ > #define READWRITE read > #define RWContext GetBitContext > > -#define xui(width, name, var, subs, ...) do { \ > +#define xui(width, name, var, range_min, range_max, subs, ...) do { \ > uint32_t value = 0; \ > CHECK(ff_cbs_read_unsigned(ctx, rw, width, #name, \ > SUBSCRIPTS(subs, __VA_ARGS__), \ > - &value, 0, (1 << width) - 1)); \ > + &value, range_min, range_max)); \ > var = value; \ > } while (0) > > @@ -81,10 +83,10 @@ > #define READWRITE write > #define RWContext PutBitContext > > -#define xui(width, name, var, subs, ...) do { \ > +#define xui(width, name, var, range_min, range_max, subs, ...) do { \ > CHECK(ff_cbs_write_unsigned(ctx, rw, width, #name, \ > SUBSCRIPTS(subs, __VA_ARGS__), \ > - var, 0, (1 << width) - 1)); \ > + var, range_min, range_max)); \ > } while (0) > > #define marker_bit() do { \ > diff --git a/libavcodec/cbs_mpeg2_syntax_template.c b/libavcodec/cbs_mpeg2_syntax_template.c > index 10aaea7734..27dcaad316 100644 > --- a/libavcodec/cbs_mpeg2_syntax_template.c > +++ b/libavcodec/cbs_mpeg2_syntax_template.c > @@ -26,8 +26,8 @@ static int FUNC(sequence_header)(CodedBitstreamContext *ctx, RWContext *rw, > > ui(8, sequence_header_code); > > - ui(12, horizontal_size_value); > - ui(12, vertical_size_value); > + uir(12, horizontal_size_value, 1, 4095); > + uir(12, vertical_size_value, 1, 4095); > > mpeg2->horizontal_size = current->horizontal_size_value; > mpeg2->vertical_size = current->vertical_size_value; > @@ -79,7 +79,7 @@ static int FUNC(user_data)(CodedBitstreamContext *ctx, RWContext *rw, > #endif > > for (k = 0; k < current->user_data_length; k++) > - xui(8, user_data, current->user_data[k], 0); > + xui(8, user_data, current->user_data[k], 0, 255, 0); Not sure why this is using xui() to being with, when it could simply be uis(8, user_data[k], 1, k); And it would also look better in the trace output. > > return 0; > } > @@ -125,9 +125,9 @@ static int FUNC(sequence_display_extension)(CodedBitstreamContext *ctx, RWContex > > ui(1, colour_description); > if (current->colour_description) { > - ui(8, colour_primaries); > - ui(8, transfer_characteristics); > - ui(8, matrix_coefficients); > + uir(8, colour_primaries, 1, 255); > + uir(8, transfer_characteristics, 1, 255); > + uir(8, matrix_coefficients, 1, 255); > } > > ui(14, display_horizontal_size); > @@ -366,16 +366,16 @@ static int FUNC(slice_header)(CodedBitstreamContext *ctx, RWContext *rw, > if (!current->extra_information) > return AVERROR(ENOMEM); > for (k = 0; k < current->extra_information_length; k++) { > - xui(1, extra_bit_slice, bit, 0); > + xui(1, extra_bit_slice, bit, 1, 1, 0); > xui(8, extra_information_slice[k], > - current->extra_information[k], 1, k); > + current->extra_information[k], 0, 255, 1, k); > } > } > #else > for (k = 0; k < current->extra_information_length; k++) { > - xui(1, extra_bit_slice, 1, 0); > + xui(1, extra_bit_slice, 1, 1, 1, 0); > xui(8, extra_information_slice[k], > - current->extra_information[k], 1, k); > + current->extra_information[k], 0, 255, 1, k); > } > #endif > } Should be good in any case. Mark can comment on the above and/or push it.
On 23/04/2019 23:55, Andreas Rheinhardt wrote: > horizontal/vertical_size_value (containing the twelve least significant > bits of the frame size) mustn't be zero according to the specifications; > and the value 0 is forbidden for the colour_description elements. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > The actual function calls after macro expansion are the same as in the > earlier versions with the exception of the extra_bit_slice calls. > libavcodec/cbs_mpeg2.c | 14 ++++++++------ > libavcodec/cbs_mpeg2_syntax_template.c | 20 ++++++++++---------- > 2 files changed, 18 insertions(+), 16 deletions(-) > > diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c > index cdde68ea38..fc21745a51 100644 > --- a/libavcodec/cbs_mpeg2.c > +++ b/libavcodec/cbs_mpeg2.c > @@ -41,20 +41,22 @@ > #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL) > > #define ui(width, name) \ > - xui(width, name, current->name, 0) > + xui(width, name, current->name, 0, MAX_UINT_BITS(width), 0) > +#define uir(width, name, range_min, range_max) \ > + xui(width, name, current->name, range_min, range_max, 0) > #define uis(width, name, subs, ...) \ > - xui(width, name, current->name, subs, __VA_ARGS__) > + xui(width, name, current->name, 0, MAX_UINT_BITS(width), subs, __VA_ARGS__) > > > #define READ > #define READWRITE read > #define RWContext GetBitContext > > -#define xui(width, name, var, subs, ...) do { \ > +#define xui(width, name, var, range_min, range_max, subs, ...) do { \ > uint32_t value = 0; \ > CHECK(ff_cbs_read_unsigned(ctx, rw, width, #name, \ > SUBSCRIPTS(subs, __VA_ARGS__), \ > - &value, 0, (1 << width) - 1)); \ > + &value, range_min, range_max)); \ > var = value; \ > } while (0) > > @@ -81,10 +83,10 @@ > #define READWRITE write > #define RWContext PutBitContext > > -#define xui(width, name, var, subs, ...) do { \ > +#define xui(width, name, var, range_min, range_max, subs, ...) do { \ > CHECK(ff_cbs_write_unsigned(ctx, rw, width, #name, \ > SUBSCRIPTS(subs, __VA_ARGS__), \ > - var, 0, (1 << width) - 1)); \ > + var, range_min, range_max)); \ > } while (0) > > #define marker_bit() do { \ > diff --git a/libavcodec/cbs_mpeg2_syntax_template.c b/libavcodec/cbs_mpeg2_syntax_template.c > index 10aaea7734..27dcaad316 100644 > --- a/libavcodec/cbs_mpeg2_syntax_template.c > +++ b/libavcodec/cbs_mpeg2_syntax_template.c > @@ -26,8 +26,8 @@ static int FUNC(sequence_header)(CodedBitstreamContext *ctx, RWContext *rw, > > ui(8, sequence_header_code); > > - ui(12, horizontal_size_value); > - ui(12, vertical_size_value); > + uir(12, horizontal_size_value, 1, 4095); > + uir(12, vertical_size_value, 1, 4095); > > mpeg2->horizontal_size = current->horizontal_size_value; > mpeg2->vertical_size = current->vertical_size_value; > @@ -79,7 +79,7 @@ static int FUNC(user_data)(CodedBitstreamContext *ctx, RWContext *rw, > #endif > > for (k = 0; k < current->user_data_length; k++) > - xui(8, user_data, current->user_data[k], 0); > + xui(8, user_data, current->user_data[k], 0, 255, 0); This could include the subscript while you're changing it. uis(8, user_data[k], 1, k); > > return 0; > } > @@ -125,9 +125,9 @@ static int FUNC(sequence_display_extension)(CodedBitstreamContext *ctx, RWContex > > ui(1, colour_description); > if (current->colour_description) { > - ui(8, colour_primaries); > - ui(8, transfer_characteristics); > - ui(8, matrix_coefficients); > + uir(8, colour_primaries, 1, 255); > + uir(8, transfer_characteristics, 1, 255); > + uir(8, matrix_coefficients, 1, 255); I'm a bit unsure about this one. While the zero value is indeed banned by the standard, it isn't uncommon to find (at least in H.264) streams with zeroes in these fields. (The libavcodec encoder for MPEG-2 is happy to write such a stream, too.) Start code aliasing is presumably the reason for the standard disallowing zeroes, but they probably won't actually be a problem unless display_horizontal_size happens to have a specific range of bad values (in which case we already fail in a different way). > } > > ui(14, display_horizontal_size); > @@ -366,16 +366,16 @@ static int FUNC(slice_header)(CodedBitstreamContext *ctx, RWContext *rw, > if (!current->extra_information) > return AVERROR(ENOMEM); > for (k = 0; k < current->extra_information_length; k++) { > - xui(1, extra_bit_slice, bit, 0); > + xui(1, extra_bit_slice, bit, 1, 1, 0); > xui(8, extra_information_slice[k], > - current->extra_information[k], 1, k); > + current->extra_information[k], 0, 255, 1, k); > } > } > #else > for (k = 0; k < current->extra_information_length; k++) { > - xui(1, extra_bit_slice, 1, 0); > + xui(1, extra_bit_slice, 1, 1, 1, 0); > xui(8, extra_information_slice[k], > - current->extra_information[k], 1, k); > + current->extra_information[k], 0, 255, 1, k); > } > #endif > } > Everything else about this series LGTM. Thanks, - Mark
Mark Thompson: > On 23/04/2019 23:55, Andreas Rheinhardt wrote: >> horizontal/vertical_size_value (containing the twelve least significant >> bits of the frame size) mustn't be zero according to the specifications; >> and the value 0 is forbidden for the colour_description elements. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> The actual function calls after macro expansion are the same as in the >> earlier versions with the exception of the extra_bit_slice calls. >> libavcodec/cbs_mpeg2.c | 14 ++++++++------ >> libavcodec/cbs_mpeg2_syntax_template.c | 20 ++++++++++---------- >> 2 files changed, 18 insertions(+), 16 deletions(-) >> >> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c >> index cdde68ea38..fc21745a51 100644 >> --- a/libavcodec/cbs_mpeg2.c >> +++ b/libavcodec/cbs_mpeg2.c >> @@ -41,20 +41,22 @@ >> #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL) >> >> #define ui(width, name) \ >> - xui(width, name, current->name, 0) >> + xui(width, name, current->name, 0, MAX_UINT_BITS(width), 0) >> +#define uir(width, name, range_min, range_max) \ >> + xui(width, name, current->name, range_min, range_max, 0) >> #define uis(width, name, subs, ...) \ >> - xui(width, name, current->name, subs, __VA_ARGS__) >> + xui(width, name, current->name, 0, MAX_UINT_BITS(width), subs, __VA_ARGS__) >> >> >> #define READ >> #define READWRITE read >> #define RWContext GetBitContext >> >> -#define xui(width, name, var, subs, ...) do { \ >> +#define xui(width, name, var, range_min, range_max, subs, ...) do { \ >> uint32_t value = 0; \ >> CHECK(ff_cbs_read_unsigned(ctx, rw, width, #name, \ >> SUBSCRIPTS(subs, __VA_ARGS__), \ >> - &value, 0, (1 << width) - 1)); \ >> + &value, range_min, range_max)); \ >> var = value; \ >> } while (0) >> >> @@ -81,10 +83,10 @@ >> #define READWRITE write >> #define RWContext PutBitContext >> >> -#define xui(width, name, var, subs, ...) do { \ >> +#define xui(width, name, var, range_min, range_max, subs, ...) do { \ >> CHECK(ff_cbs_write_unsigned(ctx, rw, width, #name, \ >> SUBSCRIPTS(subs, __VA_ARGS__), \ >> - var, 0, (1 << width) - 1)); \ >> + var, range_min, range_max)); \ >> } while (0) >> >> #define marker_bit() do { \ >> diff --git a/libavcodec/cbs_mpeg2_syntax_template.c b/libavcodec/cbs_mpeg2_syntax_template.c >> index 10aaea7734..27dcaad316 100644 >> --- a/libavcodec/cbs_mpeg2_syntax_template.c >> +++ b/libavcodec/cbs_mpeg2_syntax_template.c >> @@ -26,8 +26,8 @@ static int FUNC(sequence_header)(CodedBitstreamContext *ctx, RWContext *rw, >> >> ui(8, sequence_header_code); >> >> - ui(12, horizontal_size_value); >> - ui(12, vertical_size_value); >> + uir(12, horizontal_size_value, 1, 4095); >> + uir(12, vertical_size_value, 1, 4095); >> >> mpeg2->horizontal_size = current->horizontal_size_value; >> mpeg2->vertical_size = current->vertical_size_value; >> @@ -79,7 +79,7 @@ static int FUNC(user_data)(CodedBitstreamContext *ctx, RWContext *rw, >> #endif >> >> for (k = 0; k < current->user_data_length; k++) >> - xui(8, user_data, current->user_data[k], 0); >> + xui(8, user_data, current->user_data[k], 0, 255, 0); > > This could include the subscript while you're changing it. > > uis(8, user_data[k], 1, k); > Yes, James remarked the same. >> >> return 0; >> } >> @@ -125,9 +125,9 @@ static int FUNC(sequence_display_extension)(CodedBitstreamContext *ctx, RWContex >> >> ui(1, colour_description); >> if (current->colour_description) { >> - ui(8, colour_primaries); >> - ui(8, transfer_characteristics); >> - ui(8, matrix_coefficients); >> + uir(8, colour_primaries, 1, 255); >> + uir(8, transfer_characteristics, 1, 255); >> + uir(8, matrix_coefficients, 1, 255); > > I'm a bit unsure about this one. While the zero value is indeed banned by the standard, it isn't uncommon to find (at least in H.264) streams with zeroes in these fields. (The libavcodec encoder for MPEG-2 is happy to write such a stream, too.) Start code aliasing is presumably the reason for the standard disallowing zeroes, but they probably won't actually be a problem unless display_horizontal_size happens to have a specific range of bad values (in which case we already fail in a different way). > Yeah, that is certainly because of start code emulation. What do you think about correcting these values while reading? Would be especially good considering that mpeg2_metadata under certain circumstances created such files in the first place. - Andreas
On 28/04/2019 23:15, Andreas Rheinhardt wrote: > Mark Thompson: >> On 23/04/2019 23:55, Andreas Rheinhardt wrote: >>> horizontal/vertical_size_value (containing the twelve least significant >>> bits of the frame size) mustn't be zero according to the specifications; >>> and the value 0 is forbidden for the colour_description elements. >>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>> --- >>> The actual function calls after macro expansion are the same as in the >>> earlier versions with the exception of the extra_bit_slice calls. >>> libavcodec/cbs_mpeg2.c | 14 ++++++++------ >>> libavcodec/cbs_mpeg2_syntax_template.c | 20 ++++++++++---------- >>> 2 files changed, 18 insertions(+), 16 deletions(-) >>> >>> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c >>> index cdde68ea38..fc21745a51 100644 >>> --- a/libavcodec/cbs_mpeg2.c >>> +++ b/libavcodec/cbs_mpeg2.c >>> ... >>> @@ -125,9 +125,9 @@ static int FUNC(sequence_display_extension)(CodedBitstreamContext *ctx, RWContex >>> >>> ui(1, colour_description); >>> if (current->colour_description) { >>> - ui(8, colour_primaries); >>> - ui(8, transfer_characteristics); >>> - ui(8, matrix_coefficients); >>> + uir(8, colour_primaries, 1, 255); >>> + uir(8, transfer_characteristics, 1, 255); >>> + uir(8, matrix_coefficients, 1, 255); >> >> I'm a bit unsure about this one. While the zero value is indeed banned by the standard, it isn't uncommon to find (at least in H.264) streams with zeroes in these fields. (The libavcodec encoder for MPEG-2 is happy to write such a stream, too.) Start code aliasing is presumably the reason for the standard disallowing zeroes, but they probably won't actually be a problem unless display_horizontal_size happens to have a specific range of bad values (in which case we already fail in a different way). >> > Yeah, that is certainly because of start code emulation. What do you > think about correcting these values while reading? Would be especially > good considering that mpeg2_metadata under certain circumstances > created such files in the first place. By correcting the values, you are thinking that if you see a zero in one of those fields during read, change it to a two? (And always reject it entirely on write.) That feels slightly dubious to me because the intent is to read the content of the stream as-is, but equally it's reasonable to argue that since the stream is invalid we can apply undefined behaviour logic to reinterpret it in a more sensible way. I think I'd be happy with that answer if you want to go with it? Thanks, - Mark
diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c index cdde68ea38..fc21745a51 100644 --- a/libavcodec/cbs_mpeg2.c +++ b/libavcodec/cbs_mpeg2.c @@ -41,20 +41,22 @@ #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL) #define ui(width, name) \ - xui(width, name, current->name, 0) + xui(width, name, current->name, 0, MAX_UINT_BITS(width), 0) +#define uir(width, name, range_min, range_max) \ + xui(width, name, current->name, range_min, range_max, 0) #define uis(width, name, subs, ...) \ - xui(width, name, current->name, subs, __VA_ARGS__) + xui(width, name, current->name, 0, MAX_UINT_BITS(width), subs, __VA_ARGS__) #define READ #define READWRITE read #define RWContext GetBitContext -#define xui(width, name, var, subs, ...) do { \ +#define xui(width, name, var, range_min, range_max, subs, ...) do { \ uint32_t value = 0; \ CHECK(ff_cbs_read_unsigned(ctx, rw, width, #name, \ SUBSCRIPTS(subs, __VA_ARGS__), \ - &value, 0, (1 << width) - 1)); \ + &value, range_min, range_max)); \ var = value; \ } while (0) @@ -81,10 +83,10 @@ #define READWRITE write #define RWContext PutBitContext -#define xui(width, name, var, subs, ...) do { \ +#define xui(width, name, var, range_min, range_max, subs, ...) do { \ CHECK(ff_cbs_write_unsigned(ctx, rw, width, #name, \ SUBSCRIPTS(subs, __VA_ARGS__), \ - var, 0, (1 << width) - 1)); \ + var, range_min, range_max)); \ } while (0) #define marker_bit() do { \ diff --git a/libavcodec/cbs_mpeg2_syntax_template.c b/libavcodec/cbs_mpeg2_syntax_template.c index 10aaea7734..27dcaad316 100644 --- a/libavcodec/cbs_mpeg2_syntax_template.c +++ b/libavcodec/cbs_mpeg2_syntax_template.c @@ -26,8 +26,8 @@ static int FUNC(sequence_header)(CodedBitstreamContext *ctx, RWContext *rw, ui(8, sequence_header_code); - ui(12, horizontal_size_value); - ui(12, vertical_size_value); + uir(12, horizontal_size_value, 1, 4095); + uir(12, vertical_size_value, 1, 4095); mpeg2->horizontal_size = current->horizontal_size_value; mpeg2->vertical_size = current->vertical_size_value; @@ -79,7 +79,7 @@ static int FUNC(user_data)(CodedBitstreamContext *ctx, RWContext *rw, #endif for (k = 0; k < current->user_data_length; k++) - xui(8, user_data, current->user_data[k], 0); + xui(8, user_data, current->user_data[k], 0, 255, 0); return 0; } @@ -125,9 +125,9 @@ static int FUNC(sequence_display_extension)(CodedBitstreamContext *ctx, RWContex ui(1, colour_description); if (current->colour_description) { - ui(8, colour_primaries); - ui(8, transfer_characteristics); - ui(8, matrix_coefficients); + uir(8, colour_primaries, 1, 255); + uir(8, transfer_characteristics, 1, 255); + uir(8, matrix_coefficients, 1, 255); } ui(14, display_horizontal_size); @@ -366,16 +366,16 @@ static int FUNC(slice_header)(CodedBitstreamContext *ctx, RWContext *rw, if (!current->extra_information) return AVERROR(ENOMEM); for (k = 0; k < current->extra_information_length; k++) { - xui(1, extra_bit_slice, bit, 0); + xui(1, extra_bit_slice, bit, 1, 1, 0); xui(8, extra_information_slice[k], - current->extra_information[k], 1, k); + current->extra_information[k], 0, 255, 1, k); } } #else for (k = 0; k < current->extra_information_length; k++) { - xui(1, extra_bit_slice, 1, 0); + xui(1, extra_bit_slice, 1, 1, 1, 0); xui(8, extra_information_slice[k], - current->extra_information[k], 1, k); + current->extra_information[k], 0, 255, 1, k); } #endif }
horizontal/vertical_size_value (containing the twelve least significant bits of the frame size) mustn't be zero according to the specifications; and the value 0 is forbidden for the colour_description elements. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- The actual function calls after macro expansion are the same as in the earlier versions with the exception of the extra_bit_slice calls. libavcodec/cbs_mpeg2.c | 14 ++++++++------ libavcodec/cbs_mpeg2_syntax_template.c | 20 ++++++++++---------- 2 files changed, 18 insertions(+), 16 deletions(-)