Import and compile SSE routines for the speex resampler

RESOLVED FIXED in mozilla25

Status

()

Core
Web Audio
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: padenot, Assigned: padenot)

Tracking

unspecified
mozilla25
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

5 years ago
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/
(Assignee)

Comment 1

5 years ago
Created attachment 777812 [details] [diff] [review]
Import SSE-optimized routines for the speex resampler. r=

This imports the needed file and updates the update.sh scripts.
Attachment #777812 - Flags: review?(ehsan)
(Assignee)

Comment 2

5 years ago
Created attachment 777816 [details] [diff] [review]
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.
Attachment #777816 - Flags: review?(ted)
Attachment #777816 - Flags: review?(ehsan)
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.
Attachment #777816 - Flags: review?(ted)

Updated

5 years ago
Attachment #777812 - Flags: review?(ehsan) → review+

Updated

5 years ago
Attachment #777816 - Flags: review?(ehsan) → review+
(Assignee)

Comment 4

5 years ago
Created attachment 778450 [details] [diff] [review]
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.
Attachment #778450 - Flags: review?(mh+mozilla)
(Assignee)

Updated

5 years ago
Attachment #777816 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
Created attachment 779257 [details] [diff] [review]
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.
Attachment #779257 - Flags: review?(mh+mozilla)
(Assignee)

Updated

5 years ago
Attachment #778450 - Attachment is obsolete: true
Attachment #778450 - Flags: review?(mh+mozilla)
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.
Attachment #779257 - Flags: review?(mh+mozilla) → review-
(Assignee)

Comment 7

5 years ago
(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).
Flags: needinfo?(mh+mozilla)
(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.
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 9

5 years ago
Created attachment 780348 [details] [diff] [review]
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
Flags: needinfo?(mh+mozilla)
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.
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 11

5 years ago
Created attachment 780985 [details] [diff] [review]
move-sse-h-cpp

Yes, it works just fine with that.
Attachment #780985 - Flags: review?(mh+mozilla)
(Assignee)

Updated

5 years ago
Attachment #780348 - Attachment is obsolete: true
(Assignee)

Comment 12

5 years ago
Created attachment 780991 [details] [diff] [review]
speex-sse-runtime

Re-requesting review for the changes in media/libspeex_resampler/src/Makefile.in.
Attachment #779257 - Attachment is obsolete: true
Attachment #780991 - Flags: review?(mh+mozilla)
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
Attachment #780985 - Flags: review?(mh+mozilla) → review+
Attachment #780991 - Flags: review?(mh+mozilla) → review+
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
(Assignee)

Comment 16

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

Updated

5 years ago
Depends on: 899045
You need to log in before you can comment on or make changes to this bug.