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

Andreas Rheinhardt andreas.rheinhardt at gmail.com
Thu Dec 12 03:53:24 EET 2019


On Wed, Dec 11, 2019 at 11:58 PM Michael Niedermayer <michael at niedermayer.cc>
wrote:

> On Wed, Dec 11, 2019 at 10:50:54PM +0100, Andreas Rheinhardt 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
>
> yes
>
>
> > > 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;
>
> yes
>
>
> >
> > 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 dont see why it could not be represented.
>

I think I was wrong.


> Also the code you quoted above shows that there are NAL units where the
> removal of zeros would damage the units.
>
> Yes. But not removing zeroes could also alter the units, because these
zeroes might lead to the addition of 0x03 escape bytes when reassembling.
Btw: Where is the sample that triggered this?


> I wonder if we should assume that NALs not described in the current H26*
> specs
> follow exactly the RBSP syntax and can saftely be truncated.
>

As has been said in my other mail: Anything that may contain start codes
inside their NAL units can't be put into annex b anyway. Unfortunately
systems using such units exist.


> My patch went in the direction of rather leaving that alone.
>

Even with your patch they would not be left completely alone: The
unescaping->escaping process changes things for units initially unescaped;
not to mention the possibility of start code emulation within NAL units for
units that may contain start codes.
(The utmost that could be done to leave these units alone would be to use
the raw data of the unit instead of the unescaped unit in case it is one of
the unspecified units. And upon reassembling, no escaping would have to be
performed for these units, just a straight memcpy. But this would still not
solve the problem that h2645_packet_split searches the units for start
codes and treats them as start of new units. And even if this were solved,
one would have the problem that units that contain start codes can't be put
in annex b anyway. It's just a mess.)


> But of course we can do something else or i can try to implement that more
> completely also preserving that on the input side but having no real world
> testcase that would not be tested.
>
> What do you prefer? you seem to have a strong oppinion here
>
> The current code works with systems that follow the typical RBSP scheme;
supporting anything else would require significant changes to not only
cbs_h2645, but also the H.2645 parsing functions (or cbs_h2645 would have
to abandon them and implement their own). I don't think we have the
manpower, the samples and the motivation for this. So if it works, fine; if
not, then not. (We don't even support the SVC/MVC/3d extensions yet and
these are official.)
While this RBSP scheme would allow a NAL unit with empty RBSP that consists
of a single 0x00, I don't think that this will intentionally happen in
practice. And now that I have found out that I was wrong about such units
not being representable in annex b, I don't have a strong opinion any more
on whether these units should be discarded or kept as size one units. I am
still leaning towards discarding though, because I don't see a real-world
usecase for them and because they might confuse players in general (but I
haven't tested it to back this up, it's just a feeling).
But that's just my opinion. Actually, Mark should decide.

- Andreas


More information about the ffmpeg-devel mailing list