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.rheinha...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
