Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1820133002:
Implement CPU feature detection for ARM Linux. (Closed)
|
Created:
4 years, 9 months ago by Simon Hosie Modified:
4 years, 8 months ago CC:
webrtc-reviews_webrtc.org, henrika_webrtc, zhengzhonghou_agora.io, tterriberry_mozilla.com, fengyue_agora.io, peah-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionImplement CPU feature detection for ARM Linux.
BUG=webrtc:5057
NOTRY=True
Committed: https://crrev.com/953b1c185fcb4d9a1da063ca70a79f14b88c47a3
Cr-Commit-Position: refs/heads/master@{#12270}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Fixes for chromium and x86 builds #
Total comments: 15
Patch Set 3 : GN correction, dead file removal, formatting, stricter string structure test for AT_PLATFORM. #
Messages
Total messages: 27 (10 generated)
kjellander@webrtc.org changed reviewers: + kjellander@webrtc.org
https://codereview.webrtc.org/1820133002/diff/1/webrtc/system_wrappers/system... File webrtc/system_wrappers/system_wrappers.gyp (right): https://codereview.webrtc.org/1820133002/diff/1/webrtc/system_wrappers/system... webrtc/system_wrappers/system_wrappers.gyp:127: 'cpu_features_webrtc.gyp:cpu_features_linux', I'm not sure if this will compile fine with Chromium. Would it be possible for you to create a Chromium checkout and compile Chromium for regular Linux x86 and Linux ARM with this patch applied to src/third_party/webrtc? Notice you'll have to cut away one directory level when applying a patch into that location since it's only mapping the webrtc/ subdirectory of the standalone WebRTC repo.
On 2016/03/30 19:11:17, kjellander (webrtc) wrote: > https://codereview.webrtc.org/1820133002/diff/1/webrtc/system_wrappers/system... > File webrtc/system_wrappers/system_wrappers.gyp (right): > > https://codereview.webrtc.org/1820133002/diff/1/webrtc/system_wrappers/system... > webrtc/system_wrappers/system_wrappers.gyp:127: > 'cpu_features_webrtc.gyp:cpu_features_linux', > I'm not sure if this will compile fine with Chromium. > Would it be possible for you to create a Chromium checkout and compile Chromium > for regular Linux x86 and Linux ARM with this patch applied to > src/third_party/webrtc? > Notice you'll have to cut away one directory level when applying a patch into > that location since it's only mapping the webrtc/ subdirectory of the standalone > WebRTC repo. Well it did fail but with a trivial compilation error (ssize_t/size_t mismatch). Fixing that (and the missing header for x86) I don't see any other errors in any configuration I've tried. I made it conditional anyway, because the new code is apparently superfluous. Is there a GYP define to make NEON a run-time feature test in Chromium? I've probably overlooked that.
kjellander@webrtc.org changed reviewers: + torbjorng@webrtc.org
+torbjorng to review the C code in webrtc/system_wrappers/source/cpu_features_linux.c since my C skills are not sufficient here. Thanks for doing the work for the Chromium build, see my comment about exposing the code to the Chromium build as well (not sure it's easily doable of if we necessarily need to do it in this CL though). Regarding ARM NEON run-time detection in Chromium - there's nothing AFAIK, but I know libvpx has some in their code. I believe it happens here: https://chromium.googlesource.com/webm/libvpx/+/master/vpx_ports/arm_cpudetect.c You might want to check with the authors of that source if you have questions on how/if it's used inside Chromium (my guess would be that the use is limited to libvpx). https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/BU... File webrtc/system_wrappers/BUILD.gn (right): https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/BU... webrtc/system_wrappers/BUILD.gn:111: deps += [ ":cpu_features_linux" ] Please introduce the similar if (build_with_chromium) condition here as for the GYP build. https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/cp... File webrtc/system_wrappers/cpu_features_linux.gyp (right): https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/cp... webrtc/system_wrappers/cpu_features_linux.gyp:1: # Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. You already added a target in webrtc/system_wrappers/cpu_features_webrtc.gyp; there's no need for this file. https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/cp... File webrtc/system_wrappers/cpu_features_webrtc.gyp (right): https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/cp... webrtc/system_wrappers/cpu_features_webrtc.gyp:29: 'target_name': 'cpu_features_linux', Does it make sense to add similar target in webrtc/system_wrappers/cpu_features_chromium.gyp or will it not build and/or provide any value to the Chromium build? Since they also have some support for Linux ARM, I imagine they would also want to use this code.
The formatting of this file deviates from what we use in Chromium and WebRTC. Please use "git cl format" to reformat. You need to temporarily rename the file to .cc (and git cl add that) for the format command to take effect. https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/so... File webrtc/system_wrappers/source/cpu_features_linux.c (right): https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/so... webrtc/system_wrappers/source/cpu_features_linux.c:39: while ((hwcap == 0) || (platform == NULL)) { We don't usually use redundant parentheses. https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/so... webrtc/system_wrappers/source/cpu_features_linux.c:52: (void)auxv.a_un.a_val; Please remove this. https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/so... webrtc/system_wrappers/source/cpu_features_linux.c:69: architecture = atoi(platform + 1); If the |platform| string end up not being 0-terminated, this atoi might run off in memory. https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/so... webrtc/system_wrappers/source/cpu_features_linux.c:75: if (architecture >= 7) result |= kCPUFeatureARMv7; This code will not detect CPU archs properly if the atoi above ever read more than just one digit. E.g., no subarchs every append another digit, the result will be greater than 7. To fix, please read just one digit above without using atoi.
Oops, one sentence there got too many typos to be readable. Let me try again: E.g., if subarchs ever append another digit, the result will be greater than 7.
https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/BU... File webrtc/system_wrappers/BUILD.gn (right): https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/BU... webrtc/system_wrappers/BUILD.gn:111: deps += [ ":cpu_features_linux" ] On 2016/04/04 06:09:45, kjellander (webrtc) wrote: > Please introduce the similar if (build_with_chromium) condition here as for the > GYP build. Done. https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/cp... File webrtc/system_wrappers/cpu_features_linux.gyp (right): https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/cp... webrtc/system_wrappers/cpu_features_linux.gyp:1: # Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2016/04/04 06:09:45, kjellander (webrtc) wrote: > You already added a target in webrtc/system_wrappers/cpu_features_webrtc.gyp; > there's no need for this file. Done. https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/cp... File webrtc/system_wrappers/cpu_features_webrtc.gyp (right): https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/cp... webrtc/system_wrappers/cpu_features_webrtc.gyp:29: 'target_name': 'cpu_features_linux', On 2016/04/04 06:09:45, kjellander (webrtc) wrote: > Does it make sense to add similar target in > webrtc/system_wrappers/cpu_features_chromium.gyp or will it not build and/or > provide any value to the Chromium build? Since they also have some support for > Linux ARM, I imagine they would also want to use this code. I would have hoped so, but I can't seem to produce a link error in Chromium either with or without this function. This suggests that Chromium hard-codes WebRTC's NEON support always-on or always-off, with no run-time detection under Linux. If it's always-off that's obviously not ideal, and I'm not sure how far that issue would currently extend. Android may be configured the same way, and I would expect ChromeOS to follow the Linux path. If there hasn't been any complaint about building for ChromeOS previously then that may also be missing out on NEON (although it could equally be hard-coded always-on on a per-platform basis). I don't know what GYP_DEFINE (or similar) is necessary to enable run-time detection. The original build-blocker, and the way I tested the function, was in one of the unit tests which calls WebRtc_GetCPUFeaturesARM() outside of a WEBRTC_DETECT_NEON guard (which makes sense in context; no need to fix that). If I find out what to do to Chromium to make sure it requests run-time NEON detection from WebRTC, then I can try to figure out what fits where. https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/so... File webrtc/system_wrappers/source/cpu_features_linux.c (right): https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/so... webrtc/system_wrappers/source/cpu_features_linux.c:39: while ((hwcap == 0) || (platform == NULL)) { On 2016/04/04 08:35:42, torbjorng (webrtc) wrote: > We don't usually use redundant parentheses. Done. https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/so... webrtc/system_wrappers/source/cpu_features_linux.c:52: (void)auxv.a_un.a_val; On 2016/04/04 08:35:42, torbjorng (webrtc) wrote: > Please remove this. Done. https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/so... webrtc/system_wrappers/source/cpu_features_linux.c:69: architecture = atoi(platform + 1); On 2016/04/04 08:35:42, torbjorng (webrtc) wrote: > If the |platform| string end up not being 0-terminated, this atoi might run off > in memory. getauxval() defines `platform` as a pointer to a string. Since there's no other way to determine its length it really must be nul-terminated. https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/so... webrtc/system_wrappers/source/cpu_features_linux.c:75: if (architecture >= 7) result |= kCPUFeatureARMv7; On 2016/04/04 08:35:42, torbjorng (webrtc) wrote: > This code will not detect CPU archs properly if the atoi above ever read more > than just one digit. E.g., no subarchs every append another digit, the result > will be greater than 7. To fix, please read just one digit above without using > atoi. Reading just one digit makes "v10" less than "v7". To avoid guessing which is more likely I'll make it fail when there's more than one digit (so both "v10l" and "v82l" will fail to parse). Somebody can fix it either when ARMv10 ships or when sub-architectures are inserted into this string. (but it won't matter because no caller it WebRTC actually uses this flag)
lgtm wrt cpu_features_linux.c.
Description was changed from ========== Implement CPU feature detection for ARM Linux. BUG=webrtc:5057 ========== to ========== Implement CPU feature detection for ARM Linux. BUG=webrtc:5057 NOTRY=True ==========
lgtm! Feel free to send this to the CQ if the trybots I've triggered are green (no need to run a full set for this patch, which is why I added NOTRY=True). https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/cp... File webrtc/system_wrappers/cpu_features_webrtc.gyp (right): https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/cp... webrtc/system_wrappers/cpu_features_webrtc.gyp:29: 'target_name': 'cpu_features_linux', On 2016/04/05 01:15:48, Simon Hosie wrote: > On 2016/04/04 06:09:45, kjellander (webrtc) wrote: > > Does it make sense to add similar target in > > webrtc/system_wrappers/cpu_features_chromium.gyp or will it not build and/or > > provide any value to the Chromium build? Since they also have some support for > > Linux ARM, I imagine they would also want to use this code. > > I would have hoped so, but I can't seem to produce a link error in Chromium > either with or without this function. This suggests that Chromium hard-codes > WebRTC's NEON support always-on or always-off, with no run-time detection under > Linux. I remember that in the past CPU detection was enabled for libvpx in Chromium since they shipped to devices that doesn't have Neon. IIRC that was about to change and it might have now. I've asked johannkoenig@ to comment on this (CC'ed). Let's not worry more about the Chromium case in this CL - that can always be worked on in the future. The important thing now it unblock this work. > If it's always-off that's obviously not ideal, and I'm not sure how far that > issue would currently extend. Android may be configured the same way, and I > would expect ChromeOS to follow the Linux path. If there hasn't been any > complaint about building for ChromeOS previously then that may also be missing > out on NEON (although it could equally be hard-coded always-on on a per-platform > basis). > > I don't know what GYP_DEFINE (or similar) is necessary to enable run-time > detection. The original build-blocker, and the way I tested the function, was > in one of the unit tests which calls WebRtc_GetCPUFeaturesARM() outside of a > WEBRTC_DETECT_NEON guard (which makes sense in context; no need to fix that). I'm pretty sure there is no GYP_DEFINES setting that controls this. > If I find out what to do to Chromium to make sure it requests run-time NEON > detection from WebRTC, then I can try to figure out what fits where. Sounds good, let's do that separate from this CL though.
kjellander@webrtc.org changed reviewers: + johannkoenig@chromium.org
On 2016/04/05 09:27:51, kjellander (webrtc) wrote: > lgtm! > Feel free to send this to the CQ if the trybots I've triggered are green (no > need to run a full set for this patch, which is why I added NOTRY=True). > > https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/cp... > File webrtc/system_wrappers/cpu_features_webrtc.gyp (right): > > https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/cp... > webrtc/system_wrappers/cpu_features_webrtc.gyp:29: 'target_name': > 'cpu_features_linux', > On 2016/04/05 01:15:48, Simon Hosie wrote: > > On 2016/04/04 06:09:45, kjellander (webrtc) wrote: > > > Does it make sense to add similar target in > > > webrtc/system_wrappers/cpu_features_chromium.gyp or will it not build and/or > > > provide any value to the Chromium build? Since they also have some support > for > > > Linux ARM, I imagine they would also want to use this code. > > > > I would have hoped so, but I can't seem to produce a link error in Chromium > > either with or without this function. This suggests that Chromium hard-codes > > WebRTC's NEON support always-on or always-off, with no run-time detection > under > > Linux. > > I remember that in the past CPU detection was enabled for libvpx in Chromium > since > they shipped to devices that doesn't have Neon. IIRC that was about to change > and it > might have now. > I've asked johannkoenig@ to comment on this (CC'ed). > > Let's not worry more about the Chromium case in this CL - that can always be > worked > on in the future. The important thing now it unblock this work. > > > If it's always-off that's obviously not ideal, and I'm not sure how far that > > issue would currently extend. Android may be configured the same way, and I > > would expect ChromeOS to follow the Linux path. If there hasn't been any > > complaint about building for ChromeOS previously then that may also be missing > > out on NEON (although it could equally be hard-coded always-on on a > per-platform > > basis). > > > > I don't know what GYP_DEFINE (or similar) is necessary to enable run-time > > detection. The original build-blocker, and the way I tested the function, was > > in one of the unit tests which calls WebRtc_GetCPUFeaturesARM() outside of a > > WEBRTC_DETECT_NEON guard (which makes sense in context; no need to fix that). > > I'm pretty sure there is no GYP_DEFINES setting that controls this. > > > If I find out what to do to Chromium to make sure it requests run-time NEON > > detection from WebRTC, then I can try to figure out what fits where. > > Sounds good, let's do that separate from this CL though. Johann: can you answer my comment at https://codereview.webrtc.org/1820133002/diff/20001/webrtc/system_wrappers/cp... ?
On 2016/04/05 09:27:51, kjellander (webrtc) wrote: > > I would have hoped so, but I can't seem to produce a link error in Chromium > > either with or without this function. This suggests that Chromium hard-codes > > WebRTC's NEON support always-on or always-off, with no run-time detection > under > > Linux. > > I remember that in the past CPU detection was enabled for libvpx in Chromium > since > they shipped to devices that doesn't have Neon. IIRC that was about to change > and it > might have now. > I've asked johannkoenig@ to comment on this (CC'ed). > > Let's not worry more about the Chromium case in this CL - that can always be > worked > on in the future. The important thing now it unblock this work. ChromeOS has never shipped a device without NEON. The only reason to hold on to CPU detection is for Android devices. Even there, I'd suggest gating the whole video thing on the existence of NEON such that if you have NEON it works, and if you don't, fall back to audio-only, possibly with video decoding but skip encoding entirely. The devices that lack NEON will probably fall down when trying to encode without it.
The CQ bit was checked by simon.hosie@arm.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1820133002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1820133002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/4617)
kjellander@webrtc.org changed reviewers: + henrikg@webrtc.org
+henrikg@ for system_wrappers OWNERS
Owners rs lgtm
The CQ bit was checked by simon.hosie@arm.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1820133002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1820133002/40001
Message was sent while issue was closed.
Description was changed from ========== Implement CPU feature detection for ARM Linux. BUG=webrtc:5057 NOTRY=True ========== to ========== Implement CPU feature detection for ARM Linux. BUG=webrtc:5057 NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Implement CPU feature detection for ARM Linux. BUG=webrtc:5057 NOTRY=True ========== to ========== Implement CPU feature detection for ARM Linux. BUG=webrtc:5057 NOTRY=True Committed: https://crrev.com/953b1c185fcb4d9a1da063ca70a79f14b88c47a3 Cr-Commit-Position: refs/heads/master@{#12270} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/953b1c185fcb4d9a1da063ca70a79f14b88c47a3 Cr-Commit-Position: refs/heads/master@{#12270} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
