[FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader

[Hendrik Leppkes]

On Sat, Jul 8, 2017 at 7:09 PM, foo86 <foobaz86 at gmail.com> wrote:
> On Sat, Jul 08, 2017 at 11:12:06AM +0200, Paul B Mahol wrote:
>> [...]
>
>>  static inline void skip_bits(GetBitContext *s, int n)
>>  {
>> +#ifdef CACHED_BITSTREAM_READER
>> +    if (n <= s->bits_left)
>> +        skip_remaining(s, n);
>> +    else {
>> +        n -= s->bits_left;
>> +        skip_remaining(s, s->bits_left);
> This causes undefined behavior if s->bits_left == 64.
>
>> +        if (n >= 64) {
>> +            unsigned skip = n;
>> +
>> +            n -= skip;
>> +            s->index += skip;
>> +        }
> This block looks strange.
>
>> +        refill_32(s);
>> +        if (n)
>> +            skip_remaining(s, n);
>> +    }
>> +#else
>>      OPEN_READER(re, s);
>>      LAST_SKIP_BITS(re, s, n);
>>      CLOSE_READER(re, s);
>> +#endif
>> +}
>> +
>> +static inline void skip_bits_long(GetBitContext *s, int n)
>> +{
>> +#ifdef CACHED_BITSTREAM_READER
>> +    skip_bits(s, n);
>> +#else
>> +#if UNCHECKED_BITSTREAM_READER
>> +    s->index += n;
>> +#else
>> +    s->index += av_clip(n, -s->index, s->size_in_bits_plus8 - s->index);
> Uncached bitstream reader allows seeking back by passing negative n
> here. If cached bitstream reader disallows this, there should be a
> comment saying so (and possibly an assert).

This seems like an undocumented and possibly insecure/invalid use of
the API, maybe we should just generally discourage such use.

Why would you need to skip backwards anyway? Usually code uses
show_bits, or creates a copy of the reader so it can revert to the
original if needed.

- Hendrik