1042508 - Enable speex resampler neon optimizations where available

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Karl Tomlinson (:karlt)]

These optimizations have only been implemented upstream for the direct resamplers.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: include resample_neon.h from speexdsp 305e54ea

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: tie resampler sample type choice to MOZ_SAMPLE_TYPE_S16

When built with FIXED_POINT, speex_resampler_process_float() assumes the
samples are in the range ±0x7fff and so Gecko instead uses ConvertAudioSamples
and speex_resampler_process_int().  This decision is based on
MOZ_SAMPLE_TYPE_S16.

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: move resampler simd optimizations to separate translation units

This ensures that code in resample.c will run on Intel x86 cpus even when SSE
support has been compiled, and will provide similarly for neon support when
enabled.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: remove : of empty Clobber asm argument

https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Extended-Asm says

  asm [volatile] ( AssemblerTemplate : [OutputOperands] [ : [InputOperands] [ : [Clobbers] ] ] )

which implies that Clobbers is optional even after the third colon, but
the gcc used for b2g_try_emulator_dep builds says

resample_neon.c: In function 'saturate_32bit_to_16bit':
resample_neon.c:50: error: expected string literal before ')' token

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: enable neon resampler optimizations

Comment 6

[Karl Tomlinson (:karlt)]

Note that we miss out on the WORD2INT saturation optimization, but that is only used during sinct table initialization.  The SIMD optimization is for O(filt_len) operations per processed sample.

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 8460754 [details] [diff] [review]
move resampler simd optimizations to separate translation units

It seems mercurial has lost the file renames somewhere.

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: part 3: move resampler simd optimizations to separate translation units

Comment 9

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85e2540e752f
https://hg.mozilla.org/integration/mozilla-inbound/rev/2566329e44b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/82f2fe2768d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2404857df44
https://hg.mozilla.org/integration/mozilla-inbound/rev/16cb26a14843
https://hg.mozilla.org/integration/mozilla-inbound/rev/42f62e9467f4

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85e2540e752f
https://hg.mozilla.org/mozilla-central/rev/2566329e44b9
https://hg.mozilla.org/mozilla-central/rev/82f2fe2768d5
https://hg.mozilla.org/mozilla-central/rev/a2404857df44
https://hg.mozilla.org/mozilla-central/rev/16cb26a14843

Comment 11

[Diego Wilson [:diego]]

[Blocking Requested - why for this release]: This reduces the CPU usage of a WebRTC call by 5%-8%. Given how resource intensive WebRTC is, we could really use this enhancement on v2.0.

Comment 12

[bhavana bajaj [:bajaj]]

