[FFmpeg-devel] [PATCH v3 2/4] cbs_mpeg2: Improve checks for invalid values

[Mark Thompson]

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 at 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