[FFmpeg-devel] [PATCH 2/3] avcodec/cbs_h2645: Do not shorten reserved and unspecified NAL units in H264

[Andreas Rheinhardt]

On Wed, Dec 11, 2019 at 10:50 PM Andreas Rheinhardt <
andreas.rheinhardt at gmail.com> wrote:

> On Wed, Dec 11, 2019 at 10:38 PM James Almer <jamrial at gmail.com> wrote:
>
>> On 12/11/2019 6:03 PM, Michael Niedermayer wrote:
>> > Its unclear if shortening these NAL units would have no effect on them
>> >
>> > Fixes: assertion failure
>> > Fixes:
>> 19286/clusterfuzz-testcase-minimized-ffmpeg_BSF_H264_REDUNDANT_PPS_fuzzer-5707990724509696
>> >
>> > Found-by: continuous fuzzing process
>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>> > ---
>> >  libavcodec/cbs_h2645.c | 9 +++++++--
>> >  1 file changed, 7 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>> > index 5f71d80584..b38b1d99ef 100644
>> > --- a/libavcodec/cbs_h2645.c
>> > +++ b/libavcodec/cbs_h2645.c
>> > @@ -564,11 +564,16 @@ static int
>> cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx,
>> >          const H2645NAL *nal = &packet->nals[i];
>> >          AVBufferRef *ref;
>> >          size_t size = nal->size;
>> > +        int shorten = 1;
>> > +
>> > +        // Do not shorten reserved and unspecified NALs
>> > +        if (ctx->codec->codec_id == AV_CODEC_ID_H264) {
>> > +            shorten = nal->type > 0 && nal->type < 23;
>> > +        }
>> >
>> >          // Remove trailing zeroes.
>> > -        while (size > 0 && nal->data[size - 1] == 0)
>> > +        while (shorten && size > 0 && nal->data[size - 1] == 0)
>> >              --size;
>> > -        av_assert0(size > 0);
>>
>> So this is a NAL unit with a bunch of zero bytes? How did it go through
>> the filter in h2645_parse? It's supposed to skip any NAL like this.
>>
>> I guess it triggered this workaround:
>         /* see commit 3566042a0 */
>         if (bytestream2_get_bytes_left(&bc) >= 4 &&
>             bytestream2_peek_be32(&bc) == 0x000001E0)
>             skip_trailing_zeros = 0;
>
> If I am not mistaken, then all NAL units except one consisting solely of a
> single 0x00 (where the RBSP is empty) have to have a rbsp_stop_one_bit. So
> the only instance where stripping zeroes is not good is for such a 0x00
> unit. And such a unit should be stripped, because it actually can't be
> represented in annex b (which we output) at all.
>
> I think I am wrong about the very last part. From the H.264 spec (the HEVC
is the same):
"NumBytesInNALunit is set equal to the number of bytes starting with the
byte at the current position in the byte
stream up to and including the last byte that precedes the location of any
of the following:
– A subsequent byte-aligned three-byte sequence equal to 0x000000,
– A subsequent byte-aligned three-byte sequence equal to 0x000001,
– The end of the byte stream, as determined by unspecified means."
My earlier interpretation was that the three bytes following the first
startcode in this sequence 00 00 01 00 00 00 01 xx would mean that
NumBytesinNALunit were zero, because the last byte in the above sentence is
the 0x01 from the first startcode. But upon rereading I think that this is
ruled out by "subsequent", so that the NAL unit would be ended upon
encountering the second startcode (in this case, three bytes long).
But it still shows that if someone used one of the unspecified NAL units
and if these NAL units were allowed to end with zero bytes that are part of
the NAL unit, then said unit can't be represented in annex b (which
cbs_h2645 outputs) anyway. Furthermore, cbs_h2645 currently presumes that
all NAL units are escaped and consequently unescapes them upon reading and
escapes them upon writing. If unspecified NAL units were to deviate from
this, they would not survive this process unharmed (0x03 escapes might be
added). In particular, this applies to the aggregators from ISO/IEC
14496-15:
"The syntax of Aggregators may not follow the NAL unit syntax and the NAL
unit constraints specified in ISO/IEC
14496-10 or ISO/IEC 23008-2. For example, there may exist three continuous
bytes equal to a value in the range of
0x000000 to 0x000010, inclusive. This specifiation disallows the presence
of Aggregators in a video bitstream output
from parsing a file, therefore formal non-compliance with the video
specifications is immaterial as they will never be
presented to a video decoder."
Such units can't be represented in annex b at all, given the potential for
start code emulation. So I think the best we can do is what we already do:
Unescape every unit and escape them later. If any of the unspecified NAL
units uses the typical escaping scheme, it is fine; if not, we can't do
anything anyway.
Furthermore, our code for splitting a packet searches for 0x00 00 01 inside
an mp4/mkv NAL unit and views these as start of a new NAL unit (because
there are invalid files out there doing it that way). This of course has a
potential for start code emulation and so input that may contain start
codes within the NAL units would already be broken at this stage. I don't
think that there is a generic way to deal with this scenario; one would
have to specifically add support for every system that uses these
unspecified units. I think it's safe to say that this won't happen. Any
system that uses the typical escaping scheme works, the rest won't.

- Andreas