(In reply to Diego Wilson [:diego] from comment #11)
> [Blocking Requested - why for this release]: This reduces the CPU usage of a
> WebRTC call by 5%-8%. Given how resource intensive WebRTC is, we could
> really use this enhancement on v2.0.

Hey diego, this might help here but given the timeline for 2.0 and the baketime this has had, IMHO this looks risky to uplift at this point. I am also neeing Karl to give his input on risk profile here.

Comment 13

[Karl Tomlinson (:karlt)]

(In reply to bhavana bajaj [:bajaj] from comment #12)
> Karl to give his input on risk profile here.

I don't know neon assembler myself, so I don't know the potential for edge
case issues such as incorrect saturation.  Things will sound bad anyway if
content causes the corner cases are being reached.  The common case has
automated tests, which would pick up major bugs but maybe not smaller changes
in quality.  IIRC VLC has been using similar code for a long time.

Porting this requires the SATURATE32PSHR changes from bug 1042504 and some of
the refactoring of conditionals there.  That's probably more costly in terms
of developer work than risk.

The greatest risk is probably generating and running the wrong code on
platforms that won't run it.  Such effects would be fatal, but therefore
easier to detect.

Comment 14

[bhavana bajaj [:bajaj]]

(In reply to Karl Tomlinson (:karlt) from comment #13)
> (In reply to bhavana bajaj [:bajaj] from comment #12)
> > Karl to give his input on risk profile here.
> 
> I don't know neon assembler myself, so I don't know the potential for edge
> case issues such as incorrect saturation.  Things will sound bad anyway if
> content causes the corner cases are being reached.  The common case has
> automated tests, which would pick up major bugs but maybe not smaller changes
> in quality.  IIRC VLC has been using similar code for a long time.
> 
> Porting this requires the SATURATE32PSHR changes from bug 1042504 and some of
> the refactoring of conditionals there.  That's probably more costly in terms
> of developer work than risk.
> 
> The greatest risk is probably generating and running the wrong code on
> platforms that won't run it.  Such effects would be fatal, but therefore
> easier to detect.

Thanks karl. I've looped in :jsmith from QA to help get additional testing here. He'll work through some of the follow0up questions he may have.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for b2g32 uplift.

Comment 16

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Karl isn't available to work on this. Further, uplifting is dead time in that the fixes will be available in the next release. Eric - is there someone available in Taipei to uplift the neon optimisations?

Comment 17

[Eric Chou [:ericchou] [:echou]]

Hi Anthony,

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #16)
> Karl isn't available to work on this. Further, uplifting is dead time in
> that the fixes will be available in the next release. Eric - is there
> someone available in Taipei to uplift the neon optimisations?

Star is working on another potential 2.0 issue and Bruce are still on 2.1 feature. Blake might be available, but do you think it's ok for us to do the uplift even if we don't really know the logic behind the scene? :|

Could Paul help out with uplifting?

Comment 18

[Eric Chou [:ericchou] [:echou]]

Oops, sorry for the wrong needinfo.

Comment 19

[Paul Adenot (:padenot)]

This is hard to uplift as is. We have two options:
(1) We can try to uplift the minimum of code needed, but that means writing new code.
(2) We can uplift all the exact patches that landed on central in the exact same order (bugs 1033140, 1042504, 1042508, and backout and reapply 995075)

I think (1) is riskier and longer to do than (2), but will eventually result in a smaller uplift (which should not be a criteria).

(2) is, considering the patches have baked on central for some time, now, what I'm leaning towards. Ideally, when it's done, we would have the same code in b2g31 and central, so we benefit from the QA and test runs done there.

That said, have we profiled the code? Are the optimizations really worth the uplift?

Comment 20

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

There isn't anyone available to work on this. RyanVM could try uplifting the chain of patches that Paul suggested. Otherwise we should cancel the uplift.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

I'm happy to do so, but it sounds like maybe we should get some perf measurements on 2.1 first to make sure it's worth the effort?

Comment 22

[Maire Reavy [:mreavy]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #21)
> I'm happy to do so, but it sounds like maybe we should get some perf
> measurements on 2.1 first to make sure it's worth the effort?

That's exactly what I think we need.  Diego -- Can you provide some before and after perf numbers for these sets of patches?

Comment 23

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #22)
> That's exactly what I think we need.  Diego -- Can you provide some before
> and after perf numbers for these sets of patches?

We need to know if we're near 100%.

Comment 24

[Diego Wilson [:diego]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #23)
> (In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #22)
> > That's exactly what I think we need.  Diego -- Can you provide some before
> > and after perf numbers for these sets of patches?
> 
> We need to know if we're near 100%.

IIRC the CPU usage is near 95% for WebRTC on FFOS v2.0 8x10 

Jay, can you share our perf improvements numbers for WebRTC?

Comment 25

[Marcia Knous [:marcia]]

Removing qawanted - jsmith recommends adding mlee to this bug.

Comment 26

[Mike Lee [:mlee]]

As part of Li Gong's reorganization the fxOS Performance team has been dissolved. Performance issues will now be directly handled by component teams.

Comment 27

[Jay]

(In reply to Diego Wilson [:diego] from comment #24)
> (In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #23)
> > (In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #22)
> > > That's exactly what I think we need.  Diego -- Can you provide some before
> > > and after perf numbers for these sets of patches?
> > 
> > We need to know if we're near 100%.
> 
> IIRC the CPU usage is near 95% for WebRTC on FFOS v2.0 8x10 
> 
> Jay, can you share our perf improvements numbers for WebRTC?
Without this patch, we see that average CPU % is around 97.3% and encoder FPS is around 25-28FPS.
With the patch, the average CPU% is around 95.3% and the encodoer FPS goes up to 28-30FPS range. This is measured with end-to-end webRTC use-case. With the local WebRTC loopback, both cases gives about 30FPS, but the neon resempler helps to reduce CPU utilization by 5%.

Comment 28

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Mike Lee [:mlee] from comment #26)
> As part of Li Gong's reorganization the fxOS Performance team has been
> dissolved. Performance issues will now be directly handled by component
> teams.

Lets hope the patches apply cleanly then.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/?tree=Try&rev=6e3c7a993a6e

I'll go ahead and requested 2.0+ on the other bugs.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Try is green, just waiting on the approvals.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/267c11d34c91
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/5550e4079b31
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/56ef32b42cb5
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/5ffe87c3f6f0
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/1843d25906fd

Comment 32

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Ryan - thanks for uplifting us out of a pickle!