Re: [FFmpeg-devel] [PATCH 2/3] lavc: add standalone cached bitstream reader

[Anton Khirnov <anton@khirnov.net>]

Quoting Andreas Rheinhardt (2022-07-03 15:16:39)
> Anton Khirnov:
> > +
> > +#include "mathops.h"
> 
> What exactly is mathops.h for? get_bits.h uses it for NEG_USR32 and
> NEG_SSR32, but you are not using it here.

sign_extend()?

> > +/**
> > + * Skip bits to a byte boundary.
> > + */
> > +static inline const uint8_t *bits_align(BitstreamContext *bc)
> > +{
> > +    unsigned int n = -bits_tell(bc) & 7;
> > +    if (n)
> > +        bits_skip(bc, n);
> 
> Is there a reason that I don't see that makes you not simply use
> bc->bits_left &= ~0xff?

I don't see how that is supposed to work.

> 
> > +    return bc->buffer + (bits_tell(bc) >> 3);
> > +}
> > +
> > +/**
> > + * Read MPEG-1 dc-style VLC (sign bit + mantissa with no MSB).
> > + * If MSB not set it is negative.
> > + * @param n length in bits
> > + */
> > +static inline int bits_read_xbits(BitstreamContext *bc, unsigned int n)
> > +{
> > +    int32_t cache = bits_peek(bc, 32);
> > +    int sign = ~cache >> 31;
> > +    bits_priv_skip_remaining(bc, n);
> 
> FYI: You are potentially skipping more bits here than you have.

Yes, you made that clear in your fake-caching patch. Do you require that
be fixed now? Or can this go in, then we apply some form of your patch?

> > +/* Read sign bit and flip the sign of the provided value accordingly. */
> > +static inline int bits_apply_sign(BitstreamContext *bc, int val)
> > +{
> > +    int sign = bits_read_signed(bc, 1);
> 
> Is there a reason you are not using bits_read_bit here?

I want a signed result.

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".