remove direct convolution stage; instead use largest initial FFT stage possible

RESOLVED FIXED in Firefox 45

Status

()

defect
P1
normal
Rank:
12
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

(Blocks 1 bug, {perf})

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(4 attachments)

Replacing direct convolution with an fft stage of the same size reduces
rendering thread cpu usage by 6% on attachment 8683409 [details].

Removing the first two half-block-size stages reduces rendering thread cpu usage by a further 2.5%.
Blocks: 1221836
bug 1221833 reduce FFTConvolver latency by one block r?padenot
Attachment #8683445 - Flags: review?(padenot)
bug 1221833 replace initial direct convolution stage with fft r?padenot
Attachment #8683446 - Flags: review?(padenot)
bug 1221833 remove first two half-block-size convolver stages r?padenot

Efficiency is proportional to stage size, so start with the largest size
possible.
Attachment #8683447 - Flags: review?(padenot)
bug 1221833 remove now-unused direct convolver r?padenot
Attachment #8683448 - Flags: review?(padenot)
Rank: 12
Priority: -- → P1
(In reply to Karl Tomlinson (ni?:karlt) from comment #2)
> Created attachment 8683446 [details]
> MozReview Request: bug 1221833 replace initial direct convolution stage with
> fft r?padenot
> 
> bug 1221833 replace initial direct convolution stage with fft r?padenot

Why do you think it got initially written using direct computation? The fft size always has been big enough so that doing it in the frequency domain should have been a win, no?
Flags: needinfo?(karlt)
Comment on attachment 8683445 [details]
MozReview Request: bug 1221833 reduce FFTConvolver latency by one block r?padenot

https://reviewboard.mozilla.org/r/24351/#review21867
Attachment #8683445 - Flags: review?(padenot) → review+
Comment on attachment 8683446 [details]
MozReview Request: bug 1221833 replace initial direct convolution stage with fft r?padenot

https://reviewboard.mozilla.org/r/24353/#review21869
Attachment #8683446 - Flags: review?(padenot) → review+
Attachment #8683447 - Flags: review?(padenot) → review+
Comment on attachment 8683447 [details]
MozReview Request: bug 1221833 remove first two half-block-size convolver stages r?padenot

https://reviewboard.mozilla.org/r/24355/#review21871
Comment on attachment 8683448 [details]
MozReview Request: bug 1221833 remove now-unused direct convolver r?padenot

https://reviewboard.mozilla.org/r/24357/#review21873
Attachment #8683448 - Flags: review?(padenot) → review+
(In reply to Paul Adenot (:padenot) from comment #5)
> Why do you think it got initially written using direct computation? The fft
> size always has been big enough so that doing it in the frequency domain
> should have been a win, no?

The first FFT stage size was the same size as the direct stage, so, yes, I don't think the size of the stage was the reason for choosing to use the direct computation.

I suspect the reason for using the direct stage was just that the FFT stage implementation didn't support zero latency at any stage sizes.

I haven't looked at the code history but the existence of the Reverb/ReverbConvolver::latencyFrames() methods (to be removed in bug 1221831) and the reverbTotalLatency variable in the ReverbConvolver constructor might indicate that these were once non-zero.  Perhaps, the FFTConvolver implementation may have been designed for a more general situation with variable block sizes.
Flags: needinfo?(karlt)
You need to log in before you can comment on or make changes to this bug.