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>
RetroSearch is an open source project built by @garambo | Open a GitHub Issue
Search and Browse the WWW like it's 1997 | Search results from DuckDuckGo
HTML:
3.2
| Encoding:
UTF-8
| Version:
0.7.4