[Ffmpeg-devel] Cook stereo (MONO_COOK2) bug and patch

[Ian Braithwaite]

Hi


Thanks Michael for the feedback on the patch.

>> --- ffmpeg-export-2006-12-15/libavcodec/cook.c	2006-12-14
>> 18:58:25.000000000 +0100
>> +++ ffmpeg-idb/libavcodec/cook.c	2007-01-18 15:23:09.000000000 +0100
>> @@ -277,30 +277,21 @@
>>  /**
>>   * Cook indata decoding, every 32 bits are XORed with 0x37c511f2.
>>   * Why? No idea, some checksum/error detection method maybe.
>> + * There's no guarantee that inbuffer is word aligned, nor that
>> + * bytes is divisible by 4.
>>   * Nice way to waste CPU cycles.
>>   *
>> - * @param in        pointer to 32bit array of indata
>> - * @param bits      amount of bits
>> - * @param out       pointer to 32bit array of outdata
>> + * @param inbuffer  pointer to byte array of indata
>> + * @param out       pointer to byte array of outdata
>> + * @param bytes     number of bytes
>>   */
>>
>>  static inline void decode_bytes(uint8_t* inbuffer, uint8_t* out, int
>> bytes){
>>      int i;
>> -    uint32_t* buf = (uint32_t*) inbuffer;
>> -    uint32_t* obuf = (uint32_t*) out;
>> -    /* FIXME: 64 bit platforms would be able to do 64 bits at a time.
>> -     * I'm too lazy though, should be something like
>> -     * for(i=0 ; i<bitamount/64 ; i++)
>> -     *     (int64_t)out[i] =
>> 0x37c511f237c511f2^be2me_64(int64_t)in[i]);
>> -     * Buffer alignment needs to be checked. */
>> +    static uint8_t x[4] = {0x37, 0xc5, 0x11, 0xf2};
>>
>> -
>> -    for(i=0 ; i<bytes/4 ; i++){
>> -#ifdef WORDS_BIGENDIAN
>> -        obuf[i] = 0x37c511f2^buf[i];
>> -#else
>> -        obuf[i] = 0xf211c537^buf[i];
>> -#endif
>> +    for(i=0 ; i<bytes ; i++){
>> +        out[i] = x[i % 4] ^ inbuffer[i];
>>      }
>
> rejected, this is unrelated and a senseless deoptmization, fix the
> alignment
> or change the code so it works with missaligned arrays

OK, I've (re)optimised for 32 bit words.
It fixes two things:
1)  rounding *up* the number of bytes to a multiple of 4, which is
    an unrelated bug, but a bug none the less.
2)  working for missaligned arrays, which is needed for this fix.

I don't have a big-endian machine to test on, so that bit's untried.


>> @@ -1012,8 +1003,18 @@
>>          memcpy(&q->gain_now, &q->gain_previous, sizeof(COOKgain));
>>          memcpy(&q->gain_previous, &q->gain_current, sizeof(COOKgain));
>>
>> +#ifdef COOKDEBUG
>> +	if (get_bits_count(&q->gb) > q->bits_per_subpacket)
>> +	  av_log(NULL,AV_LOG_DEBUG,"bit overrun: %d out of %d\n",
>> +		 get_bits_count(&q->gb), q->bits_per_subpacket);
>> +#endif
>
> tabs are forbidden in svn and
> such debuging code should be in a seperate patch

Sorry about the tabs.
I've removed the debugging stuff, it shouldn't be there.


>>      } else {
>> +        decode_bytes(inbuffer, q->decoded_bytes_buffer,
>> sub_packet_size);
>> +        init_get_bits(&q->gb, q->decoded_bytes_buffer,
>> sub_packet_size*8);
>> +        decode_gain_info(&q->gb, &q->gain_current);
>> +
>
> these 3 lines seem to be near duplicated all over the place, put them in a
> function or otherwise factor them out, code duplication is not accpetable


Well, I was kind of following the general style of that function :-)
OK - I've factored out the three lines to a new function.

The function in question is really in need of a bigger clean-up.
I've included a second patch. It:
1)  Factors out more duplicated code.
2)  Removes a lot of unnecessary buffers.
3)  Replaces a number of memcpy()'s with pointer swapping.


Obviously I'm pretty new to this code. I'm hoping that someone
much more intimate with the decoder can check I havn't messed up.


It would be great to get stereo working!


Cheers,
Ian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch1
Type: application/octet-stream
Size: 5131 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070120/cfee4ca2/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch2
Type: application/octet-stream
Size: 10814 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070120/cfee4ca2/attachment-0001.obj>