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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
