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)

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: away, Assigned: away)

References

Details

Attachments

(5 files, 1 obsolete file)

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
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)
Attached patch Regenerate webrtc moz.build files. (obsolete) (deleted) — Splinter Review
Attachment #9024114 - Flags: review?(dminor)
Depends on: 1376873
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+
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
[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)
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)
Attached patch Add gn json files for x86-linux (deleted) — Splinter Review
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)
Attachment #9024114 - Attachment is obsolete: true
Attachment #9025057 - Flags: review?(dminor)
Attachment #9025055 - Flags: review?(dminor) → review+
Attachment #9025056 - Flags: review?(dminor) → review+
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
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
No longer depends on: 1646904
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: