_w7_ownsReqCore_AAC_I creates wrong result

[bj2]

Coefs between 4 and 15 mess up in this routine...still.
If you have someone to look at this, now's a good time.
The top level of this is from ippsQuantInv_AAC_32s_I.

Code:

_w7_ownsReqCore_AAC_I:
 :
00617C5D  cmp   ecx,4
00617C60  jge   _w7_ownsReqCore_AAC_I+0B3h (617C6Bh)
00617C62  mov   eax,dword ptr [esp+ecx*4+4]
00617C66  jmp   _w7_ownsReqCore_AAC_I+1DAh (617D92h)
00617C6B  cmp   ecx,10h
00617C6E  jge   _w7_ownsReqCore_AAC_I+0CEh (617C86h)
00617C70  mov   edx,dword ptr [esp+20h]
00617C74  mov   eax,dword ptr [edx+ecx*4+647D80h]    // coef=6: eax=0xCF73154 (ok)
00617C7B  mov   ecx,dword ptr [esp+1Ch]              // this loads ecx as 0 (bzt) re:scale
00617C7F  sar   eax,cl                               // should be shift >> 32 (yup)
00617C81  jmp   _w7_ownsReqCore_AAC_I+1DAh (617D92h) // so instead of 0 this gets CF73154

Same with the base (non-sse2) version. And as I understand the x86, a shift is limited to 31, so the 32 (if it were there) won't even have an effect. It will on some other CPUs:movr0, r0, lsr r1 with r1 = 0x20 will zero r0 on an ARM, for example. Anyway, the case of the coefs being 4 to 15, and very small scale (I suppose) is rare, but happens.

Message Edited by BJ2 on 05-22-200609:04 PM

[Vladimir_Dudnik]

Hello,

many thanks for reporting this issue, I'll forward your information to developers. The only question I still have to you, what version of IPP did you use?

As you probably know, the latest version of IPP is version 5.1, so there is a chance that this issue already was detected and fixed.

Regards,
Vladimir

[bj2]

All win32 versions up to 5.1 have done this. The result is a very loud tick since this normally should be a very quiet frame. I've been usingother code and try these each new update. I had the time to figure out why finally (it took16 hours). Funny no one else has this problem. For me, using a recent iTunes encoder, it happens on about 1 in 20 tracks. It's nothappened on any bought iTunes m4a, though none of which have been bought within the last year, and were probably alreadyseveral years old. It has happened on other encoders I've tested, and those are very old now, so it's not iTunes-specific.

[Vladimir_Dudnik]

Hello, we confirm that this bug is also found in IPP v5.1 function. It will be fixed in the next IPP release. There is possibleworkaround if you will move to new variants of MP3 API. As you probably saw, we have two implementations of MP3 functions, with different API: fixed-point and float point. If your target plaform is IA32 or EM64T it is recommended to you float-point based API because of performance and accuracy. Please take a look on IPP media sample, to see how we use those functions to implement MP3 decoder

Regards,
Vladimir

[bj2]

Decode performance is < 1% of CPU so changing the entire style of the AAC (mp3 doesn't have this problem) decode is not feasible. I likely couldn't even measure it. The fix could probably be donewith a runtime patch since the two bugs (alluded to previously) occur in the same area and samecode path (only a few dozen bytes big,so one jmp to thenew code,and one jmp back, bypassing the old buggy code).

Bug one: the final quarter-scale shift is not calculated correctly (should be 22-x instead of neg(x)+neg(10), in shorthand/from memory, where x can be negative and is in the cases where bug2 becomes noticed, such as x = -10). Bug two:the shift is mod 32when it should not. Bug one is not often noticed since, even thoughit's the wrong scale, it's inan already very quiet frame so it's not something you could hear (a wrong level of quiet), and bug two, well, bug two is the fingernail down the blackboard. I'd expect the accuracy to be as good as it can get after this is corrected. The umc stuff is a bit too much for me right now. It may be useful down the road. You may mark this case, closed! Thank you very much!

[Vladimir_Dudnik]

thank you again for such a detailed anaysis, you definetely did our job. Unfortunately we not provide partial binary patches, only whole new release of all SDK is possible. So, the version will fix this problem

Thanks,
Vladimir