Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1412653006:
Fix Visual Studio 2015 WebRtc x86 build. (Closed)
|
Created:
5 years, 1 month ago by GeneralFault Modified:
4 years, 10 months ago CC:
aluebs-webrtc, andresp, Andrew MacDonald, bjornv1, kwiberg-webrtc, mflodman, niklas.enbom, peah-webrtc, perkj_webrtc, qiang.lu, the sun, stefan-webrtc, tterriberry_mozilla.com, webrtc-reviews_webrtc.org, yujie_mao (webrtc) Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix WebRtc ninja x86 build using Visual Studio 2015 (set GYP_MSVS_VERSION=2015).
Visual Studio 2015 balks at the implicit truncation of values. Easily fixed with an explicit cast.
Fixed redefinition of CLOCKS_PER_SEC when using Visual Studio 2015 and the Windows 10 SDK. CLOCKS_PER_SEC is also defined in "<WIN10 SDK DIR>\include\10.0.10240.0\ucrt\time.h" and also has the value of 1000
Hiding snprintf definition if building with Visual Studio 2015
Fixed C4573 compiler complaint in audio_processing_impl_locking_unittest.cc.
BUG=webrtc:5183
Committed: https://crrev.com/3f70562bbbc217d292c745143b07f9139a6a6f0b
Cr-Commit-Position: refs/heads/master@{#11434}
Patch Set 1 #Patch Set 2 : Fix the Visual Studio 2015 build #
Total comments: 1
Patch Set 3 : convention compliance #
Total comments: 4
Patch Set 4 : PTAL #Patch Set 5 : Another line-ending fix #
Total comments: 1
Patch Set 6 : PTAL #Patch Set 7 : Added comments for VS2015 compiler bug. #
Messages
Total messages: 38 (11 generated)
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.webrtc.org/1412363004/ by conceptgenesis@gmail.com. The reason for reverting is: Must have messed up the line endings. Will redo the changelist..
The purpose of this change is to allow the use of the WebRtc native API in a Visual Studio 2015 project. There were a few minor issues, but all were addressed in a way that I think were in line with the accepted fixes for issue chromium:440500. This is my first commit to any Google project, so please forgive any errors in procedure.
BUG=webrtc:5183
On 2015/11/08 07:33:00, GeneralFault wrote: > A revert of this CL (patchset #1 id:1) has been created in > https://codereview.webrtc.org/1412363004/ by mailto:conceptgenesis@gmail.com. > > The reason for reverting is: Must have messed up the line endings. Will redo the > changelist.. FYI - reverts only work for changelists that have been committed. If you're working on this CL and need to make further improvements, you can just upload a new patch set to the same CL.
On 2015/11/08 at 09:01:22, tommi wrote: > On 2015/11/08 07:33:00, GeneralFault wrote: > > A revert of this CL (patchset #1 id:1) has been created in > > https://codereview.webrtc.org/1412363004/ by mailto:conceptgenesis@gmail.com. > > > > The reason for reverting is: Must have messed up the line endings. Will redo the > > changelist.. > > FYI - reverts only work for changelists that have been committed. If you're working on this CL and need to make further improvements, you can just upload a new patch set to the same CL. Thanks tommi. Suggestion appreciated. This is my first time working with changelists in git. Is it better to submit each file within an OWNERS scope as its own changelist?
Description was changed from ========== With these changes, WebRtc x86 will build Visual Studio 2015. Fixed line endings. Visual Studio 2015 balks at the implicit truncation of values. Easily fixed with an implicit cast. Fixed redefinition of CLOCKS_PER_SEC when using Visual Studio 2015 and the Windows 10 SDK. CLOCKS_PER_SEC is also defined in "<WIN10 SDK DIR>\include\10.0.10240.0\ucrt\time.h" and also has the value of 1000 Hiding snprintf definition if building with Visual Studio 2015 BUG= ========== to ========== With these changes, WebRtc x86 will build Visual Studio 2015. Fixed line endings. Visual Studio 2015 balks at the implicit truncation of values. Easily fixed with an implicit cast. Fixed redefinition of CLOCKS_PER_SEC when using Visual Studio 2015 and the Windows 10 SDK. CLOCKS_PER_SEC is also defined in "<WIN10 SDK DIR>\include\10.0.10240.0\ucrt\time.h" and also has the value of 1000 Hiding snprintf definition if building with Visual Studio 2015 BUG=webrtc:5183 ==========
Added reviewers.
conceptgenesis@gmail.com changed reviewers: + henrik.lundin@webrtc.org, mflodman@webrtc.org, tina.legrand@webrtc.org, tommi@webrtc.org
I'm trying to get the hang of this codereview system. I apologize if anybody is getting spammed with these comments. Added reviewers.
https://codereview.webrtc.org/1412653006/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/transient/file_utils_unittest.cc (right): https://codereview.webrtc.org/1412653006/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/transient/file_utils_unittest.cc:368: written_buffer[0] = (float)kPi; Please, use static_cast<float)>(kPi) and so on.
Description was changed from ========== With these changes, WebRtc x86 will build Visual Studio 2015. Fixed line endings. Visual Studio 2015 balks at the implicit truncation of values. Easily fixed with an implicit cast. Fixed redefinition of CLOCKS_PER_SEC when using Visual Studio 2015 and the Windows 10 SDK. CLOCKS_PER_SEC is also defined in "<WIN10 SDK DIR>\include\10.0.10240.0\ucrt\time.h" and also has the value of 1000 Hiding snprintf definition if building with Visual Studio 2015 BUG=webrtc:5183 ========== to ========== With these changes, WebRtc x86 will build Visual Studio 2015. Fixed line endings. Visual Studio 2015 balks at the implicit truncation of values. Easily fixed with an explicit cast. Fixed redefinition of CLOCKS_PER_SEC when using Visual Studio 2015 and the Windows 10 SDK. CLOCKS_PER_SEC is also defined in "<WIN10 SDK DIR>\include\10.0.10240.0\ucrt\time.h" and also has the value of 1000 Hiding snprintf definition if building with Visual Studio 2015 BUG=webrtc:5183 ==========
On 2015/11/11 at 15:59:38, henrik.lundin wrote: > https://codereview.webrtc.org/1412653006/diff/20001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/transient/file_utils_unittest.cc (right): > > https://codereview.webrtc.org/1412653006/diff/20001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/transient/file_utils_unittest.cc:368: written_buffer[0] = (float)kPi; > Please, use static_cast<float)>(kPi) and so on. I've switched to the safer (and approved) static_cast as requested. Is there a next step that I need to do to get these changes reviewed and hopefully committed?
On 2015/12/29 18:02:56, GeneralFault wrote: > On 2015/11/11 at 15:59:38, henrik.lundin wrote: > > > https://codereview.webrtc.org/1412653006/diff/20001/webrtc/modules/audio_proc... > > File webrtc/modules/audio_processing/transient/file_utils_unittest.cc (right): > > > > > https://codereview.webrtc.org/1412653006/diff/20001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/transient/file_utils_unittest.cc:368: > written_buffer[0] = (float)kPi; > > Please, use static_cast<float)>(kPi) and so on. > > I've switched to the safer (and approved) static_cast as requested. Is there a > next step that I need to do to get these changes reviewed and hopefully > committed? webrtc/modules: LGTM You need approval for the rest of the files (mflodman and tommi should be able to cover that). When that is done, a project committer will have to hit the Commit checkbox for you. For reference, the CLA was signed by conceptgenesis@gmail.com on Nov 7, 2015, so all is green there.
lgtm
LGTM for payload_router_unittest.cc
Thanks for fixing this! A few things you need to fix before it's ready to commit. https://codereview.webrtc.org/1412653006/diff/40001/AUTHORS File AUTHORS (right): https://codereview.webrtc.org/1412653006/diff/40001/AUTHORS#newcode34 AUTHORS:34: Cody Barnes <conceptgenesis@gmail.com> The list is in alphabetic order. Move your name to below "Christphe Dumez". https://codereview.webrtc.org/1412653006/diff/40001/webrtc/video_engine/paylo... File webrtc/video_engine/payload_router_unittest.cc (right): https://codereview.webrtc.org/1412653006/diff/40001/webrtc/video_engine/paylo... webrtc/video_engine/payload_router_unittest.cc:144: const uint16_t kDefaultMaxLength = 1500 - 20 - 8 - 12 - 4; Don't change the type here. https://codereview.webrtc.org/1412653006/diff/40001/webrtc/video_engine/paylo... webrtc/video_engine/payload_router_unittest.cc:165: const uint16_t kTestMinPayloadLength = 1001; Same here, keep size_t.
https://codereview.webrtc.org/1412653006/diff/40001/webrtc/video_engine/paylo... File webrtc/video_engine/payload_router_unittest.cc (right): https://codereview.webrtc.org/1412653006/diff/40001/webrtc/video_engine/paylo... webrtc/video_engine/payload_router_unittest.cc:144: const uint16_t kDefaultMaxLength = 1500 - 20 - 8 - 12 - 4; On 2016/01/21 14:54:06, tlegrand-webrtc wrote: > Don't change the type here. Good catch! I had a too old local checkout when I verified this, so I agree this be size_t if you rebase your checkout.
On 2016/01/22 at 08:33:04, mflodman wrote: > https://codereview.webrtc.org/1412653006/diff/40001/webrtc/video_engine/paylo... > File webrtc/video_engine/payload_router_unittest.cc (right): > > https://codereview.webrtc.org/1412653006/diff/40001/webrtc/video_engine/paylo... > webrtc/video_engine/payload_router_unittest.cc:144: const uint16_t kDefaultMaxLength = 1500 - 20 - 8 - 12 - 4; > On 2016/01/21 14:54:06, tlegrand-webrtc wrote: > > Don't change the type here. > > Good catch! I had a too old local checkout when I verified this, so I agree this be size_t if you rebase your checkout. Thanks Tina. I've restored the size_t types in payload_unittest and confirmed that it complies fine. But to be honest, I'm not sure why it works. The RtpRtcp.MaxDataPayloadLength (and the related mock) still returns a uint16_t. So the EXPECT_CALL return value on those must be truncated from a size_t. I was under the impression that would throw a warning. No need to dig into that for me. If I get curious, I'll look further. Mr. Alexander Brauckmann appears to have the same "alphabetical blindness" that I suffer from. So I've re-ordered both of our names in the AUTHORS file. With the latest source, I ran into another compiler error in webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc. Wasn't sure if I should add it to this CL or create a new one just for it. Looking for guidance for future commits. I've documented the problem in the notes for https://bugs.chromium.org/p/webrtc/issues/detail?id=5183. Comes down to a rename of a variable with what should be an overriding scope to compensate for what appears to be a compiler bug.
Description was changed from ========== With these changes, WebRtc x86 will build Visual Studio 2015. Fixed line endings. Visual Studio 2015 balks at the implicit truncation of values. Easily fixed with an explicit cast. Fixed redefinition of CLOCKS_PER_SEC when using Visual Studio 2015 and the Windows 10 SDK. CLOCKS_PER_SEC is also defined in "<WIN10 SDK DIR>\include\10.0.10240.0\ucrt\time.h" and also has the value of 1000 Hiding snprintf definition if building with Visual Studio 2015 BUG=webrtc:5183 ========== to ========== Fix WebRtc ninja x86 build using Visual Studio 2015 (set GYP_MSVS_VERSION=2015). Visual Studio 2015 balks at the implicit truncation of values. Easily fixed with an explicit cast. Fixed redefinition of CLOCKS_PER_SEC when using Visual Studio 2015 and the Windows 10 SDK. CLOCKS_PER_SEC is also defined in "<WIN10 SDK DIR>\include\10.0.10240.0\ucrt\time.h" and also has the value of 1000 Hiding snprintf definition if building with Visual Studio 2015 Fixed C4573 compiler complaint in audio_processing_impl_locking_unittest.cc. BUG=webrtc:5183 ==========
On 2016/01/27 07:24:29, GeneralFault wrote: > On 2016/01/22 at 08:33:04, mflodman wrote: > > > https://codereview.webrtc.org/1412653006/diff/40001/webrtc/video_engine/paylo... > > File webrtc/video_engine/payload_router_unittest.cc (right): > > > > > https://codereview.webrtc.org/1412653006/diff/40001/webrtc/video_engine/paylo... > > webrtc/video_engine/payload_router_unittest.cc:144: const uint16_t > kDefaultMaxLength = 1500 - 20 - 8 - 12 - 4; > > On 2016/01/21 14:54:06, tlegrand-webrtc wrote: > > > Don't change the type here. > > > > Good catch! I had a too old local checkout when I verified this, so I agree > this be size_t if you rebase your checkout. > > Thanks Tina. I've restored the size_t types in payload_unittest and confirmed > that it complies fine. But to be honest, I'm not sure why it works. The > RtpRtcp.MaxDataPayloadLength (and the related mock) still returns a uint16_t. So > the EXPECT_CALL return value on those must be truncated from a size_t. I was > under the impression that would throw a warning. No need to dig into that for > me. If I get curious, I'll look further. You're right, RtpRtcp.MaxDataPayloadLength still returns uint16_t. Not sure why the compiler doesn't complain, but I'm fine with keeping size_t in the test. > > Mr. Alexander Brauckmann appears to have the same "alphabetical blindness" that > I suffer from. So I've re-ordered both of our names in the AUTHORS file. Perfect, thanks! > > With the latest source, I ran into another compiler error in > webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc. > Wasn't sure if I should add it to this CL or create a new one just for it. > Looking for guidance for future commits. I've documented the problem in the > notes for https://bugs.chromium.org/p/webrtc/issues/detail?id=5183. Comes down > to a rename of a variable with what should be an overriding scope to compensate > for what appears to be a compiler bug.
Regarding the new compiler error, I'm worried we'll not be able to make sure we're not introducing new breakages. I'm fine with the change, though, but please rename the variable. LGTM https://codereview.webrtc.org/1412653006/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): https://codereview.webrtc.org/1412653006/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:202: test_config.aec_type = aec_type_i; Is this needed because TestConfig has a "member" with the same name? If this is a compiler error for VS 2015, how do we prevent new breakages? Would be good to add a comment here, and I'd prefer to change "aec_type_i" to just "type" instead. What do you think?
On 2016/01/27 at 12:39:43, tina.legrand wrote: > Regarding the new compiler error, I'm worried we'll not be able to make sure we're not introducing new breakages. > I'm fine with the change, though, but please rename the variable. Fortunately any breakages would be in a test rather than production code. I think it's just a matter of the compiler failing to find the overridden name when testing closures. > > LGTM > > https://codereview.webrtc.org/1412653006/diff/80001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): > > https://codereview.webrtc.org/1412653006/diff/80001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:202: test_config.aec_type = aec_type_i; > Is this needed because TestConfig has a "member" with the same name? If this is a compiler error for VS 2015, how do we prevent new breakages? Would be good to add a comment here, and I'd prefer to change "aec_type_i" to just "type" instead. What do you think? It appears so. The only other related information I could find was in reference to an old gcc bug with the same behavior. I've since lost the link but I'll post it here again if I find it. I'd like to either find a known issue for the VS2015 compiler or submit one to MS before I commit a comment with poorly understood information. Besides, I forgot to add it before checking in... but I'll see if I can get some better information for a comment. Name changed as suggested.
On 2016/01/28 at 01:11:45, GeneralFault wrote: > On 2016/01/27 at 12:39:43, tina.legrand wrote: > > Regarding the new compiler error, I'm worried we'll not be able to make sure we're not introducing new breakages. > > I'm fine with the change, though, but please rename the variable. > Fortunately any breakages would be in a test rather than production code. I think it's just a matter of the compiler failing to find the overridden name when testing closures. > > > > LGTM > > > > https://codereview.webrtc.org/1412653006/diff/80001/webrtc/modules/audio_proc... > > File webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc (right): > > > > https://codereview.webrtc.org/1412653006/diff/80001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:202: test_config.aec_type = aec_type_i; > > Is this needed because TestConfig has a "member" with the same name? If this is a compiler error for VS 2015, how do we prevent new breakages? Would be good to add a comment here, and I'd prefer to change "aec_type_i" to just "type" instead. What do you think? > > It appears so. The only other related information I could find was in reference to an old gcc bug with the same behavior. I've since lost the link but I'll post it here again if I find it. I'd like to either find a known issue for the VS2015 compiler or submit one to MS before I commit a comment with poorly understood information. Besides, I forgot to add it before checking in... but I'll see if I can get some better information for a comment. > Name changed as suggested. Ended up submitting an issue to Microsoft regarding compiler error. https://connect.microsoft.com/VisualStudio/feedback/details/2291755 I was able to reproduce it by attempting to compile the following code: struct MyStuct { int namedmember = 1; static void SomeMethod() { auto action = []() { int lst[] = { 1,2,3 }; for (auto namedmember : lst) { //error C4573: the usage of 'MyStuct::namedmember' requires the compiler to capture 'this' but the current default capture mode does not allow it namedmember += 1; } }; } };
On 2016/01/28 02:03:09, GeneralFault wrote: > On 2016/01/28 at 01:11:45, GeneralFault wrote: > > On 2016/01/27 at 12:39:43, tina.legrand wrote: > > > Regarding the new compiler error, I'm worried we'll not be able to make sure > we're not introducing new breakages. > > > I'm fine with the change, though, but please rename the variable. > > Fortunately any breakages would be in a test rather than production code. I > think it's just a matter of the compiler failing to find the overridden name > when testing closures. > > > > > > LGTM > > > > > > > https://codereview.webrtc.org/1412653006/diff/80001/webrtc/modules/audio_proc... > > > File > webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc > (right): > > > > > > > https://codereview.webrtc.org/1412653006/diff/80001/webrtc/modules/audio_proc... > > > > webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc:202: > test_config.aec_type = aec_type_i; > > > Is this needed because TestConfig has a "member" with the same name? If this > is a compiler error for VS 2015, how do we prevent new breakages? Would be good > to add a comment here, and I'd prefer to change "aec_type_i" to just "type" > instead. What do you think? > > > > It appears so. The only other related information I could find was in > reference to an old gcc bug with the same behavior. I've since lost the link but > I'll post it here again if I find it. I'd like to either find a known issue for > the VS2015 compiler or submit one to MS before I commit a comment with poorly > understood information. Besides, I forgot to add it before checking in... but > I'll see if I can get some better information for a comment. > > Name changed as suggested. > > Ended up submitting an issue to Microsoft regarding compiler error. > https://connect.microsoft.com/VisualStudio/feedback/details/2291755 Great! Thanks for adding the comment too. > > I was able to reproduce it by attempting to compile the following code: > > struct MyStuct { > int namedmember = 1; > > static void SomeMethod() { > auto action = []() { > int lst[] = { 1,2,3 }; > for (auto namedmember : lst) { //error C4573: the usage of > 'MyStuct::namedmember' requires the compiler to capture 'this' but the current > default capture mode does not allow it > namedmember += 1; > } > }; > } > };
The CQ bit was checked by conceptgenesis@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org, henrik.lundin@webrtc.org, mflodman@webrtc.org, tina.legrand@webrtc.org Link to the patchset: https://codereview.webrtc.org/1412653006/#ps120001 (title: "Added comments for VS2015 compiler bug.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412653006/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412653006/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by conceptgenesis@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412653006/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412653006/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by conceptgenesis@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412653006/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412653006/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Fix WebRtc ninja x86 build using Visual Studio 2015 (set GYP_MSVS_VERSION=2015). Visual Studio 2015 balks at the implicit truncation of values. Easily fixed with an explicit cast. Fixed redefinition of CLOCKS_PER_SEC when using Visual Studio 2015 and the Windows 10 SDK. CLOCKS_PER_SEC is also defined in "<WIN10 SDK DIR>\include\10.0.10240.0\ucrt\time.h" and also has the value of 1000 Hiding snprintf definition if building with Visual Studio 2015 Fixed C4573 compiler complaint in audio_processing_impl_locking_unittest.cc. BUG=webrtc:5183 ========== to ========== Fix WebRtc ninja x86 build using Visual Studio 2015 (set GYP_MSVS_VERSION=2015). Visual Studio 2015 balks at the implicit truncation of values. Easily fixed with an explicit cast. Fixed redefinition of CLOCKS_PER_SEC when using Visual Studio 2015 and the Windows 10 SDK. CLOCKS_PER_SEC is also defined in "<WIN10 SDK DIR>\include\10.0.10240.0\ucrt\time.h" and also has the value of 1000 Hiding snprintf definition if building with Visual Studio 2015 Fixed C4573 compiler complaint in audio_processing_impl_locking_unittest.cc. BUG=webrtc:5183 Committed: https://crrev.com/3f70562bbbc217d292c745143b07f9139a6a6f0b Cr-Commit-Position: refs/heads/master@{#11434} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/3f70562bbbc217d292c745143b07f9139a6a6f0b Cr-Commit-Position: refs/heads/master@{#11434} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
