Hey Christian,
On 12/20/2011 02:16 PM, Christian König wrote:
> On 20.12.2011 12:43, Maarten Lankhorst wrote:
>> And add more sanity checks to stream. This shouldn't break things beyond
>> those that aren't broken already.
>>
>> Signed-off-by: Maarten Lankhorst<[email protected]>
>>
>> ---
>> And yes Andy, I mean that I haven't found a good video yet to fix the
>> playback parts that are still broken..
> 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.. 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.
> Se below for some further comments.
>> src/gallium/auxiliary/vl/vl_mpeg12_bitstream.c | 56
>> +++++++++++-------------
>> src/gallium/auxiliary/vl/vl_vlc.h | 33 +++++++++-----
>> 2 files changed, 47 insertions(+), 42 deletions(-)
>>
>> diff --git a/src/gallium/auxiliary/vl/vl_mpeg12_bitstream.c
>> b/src/gallium/auxiliary/vl/vl_mpeg12_bitstream.c
>> index 936cf2c..62d08d7 100644
>> --- a/src/gallium/auxiliary/vl/vl_mpeg12_bitstream.c
>> +++ b/src/gallium/auxiliary/vl/vl_mpeg12_bitstream.c
>> @@ -787,7 +787,7 @@ entry:
>> }
>>
>> static INLINE bool
>> -decode_slice(struct vl_mpg12_bs *bs)
>> +decode_slice(struct vl_mpg12_bs *bs, unsigned code)
>> {
>> struct pipe_mpeg12_macroblock mb;
>> short dct_blocks[64*6];
>> @@ -796,24 +796,29 @@ decode_slice(struct vl_mpg12_bs *bs)
>>
>> memset(&mb, 0, sizeof(mb));
>> mb.base.codec = PIPE_VIDEO_CODEC_MPEG12;
>> - mb.y = vl_vlc_get_uimsbf(&bs->vlc, 8) - 1;
>> + mb.y = code - 1;
>> mb.blocks = dct_blocks;
>>
>> reset_predictor(bs);
>> + vl_vlc_fillbits(&bs->vlc);
>> dct_scale =
>> quant_scale[bs->desc.q_scale_type][vl_vlc_get_uimsbf(&bs->vlc, 5)];
>>
>> - if (vl_vlc_get_uimsbf(&bs->vlc, 1))
>> + if (vl_vlc_get_uimsbf(&bs->vlc, 1)) {
>> + vl_vlc_fillbits(&bs->vlc);
>> while (vl_vlc_get_uimsbf(&bs->vlc, 9)& 1)
>> vl_vlc_fillbits(&bs->vlc);
>> + }
>>
>> vl_vlc_fillbits(&bs->vlc);
>> + assert(vl_vlc_bits_left(&bs->vlc)> 23&& vl_vlc_peekbits(&bs->vlc, 23));
>> do {
>> int inc = 0;
>>
>> - while (vl_vlc_peekbits(&bs->vlc, 11) == 15) {
>> - vl_vlc_eatbits(&bs->vlc, 11);
>> - vl_vlc_fillbits(&bs->vlc);
>> - }
>> + if (bs->decoder->profile == PIPE_VIDEO_PROFILE_MPEG1)
>> + while (vl_vlc_peekbits(&bs->vlc, 11) == 15) {
>> + vl_vlc_eatbits(&bs->vlc, 11);
>> + vl_vlc_fillbits(&bs->vlc);
>> + }
>>
>> while (vl_vlc_peekbits(&bs->vlc, 11) == 8) {
>> vl_vlc_eatbits(&bs->vlc, 11);
>> @@ -959,32 +964,23 @@ void
>> vl_mpg12_bs_decode(struct vl_mpg12_bs *bs, unsigned num_bytes, const
>> uint8_t *buffer)
>> {
>> assert(bs);
>> - assert(buffer&& num_bytes);
>> -
>> - while(num_bytes> 2) {
>> - if (buffer[0] == 0x00&& buffer[1] == 0x00&& buffer[2] == 0x01&&
>> - buffer[3]>= 0x01&& buffer[3]< 0xAF) {
>> - unsigned consumed;
>>
>> - buffer += 3;
>> - num_bytes -= 3;
>> + vl_vlc_init(&bs->vlc, 1, num_bytes, (void const * const
>> *)&buffer,&num_bytes);
>>
>> - vl_vlc_init(&bs->vlc, buffer, num_bytes);
>> -
>> - if (!decode_slice(bs))
>> + while (vl_vlc_bits_left(&bs->vlc)>= 32) {
>> + vl_vlc_fillbits(&bs->vlc);
>> + if (vl_vlc_peekbits(&bs->vlc, 24) == 0x000001) {
>> + unsigned code;
>> + vl_vlc_eatbits(&bs->vlc, 24);
>> + code = vl_vlc_get_uimsbf(&bs->vlc, 8);
>> + if (!code || code> 0xaf)
>> + continue;
>> + if (!decode_slice(bs, code))
>> return;
>> -
>> - consumed = num_bytes - vl_vlc_bits_left(&bs->vlc) / 8;
>> -
>> - /* crap, this is a bug we have consumed more bytes than left in
>> the buffer */
>> - assert(consumed<= num_bytes);
>> -
>> - num_bytes -= consumed;
>> - buffer += consumed;
>> -
>> + vl_vlc_bitalign(&bs->vlc);
>> } else {
>> - ++buffer;
>> - --num_bytes;
>> + vl_vlc_eatbits(&bs->vlc, 8);
>> + continue;
>> }
>> }
>> }
>> diff --git a/src/gallium/auxiliary/vl/vl_vlc.h
>> b/src/gallium/auxiliary/vl/vl_vlc.h
>> index dc4faed..a4ded70 100644
>> --- a/src/gallium/auxiliary/vl/vl_vlc.h
>> +++ b/src/gallium/auxiliary/vl/vl_vlc.h
>> @@ -38,7 +38,7 @@
>> struct vl_vlc
>> {
>> uint64_t buffer;
>> - unsigned valid_bits;
>> + uint32_t valid_bits;
>> uint32_t *data;
>> uint32_t *end;
>> };
>> @@ -74,11 +74,9 @@ vl_vlc_init_table(struct vl_vlc_entry *dst, unsigned
>> dst_size, const struct vl_v
>> static INLINE void
>> vl_vlc_fillbits(struct vl_vlc *vlc)
>> {
>> - if (vlc->valid_bits< 32) {
>> + if (vlc->valid_bits< 32&& vlc->data< vlc->end) {
>> uint32_t value = *vlc->data;
> 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.
>>
>> - //assert(vlc->data<= vlc->end);
>> -
>> #ifndef PIPE_ARCH_BIG_ENDIAN
>> value = util_bswap32(value);
>> #endif
>> @@ -90,10 +88,14 @@ vl_vlc_fillbits(struct vl_vlc *vlc)
>> }
>>
>> static INLINE void
>> -vl_vlc_init(struct vl_vlc *vlc, const uint8_t *data, unsigned len)
>> +vl_vlc_init(struct vl_vlc *vlc,
>> + const unsigned array_size, unsigned total_len,
>> + const const void *const *datas, const unsigned *lens)
>> {
>> + const uint8_t *data = datas[0];
>> assert(vlc);
>> - assert(data&& len);
>> + assert(array_size == 1); // TODO
>> + assert(datas[0]&& lens[0]);
> 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.
>>
>> vlc->buffer = 0;
>> vlc->valid_bits = 0;
>> @@ -102,11 +104,11 @@ vl_vlc_init(struct vl_vlc *vlc, const uint8_t *data,
>> unsigned len)
>> while (pointer_to_uintptr(data)& 3) {
>> vlc->buffer |= (uint64_t)*data<< (56 - vlc->valid_bits);
>> ++data;
>> - --len;
>> + --total_len;
>> vlc->valid_bits += 8;
>> }
>> vlc->data = (uint32_t*)data;
>> - vlc->end = (uint32_t*)(data + len);
>> + vlc->end = (uint32_t*)(data + total_len);
>>
>> vl_vlc_fillbits(vlc);
>> vl_vlc_fillbits(vlc);
>> @@ -122,7 +124,7 @@ vl_vlc_bits_left(struct vl_vlc *vlc)
>> static INLINE unsigned
>> struct vl_vlc *vlc, unsigned num_bits)
>> {
>> - //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.
>
>>
>> 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.
>>
>> vlc->buffer<<= num_bits;
>> vlc->valid_bits -= num_bits;
>> @@ -141,7 +143,7 @@ vl_vlc_get_uimsbf(struct vl_vlc *vlc, unsigned num_bits)
>> {
>> unsigned value;
>>
>> - //assert(vlc->valid_bits>= num_bits);
>> + assert(vlc->valid_bits>= num_bits);
>>
>> value = vlc->buffer>> (64 - num_bits);
>> vl_vlc_eatbits(vlc, num_bits);
>> @@ -154,7 +156,7 @@ vl_vlc_get_simsbf(struct vl_vlc *vlc, unsigned num_bits)
>> {
>> signed value;
>>
>> - //assert(vlc->valid_bits>= num_bits);
>> + assert(vlc->valid_bits>= num_bits);
>>
>> value = ((int64_t)vlc->buffer)>> (64 - num_bits);
>> vl_vlc_eatbits(vlc, num_bits);
>> @@ -167,7 +169,14 @@ vl_vlc_get_vlclbf(struct vl_vlc *vlc, const struct
>> vl_vlc_entry *tbl, unsigned n
>> {
>> tbl += vl_vlc_peekbits(vlc, num_bits);
>> vl_vlc_eatbits(vlc, tbl->length);
>> + assert(tbl->length);
>> return tbl->value;
>> }
>>
>> +static INLINE void
>> +vl_vlc_bitalign(struct vl_vlc *vlc)
>> +{
>> + vl_vlc_eatbits(vlc, vlc->valid_bits& 7);
>> +}
>> +
>> #endif /* vl_vlc_h */
>>
>>
>> _______________________________________________
>> mesa-dev mailing list
>> [email protected]
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
> Christian.
~Maarten
_______________________________________________
mesa-dev mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
