894941 - Import and compile SSE routines for the speex resampler

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Paul Adenot (:padenot)]

Resampling shows up pretty high in the profiles for some application, so it would be nice to speed it up.

This bug is about importing the code that is in the speex repo [1] and enabling it when we are on Intel, at compile time. Then, we need to add runtime checks in a couple locations.

[1]: http://git.xiph.org/speex.git/

Comment 1

[Paul Adenot (:padenot)]

Attachment: Import SSE-optimized routines for the speex resampler. r=

This imports the needed file and updates the update.sh scripts.

Comment 2

[Paul Adenot (:padenot)]

Attachment: Patch the speex resampler to do runtime checks for SSE. r=

(Ted for the Makefile.in/moz.build changes here)

So, will this force us to carry a second local patch for the resampler. It uses
the xpcpm/glue/SSE.h header to get the SSE detection, but I had to wrap it in a
header with proper #ifdef __cplusplus, because SSE.h uses a lot of C++ features
(namespaces, bools), and resampler.c is compiled using a C compiler. I haven't
exported the patch yet and added it to the update.sh script becaue I'm not
actually sure this patch is correct for every configuration.

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 777816 [details] [diff] [review]
Patch the speex resampler to do runtime checks for SSE. r=

Review of attachment 777816 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libspeex_resampler/src/Makefile.in
@@ +10,5 @@
>  include $(DEPTH)/config/autoconf.mk
>  
>  MSVC_ENABLE_PGO := 1
>  FORCE_STATIC_LIB = 1
> +VISIBILITY_FLAGS =

I don't understand why you need this.

@@ +37,5 @@
> +DEFINES += -D_USE_SSE -D_USE_SSE2
> +ifdef GNU_CC
> +resample.$(OBJ_SUFFIX): CXXFLAGS+=-msse2
> +endif
> +endif

This depresses me, but I know you've cargo-culted it from elsewhere.

Comment 4

[Paul Adenot (:padenot)]

Attachment: Patch the speex resampler to do runtime checks for SSE. r=

Mike, Ted went on vacation without r+ing, can you check the Makefile.in/moz.buld
changes?

I'm not sure what he means with the cargo-cult comment, I though -msse2 was
necessary. Happy to do whatever is necessary to make this work.

Comment 5

[Paul Adenot (:padenot)]

Attachment: Patch the speex resampler to do runtime checks for SSE. r=

Turns out the previous patch had a couple compile issues on GCC 32bits and MSVC,
addressed here.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 779257 [details] [diff] [review]
Patch the speex resampler to do runtime checks for SSE. r=

Review of attachment 779257 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/media/Makefile.in
@@ +72,5 @@
>  
>  ifdef MOZ_SPEEX_RESAMPLER
>  SHARED_LIBRARY_LIBS 	+= \
>  	$(DEPTH)/media/libspeex_resampler/src/$(LIB_PREFIX)speex_resampler.$(LIB_SUFFIX) \
> +  $(DEPTH)/xpcom/glue/$(LIB_PREFIX)xpcomglue_s.$(LIB_SUFFIX) \

