Closed
Bug 1503363
Opened 6 years ago
Closed 6 years ago
Don't assume that all WINNT targets support sse2
Categories
(Core :: WebRTC, enhancement, P3)
Core
WebRTC
Tracking
()
RESOLVED
FIXED
mozilla65
Tracking | Status | |
---|---|---|
firefox65 | --- | fixed |
People
(Reporter: away, Assigned: away)
References
Details
Attachments
(5 files, 1 obsolete file)
(deleted),
patch
|
dminor
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dminor
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dminor
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dminor
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dminor
:
review+
|
Details | Diff | Splinter Review |
In particular, aarch64-windows does not. (But it does support neon)
https://searchfox.org/mozilla-central/rev/fc3d974254660b34638b2af9d5431618b191b233/media/webrtc/trunk/moz.build#144,147-149,152
Comment 1•6 years ago
|
||
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?
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
Updated•6 years ago
|
Priority: -- → P3
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)
Attachment #9024113 -
Flags: review?(dminor)
Attachment #9024114 -
Flags: review?(dminor)
Updated•6 years ago
|
Attachment #9024112 -
Flags: review?(dminor) → review+
Comment 7•6 years ago
|
||
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 8•6 years ago
|
||
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+
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
Comment 10•6 years ago
|
||
Backed out 3 changesets (Bug 1503363) for build bustages on mozbuild.
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/31c0c15ad3ace7f01cb22e7eec6ff187defa680b
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=adc6f14f89e418d1c40b6d3b40a55833f07d7719&selectedJob=211462895
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=211462895&repo=mozilla-inbound&lineNumber=1387
Flags: needinfo?(dmajor)
Assignee | ||
Comment 11•6 years 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)
Comment 12•6 years ago
|
||
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 '*'
Comment 13•6 years ago
|
||
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 years 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?
Comment 15•6 years ago
|
||
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.
Comment 16•6 years ago
|
||
This usually indicates a bug in the moz.build generation. I will look into this today.
Comment 17•6 years ago
|
||
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)
Comment 18•6 years ago
|
||
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)
Comment 19•6 years ago
|
||
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.
Comment 20•6 years ago
|
||
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 years 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 22•6 years ago
|
||
Also an `hg cp` -- nothing really noteworthy, just wd4267 (both added and removed!) due to cases like
https://searchfox.org/mozilla-central/rev/d850d799a0009f851b5535580e0a8b4bb2c591d7/media/webrtc/trunk/webrtc/build/config/compiler/BUILD.gn#987,993
and
https://searchfox.org/mozilla-central/rev/d850d799a0009f851b5535580e0a8b4bb2c591d7/media/webrtc/trunk/webrtc/build/config/compiler/BUILD.gn#1279-1280
Attachment #9025056 -
Flags: review?(dminor)
Assignee | ||
Comment 23•6 years ago
|
||
Attachment #9024114 -
Attachment is obsolete: true
Attachment #9025057 -
Flags: review?(dminor)
Updated•6 years ago
|
Attachment #9025055 -
Flags: review?(dminor) → review+
Updated•6 years ago
|
Attachment #9025056 -
Flags: review?(dminor) → review+
Updated•6 years ago
|
Attachment #9025057 -
Flags: review?(dminor) → review+
Comment 24•6 years ago
|
||
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 years 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
Comment 26•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4a184c23e5e7
https://hg.mozilla.org/mozilla-central/rev/a5ac0c2a7dcb
https://hg.mozilla.org/mozilla-central/rev/1fba6d2e67d8
https://hg.mozilla.org/mozilla-central/rev/9863a841090f
https://hg.mozilla.org/mozilla-central/rev/fcbdfdfb0776
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in
before you can comment on or make changes to this bug.
Description
•