On 03/05/16 16:50, Vittorio Giovara wrote:
> On Tue, May 3, 2016 at 7:25 AM, Anton Khirnov <[email protected]> wrote:
>> Quoting Kieran Kunhya (2016-05-03 11:33:42)
>>> On Tue, 3 May 2016 at 07:43 Luca Barbato <[email protected]> wrote:
>>>
>>>> On 03/05/16 15:34, Kieran Kunhya wrote:
>>>>> I disagree, the old names are relatively clear. Whilst I think the speed
>>>>> improvements in this patch are great, the function names like
>>>> bitstream_read_32
>>>>> are really confusing. IMO adding a number suffix should be the exception
>>>>> rather than the norm (i.e when reading large numbers of bits).
>>>>
>>>> The past code shown that not having the number of bits would make people
>>>> assume such functions work for the wrong range.
>>>>
>>>> The new functions support a larger range BUT I had bitten once too many
>>>> to consider using _long for the 63 bits variant.
>>>>
>>>> Yes but reading > 32 bits isn't very common so it should be treated as the
>>> special case.
>>> All these _32s make things very very unreadable. I want the unusual cases
>>> to have special suffixes.
>>
>> I'm not buying those "common" vs "uncommon" arguments. Experience shows
>> that people get it wrong all the time with the current code, so the new
>> API should make it very explicit what limitation does each variant have.
>
> IMO Anton is right wrt common/uncommon, but Kieran is right for the
> trailing number being confusing.
> I would propose bitstream_uintread and bitstream_longread which would
> make it perfectly clear the maximum length of the read value.
I think the most common case is "I want to read some bits, often a constant
number but maybe not, and I'd like it to be fast but if I sacrifice some
marginal speed for readability then fine".
So, how about:
#if GNU_SOMETHING
#define bitstream_read(ctx, len) \
(__builtin_constant_p(len) ? ((len) < 0 ? barf() : \
(len) == 0 ? 0 : \
(len) == 1 ? bitstream_read_bit(ctx) : \
(len) < 32 ? bitstream_read_32((ctx), (len)) : \
(len) < 64 ? bitstream_read_63((ctx), (len)) : \
barf()) : \
bitstream_read_63((ctx), (len)))
#else
#define bitstream_read(ctx, len) bitstream_read_63((ctx), (len))
#endif
?
Cases where performance is very important can choose the correct inner function
as appropriate. Everyone else will usually get the right one anyway, and not
have to be careful about the internals and exactly how wide things are.
(Also maybe something microsofty as another variant, I don't know how that
works.)
- Mark
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
