On Wed, Dec 11, 2019 at 10:50 PM Andreas Rheinhardt < andreas.rheinha...@gmail.com> wrote:
> On Wed, Dec 11, 2019 at 10:38 PM James Almer <jamr...@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 <mich...@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 _______________________________________________ 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".
