Closed Bug 1158741 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: Web Audio, defect, P2)

32 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: padenot, Assigned: dminor)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(1 file)

This is just a matter of removing the scaling at the end of the function.
Depends on: 926838
Just guessing at priority...
Rank: 25
Priority: -- → P2
Blocks: webaudioperf
Keywords: perf
I'll pick this one up.
Assignee: nobody → dminor
Status: NEW → ASSIGNED
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/
Attachment #8711631 - Flags: review?(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.
Attachment #8711631 - Flags: review?(padenot)
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
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.
Flags: needinfo?(padenot)
I think just a comment is enough.
Flags: needinfo?(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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32269/diff/1-2/
Attachment #8711631 - Flags: review?(padenot)
Sorry about the long delay in getting this up for another review.
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 !
Attachment #8711631 - Flags: review?(padenot) → review+
https://hg.mozilla.org/mozilla-central/rev/56708f95249c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.