Hi,
2016-04-27 13:37 GMT+02:00 Alexandra Hájková <[email protected]>:
> +typedef struct BitstreamContext {
> + uint64_t bits; // stores bits read from the buffer
It is 32 bits for get_bits.
So, did you write the new code with a 64b from the start? Because,
except for the _long/_64 version, it would have been probably simple &
interesting to see what a version with 32b provides.
Extending get_bits with a 64b type is probably bothersome, and you
already wrote a new implementation, but that would have been
interesting to see the benefit.
> +/**
> + * Return one bit from the buffer.
> + */
> +static inline unsigned int bitstream_read_bit(BitstreamContext *s)
> +{
> + if (av_unlikely(!s->bits_left))
> + refill_64(s);
> +
> + return (uint8_t)get_val(s, 1);
> +}
This seems to behave differently from bitstream_read_32(s, 1). Is
there a particular rule/hypothesis for using bitstream_read_bit ?
> +/**
> + * Return n bits from the buffer, n has to be in the 0-63 range.
> + * But do not change the buffer state.
> + */
> +static inline unsigned int bitstream_peek_63(BitstreamContext *s, int n)
> +{
> + BitstreamContext tmp = *s;
> +
> + return bitstream_read_63(&tmp, n);
> +}
> +
> +static inline unsigned int show_val(BitstreamContext *s, int n)
> +{
> + int ret;
> +
> +#ifdef BITSTREAM_READER_LE
> + ret = s->bits & ((UINT64_C(1) << n) - 1);
> +#else
> + ret = s->bits >> (64 - n);
> +#endif
> +
> + return ret;
> +}
> +
> +/**
> + * Return n bits from the buffer, n has to be in the 0-32 range.
> + * But do not change the buffer state.
> + */
> +static inline unsigned int bitstream_peek_32(BitstreamContext *s, int n)
> +{
> + if (av_unlikely(n > s->bits_left))
> + refill_32(s);
> +
> + return show_val(s, n);
> +}
Why is bitstream_peek_63 different from bitstream_peek_32? Can't
show_val be used?
Hopefully the compiler optimizes the function a lot and does not
really copy things... Although it's probably not that useful a hot
function.
> +
> +/**
> + * Seek to the given bit position.
> + */
> +static inline void bitstream_seek(BitstreamContext *s, unsigned pos)
> +{
> + s->ptr = s->buffer;;
Stray ';'
> +static inline int bitstream_init(BitstreamContext *s, const uint8_t *buffer,
> int bit_size)
> +{
> + int buffer_size;
> + int ret = 0;
> +
> + if (bit_size > INT_MAX - 7 || bit_size < 0 || !buffer) {
> + buffer = s->buffer = s->ptr = NULL;
> + s->bits_left = 0;
> + return AVERROR_INVALIDDATA;
> + }
> +
> + buffer_size = (bit_size + 7) >> 3;
> +
> + s->buffer = buffer;
> + s->buffer_end = buffer + buffer_size;
> + s->ptr = s->buffer;
> + s->size_in_bits = bit_size;
> + s->bits_left = 0;
> + s->bits = 0;
> +
> + refill_64(s);
> +
> + return ret;
> +}
get_bits has it inlined too. Maybe not that useful? Only if removing
the inline doesn't actually increase size.
> +/* Skip bits to a byte boundary */
> +static inline const uint8_t *bitstream_align(BitstreamContext *s)
> +{
> + int n = -bitstream_tell(s) & 7;
> + if (n)
> + bitstream_skip(s, n);
> + return s->buffer + (bitstream_tell(s) >> 3);
> +}
Is doing the extra _tell operation that useful if nobody uses it and
it can be done separately when needed?
Probably not a hot function though.
> +/**
> + * Return the LUT element for the given bitstream configuration.
> + */
> +static int set_idx(BitstreamContext *s, int code, int *n, int *nb_bits,
> VLC_TYPE (*table)[2])
Why no inline there? Again, if it increases significantly the binary
size when inlined
--
Christophe
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
