Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2210443002:
[WebRTC] Implement Windows ScreenDrawer to test ScreenCapturer* (Closed)
|
Created:
4 years, 4 months ago by Hzj_jie Modified:
4 years, 4 months ago Reviewers:
Sergey Ulanov CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, Jamie Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionCurrently there is not way to programmically test whether a ScreenCapturer
implementation can accurately capture updated regions. Especially in
ScreenCapturerWinDirectx, which has a specific updated region spreading logic
and cannot be tested through regular code path. So we need a controllable
ScreenDrawer to draw some basic shapes on the screen. And a platform independent
test case can use the ScreenDrawer to test a ScreenCapturer.
So this change addes a ScreenDrawer virtual class, and its Windows
implementation ScreenDrawerWin. A disabled gtest ScreenDrawerTest.DrawRectangles
is also added to manually test whether ScreenDrawer can work on a certain
platform.
BUG=314516
TBR=kjellander@webrtc.org
Committed: https://crrev.com/49c01d7f34aff7f80f647994be6198e80b73a787
Cr-Commit-Position: refs/heads/master@{#13788}
Patch Set 1 #Patch Set 2 : Implement ScreenDrawer for X11 #
Total comments: 34
Patch Set 3 : Resolve review comments #Patch Set 4 : Move ScreenDrawer::Clear to cc file #
Total comments: 2
Patch Set 5 : Resolve review comments #Patch Set 6 : Remove XErrorHandler #Patch Set 7 : Lint error, inconsistent local / online lint results #
Messages
Total messages: 41 (18 generated)
Description was changed from ========== Implement Windows ScreenDrawer BUG= ========== to ========== Currently there is not way to programmically test whether a ScreenCapturer implementation can accurately capture updated regions. Especially in ScreenCapturerWinDirectx, which has a specific updated region spreading logic and cannot be tested through regular code path. So we need a controllable ScreenDrawer to draw some basic shapes on the screen. And a platform independent test case can use the ScreenDrawer to test a ScreenCapturer. So this change addes a ScreenDrawer virtual class, and its Windows implementation ScreenDrawerWin. A disabled gtest ScreenDrawerTest.DrawRectangles is also added to manually test whether ScreenDrawer can work on a certain platform. BUG=314516 ==========
zijiehe@chromium.org changed reviewers: + sergeyu@chromium.org
https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer.cc (right): https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer.cc:16: ScreenDrawer::~ScreenDrawer() = default; these can be in the header, since it's an abstract interface. Also please use {} instead of =default; It's more readable and makes no difference for this class. https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer.h (right): https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer.h:27: // Creates a ScreenDrawer for a certain platform. s/a certain/the current/ https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer.h:42: void Clear() { DrawRectangle(DrawableRegion(), 0); } Thus us an interface, so it shouldn't have any non-virtual methods. https://google.github.io/styleguide/cppguide.html#Interfaces https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer_linux.cc (right): https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.cc:24: XCreateSimpleWindow(display_->display(), I'd prefer moving all this initialization logic out of the initializer list. Many of these operation may fail (e.g. test running outside of XWindow) and so you need to check the results. https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.cc:34: XMapWindow(display_->display(), window_); Verify that these succeed. Since this is test-only code just CHECK()'ing the result is appropriate. https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.cc:60: int b = rgba_array[2]; r = rgba & 0xff; g = (rgba & 0xff00) >> 8; b = (rgba & 0xff0000) >> 8; This would work correctly on big-endian cpus https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.cc:63: color.red = r << 8; r * 256 would be more readable as it makes it clearer what happens here. https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer_linux.h (right): https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.h:11: #ifndef MODULES_DESKTOP_CAPTURE_SCREEN_DRAWER_LINUX_H_ This file should be called screen_drawer_x11.h https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.h:11: #ifndef MODULES_DESKTOP_CAPTURE_SCREEN_DRAWER_LINUX_H_ For platform-specific screen capturers we don't have .h files. Instead the corresponding classes are declared in .cc file. Follow the same pattern here? https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.h:19: // A ScreenDrawer implementation for X11dows. typo: X11dows https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.h:25: DesktopRect DrawableRegion() override; // ScreenDrawer interface. https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer_unittest.cc (right): https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_unittest.cc:11: // TODO(zijiehe): Implement ScreenDrawerLinux / ScreenDrawerMac. Don't you already have it implemented in this CL? https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_unittest.cc:12: #if defined(WEBRTC_WIN) || defined(USE_X11) move this next to the test itself? Alternatively you can add stub implementation of ScreenDrawer::Create() for mac that just returns nullptr. That way the test won't need to be modified when you add the new impl https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_unittest.cc:29: TEST(ScreenDrawerTest, DISABLED_DrawRectangles) { MANUAL_ prefix is usually used for manual tests. https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_unittest.cc:41: random.Rand<uint32_t>()); I think you want to limit the range to [0..2^24)? https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_unittest.cc:55: #endif // defined(WEBRTC_WIN) || defined(USE_X11)
BTW, thanks for working on these tests - it's great that we will have proper test coverage for that code!
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer.cc (right): https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer.cc:16: ScreenDrawer::~ScreenDrawer() = default; On 2016/08/09 17:43:00, Sergey Ulanov wrote: > these can be in the header, since it's an abstract interface. Also please use {} > instead of =default; It's more readable and makes no difference for this class. Done. https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer.h (right): https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer.h:27: // Creates a ScreenDrawer for a certain platform. On 2016/08/09 17:43:01, Sergey Ulanov wrote: > s/a certain/the current/ Done. https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer.h:42: void Clear() { DrawRectangle(DrawableRegion(), 0); } On 2016/08/09 17:43:01, Sergey Ulanov wrote: > Thus us an interface, so it shouldn't have any non-virtual methods. > https://google.github.io/styleguide/cppguide.html#Interfaces Is there any benefit we treat this class as a pure interface? https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer_linux.cc (right): https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.cc:24: XCreateSimpleWindow(display_->display(), On 2016/08/09 17:43:01, Sergey Ulanov wrote: > I'd prefer moving all this initialization logic out of the initializer list. > Many of these operation may fail (e.g. test running outside of XWindow) and so > you need to check the results. Done. https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.cc:34: XMapWindow(display_->display(), window_); On 2016/08/09 17:43:01, Sergey Ulanov wrote: > Verify that these succeed. Since this is test-only code just CHECK()'ing the > result is appropriate. I believe the return value of these functions are not so useful. X11 uses a different error handling mechanism. By default, it will print error message and force quite. I have redirected the error to RTC_CHECK, so we can catch them in the logs. https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.cc:60: int b = rgba_array[2]; On 2016/08/09 17:43:01, Sergey Ulanov wrote: > r = rgba & 0xff; > g = (rgba & 0xff00) >> 8; > b = (rgba & 0xff0000) >> 8; > > This would work correctly on big-endian cpus Good point. Done. https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.cc:63: color.red = r << 8; On 2016/08/09 17:43:01, Sergey Ulanov wrote: > r * 256 would be more readable as it makes it clearer what happens here. Done. https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer_linux.h (right): https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.h:11: #ifndef MODULES_DESKTOP_CAPTURE_SCREEN_DRAWER_LINUX_H_ On 2016/08/09 17:43:01, Sergey Ulanov wrote: > For platform-specific screen capturers we don't have .h files. Instead the > corresponding classes are declared in .cc file. Follow the same pattern here? Done. https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.h:11: #ifndef MODULES_DESKTOP_CAPTURE_SCREEN_DRAWER_LINUX_H_ On 2016/08/09 17:43:01, Sergey Ulanov wrote: > This file should be called screen_drawer_x11.h I thought linux suffix can trigger GN / GYP exclusive rule for other platforms. But I am happy to rename this class to ScreenDrawerLinux. https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.h:19: // A ScreenDrawer implementation for X11dows. On 2016/08/09 17:43:01, Sergey Ulanov wrote: > typo: X11dows Done. https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.h:25: DesktopRect DrawableRegion() override; On 2016/08/09 17:43:01, Sergey Ulanov wrote: > // ScreenDrawer interface. Done. https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer_unittest.cc (right): https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_unittest.cc:11: // TODO(zijiehe): Implement ScreenDrawerLinux / ScreenDrawerMac. On 2016/08/09 17:43:01, Sergey Ulanov wrote: > Don't you already have it implemented in this CL? Should be updated, no, I have not implemented ScreenDrawerMac, as I do not have MAC machine. https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_unittest.cc:12: #if defined(WEBRTC_WIN) || defined(USE_X11) On 2016/08/09 17:43:01, Sergey Ulanov wrote: > move this next to the test itself? > Alternatively you can add stub implementation of ScreenDrawer::Create() for mac > that just returns nullptr. That way the test won't need to be modified when you > add the new impl Good point. Done. https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_unittest.cc:29: TEST(ScreenDrawerTest, DISABLED_DrawRectangles) { On 2016/08/09 17:43:01, Sergey Ulanov wrote: > MANUAL_ prefix is usually used for manual tests. Done. https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_unittest.cc:41: random.Rand<uint32_t>()); On 2016/08/09 17:43:01, Sergey Ulanov wrote: > I think you want to limit the range to [0..2^24)? I still prefer to use RGBA, though both Windows and X11 do not support alpha. https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_unittest.cc:55: #endif On 2016/08/09 17:43:01, Sergey Ulanov wrote: > // defined(WEBRTC_WIN) || defined(USE_X11) This line has been removed.
https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer.h (right): https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer.h:42: void Clear() { DrawRectangle(DrawableRegion(), 0); } On 2016/08/11 18:42:03, Hzj_jie wrote: > On 2016/08/09 17:43:01, Sergey Ulanov wrote: > > Thus us an interface, so it shouldn't have any non-virtual methods. > > https://google.github.io/styleguide/cppguide.html#Interfaces > > Is there any benefit we treat this class as a pure interface? The issue here is not whether we decide to call it interface or not, but whether the reader of this code know that it's not interface. Everything above this line looks like a pure interface and is consistent with other platform-specific code in this directory. So it will be confusing for the reader if you make an exception for this one method. For example someone may be looking for x11 implementation of this method in screen_drawer_x11.cc and would be surprised not to find it there. Also potentially Clear() may be implemented differently on some platforms, so there is a benefit to making it virtual anyway. Another issue here is that this is non-trivial method, and I don't think we want it inlined in a header, even if it's not virtual.
On 2016/08/12 05:49:23, Sergey Ulanov wrote: > https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... > File webrtc/modules/desktop_capture/screen_drawer.h (right): > > https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... > webrtc/modules/desktop_capture/screen_drawer.h:42: void Clear() { > DrawRectangle(DrawableRegion(), 0); } > On 2016/08/11 18:42:03, Hzj_jie wrote: > > On 2016/08/09 17:43:01, Sergey Ulanov wrote: > > > Thus us an interface, so it shouldn't have any non-virtual methods. > > > https://google.github.io/styleguide/cppguide.html#Interfaces > > > > Is there any benefit we treat this class as a pure interface? > > The issue here is not whether we decide to call it interface or not, but whether > the reader of this code know that it's not interface. Everything above this line > looks like a pure interface and is consistent with other platform-specific code > in this directory. So it will be confusing for the reader if you make an > exception for this one method. For example someone may be looking for x11 > implementation of this method in screen_drawer_x11.cc and would be surprised not > to find it there. Also potentially Clear() may be implemented differently on > some platforms, so there is a benefit to making it virtual anyway. > > Another issue here is that this is non-trivial method, and I don't think we want > it inlined in a header, even if it's not virtual. Then will it be acceptable if I make this function virtual, but provide a default implementation in screen_drawer.cc?
On 2016/08/12 19:17:22, Hzj_jie wrote: > On 2016/08/12 05:49:23, Sergey Ulanov wrote: > > > https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... > > File webrtc/modules/desktop_capture/screen_drawer.h (right): > > > > > https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... > > webrtc/modules/desktop_capture/screen_drawer.h:42: void Clear() { > > DrawRectangle(DrawableRegion(), 0); } > > On 2016/08/11 18:42:03, Hzj_jie wrote: > > > On 2016/08/09 17:43:01, Sergey Ulanov wrote: > > > > Thus us an interface, so it shouldn't have any non-virtual methods. > > > > https://google.github.io/styleguide/cppguide.html#Interfaces > > > > > > Is there any benefit we treat this class as a pure interface? > > > > The issue here is not whether we decide to call it interface or not, but > whether > > the reader of this code know that it's not interface. Everything above this > line > > looks like a pure interface and is consistent with other platform-specific > code > > in this directory. So it will be confusing for the reader if you make an > > exception for this one method. For example someone may be looking for x11 > > implementation of this method in screen_drawer_x11.cc and would be surprised > not > > to find it there. Also potentially Clear() may be implemented differently on > > some platforms, so there is a benefit to making it virtual anyway. > > > > Another issue here is that this is non-trivial method, and I don't think we > want > > it inlined in a header, even if it's not virtual. > > Then will it be acceptable if I make this function virtual, but provide a > default implementation in screen_drawer.cc? No, I still think it's better to keep it pure virtual for the reason I explained above.
Patchset #5 (id:100001) has been deleted
On 2016/08/12 20:41:46, Sergey Ulanov wrote: > On 2016/08/12 19:17:22, Hzj_jie wrote: > > On 2016/08/12 05:49:23, Sergey Ulanov wrote: > > > > > > https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... > > > File webrtc/modules/desktop_capture/screen_drawer.h (right): > > > > > > > > > https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... > > > webrtc/modules/desktop_capture/screen_drawer.h:42: void Clear() { > > > DrawRectangle(DrawableRegion(), 0); } > > > On 2016/08/11 18:42:03, Hzj_jie wrote: > > > > On 2016/08/09 17:43:01, Sergey Ulanov wrote: > > > > > Thus us an interface, so it shouldn't have any non-virtual methods. > > > > > https://google.github.io/styleguide/cppguide.html#Interfaces > > > > > > > > Is there any benefit we treat this class as a pure interface? > > > > > > The issue here is not whether we decide to call it interface or not, but > > whether > > > the reader of this code know that it's not interface. Everything above this > > line > > > looks like a pure interface and is consistent with other platform-specific > > code > > > in this directory. So it will be confusing for the reader if you make an > > > exception for this one method. For example someone may be looking for x11 > > > implementation of this method in screen_drawer_x11.cc and would be surprised > > not > > > to find it there. Also potentially Clear() may be implemented differently on > > > some platforms, so there is a benefit to making it virtual anyway. > > > > > > Another issue here is that this is non-trivial method, and I don't think we > > want > > > it inlined in a header, even if it's not virtual. > > > > Then will it be acceptable if I make this function virtual, but provide a > > default implementation in screen_drawer.cc? > > No, I still think it's better to keep it pure virtual for the reason I explained > above. Done.
https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer_unittest.cc (right): https://codereview.chromium.org/2210443002/diff/20001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_unittest.cc:29: TEST(ScreenDrawerTest, DISABLED_DrawRectangles) { On 2016/08/11 18:42:04, Hzj_jie wrote: > On 2016/08/09 17:43:01, Sergey Ulanov wrote: > > MANUAL_ prefix is usually used for manual tests. > > Done. DISABLED_ can block gtest from running this test case automatically. As these (now only this) test cases won't be useful without a human interaction.
lgtm when my comment is addressed https://codereview.chromium.org/2210443002/diff/80001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer_linux.cc (right): https://codereview.chromium.org/2210443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.cc:51: XSetIOErrorHandler(&XIOErrorHandler); I don't think these are useful, since the default handles terminate the process anyway. Also note that there is ScopedXErrorHandler in remoting/host/linux/x11_util.h and the screen/window capturers use it. It may conflict with this error handler.
https://codereview.chromium.org/2210443002/diff/80001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer_linux.cc (right): https://codereview.chromium.org/2210443002/diff/80001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.cc:51: XSetIOErrorHandler(&XIOErrorHandler); On 2016/08/15 21:41:09, Sergey Ulanov wrote: > I don't think these are useful, since the default handles terminate the process > anyway. Also note that there is ScopedXErrorHandler in > remoting/host/linux/x11_util.h and the screen/window capturers use it. It may > conflict with this error handler. Done.
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2210443002/#ps140001 (title: "Remove XErrorHandler")
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/7438)
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2210443002/#ps160001 (title: "Lint error, inconsistent local / online lint results")
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: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by zijiehe@chromium.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/7475)
The CQ bit was checked by zijiehe@chromium.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/7479)
Description was changed from ========== Currently there is not way to programmically test whether a ScreenCapturer implementation can accurately capture updated regions. Especially in ScreenCapturerWinDirectx, which has a specific updated region spreading logic and cannot be tested through regular code path. So we need a controllable ScreenDrawer to draw some basic shapes on the screen. And a platform independent test case can use the ScreenDrawer to test a ScreenCapturer. So this change addes a ScreenDrawer virtual class, and its Windows implementation ScreenDrawerWin. A disabled gtest ScreenDrawerTest.DrawRectangles is also added to manually test whether ScreenDrawer can work on a certain platform. BUG=314516 ========== to ========== Currently there is not way to programmically test whether a ScreenCapturer implementation can accurately capture updated regions. Especially in ScreenCapturerWinDirectx, which has a specific updated region spreading logic and cannot be tested through regular code path. So we need a controllable ScreenDrawer to draw some basic shapes on the screen. And a platform independent test case can use the ScreenDrawer to test a ScreenCapturer. So this change addes a ScreenDrawer virtual class, and its Windows implementation ScreenDrawerWin. A disabled gtest ScreenDrawerTest.DrawRectangles is also added to manually test whether ScreenDrawer can work on a certain platform. BUG=314516 TBR=kjellander@webrtc.org ==========
The CQ bit was checked by zijiehe@chromium.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 ========== Currently there is not way to programmically test whether a ScreenCapturer implementation can accurately capture updated regions. Especially in ScreenCapturerWinDirectx, which has a specific updated region spreading logic and cannot be tested through regular code path. So we need a controllable ScreenDrawer to draw some basic shapes on the screen. And a platform independent test case can use the ScreenDrawer to test a ScreenCapturer. So this change addes a ScreenDrawer virtual class, and its Windows implementation ScreenDrawerWin. A disabled gtest ScreenDrawerTest.DrawRectangles is also added to manually test whether ScreenDrawer can work on a certain platform. BUG=314516 TBR=kjellander@webrtc.org ========== to ========== Currently there is not way to programmically test whether a ScreenCapturer implementation can accurately capture updated regions. Especially in ScreenCapturerWinDirectx, which has a specific updated region spreading logic and cannot be tested through regular code path. So we need a controllable ScreenDrawer to draw some basic shapes on the screen. And a platform independent test case can use the ScreenDrawer to test a ScreenCapturer. So this change addes a ScreenDrawer virtual class, and its Windows implementation ScreenDrawerWin. A disabled gtest ScreenDrawerTest.DrawRectangles is also added to manually test whether ScreenDrawer can work on a certain platform. BUG=314516 TBR=kjellander@webrtc.org ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Currently there is not way to programmically test whether a ScreenCapturer implementation can accurately capture updated regions. Especially in ScreenCapturerWinDirectx, which has a specific updated region spreading logic and cannot be tested through regular code path. So we need a controllable ScreenDrawer to draw some basic shapes on the screen. And a platform independent test case can use the ScreenDrawer to test a ScreenCapturer. So this change addes a ScreenDrawer virtual class, and its Windows implementation ScreenDrawerWin. A disabled gtest ScreenDrawerTest.DrawRectangles is also added to manually test whether ScreenDrawer can work on a certain platform. BUG=314516 TBR=kjellander@webrtc.org ========== to ========== Currently there is not way to programmically test whether a ScreenCapturer implementation can accurately capture updated regions. Especially in ScreenCapturerWinDirectx, which has a specific updated region spreading logic and cannot be tested through regular code path. So we need a controllable ScreenDrawer to draw some basic shapes on the screen. And a platform independent test case can use the ScreenDrawer to test a ScreenCapturer. So this change addes a ScreenDrawer virtual class, and its Windows implementation ScreenDrawerWin. A disabled gtest ScreenDrawerTest.DrawRectangles is also added to manually test whether ScreenDrawer can work on a certain platform. BUG=314516 TBR=kjellander@webrtc.org Committed: https://crrev.com/49c01d7f34aff7f80f647994be6198e80b73a787 Cr-Commit-Position: refs/heads/master@{#13788} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/49c01d7f34aff7f80f647994be6198e80b73a787 Cr-Commit-Position: refs/heads/master@{#13788} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
