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

[Maarten Lankhorst]

Also a remark about your patch, wish you had inlined it..

On 12/21/2011 04:41 PM, Christian König wrote:
> On 20.12.2011 19:20, Maarten Lankhorst wrote:
>> Hey Christian,
>>
>> On 12/20/2011 02:16 PM, Christian König wrote:
>>>
>>> Why do you want to change that anyway?
>>>
>>> The search for start codes was especially split out of the VLC stuff, because start codes start are byte aligned anyway and it doesn't make much sense to search for them using the slower peekbits&  fillbits functions.
>> Since 4 bytes are loaded at a time and bit aligning is fast enough, it's not that much slower, the reason I want this is to be able to add support for when the array size is>  1, currently and even with that patch its not handled, but if a slice is split between multiple buffers you can at least pick up where you left..
> Good argument, but we should complete the patch instead of leaving the code in a condition where we just bail out with an assertion if the application happens to not behave as we have planned.
>
>>   for example with mplayer I have observed that the start code 0x00,0x00,0x01 and the actual value are in separate buffers for some codecs, and for wmv the following is said:
>>   * Some VC-1 advanced profile streams do not contain slice start codes; again,
>>   * the container format must indicate where picture data begins and ends. In
>>   * this case, pictures are assumed to be progressive and to contain a single
>>   * slice. It is highly recommended that applications detect this condition,
>>   * and add the missing start codes to the bitstream passed to VDPAU. However,
>>   * VDPAU implementations must allow bitstreams with missing start codes, and
>>   * act as if a 0x0000010D (frame) start code had been present.
>>
>> Since mplayer can chop up the start code and the 0x0d, you'd need a parser to handle this.
>> I planned on using vlc to detect the lack of start code.
> Well, sounds like a plan to me.
>
>>> The correct sollution would be to let fillbits return a boolean signaling that the buffer(s) are depleted.
>> Maybe, I want to start filling from the next stream if this is the case.
> Yeah, take a look at the attachment, that's how I implemented it, but what todo if we don't have any more inputs?
>
> Currently we load up to three extra bytes and then just continue to pretend that the stream only contains zeros, well on a second though that isn't so bad after all.
>
> I played around with it a bit today and it seems that the current behavior is already the best we can do about it.
>
>>> Adding an interface definition without an implementation usually only makes sense when we could have more than one different implementation. 
>> As the TODO says, I want to add support for array_size>  1 still, also next patch uses this interface, even if I didn't add the support for array_size>  1 yet. If I don't continuously reset vlc it should work correctly even when a stream has multiple buffers.
> Take a look at the attached patch then, that should do it quite well.
>
>>>> -   //assert(vlc->valid_bits>= num_bits);
>>>> +   assert(vlc->valid_bits>= num_bits || vl_vlc_bits_left(vlc)<   num_bits);
>>> Hui? bits_left is calculation the total number of bits left in the buffer, while valid_bits is the number of currently loaded bits, so that check is erroneous.
>> Yes, but if you are near the end of the stream it is possible that peekbits can extend beyond it when doing vlc decoding, hence the second check.
>> This is a special case for peekbits that would otherwise crash unnecessarily over DCT coefficients, any call that really consumes the bits will still fail.
> Ah ok, now I understand your intentions. Well then check for "vlc->data >= vlc->end" instead, since at the end of the stream even "vl_vlc_bits_left(vlc)< num_bits" will fail ( at least if everything goes right), and in the middle of a stream it will succeed in the case when there is a missing fillbits!
>
> On 20.12.2011 21:08, Lucas Stach wrote:
>> Hi all!
>> Just jumping in with regard to the assert.
>>
>> Am Dienstag, den 20.12.2011, 19:20 +0100 schrieb Maarten Lankhorst:
>> [snip]
>>>>>       return vlc->buffer>>   (64 - num_bits);
>>>>>    }
>>>>> @@ -130,7 +132,7 @@ vl_vlc_peekbits(struct vl_vlc *vlc, unsigned num_bits)
>>>>>    static INLINE void
>>>>>    vl_vlc_eatbits(struct vl_vlc *vlc, unsigned num_bits)
>>>>>    {
>>>>> -   //assert(vlc->valid_bits>   num_bits);
>>>>> +   assert(vlc->valid_bits>= num_bits);
>>>> Just commenting in all checks isn't such a good idea, since that affect performance very badly, a define which enables all assertions at once at the beginning of the file seems the better idea.
>>> Sure.
>> Please don't define your own semantic for this checks. Assert is only
>> used in debug builds and there it shouldn't matter how it affects
>> performance. In release builds assert is typically a no-op and therefore
>> optimized away, but it could also be used to help the compiler optimize
>> for the asserted conditions.
>> Asserts should be never deactivated with code defines.
> 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.
>
> Christian.

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?

    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.

+      }
    }
 }

~Maarten