>> +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.
There are not such a member in GetBitContext, so it's not clear to me what
exactly do you mean by this. Do you mean unsigned int _cache as equivalent
variable to bits? In any case, bits can't be 32bit because I need to do shifts
and bits >> 32 would be undefined operation, also making bits variable
32bit will
cause slowdown because refill() will be called more often.
> 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 ?
What do you mean by different behaviour? This should always
return the same bit as bitstream_read_32(s, 1), if you know about
the case, when this is not true, please tell me how/when exactly
this's happening.
> +/**
>> + * 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.
You're right, show_val() can be used here, also this func is not used
anywhere and it should be discussed if it should be removed.
>
>> +
>> +/**
>> + * Seek to the given bit position.
>> + */
>> +static inline void bitstream_seek(BitstreamContext *s, unsigned pos)
>> +{
>> + s->ptr = s->buffer;;
>
> Stray ';'
Thank you, will be fixed.
>
>> +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.
I think using inline is useful here, it's just a recommendation for
the compiler, I'm not forcing inlining with av_always_inline
>
>> +/* 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.
I'm not sure, what do you mean by this. Bitstream_align/align_get_bits
is useful function used by various decoders and I don't see how this
could be done with just one "tell".
>
>> +/**
>> + * 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
yes, it's a good idea and I'll add inline here.
Best regards,
Alexandra
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
