Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 6 months ago by Mostyn Bramley-Moore Modified:
5 years, 6 months ago Reviewers:
Andrew MacDonald, kjellander_webrtc, jridges, zhongwei CC:
aluebs-webrtc, bjornv1, tterriberry_mozilla.com, webrtc-reviews_webrtc.org Base URL:
https://chromium.googlesource.com/external/webrtc@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
Descriptionfix armv7 non-neon webrtc builds
Without this patch, chromium fails to build on armv7 targets
that lack NEON, with the following error:
../../third_party/webrtc/common_audio/resampler/sinc_resampler_neon.cc:22:70: error: no 'float webrtc::SincResampler::Convolve_NEON(const float*, const float*, const float*, double)' member function declared in class 'webrtc::SincResampler'
I suspect that this problem was unearthed by this previous CL:
https://webrtc-codereview.appspot.com/49309004
BUG=webrtc:4002
Patch Set 1 #Patch Set 2 : attempt to force arm_neon=1 for target arch arm64 #Patch Set 3 : try another gyp tweak (boy do i hate gyp) #Patch Set 4 : try setting arm_neon in supplement.gypi #Patch Set 5 : try nesting a bit #Patch Set 6 : try a dirty hack instead #Patch Set 7 : quoting fix #
Total comments: 10
Created: 5 years, 6 months ago
Messages
Total messages: 20 (4 generated)
mostynb@opera.com changed reviewers: + kjellander@webrtc.org
@kjellander: PTAL. I wonder if arm64 implies NEON is available? I'm not sure if this review is OK here, or if it should be submitted to https://webrtc-codereview.appspot.com/ - sorry if this is the wrong one.
kjellander@webrtc.org changed reviewers: + andrew@webrtc.org
lgtm, and yes: this is the right issue tracker (changed recently). I'd prefer if Andrew also had a look though, just to be sure.
Oh, I'll have to take that back, this does not lgtm in the currents state (looking at the tryjobs). Please have a look at the tryjob results from the bots I fired and fix that first.
Sure- I will try to fix this after lunch.
We have the same condition in other places, e.g.: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... so I'd like to understand why things fail in this particular case. https://codereview.webrtc.org/1187833002/diff/120001/webrtc/common_audio/comm... File webrtc/common_audio/common_audio.gyp (right): https://codereview.webrtc.org/1187833002/diff/120001/webrtc/common_audio/comm... webrtc/common_audio/common_audio.gyp:149: ['(arm_version>=7 and (arm_neon==1 or arm_neon_optional==1)) or target_arch=="arm64"', { This is inside a target_arch=="arm" block so is certainly not right.
andrew@webrtc.org changed reviewers: + zhongwei.yao@chromium.org
+Zhongwei, since he's been making changes around this. I have a quick example here of what we might want to do: https://codereview.webrtc.org/1181373004 https://codereview.webrtc.org/1187833002/diff/120001/webrtc/common_audio/BUIL... File webrtc/common_audio/BUILD.gn (right): https://codereview.webrtc.org/1187833002/diff/120001/webrtc/common_audio/BUIL... webrtc/common_audio/BUILD.gn:132: if (rtc_build_armv7_neon) { This shouldn't change anything; it amounts to the same condition: current_cpu == "arm" && arm_version >= 7 https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... https://codereview.webrtc.org/1187833002/diff/120001/webrtc/common_audio/comm... File webrtc/common_audio/common_audio.gyp (right): https://codereview.webrtc.org/1187833002/diff/120001/webrtc/common_audio/comm... webrtc/common_audio/common_audio.gyp:149: ['(arm_version>=7 and (arm_neon==1 or arm_neon_optional==1)) or target_arch=="arm64"', { On 2015/06/17 03:29:23, andrew wrote: > This is inside a target_arch=="arm" block so is certainly not right. I think this should just be 'arm_neon==1 or arm_neon_optional == 1', but we should update the other places we have neon targets as well. We should probably add a build_arm_neon flag in common.gypi to consolidate this.
https://codereview.webrtc.org/1187833002/diff/120001/webrtc/common_audio/BUIL... File webrtc/common_audio/BUILD.gn (right): https://codereview.webrtc.org/1187833002/diff/120001/webrtc/common_audio/BUIL... webrtc/common_audio/BUILD.gn:132: if (rtc_build_armv7_neon) { On 2015/06/17 05:36:32, andrew wrote: > This shouldn't change anything; it amounts to the same condition: > current_cpu == "arm" && arm_version >= 7 "arm_version>=7" is wrong because neon is optional in armv7. rtc_build_armv7_neon is better because it can be overridden independently of the arm_version argument. Though perhaps it should be renamed to rtc_build_arm_neon and the defaults improved a bit in webrtc.gni ? https://codereview.webrtc.org/1187833002/diff/120001/webrtc/common_audio/comm... File webrtc/common_audio/common_audio.gyp (right): https://codereview.webrtc.org/1187833002/diff/120001/webrtc/common_audio/comm... webrtc/common_audio/common_audio.gyp:149: ['(arm_version>=7 and (arm_neon==1 or arm_neon_optional==1)) or target_arch=="arm64"', { > I think this should just be 'arm_neon==1 or arm_neon_optional == 1', but we > should update the other places we have neon targets as well. I tried this earlier, but got lost in the nested gyp scopes when trying to set arm_neon to 1 whenever target_arch=="arm64". > We should probably add a build_arm_neon flag in common.gypi to consolidate this. Sounds good to me- I will experiment some more. https://codereview.webrtc.org/1187833002/diff/120001/webrtc/common_audio/comm... webrtc/common_audio/common_audio.gyp:149: ['(arm_version>=7 and (arm_neon==1 or arm_neon_optional==1)) or target_arch=="arm64"', { On 2015/06/17 03:29:23, andrew wrote: > This is inside a target_arch=="arm" block so is certainly not right. I don't think it's incorrect, but this is definitely not a good/final solution.
On 2015/06/17 03:29:24, andrew wrote: > We have the same condition in other places, e.g.: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > so I'd like to understand why things fail in this particular case. The case you referred to just defines the ninja target, but that doesn't mean the target will be pulled in via the dependency tree. If it is pulled in, or built explicitly with a non-neon armv7 target toolchain, then it would fail to build.
On 2015/06/17 08:07:55, Mostyn Bramley-Moore wrote: > On 2015/06/17 03:29:24, andrew wrote: > > We have the same condition in other places, e.g.: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > > > so I'd like to understand why things fail in this particular case. > > The case you referred to just defines the ninja target, but that doesn't mean > the target will be pulled in via the dependency tree. If it is pulled in, or > built explicitly with a non-neon armv7 target toolchain, then it would fail to > build. @Andrew, I have another question: when I check the buildbot log (http://build.chromium.org/p/tryserver.webrtc/builders/android/builds/6329/ste...), I can't find such arm_neon=1 in GYP_DEFINES. Am I missing something here? @Mostyn, have you tried build with arm_neon_optional=1 switch? By the way, arm64 always has neon. I think the reason of the failure on lack neon ARMv7 platform is we have not distinguished 2 following cases: -. arm_version >= 7 -. arm_neon = 1 or arm_neon_optional=1 e.g. in this case: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... I think it should be changed to: 'conditions': [ ['arm_version>=7', { 'sources': [ 'signal_processing/filter_ar_fast_q12_armv7.S', ], 'sources!': [ 'signal_processing/filter_ar_fast_q12.c', ], }], ['arm_neon==1 or arm_neon_optional==1', { 'dependencies': ['common_audio_neon',], }], ], # conditions (haven't tested it yet) in_reply_to: 5709068098338816 send_mail: 1 subject: fix armv7 non-neon webrtc builds
https://codereview.webrtc.org/1187833002/diff/120001/webrtc/common_audio/comm... File webrtc/common_audio/common_audio.gyp (right): https://codereview.webrtc.org/1187833002/diff/120001/webrtc/common_audio/comm... webrtc/common_audio/common_audio.gyp:149: ['(arm_version>=7 and (arm_neon==1 or arm_neon_optional==1)) or target_arch=="arm64"', { target_arch=="arm64" is not needed here. arm64 is handled at line 160.
jridges@masque.com changed reviewers: + jridges@masque.com
https://codereview.webrtc.org/1187833002/diff/120001/webrtc/common_audio/BUIL... File webrtc/common_audio/BUILD.gn (right): https://codereview.webrtc.org/1187833002/diff/120001/webrtc/common_audio/BUIL... webrtc/common_audio/BUILD.gn:132: if (rtc_build_armv7_neon) { On 2015/06/17 07:42:26, Mostyn Bramley-Moore wrote: > On 2015/06/17 05:36:32, andrew wrote: > > This shouldn't change anything; it amounts to the same condition: > > current_cpu == "arm" && arm_version >= 7 > > "arm_version>=7" is wrong because neon is optional in armv7. > rtc_build_armv7_neon is better because it can be overridden independently of the > arm_version argument. Though perhaps it should be renamed to rtc_build_arm_neon > and the defaults improved a bit in webrtc.gni ? This clause is doing two independent things which should be separated into two clauses. "arm_version >= 7" is correct for choosing which filter_ar_fast_q12 source to use, but something like "rtc_build_armv7_neon" is correct for setting the deps. https://codereview.webrtc.org/1187833002/diff/120001/webrtc/common_audio/comm... File webrtc/common_audio/common_audio.gyp (right): https://codereview.webrtc.org/1187833002/diff/120001/webrtc/common_audio/comm... webrtc/common_audio/common_audio.gyp:149: ['(arm_version>=7 and (arm_neon==1 or arm_neon_optional==1)) or target_arch=="arm64"', { On 2015/06/17 07:42:26, Mostyn Bramley-Moore wrote: > On 2015/06/17 03:29:23, andrew wrote: > > This is inside a target_arch=="arm" block so is certainly not right. > > I don't think it's incorrect, but this is definitely not a good/final solution. There is the same problem here with ARMv7 being confused with NEON. The "filter_ar_fast_q12" module has nothing to do with NEON. https://codereview.webrtc.org/1187833002/diff/120001/webrtc/common_audio/comm... webrtc/common_audio/common_audio.gyp:215: ['(target_arch=="arm" and arm_version>=7 and (arm_neon==1 or arm_neon_optional==1)) or target_arch=="arm64"', { Is there a definitive answer as to when the arm_neon and arm_neon_optional variables get set? This seems like overkill to have to include the architecture here as well.
This CL is partially obsoleted by https://codereview.webrtc.org/1181373004 - I will wait for that to land before reworking this...
On 2015/06/17 21:19:01, Mostyn Bramley-Moore wrote: > This CL is partially obsoleted by https://codereview.webrtc.org/1181373004 - I > will wait for that to land before reworking this... I _think_ it should be fully obsoleted ;) Am I missing anything from the other one?
> I _think_ it should be fully obsoleted ;) > > Am I missing anything from the other one? I didn't notice that https://codereview.webrtc.org/1181373004 had been updated- it looks sufficient now. I will try to confirm that tomorrow morning.
On 2015/06/17 21:56:24, Mostyn Bramley-Moore wrote: > > I _think_ it should be fully obsoleted ;) > > > > Am I missing anything from the other one? > > I didn't notice that https://codereview.webrtc.org/1181373004 had been updated- > it looks sufficient now. I will try to confirm that tomorrow morning. Great, thanks.
On 2015/06/17 21:59:25, andrew wrote: > On 2015/06/17 21:56:24, Mostyn Bramley-Moore wrote: > > > I _think_ it should be fully obsoleted ;) > > > > > > Am I missing anything from the other one? > > > > I didn't notice that https://codereview.webrtc.org/1181373004 had been > updated- > > it looks sufficient now. I will try to confirm that tomorrow morning. > > Great, thanks. Patchset 4 of https://codereview.webrtc.org/1181373004 WFM - closing this issue. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
