On 12/22/2011 02:25 PM, Christian König wrote:
> Hi Maarten,
>
> On 21.12.2011 21:52, Maarten Lankhorst wrote:
>> It would be nice if you inlined patches for easier reviewing. :)
> Well I can try, but I can't promise that Thunderbird isn't badly fucking up
> all whitespaces, newest version of the patch is in-lined below.
http://www.mjmwired.net/kernel/Documentation/email-clients.txt
has some info on how to set things up. It mangles pretty badly by
default. Oddly enough enabling HTML format can help, I always
paste in my text in the preformat style, but send it as text.
+ vlc->buffer |= (uint64_t)*data<< (24 + vlc->invalid_bits);
I have no idea how your mailer comes up with such weird spaces
in the if statements though, might be the server rather than client.
:(
>> I'm spotting an overflow that could be triggered with 64 single-byte
>> unaligned buffers, maybe this is better:
> Should be fixed.
>> 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.
> Yeah, we now use it as int8_t more often, that saves at least some casts.
>
>> Another thought, this would prevent the need to read past end of file which
>> could show up erroneously in valgrind, with something like this: if
>> (end-data>= 4) { // Nom nom 4 bytes } else { // Read at most 3 bytes }
> Ok, take a look below. I now use if(bytes_left ==0) {...} else if (bytes_left
> >= 4) {...} else {...} construct, should this work or do you see any more
> corner cases I missed?
>
> I also inverted the valid_bits handling to invalid_bits and moved range where
> invalid_bits is now moving in between to -32 and 32, that should save a few
> more CPU cycles.
>
> Last but not least I added at least one sentence of documentation to each
> function. Any more thoughts or should we commit that now?
>
> diff --git a/src/gallium/auxiliary/vl/vl_vlc.h
> b/src/gallium/auxiliary/vl/vl_vlc.h
> index dc4faed..5e5e64c 100644
> --- a/src/gallium/auxiliary/vl/vl_vlc.h
> +++ b/src/gallium/auxiliary/vl/vl_vlc.h
> ...
> static INLINE void
> vl_vlc_fillbits(struct vl_vlc *vlc)
> {
> - if (vlc->valid_bits< 32) {
> - uint32_t value = *vlc->data;
> + assert(vlc);
>
> - //assert(vlc->data<= vlc->end);
> + /* as long as the buffer needs to be filled */
> + while (vlc->invalid_bits> 0) {
> + unsigned bytes_left = vlc->end - vlc->data;
> +
> + /* if this input is depleted */
> + if (bytes_left == 0) {
> +
> + 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;
> +
> + } else if (bytes_left>= 4) {
> +
> + /* enough bytes in buffer, read in a whole dword */
> + uint32_t value = *(const uint32_t*)vlc->data;
Change type to uint64_t and get rid of cast?
>
> #ifndef PIPE_ARCH_BIG_ENDIAN
> - value = util_bswap32(value);
> + value = util_bswap32(value);
> #endif
>
> - vlc->buffer |= (uint64_t)value<< (32 - vlc->valid_bits);
> - ++vlc->data;
> - vlc->valid_bits += 32;
> + vlc->buffer |= (uint64_t)value<< vlc->invalid_bits;
> + vlc->data += 4;
> + vlc->invalid_bits -= 32;
> +
> + /* buffer is now definitely filled up avoid the loop test */
> + break;
> +
> + } else while (vlc->data< vlc->end) {
> +
> + /* not enough bytes left in buffer, read single bytes */
> + vlc->buffer |= (uint64_t)*vlc->data<< (24 + vlc->invalid_bits);
> + ++vlc->data;
> + vlc->invalid_bits -= 8;
> + }
> }
> }
>
> +/*
> + * initialize vlc structure and start reading from first input buffer
> + */
> static INLINE void
> -vl_vlc_init(struct vl_vlc *vlc, const uint8_t *data, unsigned len)
> +vl_vlc_init(struct vl_vlc *vlc, unsigned num_inputs,
> + const void *const *inputs, const unsigned *sizes)
> {
> + unsigned i;
> +
> assert(vlc);
> - assert(data&& len);
> + assert(num_inputs);
>
> vlc->buffer = 0;
> - vlc->valid_bits = 0;
> + vlc->invalid_bits = 32;
> + vlc->num_inputs = num_inputs;
> + vlc->inputs = inputs;
> + vlc->sizes = sizes;
> + vlc->bytes_left = 0;
>
> - /* align the data pointer */
> - while (pointer_to_uintptr(data)& 3) {
> - vlc->buffer |= (uint64_t)*data<< (56 - vlc->valid_bits);
> - ++data;
> - --len;
> - vlc->valid_bits += 8;
> - }
> - vlc->data = (uint32_t*)data;
> - vlc->end = (uint32_t*)(data + len);
> + for (i = 0; i< num_inputs; ++i)
> + vlc->bytes_left += sizes[i];
>
> + vl_vlc_next_input(vlc);
> vl_vlc_fillbits(vlc);
> vl_vlc_fillbits(vlc);
Second fillbits is a noop now?
> }
>
> +/*
> + * number of bits still valid in bit buffer
> + */
> +static INLINE unsigned
> +vl_vlc_valid_bits(struct vl_vlc *vlc)
> +{
> + return 32 - vlc->invalid_bits;
> +}
Since all the 1 liner comments can already be implied from the function
name, I dont think it adds much. You might want to kill the comments entirely.
Personally I only prefer to add comments when I'm doing something that's not
clear from the code. If you want to make it part of doxygen you would need
to start with /** instead of /*. Up to you if you want to remove them or change
to /**.
Feel free to commit the result with those 3 changes without further review. :)
~Maarten
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
