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.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
>>> ...
>>> @@ -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
_______________________________________________
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".
