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