[Mesa-dev] [PATCH 1/3] vl: Only initialize vlc once

[Maarten Lankhorst]

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