Don't assume that all WINNT targets support sse2

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P3
normal
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: dmajor, Assigned: dmajor)

Tracking

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

I think we have to add an appropriate aarch64/windows gn config file in media/webrtc/gn-configs, along the lines of bug 1434589, and then regenerate the moz.build files based on the new configs.  Ideally, that should take care of things?
Assignee

Comment 2

7 months ago
Note to self, we'll need to add aecm_core_neon.cc to this list, for the same PART_LEN2 reason https://searchfox.org/mozilla-central/rev/fc3d974254660b34638b2af9d5431618b191b233/media/webrtc/moz.build#34
Priority: -- → P3
Assignee

Updated

7 months ago
Duplicate of this bug: 1503359
Assignee

Comment 4

7 months ago
The _neon file is the only one I need to change for my purposes, but from inspection it looks like the _mips file would have the same issue, so I included it too.
Assignee: nobody → dmajor
Attachment #9024112 - Flags: review?(dminor)
Assignee

Comment 5

7 months ago
Attachment #9024113 - Flags: review?(dminor)
Assignee

Comment 6

7 months ago
Attachment #9024114 - Flags: review?(dminor)
Assignee

Updated

7 months ago
Depends on: 1376873

Updated

6 months ago
Attachment #9024112 - Flags: review?(dminor) → review+
Comment on attachment 9024113 [details] [diff] [review]
Add gn json files for aarch64-windows.

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

Rubber-stamped.
Attachment #9024113 - Flags: review?(dminor) → review+
Comment on attachment 9024114 [details] [diff] [review]
Regenerate webrtc moz.build files.

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

Rubber-stamped.
Attachment #9024114 - Flags: review?(dminor) → review+

Comment 9

6 months ago
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c2426b552ac
De-unify some more webrtc files due to conflicting defines. r=dminor
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b7dd43c1e2e
Add gn json files for aarch64-windows. r=dminor
https://hg.mozilla.org/integration/mozilla-inbound/rev/adc6f14f89e4
Regenerate webrtc moz.build files. r=dminor
Assignee

Comment 11

6 months ago
[task 2018-11-13T16:15:28.466Z] 16:15:28     INFO -  FATAL ERROR PROCESSING MOZBUILD FILE

[task 2018-11-13T16:15:28.466Z] 16:15:28     INFO -      Source file should only be added to UNIFIED_SOURCES once: /media/webrtc/trunk/webrtc/modules/audio_processing/ns/noise_suppression.c

Any idea what happened here? https://hg.mozilla.org/integration/mozilla-inbound/diff/adc6f14f89e4/media/webrtc/trunk/webrtc/modules/audio_processing/audio_processing_c_gn/moz.build

My mozbuild update took noise_suppression.c out of WINNT and put it under x86 -- but it's still present in the Linux section, so x86 Linux builds get the file twice.

Though, I don't understand why it got moved to the x86 condition, because there are plenty of x64 configs that want this file: https://searchfox.org/mozilla-central/search?q=noise_suppression.c

Am I using the updater incorrectly or something?
Flags: needinfo?(dmajor) → needinfo?(dminor)
This failure also appeared on Bmsvc: 
https://treeherder.mozilla.org/logviewer.html#?job_id=211462938&repo=mozilla-inbound&lineNumber=18673 

16:30:30     INFO -  z:/build/build/src/media/webrtc/trunk/webrtc/modules/audio_processing/noise_suppression_impl.cc(43): error C2143: syntax error: missing ';' before '*'
I don't see offhand why noise_suppression.c should have been affected by your changes. One thing to check is to make sure you don't have any old json files in media/webrtc/gn-configs that could be throwing off the code generation. You could also check to see if the problem reproduces with the other arm64 platforms removed.

It's also possible this is a bug in the moz.build generation, maybe Chris could have a quick look.
Flags: needinfo?(dminor) → needinfo?(cmanchester)
Assignee

Comment 14

6 months ago
In case it's relevant: it _is_ correct to remove noise_suppression.c from the general WINNT section, because WINNT-aarch64 does not want it. (aarch64 wants noise_suppression_x.c instead).

So we do want to specialize on arch somewhat, but I don't know why it's affecting x86 things and not x86_64.

Oh, I wonder if this related to the fact that we don't have x86_64 jsons on Windows anymore?
Oh, ok, so that does explain why noise_suppression.c was moved. I missed that.

I used to create the x86_64 jsons by copying and pasting the x86 versions with the references to x86 changed to x64 and x86_64 as needed, so that would be quick to test, but I imagine that the moz.build generation would just just ignore cpu in that case.
This usually indicates a bug in the moz.build generation. I will look into this today.
This is a shortcoming more than a bug, but the moz.build generation is having trouble handling the situation introduced here: every linux build wants this source file, every x86 build wants this source file, and we don't have a way to tell that these configurations are overlapping but not the same set. We got away with this before because every Windows build also wanted this source file and that was good enough to get it included for every configuration that cared about it.

This is really unfortunate, I'm sorry this got unearthed here. I'll post a hacky workaround or something better soon.
Flags: needinfo?(cmanchester)
Well, I was at least partly mistaken in comment 17. I noticed while testing a workaround that we don't have any gn-configs for x86 linux. Once I add those the moz.build generation seems to do something reasonable. I'm running this through try right now, but the generated changes look fine.

Dan, is there anything to say against adding x86 linux gn-configs?
Flags: needinfo?(dminor)
Hmm, now my try push is failing because there are some necessary defines that end up not set on x86_64, I think because we don't have x86_64 configs either. I guess we need to add those, although I also feel like I missed a step here, so I'm not totally sure.
We can definitely add the extra configs back. The only reason to remove them was to avoid the copy and paste step that I mentioned in Comment 15, which was unnecessary at the time. Sorry about the trouble!
Flags: needinfo?(dminor)
Assignee

Comment 21

6 months ago
I took these from chmanchester's try push, and marked them as `hg cp` from their x64 counterparts, so that the diff only shows the changes.
Attachment #9025055 - Flags: review?(dminor)
Assignee

Comment 23

6 months ago
Attachment #9024114 - Attachment is obsolete: true
Attachment #9025057 - Flags: review?(dminor)

Updated

6 months ago
Attachment #9025055 - Flags: review?(dminor) → review+

Updated

6 months ago
Attachment #9025056 - Flags: review?(dminor) → review+

Updated

6 months ago
Attachment #9025057 - Flags: review?(dminor) → review+
We have a whitelist of flags here [1] so most of the compiler flag changes here should have no effect.

[1] https://searchfox.org/mozilla-central/rev/d850d799a0009f851b5535580e0a8b4bb2c591d7/media/webrtc/moz.build#77

Comment 25

6 months ago
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a184c23e5e7
De-unify some more webrtc files due to conflicting defines. r=dminor
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5ac0c2a7dcb
Add gn json files for aarch64-windows. r=dminor
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fba6d2e67d8
Add gn json files for x86-linux. r=dminor
https://hg.mozilla.org/integration/mozilla-inbound/rev/9863a841090f
Add gn json files for x64-windows. r=dminor
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcbdfdfb0776
Regenerate webrtc moz.build files. r=dminor
You need to log in before you can comment on or make changes to this bug.