Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2250433008:
Propagate HAVE_WEBRTC_VIDEO and HAVE_WEBRTC_VIDEO gyp settings. (Closed)
|
Created:
4 years, 4 months ago by nisse-webrtc Modified:
4 years, 4 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd rtc_media as a direct dependency of rtc_media_unittests.
Without this, the rtc_media_unittests target was only an indirect
dependency, and compiled without HAVE_WEBRTC_VIDEO. And some testcases,
in particular, all tests defined by webrtcvideocapturer_unittest.cc,
are excluded from rtc_media_unittests.
BUG=
Committed: https://crrev.com/84c03bafcff1a4e603fe190d2f03ed672bdf82e9
Cr-Commit-Position: refs/heads/master@{#13859}
Patch Set 1 #Patch Set 2 : Fix inheritance of CreateAliasedFrame method. #
Total comments: 2
Patch Set 3 : Add direct dependency. Revert earlier change. #
Total comments: 2
Messages
Total messages: 31 (17 generated)
nisse@webrtc.org changed reviewers: + kjellander@webrtc.org, perkj@webrtc.org
Not sure if there's a better way to fix this. My gut feeling is that we really ought to have consistent use of these HAVE_WEBRTC_* values for all compiled files. So set them, using some conditional, somewhere at the top-level. But if gyp is being phased out, maybe no point to think very deep about this.
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_compile_x86_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_rel...)
On 2016/08/19 12:38:40, nisse-webrtc wrote: > Not sure if there's a better way to fix this. My gut feeling is that we really > ought to have consistent use of these HAVE_WEBRTC_* values for all compiled > files. So set them, using some conditional, somewhere at the top-level. > > But if gyp is being phased out, maybe no point to think very deep about this. I agree with your gut feeling here, but it seems to break Android somehow. Can you figure that out?
On 2016/08/22 06:39:17, kjellander_webrtc wrote: > On 2016/08/19 12:38:40, nisse-webrtc wrote: > > Not sure if there's a better way to fix this. My gut feeling is that we really > > ought to have consistent use of these HAVE_WEBRTC_* values for all compiled > > files. So set them, using some conditional, somewhere at the top-level. > > > > But if gyp is being phased out, maybe no point to think very deep about this. > > I agree with your gut feeling here, but it seems to break Android somehow. Can > you figure that out? Can we just remove all defines HAVE_WEBRT_ ? I dont' think it is needed anymore.
On 2016/08/22 09:12:17, perkj_webrtc wrote: > On 2016/08/22 06:39:17, kjellander_webrtc wrote: > > On 2016/08/19 12:38:40, nisse-webrtc wrote: > > > Not sure if there's a better way to fix this. My gut feeling is that we > really > > > ought to have consistent use of these HAVE_WEBRTC_* values for all compiled > > > files. So set them, using some conditional, somewhere at the top-level. > > > > > > But if gyp is being phased out, maybe no point to think very deep about > this. > > > > I agree with your gut feeling here, but it seems to break Android somehow. Can > > you figure that out? > > Can we just remove all defines HAVE_WEBRT_ ? I dont' think it is needed anymore. That'd be nice but I'm not sure how much work it is. Maybe it's better to do once we've got WebRTC to be more modular? Since then the plan is to not rely on any compile time defines in order to include/exclude functionality.
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Let's try to avoid introducing more all-dependent inheritance and fix the dependency at the rtc_media_unittests target instead. https://codereview.webrtc.org/2250433008/diff/20001/webrtc/media/media.gyp File webrtc/media/media.gyp (right): https://codereview.webrtc.org/2250433008/diff/20001/webrtc/media/media.gyp#ne... webrtc/media/media.gyp:257: 'rtc_unittest_main', I'd prefer if you add a dependency on 'rtc_media' here instead.
Description was changed from ========== Propagate HAVE_WEBRTC_VIDEO and HAVE_WEBRTC_VIDEO gyp settings. Now declared using all_dependent_settings in the rtc_media target, rather than direct_dependent_settings. Without this, the rtc_media_unittests target, which is an indirect dependency, is compiled without HAVE_WEBRTC_VIDEO. And some testcases, in particular, all tests defined by webrtcvideocapturer_unittest.cc, are excluded from rtc_media_unittests. BUG= ========== to ========== Add rtc_media as a direct dependency of rtc_media_unittests. Without this, the rtc_media_unittests target was only an indirect dependency, and compiled without HAVE_WEBRTC_VIDEO. And some testcases, in particular, all tests defined by webrtcvideocapturer_unittest.cc, are excluded from rtc_media_unittests. BUG= ==========
Ok now? https://codereview.webrtc.org/2250433008/diff/20001/webrtc/media/media.gyp File webrtc/media/media.gyp (right): https://codereview.webrtc.org/2250433008/diff/20001/webrtc/media/media.gyp#ne... webrtc/media/media.gyp:257: 'rtc_unittest_main', On 2016/08/22 09:37:46, kjellander_webrtc wrote: > I'd prefer if you add a dependency on 'rtc_media' here instead. Done.
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by nisse@webrtc.org
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/7582)
lgtm https://codereview.webrtc.org/2250433008/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoframefactory.h (right): https://codereview.webrtc.org/2250433008/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoframefactory.h:26: using VideoFrameFactory::CreateAliasedFrame; I don't understand why this suddenly is a problem and if its related to this cl? Anyhow, you are deprecating VideoFrameFactory right?
https://codereview.webrtc.org/2250433008/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoframefactory.h (right): https://codereview.webrtc.org/2250433008/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoframefactory.h:26: using VideoFrameFactory::CreateAliasedFrame; On 2016/08/22 18:00:33, perkj_webrtc wrote: > I don't understand why this suddenly is a problem and if its related to this > cl? My guess is that enabling HAVE_WEBRTC_VIDEO when building the tests makes this code be compiled on android, which it wasn't before. And android is the only platform where we use gcc, and it seems in this case gcc is stricter than clang (see compilation errors on the earlier patchset). I researched a similar issue earlier, and in C++ method lookup is based on method *name* only, not signature. So the style of the VideoFrameFactory, where subclasses are encouraged to override only one variant, is not really working. Adding the using declaration tells the compiler that we want to inherit those methods with this name which we don't override explicitly. > Anyhow, you are deprecating VideoFrameFactory right? Yes, I'm aiming at deletion.
The CQ bit was checked by nisse@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 rtc_media as a direct dependency of rtc_media_unittests. Without this, the rtc_media_unittests target was only an indirect dependency, and compiled without HAVE_WEBRTC_VIDEO. And some testcases, in particular, all tests defined by webrtcvideocapturer_unittest.cc, are excluded from rtc_media_unittests. BUG= ========== to ========== Add rtc_media as a direct dependency of rtc_media_unittests. Without this, the rtc_media_unittests target was only an indirect dependency, and compiled without HAVE_WEBRTC_VIDEO. And some testcases, in particular, all tests defined by webrtcvideocapturer_unittest.cc, are excluded from rtc_media_unittests. BUG= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add rtc_media as a direct dependency of rtc_media_unittests. Without this, the rtc_media_unittests target was only an indirect dependency, and compiled without HAVE_WEBRTC_VIDEO. And some testcases, in particular, all tests defined by webrtcvideocapturer_unittest.cc, are excluded from rtc_media_unittests. BUG= ========== to ========== Add rtc_media as a direct dependency of rtc_media_unittests. Without this, the rtc_media_unittests target was only an indirect dependency, and compiled without HAVE_WEBRTC_VIDEO. And some testcases, in particular, all tests defined by webrtcvideocapturer_unittest.cc, are excluded from rtc_media_unittests. BUG= Committed: https://crrev.com/84c03bafcff1a4e603fe190d2f03ed672bdf82e9 Cr-Commit-Position: refs/heads/master@{#13859} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/84c03bafcff1a4e603fe190d2f03ed672bdf82e9 Cr-Commit-Position: refs/heads/master@{#13859} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
