"Ronald S. Bultje" <[email protected]> writes:
> Hi,
>
> 2011/12/15 Måns Rullgård <[email protected]>
>
>> "Ronald S. Bultje" <[email protected]> writes:
>> > @@ -144,7 +164,13 @@ for examples see get_bits, show_bits, skip_bits,
>> get_vlc
>> > # endif
>> >
>> > // FIXME name?
>> > +#if UNCHECKED_BITSTREAM_READER
>> > # define SKIP_COUNTER(name, gb, num) name##_index += (num)
>> > +#else
>> > +# define SKIP_COUNTER(name, gb, num) name##_index = \
>> > + (gb)->size_in_bits_plus1 - name##_index < (num) ? \
>> > + (gb)->size_in_bits_plus1 : name##_index + (num)
>> > +#endif
>>
>> Thinking more carefully about it, this addition is overflow-safe since
>> the skip amount must be small and we require end padding. Your old
>> version with FFMIN(size, index + num) is thus safe here after all,
>> should it result in better code.
>>
>
> It does actually (one mov less, so only an added cmp and cmova), and
> lightly faster on CAVLC/H264 (0.72s -> 0.83s instead of 0.86s on the
> super-high-bitrate sample), so we're keeping the FFMIN() then.
Great.
>> @@ -230,7 +263,12 @@ static inline int get_bits_count(const GetBitContext
>> *s) {
>> > static inline void skip_bits_long(GetBitContext *s, int n){
>> > OPEN_READER(re, s);
>> > re_bit_count += n;
>> > +#if UNCHECKED_BITSTREAM_READER
>> > re_buffer_ptr += re_bit_count>>5;
>> > +#else
>> > + re_buffer_ptr = (re_bit_count >> 5) < (const uint32_t *)
>> > s->buffer_end - re_buffer_ptr ?
>> > + re_buffer_ptr + (re_bit_count >> 5) : (const uint32_t
>> > *) s->buffer_end;
>> > +#endif
>>
>> This is again guaranteed to fall within the padding region, so whatever
>> variant gives better code is OK to use here.
>
> gcc messes up and does 4 more instructions. Fortunately it's not terribly
> important, but I'll stick to this:
>
> [..]
> re_buffer_ptr += re_bit_count>>5;
> + #if !UNCHECKED_BITSTREAM_READER
> + re_buffer_ptr = FFMIN(re_buffer_ptr, s->buffer_end);
> + #endif
Gah, I'm stupid. This is skip_bits_long(), which can be called with any
value. It must protect against overflow.
> New patch attached. Documentation not yet updated until we decide whether
> to enable by default or not.
[...]
> +#if UNCHECKED_BITSTREAM_READER
> # define SKIP_COUNTER(name, gb, num) name##_index += (num)
> +#else
> +# define SKIP_COUNTER(name, gb, num) name##_index =
> FFMIN((gb)->size_in_bits, \
> + name##_index +
> num)
> +#endif
Those lines are awfully long. Please do something about that, e.g. like
this:
# define SKIP_COUNTER(name, gb, num) \
name##_index = FFMIN((gb)->size_in_bits, name##_index + num)
--
Måns Rullgård
[email protected]
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
