Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2341433004:
Fix undefined reference to log2 on android (Closed)
|
Created:
4 years, 3 months ago by kthelgason Modified:
4 years, 3 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, stefan-webrtc, mflodman Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix undefined reference to log2 on android
R=nisse@webrtc.org
TBR=sakal@webrtc.org, sprang@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/5a20ed36e654a8721f4e67743e2a75f99c52a7a1
Patch Set 1 #
Total comments: 4
Patch Set 2 : Clarify comment #Messages
Total messages: 16 (8 generated)
nisse@webrtc.org changed reviewers: + nisse@webrtc.org
https://codereview.webrtc.org/2341433004/diff/1/webrtc/modules/video_coding/u... File webrtc/modules/video_coding/utility/quality_scaler.cc (right): https://codereview.webrtc.org/2341433004/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/quality_scaler.cc:13: #include <math.h> Why switching from cmath to math.h? Just to drop the std:: prefix? https://codereview.webrtc.org/2341433004/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/quality_scaler.cc:17: // Android does not support log2 [citation needed] Is there any documentation or bug report about this? Both my local man-page and http://en.cppreference.com/w/c/numeric/math/log2 claims that log2 is part of c99, which ought to be supported by Android. Wild guess, but have you checked if only changing to math.h solves the problem? I'm thinking, maybe c99 log2 works, but c++ std::log2 doesn't? They might correspond to different symbols when linking.
https://codereview.webrtc.org/2341433004/diff/1/webrtc/modules/video_coding/u... File webrtc/modules/video_coding/utility/quality_scaler.cc (right): https://codereview.webrtc.org/2341433004/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/quality_scaler.cc:13: #include <math.h> On 2016/09/15 08:22:12, nisse-webrtc wrote: > Why switching from cmath to math.h? Just to drop the std:: prefix? Yep. https://codereview.webrtc.org/2341433004/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/quality_scaler.cc:17: // Android does not support log2 On 2016/09/15 08:22:12, nisse-webrtc wrote: > [citation needed] > > Is there any documentation or bug report about this? > > Both my local man-page and http://en.cppreference.com/w/c/numeric/math/log2 > claims that log2 is part of c99, which ought to be supported by Android. I found some references to this on the internet, including: https://code.google.com/p/android/issues/detail?id=212634 > Wild guess, but have you checked if only changing to math.h solves the problem? > I'm thinking, maybe c99 log2 works, but c++ std::log2 doesn't? They might > correspond to different symbols when linking. I had the same hunch, unfortunately that didn't work when I tried it.
On 2016/09/15 08:25:15, kthelgason wrote: > I found some references to this on the internet, including: > https://code.google.com/p/android/issues/detail?id=212634 Good, then at least it's a known issue. I think you should add a TODO spelling out the conditions under which this hack can be removed, and link to that bug. So LGTM, if this android-workaround is properly documented. BTW, pbos is not currently working on webrtc, sprang may be a better choice of reviewer.
Description was changed from ========== Fix undefined reference to log2 on android TBR=sakal@webrtc.org,pbos@webrtc.org ========== to ========== Fix undefined reference to log2 on android TBR=sakal@webrtc.org,sprang@webrtc.org ==========
kthelgason@webrtc.org changed reviewers: + sprang@webrtc.org - pbos@webrtc.org
The CQ bit was checked by kthelgason@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org Link to the patchset: https://codereview.webrtc.org/2341433004/#ps20001 (title: "Clarify comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/10602)
Message was sent while issue was closed.
Description was changed from ========== Fix undefined reference to log2 on android TBR=sakal@webrtc.org,sprang@webrtc.org ========== to ========== Fix undefined reference to log2 on android R=nisse@webrtc.org TBR=sakal@webrtc.org, sprang@webrtc.org Committed: https://crrev.com/5a20ed36e654a8721f4e67743e2a75f99c52a7a1 Cr-Commit-Position: refs/heads/master@{#14225} ==========
Message was sent while issue was closed.
Description was changed from ========== Fix undefined reference to log2 on android R=nisse@webrtc.org TBR=sakal@webrtc.org, sprang@webrtc.org Committed: https://crrev.com/5a20ed36e654a8721f4e67743e2a75f99c52a7a1 Cr-Commit-Position: refs/heads/master@{#14225} ========== to ========== Fix undefined reference to log2 on android R=nisse@webrtc.org TBR=sakal@webrtc.org, sprang@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/5a20ed36e654a8721f4e67743... ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5a20ed36e654a8721f4e67743e2a75f99c52a7a1 Cr-Commit-Position: refs/heads/master@{#14225}
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as 5a20ed36e654a8721f4e67743e2a75f99c52a7a1 (presubmit successful). |
