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

[Andreas Rheinhardt]

Andreas Rheinhardt:
> Anton Khirnov:
>> +/* Unwind the cache so a refill_32 can fill it again. */
>> +static inline void bitstream_unwind(BitstreamContext *bc)
>> +{
>> +    int unwind = 4;
>> +    int unwind_bits = unwind * 8;
> 
> I'm surprised that you used signed types here.
> 
>> +
>> +    if (bc->bits_left < unwind_bits)
>> +        return;
>> +
>> +    bc->bits      >>= unwind_bits;
>> +    bc->bits      <<= unwind_bits;
> 
> The above won't work in LE. Best to call skip_remaining here. And you
> need to templatize this function in 3/4.

Calling skip_remaining is wrong either. Both the above (for BE) as well
as skip_remaining would skip the oldest 32 bits in the cache, but we
need to skip the newest 32 bits in the cache. So the following should do it:

    bc->bits_left -= unwind_bits;
    bc->ptr       -= unwind;
#ifdef BITSTREAM_READER_LE
    bc->bits      &= ((UINT64_C(1) << bc->bits_left) - 1);
#else
    bc->bits      &= ~(UINT64_T_MAX >> bc->bits_left);
#endif

(Given that bc->bits_left can be 0 one can't simply shift by 64 -
bits_left. I also don't know whether there should be any check before
decrementing ptr.)

> 
>> +    bc->bits_left  -= unwind_bits;
>> +    bc->ptr        -= unwind;
>> +}
>> +
>> +/* Unget up to 32 bits. */
>> +static inline void bitstream_unget(BitstreamContext *bc, uint64_t value,
>> +                                   size_t amount)
> 
> size_t is the natural type for the bytesize of objects, but not for
> bitsizes. A plane unsigned would be more natural here.
> 
>> +{
>> +    size_t cache_size = sizeof(bc->bits) * 8;
>> +
>> +    if (bc->bits_left + amount > cache_size)
>> +        bitstream_unwind(bc);
>> +
>> +    bc->bits       = (bc->bits >> amount) | (value << (cache_size - amount));
> 
> This is big-endian only, too.
> 
>> +    bc->bits_left += amount;
>> +}
>> +