Add openmax dl routines for review. MUST NOT BE LANDED

Add openmax dl routines for review.


BUG=

[Raymond Toy (Google), 2013-02-27 18:08:02]

To help out a bit with the review, here is the history of the files.  Most of
the files come from ARM's implemenation of the OpenMAX DL spec.  The
implementation is at https://codereview.chromium.org/12317152/.  Only the FFT
routines are included here.

These were minimally tweaked to get them to compile using gcc and some minor
changes to conform to the chromium code style.

The new files are *FC32* and everything in the test directory.  The FC32 code is
very similar to the SC32 code, from which it was derived.  The test code is new
and used to verify that the implementation is working; it will not be included
in any chromium build.

[palmer, 2013-03-05 23:45:53]

I'm going to send comments as I have them, rather than all in one giant batch.

https://codereview.chromium.org/12317152/diff/1/third_party/openmax_dl/dl/api...
File third_party/openmax_dl/dl/api/armCOMM.h (right):

https://codereview.chromium.org/12317152/diff/1/third_party/openmax_dl/dl/api...
third_party/openmax_dl/dl/api/armCOMM.h:2: *  Copyright (c) 2013 The WebRTC
project authors. All Rights Reserved.
Is it really legit to have this both be copyrighted by the WebRTC authors *and*
ARM Limited? What does pcounsel say?

https://codereview.chromium.org/12317152/diff/1/third_party/openmax_dl/dl/api...
third_party/openmax_dl/dl/api/armCOMM.h:52: #define armError(str)
{printf((str)); printf("\n"); exit(-1);}
This makes me weep. Perhaps no call site passes user data as |str|, but why risk
it? (Format string vulnerability.)

Do this instead:

printf("%s\n", (str));

https://codereview.chromium.org/12317152/diff/1/third_party/openmax_dl/dl/api...
third_party/openmax_dl/dl/api/armCOMM.h:72: #define armAlignToBytes(Ptr,N)     
(Ptr + ( ((N-(int)Ptr)&(N-1)) / sizeof(*Ptr) ))
sizeof(Ptr) is not necessarily == sizeof(int). Should be a cast to intptr_t,
right?

https://codereview.chromium.org/12317152/diff/1/third_party/openmax_dl/dl/api...
third_party/openmax_dl/dl/api/armCOMM.h:83: #define armIsByteAligned(Ptr,N)    
((((int)(Ptr)) % N)==0)
intptr_t again, I suspect

https://codereview.chromium.org/12317152/diff/1/third_party/openmax_dl/dl/api...
third_party/openmax_dl/dl/api/armCOMM.h:84: #define armNotByteAligned(Ptr,N)   
((((int)(Ptr)) % N)!=0)
And here

https://codereview.chromium.org/12317152/diff/1/third_party/openmax_dl/dl/api...
third_party/openmax_dl/dl/api/armCOMM.h:153: * Checks the sign of a variable:
I don't see the point of this function.

https://codereview.chromium.org/12317152/diff/1/third_party/openmax_dl/dl/api...
third_party/openmax_dl/dl/api/armCOMM.h:279: * Description: Finds the size of a
positive value and returns the same
So this takes the base 2 log of |value|?

https://codereview.chromium.org/12317152/diff/1/third_party/openmax_dl/dl/api...
third_party/openmax_dl/dl/api/armCOMM.h:548: #define DEBUG_PRINTF_0(a)          
                                    fprintf(DEBUG_STREAM, a)
Same concern here as above: Potential format string vulnerability. Depends on
behavior of call sites (which is a bad thing to depend on).

This kind of thing is better done properly with stdarg.h.

[Raymond Toy (Google), 2013-03-05 23:53:03]

+ajm

[Raymond Toy (Google), 2013-03-05 23:59:14]

armCOMM.h is what was delivered from ARM's implementation.

Perhaps we can just rip out everything that we don't use?  A quick inspection
indicates that nothing is used in this file by any of the code.

https://codereview.chromium.org/12317152/diff/1/third_party/openmax_dl/dl/api...
File third_party/openmax_dl/dl/api/armCOMM.h (right):

https://codereview.chromium.org/12317152/diff/1/third_party/openmax_dl/dl/api...
third_party/openmax_dl/dl/api/armCOMM.h:2: *  Copyright (c) 2013 The WebRTC
project authors. All Rights Reserved.
On 2013/03/05 23:45:53, Chris P. wrote:
> Is it really legit to have this both be copyrighted by the WebRTC authors
*and*
> ARM Limited? What does pcounsel say?

dannyb signed off on this format.

[Raymond Toy (Google), 2013-03-06 00:01:48]

Do not review dl/src/armCOMM.c.  It's not actually used and should not have been
included.  Sorry about that.

[Raymond Toy (Google), 2013-03-06 00:21:15]