I think it would be better to change the mozilla::sse_private variables to not be hidden, and not link against xpcomglue_s here.
This means SSE.* should probably move from xpcom/glue to xpcom/build.
And do the same with related files, like arm.* and whatever else with the same functionality (I think there's something for altivec, but as I can't find it, maybe it didn't land)

::: media/libspeex_resampler/src/Makefile.in
@@ +11,5 @@
>  
>  MSVC_ENABLE_PGO := 1
>  FORCE_STATIC_LIB = 1
>  
> +

Don't add an empty line

::: media/libspeex_resampler/src/sse_detect.cpp
@@ +6,5 @@
> +#include "mozilla/SSE.h"
> +#include "sse_detect.h"
> +
> +int moz_has_sse2() {
> +  return mozilla::supports_sse2() ? 1 : 0;

You can inline these in the header.

Comment 7

[Paul Adenot (:padenot)]

(In reply to Mike Hommey [:glandium] from comment #6)
> Comment on attachment 779257 [details] [diff] [review]
> Patch the speex resampler to do runtime checks for SSE. r=
> 
> Review of attachment 779257 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/media/Makefile.in
> @@ +72,5 @@
> >  
> >  ifdef MOZ_SPEEX_RESAMPLER
> >  SHARED_LIBRARY_LIBS 	+= \
> >  	$(DEPTH)/media/libspeex_resampler/src/$(LIB_PREFIX)speex_resampler.$(LIB_SUFFIX) \
> > +  $(DEPTH)/xpcom/glue/$(LIB_PREFIX)xpcomglue_s.$(LIB_SUFFIX) \
> 
> I think it would be better to change the mozilla::sse_private variables to
> not be hidden, and not link against xpcomglue_s here.
> This means SSE.* should probably move from xpcom/glue to xpcom/build.
> And do the same with related files, like arm.* and whatever else with the
> same functionality (I think there's something for altivec, but as I can't
> find it, maybe it didn't land)

How can I make the global variables not hidden on Windows? Is this some kind of __declspec(dllexport) or .def file stuff? Not sure where I should put that. I have a patch with the file moved that compiles, but when I use the supports_sse{2,} function from the gkmedias.dll library, it says "unresolved external symbol "bool mozilla::see:private::sse{2,}_enabled"" followed by the mangled name. I could not find library in which all this gets compiled on Windows (I tried dumpbin.exe | grep on everything I found without great success).

> ::: media/libspeex_resampler/src/sse_detect.cpp
> @@ +6,5 @@
> > +#include "mozilla/SSE.h"
> > +#include "sse_detect.h"
> > +
> > +int moz_has_sse2() {
> > +  return mozilla::supports_sse2() ? 1 : 0;
> 
> You can inline these in the header.

No, because I need to call this from a .c file compiled with a C compiler, that does not understand namespaces and stuff (maybe I should have pointed you to comment 2 above).

Comment 8

[Mike Hommey [:glandium]]

(In reply to Paul Adenot (:padenot) from comment #7)
> (In reply to Mike Hommey [:glandium] from comment #6)
> > Comment on attachment 779257 [details] [diff] [review]
> > Patch the speex resampler to do runtime checks for SSE. r=
> > 
> > Review of attachment 779257 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: layout/media/Makefile.in
> > @@ +72,5 @@
> > >  
> > >  ifdef MOZ_SPEEX_RESAMPLER
> > >  SHARED_LIBRARY_LIBS 	+= \
> > >  	$(DEPTH)/media/libspeex_resampler/src/$(LIB_PREFIX)speex_resampler.$(LIB_SUFFIX) \
> > > +  $(DEPTH)/xpcom/glue/$(LIB_PREFIX)xpcomglue_s.$(LIB_SUFFIX) \
> > 
> > I think it would be better to change the mozilla::sse_private variables to
> > not be hidden, and not link against xpcomglue_s here.
> > This means SSE.* should probably move from xpcom/glue to xpcom/build.
> > And do the same with related files, like arm.* and whatever else with the
> > same functionality (I think there's something for altivec, but as I can't
> > find it, maybe it didn't land)
> 
> How can I make the global variables not hidden on Windows? Is this some kind
> of __declspec(dllexport) or .def file stuff?

Replace NS_COM_GLUE with something that resolves to MOZ_EXPORT when IMPL_LIBXUL is defined, and MOZ_IMPORT_DATA otherwise.

Comment 9

[Paul Adenot (:padenot)]

Attachment: move-sse-h-cpp

I have this so far, I checked the new MOZ_VISIBLE macro resolves to what you said, but I still get the same error. 

> 0:07.70 sse_detect.obj : error LNK2001: unresolved external symbol
> "__declspec(dllimport) bool mozilla::sse_private::sse2_enabled"
> __imp_?sse2_enabled@sse_private@mozilla@@3_NA)
> 0:07.70
> 0:07.70 sse_detect.obj : error LNK2001: unresolved external symbol
> "__declspec(dllimport) bool mozilla::sse_private::sse_enabled"
> __imp_?sse_enabled@sse_private@mozilla@@3_NA)
> 0:07.70
> 0:07.70 gkmedias.dll : fatal error LNK1120: 2 unresolved externals
> 0:07.70

Comment 10

[Mike Hommey [:glandium]]

Ah, damn, libxul depends on gkmedias, not the other way around. The remaining option is to move those into mozglue/build, and mark the symbols MFBT_DATA.

Comment 11

[Paul Adenot (:padenot)]

Attachment: move-sse-h-cpp

Yes, it works just fine with that.

Comment 12

[Paul Adenot (:padenot)]

Attachment: speex-sse-runtime

Re-requesting review for the changes in media/libspeex_resampler/src/Makefile.in.

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 780985 [details] [diff] [review]
move-sse-h-cpp

Review of attachment 780985 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/tests/Makefile.in
@@ -45,4 @@
>  
>  CPP_UNIT_TESTS = \
>                   ShowAlignments.cpp \
> -                 ShowSSEConfig.cpp \

You might as well move this under mozglue.
Note you've been bitrotted here by bug 888643

Comment 14

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6412c912847
https://hg.mozilla.org/integration/mozilla-inbound/rev/f134b528c97d
https://hg.mozilla.org/integration/mozilla-inbound/rev/adeb3608cd41

Comment 15

[Ryan VanderMeulen [:RyanVM]]

This push caused test_mediaDecoding.html to go perma-fail on Windows.

https://tbpl.mozilla.org/php/getParsedLog.php?id=25786406&tree=Mozilla-Inbound

11:38:49     INFO -  157107 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/webaudio/test/test_mediaDecoding.html | Expected at most 72 bytes difference, found 76 bytes
11:38:49     INFO -  157108 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/webaudio/test/test_mediaDecoding.html | Received expected decoded data

Rather than backout the entire lot for it, I just upped the fuzzTolerance to 76. If that was bad, feel free to backout and do what you want.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b60ddc95007

Comment 16

[Paul Adenot (:padenot)]

Ryan, this is perfect (but for some reason it was green locally and on try. SSE is hard).

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f6412c912847
https://hg.mozilla.org/mozilla-central/rev/f134b528c97d
https://hg.mozilla.org/mozilla-central/rev/adeb3608cd41
https://hg.mozilla.org/mozilla-central/rev/7b60ddc95007