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

Michael Niedermayer michael at niedermayer.cc
Thu Dec 12 00:58:02 EET 2019


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.
Also the code you quoted above shows that there are NAL units where the
removal of zeros would damage the units.

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.
My patch went in the direction of rather leaving that alone.
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

Thanks

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20191211/a6071527/attachment.sig>


More information about the ffmpeg-devel mailing list