[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: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.

- Andreas