On 2013/03/05 23:59:14, rtoy wrote:
> armCOMM.h is what was delivered from ARM's implementation.
> 
> Perhaps we can just rip out everything that we don't use?  A quick inspection
> indicates that nothing is used in this file by any of the code.
> 

I verified that armCOMM.h is not needed by any of the current code.  All files
that include armCOMM.h still work without it.  This file can be deleted.  

Sorry about that!
>
https://codereview.chromium.org/12317152/diff/1/third_party/openmax_dl/dl/api...
> File third_party/openmax_dl/dl/api/armCOMM.h (right):
> 
>
https://codereview.chromium.org/12317152/diff/1/third_party/openmax_dl/dl/api...
> third_party/openmax_dl/dl/api/armCOMM.h:2: *  Copyright (c) 2013 The WebRTC
> project authors. All Rights Reserved.
> On 2013/03/05 23:45:53, Chris P. wrote:
> > Is it really legit to have this both be copyrighted by the WebRTC authors
> *and*
> > ARM Limited? What does pcounsel say?
> 
> dannyb signed off on this format.

[Raymond Toy (Google), 2013-03-20 21:44:57]

Please review the second patch set.  I'm not sure what happened with git cl
upload, but it looks like every file is changed between the two patch sets. 
This is not the case.  Here is what has changed:

armCOMM.h and armCOMM.c removed.

dl/dl.gyp updated.

api/armCOMM_s.h updated by removing some unused items.

Removed include of armCOMM.h which doesn't exist:
omxSP_FFTGetBufSize_C_SC16.c
omxSP_FFTGetBufSize_R_F32.c
omxSP_FFTGetBufSize_R_S16S32.c
omxSP_FFTGetBufSize_R_S32.c
omxSP_FFTInit_C_FC32.c
omxSP_FFTInit_C_SC16.c
omxSP_FFTInit_C_SC32.c
omxSP_FFTInit_R_F32.c
omxSP_FFTInit_R_S16S32.c
omxSP_FFTInit_R_S32.c

Other fixes:
armSP_FFT_CToC_FC32_Radix4_fs_unsafe_s.S
armSP_FFT_CToC_FC32_Radix4_ls_unsafe_s.S
armSP_FFT_CToC_FC32_Radix4_unsafe_s.S
armSP_FFT_CToC_FC32_Radix8_fs_unsafe_s.S

[Raymond Toy (Google), 2013-03-21 21:20:49]

Patch set 2 contains some more fixes.  And not every single file has changed.  I
don't know why git cl upload does that.  The changed files are:

omxSP_FFTInit_R_F32.c
omxSP_FFTInit_C_FC32.c
omxSP_FFTGetBufSize_R_F32.c
omxSP_FFTGetBufSize_C_FC32.c

The change just disallows FFTs of order 0.  This is ok, since we don't have to
follow OpenMAX specs for these routines that aren't part of OpenMAX.

[aedla, 2013-03-22 10:50:26]

LGTM on FC32 and F32 parts of the library.

[Raymond Toy (Google), 2013-03-22 16:10:41]

On 2013/03/22 10:50:26, aedla wrote:
> LGTM on FC32 and F32 parts of the library.

Thanks!

[kma, 2013-06-13 17:13:46]

On 2013/03/22 10:50:26, aedla wrote:
> LGTM on FC32 and F32 parts of the library.

Hi Juri,
This will be the CL for reviewing SC16 and S16 code. File names containing 16
needs to be reviewed (12 *.S files).
From the last FC32 code security review, it's likely that read OOB issues also
exit in armSP_FFT_CToC_SC16_Radix4_fs_unsafe_s.S,
armSP_FFT_CToC_SC16_Radix4_ls_unsafe_s.S, armSP_FFT_CToC_SC16_Radix4_unsafe_s.S,
and armSP_FFT_CToC_SC16_Radix8_fs_unsafe_s.S.
Thanks.

[aedla, 2013-06-21 12:56:34]

The review is slowly coming along. SC16 Radix 2 is ok. Radix 4 fs has OOB reads
as predicted.

https://codereview.chromium.org/12317152/diff/61001/third_party/openmax_dl/dl...
File third_party/openmax_dl/dl/sp/src/armSP_FFT_CToC_SC16_Radix4_fs_unsafe_s.S
(right):

https://codereview.chromium.org/12317152/diff/61001/third_party/openmax_dl/dl...
third_party/openmax_dl/dl/sp/src/armSP_FFT_CToC_SC16_Radix4_fs_unsafe_s.S:181:
VLD2    {dXr3,dXi3},[pSrc :128],setStep
This seems to read 16 bytes OOB at the last iteration. Read bytes are not used.

