Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2337073007:
Deflaky ScreenCapturerTest (Closed)
DescriptionDeflaky ScreenCapturerTest
ScreenCapturer tests may fail on trybot, so this change is to fix the issue.
Changes include,
1. Sometimes, a capturer may capture part of the change, i.e. usually the draw
actions are not atomic. So the updated_region may be inaccurate. So I have added
a MayDrawIncompleteShapes() function in ScreenDrawer. If it returns false, the
updated_region check will be ignored.
2. Several test cases may run concurrently, which makes one ScreenDrawer won't
really work. Its window may be covered by another ScreenDrawer. So I have added
a system wide lock to ensure only one ScreenDrawer is working at a certain time.
3. On unity (Linux), the top several pixels of a window may be covered by a
shadow effect if the window is not focused. So I have added a BringToFront()
function, and call it in WaitForPendingDraws().
4. On Windows, the drawn shapes are 'temporary drawing', which will be erased
once the window is covered by another one. So I repeat DrawRectangle() function
call in the test case.
TODO(zijiehe): The DISABLED_ prefixes will be added back after the code review.
And I will move these test cases into modules_test in a coming change.
BUG=647067
Committed: https://crrev.com/6a4607e1006560ce598f1e1d79fd120308b9e67f
Cr-Commit-Position: refs/heads/master@{#14674}
Patch Set 1 : The DISABLED_ prefixes will be added back #
Total comments: 2
Patch Set 2 : Resolve review comments #
Total comments: 6
Patch Set 3 : Use TOPMOST for both Linux and Windows #Patch Set 4 : Still disable real screen capturer tests #
Messages
Total messages: 52 (36 generated)
Description was changed from ========== Deflaky ScreenCapturerTest BUG= ========== to ========== Deflaky ScreenCapturerTest TwoCapturers test may fail on trybot, so this change is to fix the issue and enable this test case. BUG= ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== Deflaky ScreenCapturerTest TwoCapturers test may fail on trybot, so this change is to fix the issue and enable this test case. BUG= ========== to ========== Deflaky ScreenCapturerTest TwoCapturers test may fail on trybot, so this change is to fix the issue and enable this test case. BUG=647067 ==========
Description was changed from ========== Deflaky ScreenCapturerTest TwoCapturers test may fail on trybot, so this change is to fix the issue and enable this test case. BUG=647067 ========== to ========== Deflaky ScreenCapturerTest ScreenCapturer tests may fail on trybot, so this change is to fix the issue and enable these test cases. BUG=647067 ==========
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:200001) has been deleted
Patchset #1 (id:220001) has been deleted
Patchset #1 (id:240001) has been deleted
Patchset #1 (id:260001) has been deleted
Patchset #1 (id:280001) has been deleted
Patchset #1 (id:300001) has been deleted
Patchset #1 (id:320001) has been deleted
Patchset #1 (id:340001) has been deleted
Patchset #1 (id:360001) has been deleted
Patchset #1 (id:380001) has been deleted
Patchset #1 (id:400001) has been deleted
Patchset #1 (id:420001) has been deleted
Description was changed from ========== Deflaky ScreenCapturerTest ScreenCapturer tests may fail on trybot, so this change is to fix the issue and enable these test cases. BUG=647067 ========== to ========== Deflaky ScreenCapturerTest ScreenCapturer tests may fail on trybot, so this change is to fix the issue. BUG=647067 ==========
zijiehe@chromium.org changed reviewers: + kjellander@webrtc.org, sergeyu@chromium.org
Description was changed from ========== Deflaky ScreenCapturerTest ScreenCapturer tests may fail on trybot, so this change is to fix the issue. BUG=647067 ========== to ========== Deflaky ScreenCapturerTest ScreenCapturer tests may fail on trybot, so this change is to fix the issue. Changes include, 1. Sometimes, a capturer may capture part of the change, i.e. usually the draw actions are not atomic. So the updated_region may be inaccurate. So I have added a MayDrawIncompleteShapes() function in ScreenDrawer. If it returns false, the updated_region check will be ignored. 2. Several test cases may run concurrently, which makes one ScreenDrawer won't really work. Its window may be covered by another ScreenDrawer. So I have added a system wide lock to ensure only one ScreenDrawer is working at a certain time. 3. On unity (Linux), the top several pixels of a window may be covered by a shadow effect if the window is not focused. So I have added a BringToFront() function, and call it in WaitForPendingDraws(). 4. On Windows, the drawn shapes may randomly disappear or partially disappear. So I repeat DrawRectangle() function call in the test case. TODO(zijiehe): The DISABLED_ prefixes will be added back after the code review. And I will move these test cases into modules_test in a coming change. BUG=647067 ==========
zijiehe@chromium.org changed reviewers: + jamiewalch@chromium.org
Adding Jamie in the review.
I haven't looked at the code yet, but based on your CL description I have some concerns. Specifically, for the four issues you call out: 1. I don't think this is the right way to fix this; it essentially means we have no test coverage for the capture region on some platforms. I think the problem is essentially a race condition, correct? If so, then a better fix would be to retry it a few times, or wait a short time between drawing and capturing. 2. That seems like a reasonable fix if there's no way to mark tests as needing to run in isolation, but please investigate that option first. 3. It's not obvious from the name of the method that WaitForPendingDraws will bring a window to the front. It would be better to expose it as a method to be called explicitly if that's the desired behaviour. 4. This sounds like you're not responding to WM_PAINT messages. I'd prefer that you find a way to deflake this without resorting to painting everything multiple times (which sounds like it will make it less flaky, but not completely flake-free).
On 2016/10/12 23:43:56, Jamie wrote: > I haven't looked at the code yet, but based on your CL description I have some > concerns. Specifically, for the four issues you call out: > > 1. I don't think this is the right way to fix this; it essentially means we have > no test coverage for the capture region on some platforms. I think the problem > is essentially a race condition, correct? If so, then a better fix would be to > retry it a few times, or wait a short time between drawing and capturing. The problem here is, if we render a rectangle from (0, 0) to (3, 3), and capture it. In the first capture attempt, half of the rectangle is rendered, say (0, 0) to (1, 3), it surely cannot pass, since the other half has not been captured. In the second capture attempt, the other half is rendered, (1, 0) to (3, 3). Then the entire rectangle has been rendered, there will be no more update. But both capture attempts get part of the entire updated region. This can be done if we merge the updated regions from each attempt. Since we will add ScreenCapturerDifferWrapper in a coming change to tighten up the assertion here, I can make the change then. > > 2. That seems like a reasonable fix if there's no way to mark tests as needing > to run in isolation, but please investigate that option first. I have talked with Mike, Marc-Antoine maruel@, and Henrik kjellander@. Currently in webrtc, we do not have a way to ensure a test to run in isolation. Edward ehmaldonado@ is moving webrtc tests to swarming, which is a different test framework. But I do not see it can be done in a short time. > > 3. It's not obvious from the name of the method that WaitForPendingDraws will > bring a window to the front. It would be better to expose it as a method to be > called explicitly if that's the desired behaviour. > It looks like WaitForPendingDraws() is not a good name to describe its functionality. I will look for a different and descripsive name for WaitForPendingDraws(). From my opinion, it's ScreenDrawer's responsibility to ensure what it required to draw is actually displayed on the screen. But since we are running on multi-task systems, they usually allow another top-most window to cover a top-most window. So it's important to call BringToFront() function regularly to ensure it won't be covered by another window. WaitForPendingDraws() is called regularly, which is the reason I add BringToFront into WaitForPendingDraws. > 4. This sounds like you're not responding to WM_PAINT messages. I'd prefer that > you find a way to deflake this without resorting to painting everything multiple > times (which sounds like it will make it less flaky, but not completely > flake-free). I have not described this issue more clearly. The APIs I am using in Windows are 'temporary drawing', which will be erased if some other window has covered it. (I doubt why there are such kind of APIs in Windows, but the fact is cruel.) So the root cause of this issue is similar as the 3rd one. After the top-most ScreenDrawer window has been covered by other windows, we need to bring it to front on both platforms, and redraw it on Windows. A window on Windows does not really need to respond WM_PAINT message to get paint. Indeed ScreenDrawerWin ignores all messages.
Description was changed from ========== Deflaky ScreenCapturerTest ScreenCapturer tests may fail on trybot, so this change is to fix the issue. Changes include, 1. Sometimes, a capturer may capture part of the change, i.e. usually the draw actions are not atomic. So the updated_region may be inaccurate. So I have added a MayDrawIncompleteShapes() function in ScreenDrawer. If it returns false, the updated_region check will be ignored. 2. Several test cases may run concurrently, which makes one ScreenDrawer won't really work. Its window may be covered by another ScreenDrawer. So I have added a system wide lock to ensure only one ScreenDrawer is working at a certain time. 3. On unity (Linux), the top several pixels of a window may be covered by a shadow effect if the window is not focused. So I have added a BringToFront() function, and call it in WaitForPendingDraws(). 4. On Windows, the drawn shapes may randomly disappear or partially disappear. So I repeat DrawRectangle() function call in the test case. TODO(zijiehe): The DISABLED_ prefixes will be added back after the code review. And I will move these test cases into modules_test in a coming change. BUG=647067 ========== to ========== Deflaky ScreenCapturerTest ScreenCapturer tests may fail on trybot, so this change is to fix the issue. Changes include, 1. Sometimes, a capturer may capture part of the change, i.e. usually the draw actions are not atomic. So the updated_region may be inaccurate. So I have added a MayDrawIncompleteShapes() function in ScreenDrawer. If it returns false, the updated_region check will be ignored. 2. Several test cases may run concurrently, which makes one ScreenDrawer won't really work. Its window may be covered by another ScreenDrawer. So I have added a system wide lock to ensure only one ScreenDrawer is working at a certain time. 3. On unity (Linux), the top several pixels of a window may be covered by a shadow effect if the window is not focused. So I have added a BringToFront() function, and call it in WaitForPendingDraws(). 4. On Windows, the drawn shapes are 'temporary drawing', which will be erased once the window is covered by another one. So I repeat DrawRectangle() function call in the test case. TODO(zijiehe): The DISABLED_ prefixes will be added back after the code review. And I will move these test cases into modules_test in a coming change. BUG=647067 ==========
On 2016/10/13 00:11:19, Hzj_jie wrote: > On 2016/10/12 23:43:56, Jamie wrote: > > I haven't looked at the code yet, but based on your CL description I have some > > concerns. Specifically, for the four issues you call out: > > > > 1. I don't think this is the right way to fix this; it essentially means we > have > > no test coverage for the capture region on some platforms. I think the problem > > is essentially a race condition, correct? If so, then a better fix would be to > > retry it a few times, or wait a short time between drawing and capturing. > The problem here is, if we render a rectangle from (0, 0) to (3, 3), and capture > it. In the first capture attempt, half of the rectangle is rendered, say (0, 0) > to (1, 3), it surely cannot pass, since the other half has not been captured. In > the second capture attempt, the other half is rendered, (1, 0) to (3, 3). Then > the entire rectangle has been rendered, there will be no more update. But both > capture attempts get part of the entire updated region. This can be done if we > merge the updated regions from each attempt. Since we will add > ScreenCapturerDifferWrapper in a coming change to tighten up the assertion here, > I can make the change then. I forgot the most important reason, we always return entire frame in screen capturer implementations, so this check is not really useful. > > > > 2. That seems like a reasonable fix if there's no way to mark tests as needing > > to run in isolation, but please investigate that option first. > I have talked with Mike, Marc-Antoine maruel@, and Henrik kjellander@. Currently > in webrtc, we do not have a way to ensure a test to run in isolation. Edward > ehmaldonado@ is moving webrtc tests to swarming, which is a different test > framework. But I do not see it can be done in a short time. > > > > 3. It's not obvious from the name of the method that WaitForPendingDraws will > > bring a window to the front. It would be better to expose it as a method to be > > called explicitly if that's the desired behaviour. > > > It looks like WaitForPendingDraws() is not a good name to describe its > functionality. I will look for a different and descripsive name for > WaitForPendingDraws(). > From my opinion, it's ScreenDrawer's responsibility to ensure what it required > to draw is actually displayed on the screen. But since we are running on > multi-task systems, they usually allow another top-most window to cover a > top-most window. So it's important to call BringToFront() function regularly to > ensure it won't be covered by another window. WaitForPendingDraws() is called > regularly, which is the reason I add BringToFront into WaitForPendingDraws. > > 4. This sounds like you're not responding to WM_PAINT messages. I'd prefer > that > > you find a way to deflake this without resorting to painting everything > multiple > > times (which sounds like it will make it less flaky, but not completely > > flake-free). > I have not described this issue more clearly. The APIs I am using in Windows are > 'temporary drawing', which will be erased if some other window has covered it. > (I doubt why there are such kind of APIs in Windows, but the fact is cruel.) > So the root cause of this issue is similar as the 3rd one. After the top-most > ScreenDrawer window has been covered by other windows, we need to bring it to > front on both platforms, and redraw it on Windows. > A window on Windows does not really need to respond WM_PAINT message to get > paint. Indeed ScreenDrawerWin ignores all messages.
It looks to me that this change solves the problem in the case when two ScreenCapturer tests run in parallel, but the test may still be flaky when running concurrently with any other tests that update screen content. So I think to make the test reliable it should be running in isolation, to insure that there are no other tests that may affect screen content. Henrik, do you know if there is a way to achieve that? https://codereview.chromium.org/2337073007/diff/440001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_drawer_win.cc (right): https://codereview.chromium.org/2337073007/diff/440001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_drawer_win.cc:37: ScreenDrawerLock::ScreenDrawerLock() { it looks like this code is duplicated between the two platforms, even though it implements the same functionality. Is it possible to avoid this duplication?
On 2016/10/14 17:38:25, Sergey Ulanov wrote: > It looks to me that this change solves the problem in the case when two > ScreenCapturer tests run in parallel, but the test may still be flaky when > running concurrently with any other tests that update screen content. So I think > to make the test reliable it should be running in isolation, to insure that > there are no other tests that may affect screen content. > Henrik, do you know if there is a way to achieve that? Yes, we have special binary that contains tests that doesn't work well when done in parallel (i.e. they rely exclusively on system resources like audio devices, network card etc). It's here: https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?rcl=0&l=752 It should be easy to just move the line in https://cs.chromium.org/chromium/src/third_party/webrtc/modules/BUILD.gn?rcl=... into that test binary instead. But... if there's a way to make these tests run fine in parallel, that's where we should put the effort (it doesn't sound impossible to me). > > https://codereview.chromium.org/2337073007/diff/440001/webrtc/modules/desktop... > File webrtc/modules/desktop_capture/screen_drawer_win.cc (right): > > https://codereview.chromium.org/2337073007/diff/440001/webrtc/modules/desktop... > webrtc/modules/desktop_capture/screen_drawer_win.cc:37: > ScreenDrawerLock::ScreenDrawerLock() { > it looks like this code is duplicated between the two platforms, even though it > implements the same functionality. Is it possible to avoid this duplication?
On 2016/10/17 11:47:15, kjellander_webrtc wrote: > On 2016/10/14 17:38:25, Sergey Ulanov wrote: > > It looks to me that this change solves the problem in the case when two > > ScreenCapturer tests run in parallel, but the test may still be flaky when > > running concurrently with any other tests that update screen content. So I > think > > to make the test reliable it should be running in isolation, to insure that > > there are no other tests that may affect screen content. > > Henrik, do you know if there is a way to achieve that? > > Yes, we have special binary that contains tests that doesn't work well when done > in parallel (i.e. they rely exclusively on system resources like audio devices, > network card etc). > It's here: > https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?rcl=0&l=752 > > It should be easy to just move the line in > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/BUILD.gn?rcl=... > into that test binary instead. > > But... if there's a way to make these tests run fine in parallel, that's where > we should put the effort (it doesn't sound impossible to me). > > > > > > > https://codereview.chromium.org/2337073007/diff/440001/webrtc/modules/desktop... > > File webrtc/modules/desktop_capture/screen_drawer_win.cc (right): > > > > > https://codereview.chromium.org/2337073007/diff/440001/webrtc/modules/desktop... > > webrtc/modules/desktop_capture/screen_drawer_win.cc:37: > > ScreenDrawerLock::ScreenDrawerLock() { > > it looks like this code is duplicated between the two platforms, even though > it > > implements the same functionality. Is it possible to avoid this duplication? As I have described in the comments, I have added a system wide lock to ensure only one instance is running at a certain time. I have also added logic to ensure the drawer window is always top-most. I believe currently ScreenDrawer tests are safe enough to not be broken by others. But it may impact other tests if they have same requirements. Henrik, what's your suggestion for these tests?
Patchset #2 (id:460001) has been deleted
Patchset #2 (id:480001) has been deleted
LGTM, but please see my suggestion about making the test windows always on top https://codereview.chromium.org/2337073007/diff/500001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_drawer.cc (right): https://codereview.chromium.org/2337073007/diff/500001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_drawer.cc:2: * Copyright 2016 The WebRTC project authors. All Rights Reserved. please keep (c) here. (standard chromium header doesn't have (c), but it's not the same for WebRTC. I think there is actually a presubmit check for this) https://codereview.chromium.org/2337073007/diff/500001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_drawer.h (right): https://codereview.chromium.org/2337073007/diff/500001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_drawer.h:23: // A cross application lock to ensure only one ScreenDrawer can be used at a s/cross application/cross-process/ https://codereview.chromium.org/2337073007/diff/500001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_drawer_linux.cc (right): https://codereview.chromium.org/2337073007/diff/500001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_drawer_linux.cc:164: XRaiseWindow(display_->display(), window_); If there is another test running in parallel it may still show another window on top of this window. Maybe set _NET_WM_STATE_ABOVE, to ensure that our window is always on top? Same for windows.
On 2016/10/17 22:05:32, Hzj_jie wrote: > On 2016/10/17 11:47:15, kjellander_webrtc wrote: > > On 2016/10/14 17:38:25, Sergey Ulanov wrote: > > > It looks to me that this change solves the problem in the case when two > > > ScreenCapturer tests run in parallel, but the test may still be flaky when > > > running concurrently with any other tests that update screen content. So I > > think > > > to make the test reliable it should be running in isolation, to insure that > > > there are no other tests that may affect screen content. > > > Henrik, do you know if there is a way to achieve that? > > > > Yes, we have special binary that contains tests that doesn't work well when > done > > in parallel (i.e. they rely exclusively on system resources like audio > devices, > > network card etc). > > It's here: > > https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?rcl=0&l=752 > > > > It should be easy to just move the line in > > > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/BUILD.gn?rcl=... > > into that test binary instead. > > > > But... if there's a way to make these tests run fine in parallel, that's where > > we should put the effort (it doesn't sound impossible to me). > > > > > > > > > > > > > https://codereview.chromium.org/2337073007/diff/440001/webrtc/modules/desktop... > > > File webrtc/modules/desktop_capture/screen_drawer_win.cc (right): > > > > > > > > > https://codereview.chromium.org/2337073007/diff/440001/webrtc/modules/desktop... > > > webrtc/modules/desktop_capture/screen_drawer_win.cc:37: > > > ScreenDrawerLock::ScreenDrawerLock() { > > > it looks like this code is duplicated between the two platforms, even though > > it > > > implements the same functionality. Is it possible to avoid this duplication? > > As I have described in the comments, I have added a system wide lock to ensure > only one instance is running at a certain time. I have also added logic to > ensure the drawer window is always top-most. I believe currently ScreenDrawer > tests are safe enough to not be broken by others. But it may impact other tests > if they have same requirements. > Henrik, what's your suggestion for these tests? I can't speak for the tests but if Sergey is OK with this, let's submit it and see how it goes. There's still an error on Dr Memory Light though, which probably caused by the fact that it runs much slower than a regular machine.
On 2016/10/18 05:02:08, kjellander_webrtc wrote: > On 2016/10/17 22:05:32, Hzj_jie wrote: > > On 2016/10/17 11:47:15, kjellander_webrtc wrote: > > > On 2016/10/14 17:38:25, Sergey Ulanov wrote: > > > > It looks to me that this change solves the problem in the case when two > > > > ScreenCapturer tests run in parallel, but the test may still be flaky when > > > > running concurrently with any other tests that update screen content. So I > > > think > > > > to make the test reliable it should be running in isolation, to insure > that > > > > there are no other tests that may affect screen content. > > > > Henrik, do you know if there is a way to achieve that? > > > > > > Yes, we have special binary that contains tests that doesn't work well when > > done > > > in parallel (i.e. they rely exclusively on system resources like audio > > devices, > > > network card etc). > > > It's here: > > > https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?rcl=0&l=752 > > > > > > It should be easy to just move the line in > > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/BUILD.gn?rcl=... > > > into that test binary instead. > > > > > > But... if there's a way to make these tests run fine in parallel, that's > where > > > we should put the effort (it doesn't sound impossible to me). > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2337073007/diff/440001/webrtc/modules/desktop... > > > > File webrtc/modules/desktop_capture/screen_drawer_win.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2337073007/diff/440001/webrtc/modules/desktop... > > > > webrtc/modules/desktop_capture/screen_drawer_win.cc:37: > > > > ScreenDrawerLock::ScreenDrawerLock() { > > > > it looks like this code is duplicated between the two platforms, even > though > > > it > > > > implements the same functionality. Is it possible to avoid this > duplication? > > > > As I have described in the comments, I have added a system wide lock to ensure > > only one instance is running at a certain time. I have also added logic to > > ensure the drawer window is always top-most. I believe currently ScreenDrawer > > tests are safe enough to not be broken by others. But it may impact other > tests > > if they have same requirements. > > Henrik, what's your suggestion for these tests? > > I can't speak for the tests but if Sergey is OK with this, let's submit it and > see how it goes. > There's still an error on Dr Memory Light though, which probably caused by the > fact that it runs much slower than a regular machine. Yes, I am trying to recover it. Or maybe the windows OS is in an interesting state, we cannot set the topmost window.
On 2016/10/18 18:51:20, Hzj_jie wrote: > On 2016/10/18 05:02:08, kjellander_webrtc wrote: > > On 2016/10/17 22:05:32, Hzj_jie wrote: > > > On 2016/10/17 11:47:15, kjellander_webrtc wrote: > > > > On 2016/10/14 17:38:25, Sergey Ulanov wrote: > > > > > It looks to me that this change solves the problem in the case when two > > > > > ScreenCapturer tests run in parallel, but the test may still be flaky > when > > > > > running concurrently with any other tests that update screen content. So > I > > > > think > > > > > to make the test reliable it should be running in isolation, to insure > > that > > > > > there are no other tests that may affect screen content. > > > > > Henrik, do you know if there is a way to achieve that? > > > > > > > > Yes, we have special binary that contains tests that doesn't work well > when > > > done > > > > in parallel (i.e. they rely exclusively on system resources like audio > > > devices, > > > > network card etc). > > > > It's here: > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?rcl=0&l=752 > > > > > > > > It should be easy to just move the line in > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/BUILD.gn?rcl=... > > > > into that test binary instead. > > > > > > > > But... if there's a way to make these tests run fine in parallel, that's > > where > > > > we should put the effort (it doesn't sound impossible to me). > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2337073007/diff/440001/webrtc/modules/desktop... > > > > > File webrtc/modules/desktop_capture/screen_drawer_win.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2337073007/diff/440001/webrtc/modules/desktop... > > > > > webrtc/modules/desktop_capture/screen_drawer_win.cc:37: > > > > > ScreenDrawerLock::ScreenDrawerLock() { > > > > > it looks like this code is duplicated between the two platforms, even > > though > > > > it > > > > > implements the same functionality. Is it possible to avoid this > > duplication? > > > > > > As I have described in the comments, I have added a system wide lock to > ensure > > > only one instance is running at a certain time. I have also added logic to > > > ensure the drawer window is always top-most. I believe currently > ScreenDrawer > > > tests are safe enough to not be broken by others. But it may impact other > > tests > > > if they have same requirements. > > > Henrik, what's your suggestion for these tests? > > > > I can't speak for the tests but if Sergey is OK with this, let's submit it and > > see how it goes. > > There's still an error on Dr Memory Light though, which probably caused by the > > fact that it runs much slower than a regular machine. > > Yes, I am trying to recover it. Or maybe the windows OS is in an interesting > state, we cannot set the topmost window. One of four runs failed, I prefer to submit this change first, and debug the case.
https://codereview.chromium.org/2337073007/diff/440001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_drawer_win.cc (right): https://codereview.chromium.org/2337073007/diff/440001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_drawer_win.cc:37: ScreenDrawerLock::ScreenDrawerLock() { On 2016/10/14 17:38:25, Sergey Ulanov wrote: > it looks like this code is duplicated between the two platforms, even though it > implements the same functionality. Is it possible to avoid this duplication? Yes, part of the logic is shareable. I will make a change. https://codereview.chromium.org/2337073007/diff/500001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_drawer.cc (right): https://codereview.chromium.org/2337073007/diff/500001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_drawer.cc:2: * Copyright 2016 The WebRTC project authors. All Rights Reserved. On 2016/10/18 03:26:56, Sergey Ulanov wrote: > please keep (c) here. > (standard chromium header doesn't have (c), but it's not the same for WebRTC. I > think there is actually a presubmit check for this) Unfortunately, presubmit check has not caught this. https://codereview.chromium.org/2337073007/diff/500001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_drawer.h (right): https://codereview.chromium.org/2337073007/diff/500001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_drawer.h:23: // A cross application lock to ensure only one ScreenDrawer can be used at a On 2016/10/18 03:26:56, Sergey Ulanov wrote: > s/cross application/cross-process/ Done. https://codereview.chromium.org/2337073007/diff/500001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_drawer_linux.cc (right): https://codereview.chromium.org/2337073007/diff/500001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_drawer_linux.cc:164: XRaiseWindow(display_->display(), window_); On 2016/10/18 03:26:56, Sergey Ulanov wrote: > If there is another test running in parallel it may still show another window on > top of this window. Maybe set _NET_WM_STATE_ABOVE, to ensure that our window is > always on top? > Same for windows. Another window can do exactly the same thing, but yes, it's a good point, especially on Windows. Some system windows may have already done this, such as windows update notification window.
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/2337073007/#ps540001 (title: "Still disable real screen capturer tests")
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 ========== Deflaky ScreenCapturerTest ScreenCapturer tests may fail on trybot, so this change is to fix the issue. Changes include, 1. Sometimes, a capturer may capture part of the change, i.e. usually the draw actions are not atomic. So the updated_region may be inaccurate. So I have added a MayDrawIncompleteShapes() function in ScreenDrawer. If it returns false, the updated_region check will be ignored. 2. Several test cases may run concurrently, which makes one ScreenDrawer won't really work. Its window may be covered by another ScreenDrawer. So I have added a system wide lock to ensure only one ScreenDrawer is working at a certain time. 3. On unity (Linux), the top several pixels of a window may be covered by a shadow effect if the window is not focused. So I have added a BringToFront() function, and call it in WaitForPendingDraws(). 4. On Windows, the drawn shapes are 'temporary drawing', which will be erased once the window is covered by another one. So I repeat DrawRectangle() function call in the test case. TODO(zijiehe): The DISABLED_ prefixes will be added back after the code review. And I will move these test cases into modules_test in a coming change. BUG=647067 ========== to ========== Deflaky ScreenCapturerTest ScreenCapturer tests may fail on trybot, so this change is to fix the issue. Changes include, 1. Sometimes, a capturer may capture part of the change, i.e. usually the draw actions are not atomic. So the updated_region may be inaccurate. So I have added a MayDrawIncompleteShapes() function in ScreenDrawer. If it returns false, the updated_region check will be ignored. 2. Several test cases may run concurrently, which makes one ScreenDrawer won't really work. Its window may be covered by another ScreenDrawer. So I have added a system wide lock to ensure only one ScreenDrawer is working at a certain time. 3. On unity (Linux), the top several pixels of a window may be covered by a shadow effect if the window is not focused. So I have added a BringToFront() function, and call it in WaitForPendingDraws(). 4. On Windows, the drawn shapes are 'temporary drawing', which will be erased once the window is covered by another one. So I repeat DrawRectangle() function call in the test case. TODO(zijiehe): The DISABLED_ prefixes will be added back after the code review. And I will move these test cases into modules_test in a coming change. BUG=647067 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:540001)
Message was sent while issue was closed.
Description was changed from ========== Deflaky ScreenCapturerTest ScreenCapturer tests may fail on trybot, so this change is to fix the issue. Changes include, 1. Sometimes, a capturer may capture part of the change, i.e. usually the draw actions are not atomic. So the updated_region may be inaccurate. So I have added a MayDrawIncompleteShapes() function in ScreenDrawer. If it returns false, the updated_region check will be ignored. 2. Several test cases may run concurrently, which makes one ScreenDrawer won't really work. Its window may be covered by another ScreenDrawer. So I have added a system wide lock to ensure only one ScreenDrawer is working at a certain time. 3. On unity (Linux), the top several pixels of a window may be covered by a shadow effect if the window is not focused. So I have added a BringToFront() function, and call it in WaitForPendingDraws(). 4. On Windows, the drawn shapes are 'temporary drawing', which will be erased once the window is covered by another one. So I repeat DrawRectangle() function call in the test case. TODO(zijiehe): The DISABLED_ prefixes will be added back after the code review. And I will move these test cases into modules_test in a coming change. BUG=647067 ========== to ========== Deflaky ScreenCapturerTest ScreenCapturer tests may fail on trybot, so this change is to fix the issue. Changes include, 1. Sometimes, a capturer may capture part of the change, i.e. usually the draw actions are not atomic. So the updated_region may be inaccurate. So I have added a MayDrawIncompleteShapes() function in ScreenDrawer. If it returns false, the updated_region check will be ignored. 2. Several test cases may run concurrently, which makes one ScreenDrawer won't really work. Its window may be covered by another ScreenDrawer. So I have added a system wide lock to ensure only one ScreenDrawer is working at a certain time. 3. On unity (Linux), the top several pixels of a window may be covered by a shadow effect if the window is not focused. So I have added a BringToFront() function, and call it in WaitForPendingDraws(). 4. On Windows, the drawn shapes are 'temporary drawing', which will be erased once the window is covered by another one. So I repeat DrawRectangle() function call in the test case. TODO(zijiehe): The DISABLED_ prefixes will be added back after the code review. And I will move these test cases into modules_test in a coming change. BUG=647067 Committed: https://crrev.com/6a4607e1006560ce598f1e1d79fd120308b9e67f Cr-Commit-Position: refs/heads/master@{#14674} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6a4607e1006560ce598f1e1d79fd120308b9e67f Cr-Commit-Position: refs/heads/master@{#14674} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
