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

[Michael Niedermayer]

Hi

On Thu, Jan 18, 2007 at 04:06:11PM +0100, Ian Braithwaite wrote:
> Hi,
> 
> 
> There seems to be a problem with the real audio cook codec, in
> 2 channel MONO_COOK2 mode.
> This is generally the format of BBC streams and downloads.
> 
> The problem is that only the left channel is decoded, and sent out
> to both left and right.
> I've uploaded a short file that illustrates the problem to
> upload.mplayerhq.hu, /MPlayer/incoming/idb.
> 
> I've coded up a fix, patch attached.
> 
> 
> Cheers,
> Ian

> --- 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


[...]
> @@ -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


[...]

>      } 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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070118/3290a7a4/attachment.pgp>