https://codereview.chromium.org/12317152/diff/61001/third_party/openmax_dl/dl...
third_party/openmax_dl/dl/sp/src/armSP_FFT_CToC_SC16_Radix4_fs_unsafe_s.S:238:
VLD2    {dXr3,dXi3},[pSrc :128],setStep
Same here.

[kma, 2013-06-24 16:05:12]

On 2013/06/21 12:56:34, aedla wrote:
> The review is slowly coming along. SC16 Radix 2 is ok. Radix 4 fs has OOB
reads
> as predicted.
> 
>
https://codereview.chromium.org/12317152/diff/61001/third_party/openmax_dl/dl...
> File third_party/openmax_dl/dl/sp/src/armSP_FFT_CToC_SC16_Radix4_fs_unsafe_s.S
> (right):
> 
>
https://codereview.chromium.org/12317152/diff/61001/third_party/openmax_dl/dl...
> third_party/openmax_dl/dl/sp/src/armSP_FFT_CToC_SC16_Radix4_fs_unsafe_s.S:181:
> VLD2    {dXr3,dXi3},[pSrc :128],setStep
> This seems to read 16 bytes OOB at the last iteration. Read bytes are not
used.
> 
>
https://codereview.chromium.org/12317152/diff/61001/third_party/openmax_dl/dl...
> third_party/openmax_dl/dl/sp/src/armSP_FFT_CToC_SC16_Radix4_fs_unsafe_s.S:238:
> VLD2    {dXr3,dXi3},[pSrc :128],setStep
> Same here.

Great. I uploaded https://webrtc-codereview.appspot.com/1715004/ for this bug,
and will add more upon your review.

[aedla, 2013-06-26 12:52:17]

Sorry for the delay.

https://codereview.chromium.org/12317152/diff/61001/third_party/openmax_dl/dl...
File third_party/openmax_dl/dl/sp/src/armSP_FFT_CToC_SC16_Radix8_fs_unsafe_s.S
(right):

https://codereview.chromium.org/12317152/diff/61001/third_party/openmax_dl/dl...
third_party/openmax_dl/dl/sp/src/armSP_FFT_CToC_SC16_Radix8_fs_unsafe_s.S:351:
VLD2    {dXr7,dXi7},[pSrc :128],setStep            @//  data[7]
Last iteration 16-byte OOB read here,

https://codereview.chromium.org/12317152/diff/61001/third_party/openmax_dl/dl...
third_party/openmax_dl/dl/sp/src/armSP_FFT_CToC_SC16_Radix8_fs_unsafe_s.S:391:
VLD2    {dXr7,dXi7},[pSrc :128],setStep            @//  data[7]
here,

https://codereview.chromium.org/12317152/diff/61001/third_party/openmax_dl/dl...
third_party/openmax_dl/dl/sp/src/armSP_FFT_CToC_SC16_Radix8_fs_unsafe_s.S:517:
VLD2    {dXr7,dXi7},[pSrc :128],setStep            @//  data[7]
here,

https://codereview.chromium.org/12317152/diff/61001/third_party/openmax_dl/dl...
third_party/openmax_dl/dl/sp/src/armSP_FFT_CToC_SC16_Radix8_fs_unsafe_s.S:557:
VLD2    {dXr7,dXi7},[pSrc :128],setStep            @//  data[7]
and here.

[aedla, 2013-06-26 20:24:46]

https://codereview.chromium.org/12317152/diff/61001/third_party/openmax_dl/dl...
File third_party/openmax_dl/dl/sp/src/armSP_FFT_CToC_SC16_Radix4_unsafe_s.S
(right):

https://codereview.chromium.org/12317152/diff/61001/third_party/openmax_dl/dl...
third_party/openmax_dl/dl/sp/src/armSP_FFT_CToC_SC16_Radix4_unsafe_s.S:247: VLD2
   {dXr3,dXi3},[pSrc :128],setStep            @//  data[3] & update pSrc for the
next set
16 byte OOB read when both loops are at their last iteration.

https://codereview.chromium.org/12317152/diff/61001/third_party/openmax_dl/dl...
third_party/openmax_dl/dl/sp/src/armSP_FFT_CToC_SC16_Radix4_unsafe_s.S:361: VLD1
    dW3,[pTwiddle :64],twStep                      @//[wi | wr]
8 byte OOB read at the last iteration, coming from the ping pong buffer and is
not used, so no impact really. IIRC we didn't bother to fix it in FC32.

[aedla, 2013-06-27 12:14:00]

https://codereview.chromium.org/12317152/diff/61001/third_party/openmax_dl/dl...
File third_party/openmax_dl/dl/sp/src/armSP_FFT_CToC_SC16_Radix4_ls_unsafe_s.S
(right):

