Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1250663007:
Allow webrtc compilation with stlport (Closed)
|
Created:
5 years, 5 months ago by jdduke (slow) Modified:
5 years, 5 months ago CC:
webrtc-reviews_webrtc.org, ekm, turaj Base URL:
https://chromium.googlesource.com/external/webrtc/trunk/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAllow webrtc compilation with stlport
Android has not yet finalized its libc++ build. Allow compilation with
stlport by removing several C++11 library usages.
BUG=427718, 487341, webrtc:4866
R=andrew@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/7c5304c79151f092efcbef6680c5da366a930da2
Patch Set 1 #
Total comments: 3
Patch Set 2 : Code review #
Total comments: 5
Patch Set 3 : Fix only .data() #Messages
Total messages: 23 (6 generated)
jdduke@chromium.org changed reviewers: + andrew@webrtc.org
andrew@: We may need this for M45 on Android. I'm not sure what the proper workflow is for getting webrtc patches picked to release branches (let alone just landing a webrtc patch :), maybe you can point me in the right direction?
https://codereview.webrtc.org/1250663007/diff/1/modules/audio_processing/inte... File modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1250663007/diff/1/modules/audio_processing/inte... modules/audio_processing/intelligibility/intelligibility_enhancer.cc:394: result[i] = DotProduct(&(*filter_bank_[i].begin()), var, freqs_); If freqs_ can ever be zero then I'll need to change this.
https://codereview.webrtc.org/1250663007/diff/1/modules/audio_processing/inte... File modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1250663007/diff/1/modules/audio_processing/inte... modules/audio_processing/intelligibility/intelligibility_enhancer.cc:394: result[i] = DotProduct(&(*filter_bank_[i].begin()), var, freqs_); On 2015/07/21 21:23:13, jdduke wrote: > If freqs_ can ever be zero then I'll need to change this. I think &filter_bank_[i][0] or &filter_bank[i].front() is a bit clearer. I think we can safely expect freqs_ to be greater than zero. DCHECK that at the start of this function?
andrew@webrtc.org changed reviewers: + tnakamura@chromium.org - andrew@webrtc.org
On 2015/07/21 21:22:12, jdduke wrote: > andrew@: We may need this for M45 on Android. I'm not sure what the proper > workflow is for getting webrtc patches picked to release branches (let alone > just landing a webrtc patch :), maybe you can point me in the right direction? This has changed recently, and I'm not sure myself actually ;) There's a sites page here covering the procedure: https://sites.google.com/a/google.com/rtc-platform/engineering/merging-a-webr... I CC'd Ted who should be able to field any questions.
andrew@webrtc.org changed reviewers: + andrew@webrtc.org - tnakamura@chromium.org
On 2015/07/21 22:23:48, andrew wrote: > On 2015/07/21 21:22:12, jdduke wrote: > > andrew@: We may need this for M45 on Android. I'm not sure what the proper > > workflow is for getting webrtc patches picked to release branches (let alone > > just landing a webrtc patch :), maybe you can point me in the right direction? > > This has changed recently, and I'm not sure myself actually ;) > > There's a sites page here covering the procedure: > https://sites.google.com/a/google.com/rtc-platform/engineering/merging-a-webr... > > I CC'd Ted who should be able to field any questions. The page Andrew linked does describe how to merge a change from trunk to a release branch. Taking a few steps back, the overall process is described in https://sites.google.com/a/google.com/rtc-platform/releases/release-how-to#TO... Even if you aren't certain yet if you want to to merge this into M45, please aim to land this in trunk as soon as possible so that we can at least get this into a Canary channel build within the next few days. The process from landing in trunk WebRTC to Canary Chrome, while not complicated, can take one or more days, and Chrome TPMs won't approve any merge requests that haven't been picked up in at least one Canary channel build.
Thanks for the info. At this point we're really hoping to not need this for M45 (or ever), but per tnakamura's comment we probably ought to proceed anyway. https://codereview.webrtc.org/1250663007/diff/1/modules/audio_processing/inte... File modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1250663007/diff/1/modules/audio_processing/inte... modules/audio_processing/intelligibility/intelligibility_enhancer.cc:394: result[i] = DotProduct(&(*filter_bank_[i].begin()), var, freqs_); On 2015/07/21 22:20:12, andrew wrote: > On 2015/07/21 21:23:13, jdduke wrote: > > If freqs_ can ever be zero then I'll need to change this. > > I think > &filter_bank_[i][0] or &filter_bank[i].front() > is a bit clearer. > > I think we can safely expect freqs_ to be greater than zero. DCHECK that at the > start of this function? Done.
lgtm. You should be able to use the CQ.
The CQ bit was checked by andrew@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1250663007/20001
On 2015/07/22 18:47:32, andrew wrote: > lgtm. You should be able to use the CQ. I ticked the box actually (forgot I could do that :)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_clang/builds/6710) android_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_rel/builds/4765) ios on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios/builds/8655) ios64_sim on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim/builds/1226) ios_arm64 on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64/builds/3525) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/7267) linux on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux/builds/8689) mac on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac/builds/8825) mac_x64 on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64/builds/2436) mac_x64_gn on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn/builds/3174) win_x64_gn on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn/builds/3116)
Ah, I suppose you uploaded this from a Chromium checkout. I think the CQ only works today against a standalone webrtc checkout: https://chromium.googlesource.com/external/webrtc.git/+/master I can land this for you manually.
On 2015/07/22 19:08:10, andrew wrote: > Ah, I suppose you uploaded this from a Chromium checkout. I think the CQ only > works today against a standalone webrtc checkout: > https://chromium.googlesource.com/external/webrtc.git/+/master > > I can land this for you manually. That would be fantastic, thanks!
https://codereview.webrtc.org/1250663007/diff/20001/modules/audio_processing/... File modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1250663007/diff/20001/modules/audio_processing/... modules/audio_processing/intelligibility/intelligibility_utils.cc:35: return std::isfinite(c.real()) && std::isfinite(c.imag()); This is also C++11 standard library... https://codereview.webrtc.org/1250663007/diff/20001/modules/audio_processing/... modules/audio_processing/intelligibility/intelligibility_utils.cc:39: return isnormal(c.real()) && isnormal(c.imag()); Actually this is failing for me on Linux ("use of undeclared identifier 'isnormal'"). We could add a wrapper with hacky platform #ifdefs, but I think better to remove this entirely because it shouldn't really be needed. Filed a bug here: https://code.google.com/p/webrtc/issues/detail?id=4866 Could you revert the change to this file? I can at least land the data() fix.
https://codereview.webrtc.org/1250663007/diff/20001/modules/audio_processing/... File modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1250663007/diff/20001/modules/audio_processing/... modules/audio_processing/intelligibility/intelligibility_utils.cc:35: return std::isfinite(c.real()) && std::isfinite(c.imag()); On 2015/07/22 19:41:30, andrew wrote: > This is also C++11 standard library... Ah, the joys and quirks of stlport. They seem to have haphazardly included a number of C++11 library features which makes gleaning out the real offenses a bit trickier. https://codereview.webrtc.org/1250663007/diff/20001/modules/audio_processing/... modules/audio_processing/intelligibility/intelligibility_utils.cc:39: return isnormal(c.real()) && isnormal(c.imag()); On 2015/07/22 19:41:30, andrew wrote: > Actually this is failing for me on Linux ("use of undeclared identifier > 'isnormal'"). We could add a wrapper with hacky platform #ifdefs, but I think > better to remove this entirely because it shouldn't really be needed. > > Filed a bug here: > https://code.google.com/p/webrtc/issues/detail?id=4866 > > Could you revert the change to this file? I can at least land the data() fix. Done.
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 7c5304c79151f092efcbef6680c5da366a930da2 (presubmit successful).
Message was sent while issue was closed.
ekmeyerson@webrtc.org changed reviewers: + ekmeyerson@webrtc.org
Message was sent while issue was closed.
https://codereview.webrtc.org/1250663007/diff/20001/modules/audio_processing/... File modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1250663007/diff/20001/modules/audio_processing/... modules/audio_processing/intelligibility/intelligibility_utils.cc:51: if (cplxfinite(c) && !cplxnormal(c)) { The C++11 stuff is overkill. I think we can just change this to if (c.real() == 0.f || c.imag() == 0.f) { and get completely get rid of both definitions of cplxnormal and cplxfinite above. This still passes all tests, and we haven't documented any subnormal behavior here yet so we should be ok. You'd can also get rid of the EXPECTs that call those functions in intelligibility_utils_unittest.cc, which is the only other place these are called. |
