[Mesa-dev] [PATCH 1/3] vl: Only initialize vlc once
Maarten Lankhorst
m.b.lankhorst at gmail.com
Tue Dec 20 10:20:21 PST 2011
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<m.b.lankhorst at gmail.com>
>>
>> ---
>> 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
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
> Christian.
~Maarten
More information about the mesa-dev
mailing list