Hey Christian,
On 12/21/2011 07:25 PM, Christian König wrote:
> Hi guys,
>
> On 21.12.2011 17:27, Maarten Lankhorst wrote:
>> Also a remark about your patch, wish you had inlined it..
> Sorry, not at home right now and I can't get this Thunderbird here to inline
> the patches correctly.
>
>> diff --git a/src/gallium/auxiliary/vl/vl_vlc.h
>> b/src/gallium/auxiliary/vl/vl_vlc.h
>> index dc4faed..f961b8b 100644
>> --- a/src/gallium/auxiliary/vl/vl_vlc.h
>> +++ b/src/gallium/auxiliary/vl/vl_vlc.h
>> @@ -115,22 +164,24 @@ vl_vlc_init(struct vl_vlc *vlc, const uint8_t *data,
>> unsigned len)
>> static INLINE unsigned
>> vl_vlc_bits_left(struct vl_vlc *vlc)
>> {
>> + unsigned i;
>> signed bytes_left = ((uint8_t*)vlc->end)-((uint8_t*)vlc->data);
>> + for (i = 0; i< vlc->num_inputs; ++i)
>> + bytes_left += vlc->sizes[i];
>> Calculating this more than once is wasteful, shouldn't this be done once in
>> init?
> Good idea, but doesn't make so much of a difference, since vl_vlc_bits_left
> is only called once per macroblock.
>
>> return bytes_left * 8 + vlc->valid_bits;
>> }
>>
>> ....
>>
>> @@ -86,28 +123,40 @@ vl_vlc_fillbits(struct vl_vlc *vlc)
>> vlc->buffer |= (uint64_t)value<< (32 - vlc->valid_bits);
>> ++vlc->data;
>> vlc->valid_bits += 32;
>> +
>> + /* if this input is depleted, go to next one */
>> + if (vlc->data>= vlc->end&& vlc->num_inputs) {
>> + unsigned bytes_overdue =
>> ((uint8_t*)vlc->data)-((uint8_t*)vlc->end);
>> +
>> + /* adjust valid bits */
>> + vlc->valid_bits -= bytes_overdue * 8;
>> +
>> + /* clear not valid bits */
>> + vlc->buffer&= ~((1LL<< (64 - vlc->valid_bits)) - 1);
>> +
>> + /* go on to next input */
>> + vl_vlc_next_input(vlc);
>> +
>> + /* and retry to fill the buffer */
>> + vl_vlc_fillbits(vlc);
>> Is this recursion necessary? Can't you simply change the if to a while?
>> Also if you are evil and put in a zero-sized buffer, this code will not work,
>> so the if checks needs to be reworked. Same problem with vl_vlc_next_input
>> and
>> unaligned pointers, you can cause it to crash with alignment 4n+1 and len<=
>> 2.
> Yeah, all the corner cases like len < 4, badly aligned start and end of
> buffer etc where not really covered by the last version of the patch.
>
> Take a look at the attached one, does it now cover all cases?
>
> On 21.12.2011 17:24, Younes Manton wrote:
>> 2011/12/21 Maarten Lankhorst<[email protected]>:
>>> Hey Christian,
>>>
>>> On 12/21/2011 04:41 PM, Christian König wrote:
>>>> Those functions are called a couple of million times a second, so even if
>>>> the assertion is only tested in debug builds it has quite an effect on
>>>> performance, sometimes even masquerading real performance problems. So my
>>>> practice was to only uncomment them when I work on that part of the code.
>>>> I don't want to change the Assert semantics in any way, just a simple
>>>> define enabling certain assertions only under certain conditions should be
>>>> sufficient.
>>> I think it makes sense to have the debug builds enabling those asserts by
>>> default.
>>>
>>> You could always do this in vl_mpeg12_bitstream.c when testing:
>>> #include "u_debug.h"
>>> #undef assert
>>> #include "vl_vlc.h"
>>> #define assert debug_assert
>>>
>>> On a related note, why is the vl code using assert.h directly instead of
>>> u_debug.h ?
>>>
>>> ~Maarten
>> u_debug.h didn't always have assert wrappers.
> I wasn't even aware that gallium has its own assert implementation in
> u_debug.h, cause a whole bunch of state trackers and drivers are using
> assert.h instead.
>
> Anyway, I have no idea what I tried the last time to make it perform so
> awfuly that I need to comment it out. So I think just leaving the assertions
> enabled for now should be ok. As Maarten pointed out disabling it is just a
> two liner.
It would be nice if you inlined patches for easier reviewing. :)
+ while (vlc->valid_bits < 32) {
+ /* if this input is depleted */
+ while (vlc->data >= vlc->end) {
+
+ if (vlc->num_inputs)
+ /* go on to next input */
+ vl_vlc_next_input(vlc);
+ else
+ /* or give up since we don't have anymore inputs */
+ return;
+ }
I'm spotting an overflow that could be triggered with 64 single-byte unaligned
buffers, maybe this is better:
+ while (vlc->valid_bits < 32) {
+ uint32_t value;
+ if (vlc->data >= vlc->end) {
+ if (vlc->num_inputs) {
+ vl_vlc_next_input(vlc);
+ continue;
+ } else
+ return;
+ }
+ value = *(const uint32_t*)vlc->data;
+ vlc->data += 4;
#ifndef PIPE_ARCH_BIG_ENDIAN
value = util_bswap32(value);
#endif
+ vlc->buffer |= (uint64_t)value << (32 - vlc->valid_bits);
+ vlc->valid_bits += 32;
+ if (vlc->data > vlc->end) {
....
With all the pointer math, maybe change the type for 'end' and 'data' to
uint8_t? Then you would only need that single cast in fillbits (which
I did above) and you can kill all the casts everywhere.
~Maarten
_______________________________________________
mesa-dev mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