https://codereview.chromium.org/12317152/diff/61001/third_party/openmax_dl/dl...
third_party/openmax_dl/dl/sp/src/armSP_FFT_CToC_SC16_Radix4_ls_unsafe_s.S:265:
VLD4 {dW2r,dW2i,dTmp0,dTmp1},[pw2 :256]!
dTmp0 and dTmp1 are OOB for order = 4. The are read from ping pong buffer and
are not used.

https://codereview.chromium.org/12317152/diff/61001/third_party/openmax_dl/dl...
third_party/openmax_dl/dl/sp/src/armSP_FFT_CToC_SC16_Radix4_ls_unsafe_s.S:274:
VLD3 {dW3rS32,dTmp1S32,dTmp2S32},[pw3 :64]!
Last iteration 24 byte OOB read, coming from ping pong buffer and not used. Same
for the following VLD3 [pw3].

https://codereview.chromium.org/12317152/diff/61001/third_party/openmax_dl/dl...
third_party/openmax_dl/dl/sp/src/armSP_FFT_CToC_SC16_Radix4_ls_unsafe_s.S:288:
VLD4   
{dButterfly1Real02,dButterfly1Imag02,dButterfly1Real13,dButterfly1Imag13},[pSrc
:256]! @// AC.r AC.i BD.r BD.i
Last iteration 32 byte OOB read. Same for the other three VLD4 [pSrc].

[aedla, 2013-06-28 11:47:05]

https://codereview.chromium.org/12317152/diff/61001/third_party/openmax_dl/dl...
File third_party/openmax_dl/dl/sp/src/omxSP_FFTInv_CToC_SC16_Sfs_s.S (right):

https://codereview.chromium.org/12317152/diff/61001/third_party/openmax_dl/dl...
third_party/openmax_dl/dl/sp/src/omxSP_FFTInv_CToC_SC16_Sfs_s.S:132: ADD    
scale,scale,order                   @// FFTInverse has a final scaling factor by
N
Documentation in api/omxSP.h says:
 scaleFactor - scale factor of the output. Valid value is 0 only.

So scale will always equal order after this ADD. Most of the code below is for
handling cases where scale is from 0 to order - 1 and will be unreachable. Maybe
delete it? I think only order < 4 and scaleEqualsOrder cases are reachable.

[aedla, 2013-06-28 11:57:17]

That concludes SC16. I won't review omxSP_FFTFwd_RToCCS_S16S32_Sfs_s.S and
omxSP_FFTInv_CCSToR_S32S16_Sfs_s.S because they seem to depend on SC32. Any
information on whether chrome is using SC32 yet or is planning to use any time
soon?

[Raymond Toy (Google), 2013-06-28 20:43:56]

On 2013/06/28 11:57:17, aedla wrote:
> That concludes SC16. I won't review omxSP_FFTFwd_RToCCS_S16S32_Sfs_s.S and
> omxSP_FFTInv_CCSToR_S32S16_Sfs_s.S because they seem to depend on SC32. Any
> information on whether chrome is using SC32 yet or is planning to use any time
> soon?

I have no plans on using SC32 for webaudio.  In fact, webaudio will only use the
floating-point code.

[Andrew MacDonald, 2013-06-28 21:48:21]

On 2013/06/28 20:43:56, rtoy wrote:
> On 2013/06/28 11:57:17, aedla wrote:
> > That concludes SC16. I won't review omxSP_FFTFwd_RToCCS_S16S32_Sfs_s.S and
> > omxSP_FFTInv_CCSToR_S32S16_Sfs_s.S because they seem to depend on SC32. Any
> > information on whether chrome is using SC32 yet or is planning to use any
time
> > soon?
> 
> I have no plans on using SC32 for webaudio.  In fact, webaudio will only use
the
> floating-point code.

webrtc should only be using s*16 and possibly floating point in the future.
Right Kevin?

[kma1, 2013-07-02 18:10:20]

On 2013/06/28 21:48:21, andrew wrote:
> On 2013/06/28 20:43:56, rtoy wrote:
> > On 2013/06/28 11:57:17, aedla wrote:
> > > That concludes SC16. I won't review omxSP_FFTFwd_RToCCS_S16S32_Sfs_s.S and
> > > omxSP_FFTInv_CCSToR_S32S16_Sfs_s.S because they seem to depend on SC32.
Any
> > > information on whether chrome is using SC32 yet or is planning to use any
> time
> > > soon?
> > 
> > I have no plans on using SC32 for webaudio.  In fact, webaudio will only use
> the
> > floating-point code.
> 
> webrtc should only be using s*16 and possibly floating point in the future.
> Right Kevin?

Sorry didn't see these. Will upload more changes to
https://webrtc-codereview.appspot.com/1715004 immediately.
Ray, can you add me to the CC list?
For Andrew, yes we will only use SC16 and S16, possibly floating in the future.