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

Michael Niedermayer michaelni
Sun Jan 21 17:50:20 CET 2007


Hi

On Sat, Jan 20, 2007 at 11:41:30PM +0100, Ian Braithwaite wrote:
> 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

> --- orig/cook.c	2007-01-20 17:58:41.000000000 +0100
> +++ fixed/cook.c	2007-01-20 22:55:17.000000000 +0100
> @@ -277,30 +277,57 @@
>  /**
>   * 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;
> +    int i, off;
> +    uint32_t* buf;
>      uint32_t* obuf = (uint32_t*) out;
> +
> +    /* Optimised for 32 bit word read/writes.
> +     * Functionally equivalent to:
> +     *     for(i = 0; i < bytes; i++) {
> +     *         static uint8_t x[4] = {0x37, 0xc5, 0x11, 0xf2};
> +     *         out[i] = x[i % 4] ^ inbuffer[i];
> +     *     }
> +     */
>      /* 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. */
>  
> -
> -    for(i=0 ; i<bytes/4 ; i++){
> +    off = (uint32_t)inbuffer % 4;
> +    if (off) {
> +        uint32_t tmp;
> +        buf = (uint32_t*) (inbuffer - off + 4);
> +        tmp = buf[-1];
> +        for (i = 0; i < (bytes + 3)/4; i++) {
>  #ifdef WORDS_BIGENDIAN
> -        obuf[i] = 0x37c511f2^buf[i];
> +            obuf[i] = 0x37c511f2 ^
> +              ((buf[i] >> (32 - 8*off)) | (tmp << (8*off)));
>  #else
> -        obuf[i] = 0xf211c537^buf[i];
> +            obuf[i] = 0xf211c537 ^
> +              ((buf[i] << (32 - 8*off)) | (tmp >> (8*off)));
>  #endif
> +            tmp = buf[i];
> +        }
> +    } else {
> +        buf = (uint32_t*) inbuffer;
> +        for(i = 0; i < (bytes + 3)/4; i++) {
> +#ifdef WORDS_BIGENDIAN
> +            obuf[i] = 0x37c511f2 ^ buf[i];
> +#else
> +            obuf[i] = 0xf211c537 ^ buf[i];
> +#endif
> +        }
>      }
>  }

i was more thinking of something like:

int shift= (uint32_t)inbuffer & 3;
uint32_t *buf =  inbuffer - shift;
uint32_t *obuf= outbuffer;
uint32_t c= be2me_32((0x37c511f2>>(shift*8)) | (0x37c511f2<<(32-(shift*8))));
bytes += 3+shift;
for(i=0; i<bytes; i++)
    obuf[i] = c^buf[i];

return shift;

and check that the outbuffer is allocated large enough for this ...

the remainder of patch 1 looks ok


[...]
>  static inline void
> -decode_bytes_and_gain(COOKContext *q, uint8_t *inbuffer, COOKgain *gain_ptr)
> +decode_bytes_and_gain(COOKContext *q, uint8_t *inbuffer,
> +                      COOKgain *gain_ptr[])
>  {
> +    COOKgain *tmp;
> +
>      decode_bytes(inbuffer, q->decoded_bytes_buffer, q->bits_per_subpacket/8);
>      init_get_bits(&q->gb, q->decoded_bytes_buffer, q->bits_per_subpacket);
> -    decode_gain_info(&q->gb, gain_ptr);
> +    decode_gain_info(&q->gb, gain_ptr[0]);
> +
> +    /* Swap current and previous gains */
> +    tmp = gain_ptr[0];
> +    gain_ptr[0] = gain_ptr[1];
> +    gain_ptr[1] = tmp;

FFSWAP()


[...]
> +    /* Clip and convert floats to 16 bits.
> +     */
> +    for (j = 0; j < q->samples_per_frame; j++) {
> +        value = lrintf(q->mono_mdct_output[j]);
> +        if (value < -32768) value = -32768;
> +        else if (value > 32767) value = 32767;

clip()

[...]

also note that Benjamin Larsson is maintainer of cook* so his oppinion is
what matters (especially for patch 2 which looks nice but i didnt check
that it is correct, as benjamin can probably do this quicker then me ...)

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No evil is honorable: but death is honorable; therefore death is not evil.
-- Citium Zeno
-------------- 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/20070121/c1b56c79/attachment.pgp>



More information about the ffmpeg-devel mailing list