Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2236473002:
Add video_loopback to gn. (Closed)
|
Created:
4 years, 4 months ago by stefan-webrtc Modified:
4 years, 4 months ago Reviewers:
kjellander_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd video_loopback to gn.
BUG=webrtc:5949
NOTRY=True
Committed: https://crrev.com/3ab6614d101dc6e8ff8c558bd966f9f3f05e87df
Cr-Commit-Position: refs/heads/master@{#13748}
Patch Set 1 #
Total comments: 11
Patch Set 2 : . #
Total comments: 2
Patch Set 3 : Remove dup file. #
Messages
Total messages: 16 (5 generated)
stefan@webrtc.org changed reviewers: + kjellander@webrtc.org
https://codereview.webrtc.org/2236473002/diff/1/webrtc/BUILD.gn File webrtc/BUILD.gn (right): https://codereview.webrtc.org/2236473002/diff/1/webrtc/BUILD.gn#newcode300 webrtc/BUILD.gn:300: executable("video_loopback") { Put this new target inside the existing if (!build_with_chromium) condition above instead of a new one. https://codereview.webrtc.org/2236473002/diff/1/webrtc/BUILD.gn#newcode310 webrtc/BUILD.gn:310: sources -= [ "test/run_test.cc" ] GN disencourages use of negative source entries. Can you just flip it around so that: if (is_mac) { sources += [ "test/mac/run_test.mm" ] } else { sources += [ "test/run_test.cc" ] } and then don't include "test/mac/run_test.mm", in the original sources entry above. Please make similar GYP changes too, to avoid diverging. https://codereview.webrtc.org/2236473002/diff/1/webrtc/BUILD.gn#newcode314 webrtc/BUILD.gn:314: ":webrtc", Is there a way to not depend on the monolith target "webrtc" ? I'm trying to get rid of that long-term and it can usually be replaced by other targets. https://codereview.webrtc.org/2236473002/diff/1/webrtc/BUILD.gn#newcode315 webrtc/BUILD.gn:315: "modules/video_capture:video_capture_internal_impl", Please add the added dependencies to GYP as well to avoid diverging (I guess they're implicitly pulled in). https://codereview.webrtc.org/2236473002/diff/1/webrtc/BUILD.gn#newcode327 webrtc/BUILD.gn:327: configs -= [ "//build/config/clang:find_bad_constructs" ] Could you try fixing these warnings instead of having to suppress them? It's OK to keep it if it's a lot of work but sometimes it's just a few code changes.
https://codereview.webrtc.org/2236473002/diff/1/webrtc/BUILD.gn File webrtc/BUILD.gn (right): https://codereview.webrtc.org/2236473002/diff/1/webrtc/BUILD.gn#newcode300 webrtc/BUILD.gn:300: executable("video_loopback") { On 2016/08/10 14:46:50, kjellander_webrtc wrote: > Put this new target inside the existing if (!build_with_chromium) condition > above instead of a new one. Done. https://codereview.webrtc.org/2236473002/diff/1/webrtc/BUILD.gn#newcode310 webrtc/BUILD.gn:310: sources -= [ "test/run_test.cc" ] On 2016/08/10 14:46:49, kjellander_webrtc wrote: > GN disencourages use of negative source entries. Can you just flip it around so > that: > > if (is_mac) { > sources += [ "test/mac/run_test.mm" ] > } else { > sources += [ "test/run_test.cc" ] > } > > and then don't include "test/mac/run_test.mm", in the original sources entry > above. > Please make similar GYP changes too, to avoid diverging. Done. https://codereview.webrtc.org/2236473002/diff/1/webrtc/BUILD.gn#newcode314 webrtc/BUILD.gn:314: ":webrtc", On 2016/08/10 14:46:50, kjellander_webrtc wrote: > Is there a way to not depend on the monolith target "webrtc" ? I'm trying to get > rid of that long-term and it can usually be replaced by other targets. Changed to call:call https://codereview.webrtc.org/2236473002/diff/1/webrtc/BUILD.gn#newcode315 webrtc/BUILD.gn:315: "modules/video_capture:video_capture_internal_impl", On 2016/08/10 14:46:49, kjellander_webrtc wrote: > Please add the added dependencies to GYP as well to avoid diverging (I guess > they're implicitly pulled in). Done. https://codereview.webrtc.org/2236473002/diff/1/webrtc/BUILD.gn#newcode327 webrtc/BUILD.gn:327: configs -= [ "//build/config/clang:find_bad_constructs" ] On 2016/08/10 14:46:50, kjellander_webrtc wrote: > Could you try fixing these warnings instead of having to suppress them? It's OK > to keep it if it's a lot of work but sometimes it's just a few code changes. I prefer not to in this case since it means I'll have to make interface changes and add explicit constructors to all our send and receive stream configs etc. It's a bit error prone, and this CL shouldn't really be touching APIs.
.
https://codereview.webrtc.org/2236473002/diff/1/webrtc/BUILD.gn File webrtc/BUILD.gn (right): https://codereview.webrtc.org/2236473002/diff/1/webrtc/BUILD.gn#newcode327 webrtc/BUILD.gn:327: configs -= [ "//build/config/clang:find_bad_constructs" ] On 2016/08/11 10:38:35, stefan-webrtc (holmer) wrote: > On 2016/08/10 14:46:50, kjellander_webrtc wrote: > > Could you try fixing these warnings instead of having to suppress them? It's > OK > > to keep it if it's a lot of work but sometimes it's just a few code changes. > > I prefer not to in this case since it means I'll have to make interface changes > and add explicit constructors to all our send and receive stream configs etc. > It's a bit error prone, and this CL shouldn't really be touching APIs. Fair enough. https://codereview.webrtc.org/2236473002/diff/20001/webrtc/webrtc_tests.gypi File webrtc/webrtc_tests.gypi (right): https://codereview.webrtc.org/2236473002/diff/20001/webrtc/webrtc_tests.gypi#... webrtc/webrtc_tests.gypi:280: 'test/run_test.cc', You have to remove this one to avoid duplicate symbol on Mac.
Remove dup file.
https://codereview.webrtc.org/2236473002/diff/20001/webrtc/webrtc_tests.gypi File webrtc/webrtc_tests.gypi (right): https://codereview.webrtc.org/2236473002/diff/20001/webrtc/webrtc_tests.gypi#... webrtc/webrtc_tests.gypi:280: 'test/run_test.cc', On 2016/08/15 11:27:38, kjellander_webrtc wrote: > You have to remove this one to avoid duplicate symbol on Mac. Thanks!
Description was changed from ========== Add video_loopback to gn. BUG=webrtc:5949 ========== to ========== Add video_loopback to gn. BUG=webrtc:5949 NOTRY=True ==========
lgtm I added NOTRY=True for you and ran a few iOS trybots just to be sure.
The CQ bit was checked by stefan@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Add video_loopback to gn. BUG=webrtc:5949 NOTRY=True ========== to ========== Add video_loopback to gn. BUG=webrtc:5949 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 ========== Add video_loopback to gn. BUG=webrtc:5949 NOTRY=True ========== to ========== Add video_loopback to gn. BUG=webrtc:5949 NOTRY=True Committed: https://crrev.com/3ab6614d101dc6e8ff8c558bd966f9f3f05e87df Cr-Commit-Position: refs/heads/master@{#13748} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3ab6614d101dc6e8ff8c558bd966f9f3f05e87df Cr-Commit-Position: refs/heads/master@{#13748} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
