1158741 - Implement a version of omxSP_FFTInv_CCSToR_F32_Sfs in openmax DL's FFT that is not scaled

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P2)

Description

[Paul Adenot (:padenot)]

This is just a matter of removing the scaling at the end of the function.

Comment 1

[Maire Reavy [:mreavy]]

Just guessing at priority...

Comment 2

[Dan Minor [:dminor]]

I'll pick this one up.

Comment 3

[Dan Minor [:dminor]]

Attachment: MozReview Request: Bug 1158741 - Implement a version of omxSP_FFTInv_CCSToR_F32_Sfs in openmax DL's FFT that is not scaled r?padenot

The new routine actually scales by two for consistency with the other FFT
routines in use.

Review commit: https://reviewboard.mozilla.org/r/32269/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32269/

Comment 4

[Paul Adenot (:padenot)]

Comment on attachment 8711631 [details]
MozReview Request: Bug 1158741 - Implement a version of omxSP_FFTInv_CCSToR_F32_Sfs in openmax DL's FFT that is not scaled r?padenot

https://reviewboard.mozilla.org/r/32269/#review28905

::: media/openmax_dl/dl/sp/api/omxSP.h:2610
(Diff revision 1)
> +);

Do we want to carry a patch on top of their code base, or just put that in ?

In any case, we should add a README.mozilla at media/openmax_dl that shows what modifications we made.

::: media/openmax_dl/dl/sp/src/omxSP_FFTInv_CCSToR_F32_Sfs_unscaled_s.S:15
(Diff revision 1)
> +@//  in use.

Do we need to put in a copyright notice here ? Also the last sentence does not seem quite correct.

Also I got confused by the terme "scaling". Usually, it's a division by the length of the FFT, so I was expecting a division by two, but you multiplied ?

::: media/openmax_dl/dl/sp/src/omxSP_FFTInv_CCSToR_F32_Sfs_unscaled_s.S:267
(Diff revision 1)
> +        VMUL    dX0, dX0, dScale[0]

It's not super intuitive why that works ! How did you find that you needed to multiply by two ? I would not have expected that.

Comment 5

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/32269/#review28905

> It's not super intuitive why that works ! How did you find that you needed to multiply by two ? I would not have expected that.

I was very confused by the results I was seeing until I noticed the comment here [1] where we're also multiplying by two for the av_rdft_calc routine. I was going to ask you about that :)

[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/webaudio/FFTBlock.h#113

Comment 6

[Dan Minor [:dminor]]

I'm a bit worried that having a "unscaled" routine that multiplies by two is going to trip people up in the future. We could take the approach used by the libav routine where we multiply by two in advance, but that defeats the optimization we're trying for here.

I could rename from "unscaled" to "doubled" or something like that, or maybe we should just WONTFIX this. Please let me know what you think.

Comment 7

[Paul Adenot (:padenot)]

I think just a comment is enough.

Comment 8

[Dan Minor [:dminor]]

Comment on attachment 8711631 [details]
MozReview Request: Bug 1158741 - Implement a version of omxSP_FFTInv_CCSToR_F32_Sfs in openmax DL's FFT that is not scaled r?padenot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32269/diff/1-2/

Comment 9

[Dan Minor [:dminor]]

Sorry about the long delay in getting this up for another review.

Comment 10

[Paul Adenot (:padenot)]

Comment on attachment 8711631 [details]
MozReview Request: Bug 1158741 - Implement a version of omxSP_FFTInv_CCSToR_F32_Sfs in openmax DL's FFT that is not scaled r?padenot

https://reviewboard.mozilla.org/r/32269/#review33901

Thanks !

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/56708f95249c

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/56708f95249c