Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2268093002:
[WebRTC] A real ScreenCapturer test (Closed)
Description[WebRTC] A real ScreenCapturer test
We do not have a real ScreenCapturer test before. And after CL 2210443002, a new
ScreenDrawer interface is added to the code base to draw various shapes on the
screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides test
cases, some other changes are included,
1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a
ScreenDrawer to finish all the pending draws. This function now only sleeps 50
milliseconds on X11 and 100 milliseconds on Windows.
2. A Color structure to help handle a big-endian or little-endian safe color and
provide functions to compare with DesktopFrame::data(). Both ScreenDrawer and
DesktopFrameGenerator (in change 2202443002) can use this class to create colors
and compare with or paint to a DesktopFrame.
3. ScreenDrawer now uses Color structure instead of uint32_t.
BUG=314516
TBR=kjellander@chromium.org
Committed: https://crrev.com/9d1c54ace0dc9f68da0152aa1ded2a8dba0a43ae
Cr-Commit-Position: refs/heads/master@{#14058}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Resolve review comments #
Total comments: 53
Patch Set 3 : Resolve review comments #
Total comments: 6
Patch Set 4 : Resolve review comments #Patch Set 5 : Sync latest changes #Patch Set 6 : Remove DirectX capturer changes #
Total comments: 9
Patch Set 7 : Resolve review comments #
Total comments: 8
Patch Set 8 : Resolve review comments #Patch Set 9 : Merge from 2313653003 #Messages
Total messages: 59 (29 generated)
Description was changed from ========== [WebRTC] A real ScreenCapturer test We do not have a real ScreenCapturer test before. And after CL 2210443002, a new ScreenDrawer interface is added to the code base to draw various shapes on the screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides a very simple case, some other changes are included, 1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a ScreenDrawer to finish all the pending paintins. This function now only sleeps 500 milliseconds on both X11 and Windows. 2. ScreenPainter. Instead of using ScreenDrawer, we can use ScreenPainter to draw more complex shapes, which uses a platform dependent ScreenDrawer to do the real drawing work. It also provides a Color nested class to represent a platform independent color, with comparison functions. 3. A ScreenDrawer calibration logic has been added to ScreenCapturerTest. It helps to calculate the position difference between ScreenDrawer and ScreenCapturer. i.e. ScreenDrawer may not be able to draw at (0, 0) in the ScreenCapturer visible area. BUG=314516 ========== to ========== [WebRTC] A real ScreenCapturer test We do not have a real ScreenCapturer test before. And after CL 2210443002, a new ScreenDrawer interface is added to the code base to draw various shapes on the screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides a very simple case, some other changes are included, 1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a ScreenDrawer to finish all the pending paintins. This function now only sleeps 500 milliseconds on both X11 and Windows. 2. ScreenPainter. Instead of using ScreenDrawer, we can use ScreenPainter to draw more complex shapes, which uses a platform dependent ScreenDrawer to do the real drawing work. It also provides a Color nested class to represent a platform independent color, with comparison functions. 3. A ScreenDrawer calibration logic has been added to ScreenCapturerTest. It helps to calculate the position difference between ScreenDrawer and ScreenCapturer. i.e. ScreenDrawer may not be able to draw at (0, 0) in the ScreenCapturer visible area. BUG=314516 ==========
zijiehe@chromium.org changed reviewers: + jamiewalch@chromium.org, sergeyu@chromium.org
I've only looked at the ScreenPainter class so far. I think the changes I've suggested there will require changes to the rest of the code. https://codereview.chromium.org/2268093002/diff/1/webrtc/modules/desktop_capt... File webrtc/modules/desktop_capture/screen_painter.cc (right): https://codereview.chromium.org/2268093002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/screen_painter.cc:37: memset(&result, 0, sizeof(Color)); This will give black, not white. https://codereview.chromium.org/2268093002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/screen_painter.cc:49: result.bgra[DesktopFrame::kBytesPerPixel - 1] = 0; I think you want 255 here to ensure that colours are rendered opaque, even if the platform ignores alpha on capture. https://codereview.chromium.org/2268093002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/screen_painter.cc:91: // non-zero for others, following logic may fail. Unless you have reason to believe that this is on the critical path, it would be clearer always to use this explicit loop rather than memcmp(). https://codereview.chromium.org/2268093002/diff/1/webrtc/modules/desktop_capt... File webrtc/modules/desktop_capture/screen_painter.h (right): https://codereview.chromium.org/2268093002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/screen_painter.h:30: class ScreenPainter final { I suggest removing this class. Other than randomization (which I don't think belongs here), it doesn't do much apart than clipping, and since you need to take the clip region into account both when drawing and capturing, it seems like it would be better handled by the caller. https://codereview.chromium.org/2268093002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/screen_painter.h:42: static Color White(); I don't think you need this ctor either if you supply an explicit rgb(a) ctor. https://codereview.chromium.org/2268093002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/screen_painter.h:45: static Color Random(); I don't think this file should contain any functions based on randomization. For the Color struct, it would be better to provide a rgba ctor (or just rgb, since you ignore a) and have the caller supply random colours if that's appropriate for your tests (although it generally isn't unless you're fuzzing, or otherwise running the tests more intensively than once per submit). https://codereview.chromium.org/2268093002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/screen_painter.h:73: "No extra fields should be added to Color structure."); You won't need this if you get rid of the memset/memcmp optimizations.
https://codereview.chromium.org/2268093002/diff/1/webrtc/modules/desktop_capt... File webrtc/modules/desktop_capture/screen_painter.cc (right): https://codereview.chromium.org/2268093002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/screen_painter.cc:37: memset(&result, 0, sizeof(Color)); On 2016/08/23 18:50:53, Jamie wrote: > This will give black, not white. Done. https://codereview.chromium.org/2268093002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/screen_painter.cc:49: result.bgra[DesktopFrame::kBytesPerPixel - 1] = 0; On 2016/08/23 18:50:53, Jamie wrote: > I think you want 255 here to ensure that colours are rendered opaque, even if > the platform ignores alpha on capture. Done. https://codereview.chromium.org/2268093002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/screen_painter.cc:91: // non-zero for others, following logic may fail. On 2016/08/23 18:50:53, Jamie wrote: > Unless you have reason to believe that this is on the critical path, it would be > clearer always to use this explicit loop rather than memcmp(). Done. https://codereview.chromium.org/2268093002/diff/1/webrtc/modules/desktop_capt... File webrtc/modules/desktop_capture/screen_painter.h (right): https://codereview.chromium.org/2268093002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/screen_painter.h:30: class ScreenPainter final { On 2016/08/23 18:50:54, Jamie wrote: > I suggest removing this class. Other than randomization (which I don't think > belongs here), it doesn't do much apart than clipping, and since you need to > take the clip region into account both when drawing and capturing, it seems like > it would be better handled by the caller. After talked offline with Joe, I will remove randomization from this class. But I still think this class is useful, because, 1. Color structure should belong here. Considering no platform uses the Color structure, put it in ScreenDrawer seems weird. 2. This class can still provide other convenient functions to draw complex shapes. Such as draw a dot, draw a colorful line according to input color vector. We can surely loop everywhere we draw this kind of lines, but it looks like a redundant work. https://codereview.chromium.org/2268093002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/screen_painter.h:42: static Color White(); On 2016/08/23 18:50:53, Jamie wrote: > I don't think you need this ctor either if you supply an explicit rgb(a) ctor. Done. https://codereview.chromium.org/2268093002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/screen_painter.h:45: static Color Random(); On 2016/08/23 18:50:53, Jamie wrote: > I don't think this file should contain any functions based on randomization. For > the Color struct, it would be better to provide a rgba ctor (or just rgb, since > you ignore a) and have the caller supply random colours if that's appropriate > for your tests (although it generally isn't unless you're fuzzing, or otherwise > running the tests more intensively than once per submit). Done. https://codereview.chromium.org/2268093002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/screen_painter.h:73: "No extra fields should be added to Color structure."); On 2016/08/23 18:50:53, Jamie wrote: > You won't need this if you get rid of the memset/memcmp optimizations. Done.
Patchset #2 (id:20001) has been deleted
Description was changed from ========== [WebRTC] A real ScreenCapturer test We do not have a real ScreenCapturer test before. And after CL 2210443002, a new ScreenDrawer interface is added to the code base to draw various shapes on the screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides a very simple case, some other changes are included, 1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a ScreenDrawer to finish all the pending paintins. This function now only sleeps 500 milliseconds on both X11 and Windows. 2. ScreenPainter. Instead of using ScreenDrawer, we can use ScreenPainter to draw more complex shapes, which uses a platform dependent ScreenDrawer to do the real drawing work. It also provides a Color nested class to represent a platform independent color, with comparison functions. 3. A ScreenDrawer calibration logic has been added to ScreenCapturerTest. It helps to calculate the position difference between ScreenDrawer and ScreenCapturer. i.e. ScreenDrawer may not be able to draw at (0, 0) in the ScreenCapturer visible area. BUG=314516 ========== to ========== [WebRTC] A real ScreenCapturer test We do not have a real ScreenCapturer test before. And after CL 2210443002, a new ScreenDrawer interface is added to the code base to draw various shapes on the screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides test cases, some other changes are included, 1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a ScreenDrawer to finish all the pending paintins. This function now only sleeps 50 milliseconds on X11 and 100 milliseconds on Windows. 2. A Color structure to help handle a big-ending or little-ending safe color and provide functions to compare with DesktopFrame::data(). Both ScreenDrawer and DesktopFrameGenerator (in change 2202443002) can use this class to create colors and compare with or paint to a DesktopFrame. 3. ScreenDrawer now uses Color structure instead of uint32_t. 4. A bug in DirectX capturer to cause the second ScreenCapturerWinDirectx instance to capture correct DesktopFrame. The change also makes DxgiDuplicatorController work with 2+ DesktopFrame queue. Now TwoDirectxCapturers test can pass. BUG=314516 ==========
https://codereview.chromium.org/2268093002/diff/1/webrtc/modules/desktop_capt... File webrtc/modules/desktop_capture/screen_painter.h (right): https://codereview.chromium.org/2268093002/diff/1/webrtc/modules/desktop_capt... webrtc/modules/desktop_capture/screen_painter.h:30: class ScreenPainter final { On 2016/08/26 00:06:49, Hzj_jie wrote: > On 2016/08/23 18:50:54, Jamie wrote: > > I suggest removing this class. Other than randomization (which I don't think > > belongs here), it doesn't do much apart than clipping, and since you need to > > take the clip region into account both when drawing and capturing, it seems > like > > it would be better handled by the caller. > > After talked offline with Joe, I will remove randomization from this class. But > I still think this class is useful, because, > 1. Color structure should belong here. Considering no platform uses the Color > structure, put it in ScreenDrawer seems weird. > 2. This class can still provide other convenient functions to draw complex > shapes. Such as draw a dot, draw a colorful line according to input color > vector. We can surely loop everywhere we draw this kind of lines, but it looks > like a redundant work. Sorry, my bad, I forgot to update this comment.
It seems like part (4) of this CL is not really related to the other three, and I'm having trouble separating it out from the rest. I think it would be better to put it in a separate CL (it will probably mean it lands faster too, since it seems like the smaller part). https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/color.cc (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/color.cc:22: } Do we have any capturers that return a meaningful alpha? I think they all return a post-compositing pixel value, in which case alpha is meaningless. If so, then it's better simply to ignore it when comparing (in principle, it still makes sense when rendering, but I can't think of a useful test that would require it). https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/color.h (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/color.h:20: // A four bytes strcture to store a color in BGRA format, which can be converted s/four bytes/four-byte/ s/strcture/structure/ https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/color.h:23: // or to uint32_t format, it's big-ending and little-ending safe, but it always s/ending/endian/ https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/color.h:40: uint8_t alpha); I think it would be better to include only this ctor. You're only using the uint32 version to set black, for which I think (0,0,0) is just as readable. The only place you're using FromBRGA is with a random number which I don't think makes for a very robust test. Optionally, also consider making this a ctor (which is permitted if you don't need to overload it). FromBGRA has the advantage of making the endianness explicit, but it's not particularly idiomatic C++. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/desktop_frame.h (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/desktop_frame.h:72: DesktopVector GetPosAtFrameData(const uint8_t* const pos) const; You don't seem to be using this. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/desktop_geometry.h (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/desktop_geometry.h:95: DesktopVector right_bottom) { I don't think you're using this, and I'm not convinced it's worth adding a helper for it since it's so little work for the caller to do it. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_unittest.cc (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:70: ASSERT_TRUE(updated_region.Equals(DesktopRegion(rect))); It looks like this test will pass if updated_region is larger than expected. If that's deliberate, it should be called out explicitly, though I think a better goal long-term would to have capturers return only the region that has changed and control the test environment tightly enough that other on-screen changes such as a clock are not possible. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:97: capturers.push_back(capturer_.get()); I think the code would be cleaner if you explicitly passed in the capturers to be tested. If nothing else, you wouldn't need a default value for the parameter, which I think is discouraged by the style guide. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:139: rect.Translate(drawer->DrawableRegion().top_left()); This line goes away if you use the same frame-of-reference. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:145: return; Shouldn't this be a test failure? https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:155: bool UseDirectxCapturer() { Since this returns a bool, it's not clear that it's a setter rather than a getter. I suggest calling it something like SetDirectxCaptureMode(). https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:325: std::unique_ptr<ScreenCapturer> capturer2(ScreenCapturer::Create(options)); You could instead take ownership of capturer_ and call SetDirectxCaptureMode to create a second one. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer.h (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer.h:25: // Usually ScreenPainter is preferred, instead of using this class directly. As ScreenPainter no longer exists. Please update the comment. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer.h:40: // will be captured at (100, 100) by a ScreenCapturer. Assuming it won't break existing code, let's try to stick to a single frame-of-reference (the one used by the capturer). Then this comment should be something like: Returns the region inside which DrawRectangle is expected to work, in capturer coordinates (assuming screenCapturer::SelectScreen has not been called). This region excludes regions of the screen reserved by the OS for things like menu bars or app launchers. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer.h:51: // Blocks current thread until OS finishes previous Draw* actions. Since there's only DrawRectangle, Draw* is unnecessarily vague :) https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer.h:54: virtual void WaitForPendingPaintings() = 0; s/Paintings/Draws/ (since that's the verb the corresponding function uses). https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer_linux.cc (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.cc:76: // DrawableRegion() may be covered by another window. I don't understand this comment. If you're explaining that the presence of toolbars or app launchers may affect the position of this window then I think that's better done in the header, as it's not specific to UNIX and is the entire reason for the DrawableRegion method existing. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.cc:78: root_attributes.height); I think you need to add x and y to width and height, respectively to get right and bottom. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.cc:110: DrawRectangle(rect, Color::FromBGR(0)); I think this method is simpler (doesn't need to be changed) if a single coordinate space is used https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.cc:123: if (SharedXDisplay::CreateDefault().get()) { What is this needed for? Does it belong in this CL, or is it an unrelated bugfix? https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer_win.cc (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_win.cc:93: // Rectangle function cannot draw a 1 pixel rectangle. These special cases don't look right. The documentation for Rectangle states: The rectangle that is drawn excludes the bottom and right edges. In other words, if a single-pixel rectangle doesn't work, it suggests that an arbitrary rectangle will be 1px smaller that you asked for in each direction. Maybe you just need to add 1 to both dimensions? https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.h (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.h:47: // Sequential calls Duplicate function of all the DxgiOutputDuplicators owned s/Sequential/Sequentially/
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Thanks for the comments Jamie. The 4th issue is found by the ScreenCapturerTest.TwoDirectxCapturers. I can surely move it to a separated change, but then I will need to undo part of my change in screen_capturer_unittest.cc file. And indeed, the reason to add ScreenDrawer / ScreenCapturerTest is for testing the specific logic in DirectX capturer (spread updated region to each context). https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/color.cc (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/color.cc:22: } On 2016/08/26 22:29:09, Jamie wrote: > Do we have any capturers that return a meaningful alpha? I think they all return > a post-compositing pixel value, in which case alpha is meaningless. If so, then > it's better simply to ignore it when comparing (in principle, it still makes > sense when rendering, but I can't think of a useful test that would require it). This class will also be used by FrameGenerator (in change 2202443002). The alpha is useful in its scenario. (Though I do not think we can capture an alpha channel for a screen, but a window may be partially transparent.) https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/color.h (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/color.h:20: // A four bytes strcture to store a color in BGRA format, which can be converted On 2016/08/26 22:29:09, Jamie wrote: > s/four bytes/four-byte/ > s/strcture/structure/ Done. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/color.h:23: // or to uint32_t format, it's big-ending and little-ending safe, but it always On 2016/08/26 22:29:09, Jamie wrote: > s/ending/endian/ Done. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/color.h:40: uint8_t alpha); On 2016/08/26 22:29:09, Jamie wrote: > I think it would be better to include only this ctor. You're only using the > uint32 version to set black, for which I think (0,0,0) is just as readable. The > only place you're using FromBRGA is with a random number which I don't think > makes for a very robust test. > > Optionally, also consider making this a ctor (which is permitted if you don't > need to overload it). FromBGRA has the advantage of making the endianness > explicit, but it's not particularly idiomatic C++. Done. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/desktop_frame.h (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/desktop_frame.h:72: DesktopVector GetPosAtFrameData(const uint8_t* const pos) const; On 2016/08/26 22:29:09, Jamie wrote: > You don't seem to be using this. Done. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/desktop_geometry.h (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/desktop_geometry.h:95: DesktopVector right_bottom) { On 2016/08/26 22:29:09, Jamie wrote: > I don't think you're using this, and I'm not convinced it's worth adding a > helper for it since it's so little work for the caller to do it. Sorry, both this change and DesktopFrame::GetPosAtFrameData are for last iteration. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_unittest.cc (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:70: ASSERT_TRUE(updated_region.Equals(DesktopRegion(rect))); On 2016/08/26 22:29:09, Jamie wrote: > It looks like this test will pass if updated_region is larger than expected. If > that's deliberate, it should be called out explicitly, though I think a better > goal long-term would to have capturers return only the region that has changed > and control the test environment tightly enough that other on-screen changes > such as a clock are not possible. Yes, currently it's expected. Before change 2202443002 has been submitted, we cannot guarantee all ScreenCapturer implementations return only the updated region. (At least DirectX capturer won't.) After all ScreenCapturer implementations are wrapped by ScreenCapturerDifferWrapper (in 2202443002), we can expect the changes in ScreenDrawer::DrawableRegion are all controllable. Then we won't need to take care about clock or other areas, they should cover only a very small part of the screen. From the point of view of probability, it's not likely the bug happens only in these small areas. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:97: capturers.push_back(capturer_.get()); On 2016/08/26 22:29:10, Jamie wrote: > I think the code would be cleaner if you explicitly passed in the capturers to > be tested. If nothing else, you wouldn't need a default value for the parameter, > which I think is discouraged by the style guide. Done. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:139: rect.Translate(drawer->DrawableRegion().top_left()); On 2016/08/26 22:29:10, Jamie wrote: > This line goes away if you use the same frame-of-reference. Done. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:145: return; On 2016/08/26 22:29:10, Jamie wrote: > Shouldn't this be a test failure? The assertion has been added in CaptureFrame already. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:155: bool UseDirectxCapturer() { On 2016/08/26 22:29:10, Jamie wrote: > Since this returns a bool, it's not clear that it's a setter rather than a > getter. I suggest calling it something like SetDirectxCaptureMode(). Done. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:325: std::unique_ptr<ScreenCapturer> capturer2(ScreenCapturer::Create(options)); On 2016/08/26 22:29:10, Jamie wrote: > You could instead take ownership of capturer_ and call SetDirectxCaptureMode to > create a second one. Done. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer.h (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer.h:25: // Usually ScreenPainter is preferred, instead of using this class directly. As On 2016/08/26 22:29:10, Jamie wrote: > ScreenPainter no longer exists. Please update the comment. Done. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer.h:40: // will be captured at (100, 100) by a ScreenCapturer. On 2016/08/26 22:29:10, Jamie wrote: > Assuming it won't break existing code, let's try to stick to a single > frame-of-reference (the one used by the capturer). Then this comment should be > something like: > > Returns the region inside which DrawRectangle is expected to work, in capturer > coordinates (assuming screenCapturer::SelectScreen has not been called). This > region excludes regions of the screen reserved by the OS for things like menu > bars or app launchers. Done. One minor change, 'This region _may_ excludes regions of the screen ...'. ScreenDrawerWin can cover entire screen. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer.h:51: // Blocks current thread until OS finishes previous Draw* actions. On 2016/08/26 22:29:10, Jamie wrote: > Since there's only DrawRectangle, Draw* is unnecessarily vague :) Done. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer.h:54: virtual void WaitForPendingPaintings() = 0; On 2016/08/26 22:29:10, Jamie wrote: > s/Paintings/Draws/ (since that's the verb the corresponding function uses). Done. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer_linux.cc (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.cc:76: // DrawableRegion() may be covered by another window. On 2016/08/26 22:29:10, Jamie wrote: > I don't understand this comment. If you're explaining that the presence of > toolbars or app launchers may affect the position of this window then I think > that's better done in the header, as it's not specific to UNIX and is the entire > reason for the DrawableRegion method existing. Not really, AFAICT, this comment is for unity only. I cannot quite tell how many window manager systems may be impacted by this issue. In Windows, creating a full screen window will get a window to cover one of several monitors, the width and height are just the size of the monitor. On unity (default ubuntu desktop environment), the window size is always the combination of all monitors. But indeed if the window is created on the first monitor, the half on the second monitor will be covered by the root window. i.e. You can only see the area on the first monitor. If the window is created on the second monitor, though the second half is out of display area, we can still get a correct DrawableRegion(), since left & width indicate a correct area. (0 to 3840 vs 1920 to 3840). I searched around, but I have not found a simple way to indicate which screen current window is on. So on two-monitor linux based system, this could cause errors if you run the test case on the first monitor with a less than 512 horizontal resolution. Though this is not likely to happen (512 horizontal resolution), it's still a potential issue. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.cc:78: root_attributes.height); On 2016/08/26 22:29:10, Jamie wrote: > I think you need to add x and y to width and height, respectively to get right > and bottom. The explanation is same as the one above. i.e. root_attributes.width and root_attributes.height are always the entire screen size, which covers all monitors. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.cc:110: DrawRectangle(rect, Color::FromBGR(0)); On 2016/08/26 22:29:10, Jamie wrote: > I think this method is simpler (doesn't need to be changed) if a single > coordinate space is used Done. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.cc:123: if (SharedXDisplay::CreateDefault().get()) { On 2016/08/26 22:29:10, Jamie wrote: > What is this needed for? Does it belong in this CL, or is it an unrelated > bugfix? Before this change, this function does not use in trybots. (The ScreenDrawer test is a disabled test.) But AFAICT, some trybots do not have X11, and cause the ScreenCapturerTest.CaptureUpdatedRegion to fail. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer_win.cc (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_win.cc:93: // Rectangle function cannot draw a 1 pixel rectangle. On 2016/08/26 22:29:10, Jamie wrote: > These special cases don't look right. The documentation for Rectangle states: > > The rectangle that is drawn excludes the bottom and right edges. > > In other words, if a single-pixel rectangle doesn't work, it suggests that an > arbitrary rectangle will be 1px smaller that you asked for in each direction. > Maybe you just need to add 1 to both dimensions? The ScreenDrawer::DrawRectangle has exactly the same behavior, i.e drawing a rectangle from (a, b) to (c, d) will cover only (a, b) to (c - 1, d - 1). (Excludes the bottom and right edges.) So if c == a + 1, i.e. 1 pixel width, it's only a line from (a, b) to (a, d - 1). And since I am not using PS_NULL, (AFAICT, it's BLACK_PEN with a different color.) the border is also painted by |color|. I believe this is a limitation of Windows Rectangle function, i.e. 1px height or width rectangle is not existing in Windows GDI world. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.h (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.h:47: // Sequential calls Duplicate function of all the DxgiOutputDuplicators owned On 2016/08/26 22:29:10, Jamie wrote: > s/Sequential/Sequentially/ Done.
Description was changed from ========== [WebRTC] A real ScreenCapturer test We do not have a real ScreenCapturer test before. And after CL 2210443002, a new ScreenDrawer interface is added to the code base to draw various shapes on the screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides test cases, some other changes are included, 1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a ScreenDrawer to finish all the pending paintins. This function now only sleeps 50 milliseconds on X11 and 100 milliseconds on Windows. 2. A Color structure to help handle a big-ending or little-ending safe color and provide functions to compare with DesktopFrame::data(). Both ScreenDrawer and DesktopFrameGenerator (in change 2202443002) can use this class to create colors and compare with or paint to a DesktopFrame. 3. ScreenDrawer now uses Color structure instead of uint32_t. 4. A bug in DirectX capturer to cause the second ScreenCapturerWinDirectx instance to capture correct DesktopFrame. The change also makes DxgiDuplicatorController work with 2+ DesktopFrame queue. Now TwoDirectxCapturers test can pass. BUG=314516 ========== to ========== [WebRTC] A real ScreenCapturer test We do not have a real ScreenCapturer test before. And after CL 2210443002, a new ScreenDrawer interface is added to the code base to draw various shapes on the screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides test cases, some other changes are included, 1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a ScreenDrawer to finish all the pending paintins. This function now only sleeps 50 milliseconds on X11 and 100 milliseconds on Windows. 2. A Color structure to help handle a big-endian or little-endian safe color and provide functions to compare with DesktopFrame::data(). Both ScreenDrawer and DesktopFrameGenerator (in change 2202443002) can use this class to create colors and compare with or paint to a DesktopFrame. 3. ScreenDrawer now uses Color structure instead of uint32_t. 4. A bug in DirectX capturer to cause the second ScreenCapturerWinDirectx instance to capture correct DesktopFrame. The change also makes DxgiDuplicatorController work with 2+ DesktopFrame queue. Now TwoDirectxCapturers test can pass. BUG=314516 ==========
Regarding splitting part 4 out of this CL, I think the right question to ask is "Would I need to reland this if the rest of the CL was permanently reverted?" If the answer is yes then it doesn't belong in the same CL. If you are going to leave it in this CL, please provide mode context regarding what the bug is: "A bug in DirectX capturer to cause the second ScreenCapturerWinDirectx instance to capture correct DesktopFrame" doesn't give me any clue as to what the proposed fix is supposed to do so I can't verify that it's correct. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/color.cc (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/color.cc:22: } On 2016/08/29 21:57:27, Hzj_jie wrote: > On 2016/08/26 22:29:09, Jamie wrote: > > Do we have any capturers that return a meaningful alpha? I think they all > return > > a post-compositing pixel value, in which case alpha is meaningless. If so, > then > > it's better simply to ignore it when comparing (in principle, it still makes > > sense when rendering, but I can't think of a useful test that would require > it). > > This class will also be used by FrameGenerator (in change 2202443002). The alpha > is useful in its scenario. (Though I do not think we can capture an alpha > channel for a screen, but a window may be partially transparent.) Assuming that this class will only be used by tests, that's my point; if the capturer cannot capture alpha values, then what are we testing by specifying an alpha != 255 when painting? Either we know what colour we're blending with (in which case why not just render the resultant colour fully-opaque?) or we don't (in which case the test will be flaky). https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_unittest.cc (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:70: ASSERT_TRUE(updated_region.Equals(DesktopRegion(rect))); On 2016/08/29 21:57:28, Hzj_jie wrote: > On 2016/08/26 22:29:09, Jamie wrote: > > It looks like this test will pass if updated_region is larger than expected. > If > > that's deliberate, it should be called out explicitly, though I think a better > > goal long-term would to have capturers return only the region that has changed > > and control the test environment tightly enough that other on-screen changes > > such as a clock are not possible. > > Yes, currently it's expected. Before change 2202443002 has been submitted, we > cannot guarantee all ScreenCapturer implementations return only the updated > region. (At least DirectX capturer won't.) > > After all ScreenCapturer implementations are wrapped by > ScreenCapturerDifferWrapper (in 2202443002), we can expect the changes in > ScreenDrawer::DrawableRegion are all controllable. Then we won't need to take > care about clock or other areas, they should cover only a very small part of the > screen. From the point of view of probability, it's not likely the bug happens > only in these small areas. So will you update this test to be more exact once CL 2202443002 has landed, or will it still be too difficult to ensure that no genuine changes occur outside the expected area? https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer.h (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer.h:40: // will be captured at (100, 100) by a ScreenCapturer. On 2016/08/29 21:57:28, Hzj_jie wrote: > On 2016/08/26 22:29:10, Jamie wrote: > > Assuming it won't break existing code, let's try to stick to a single > > frame-of-reference (the one used by the capturer). Then this comment should be > > something like: > > > > Returns the region inside which DrawRectangle is expected to work, in capturer > > coordinates (assuming screenCapturer::SelectScreen has not been called). This > > region excludes regions of the screen reserved by the OS for things like menu > > bars or app launchers. > > Done. > One minor change, 'This region _may_ excludes regions of the screen ...'. > ScreenDrawerWin can cover entire screen. That also works (but if you add "may" you also need to use "exclude", not "excludes"). https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer_linux.cc (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.cc:76: // DrawableRegion() may be covered by another window. On 2016/08/29 21:57:28, Hzj_jie wrote: > On 2016/08/26 22:29:10, Jamie wrote: > > I don't understand this comment. If you're explaining that the presence of > > toolbars or app launchers may affect the position of this window then I think > > that's better done in the header, as it's not specific to UNIX and is the > entire > > reason for the DrawableRegion method existing. > > Not really, AFAICT, this comment is for unity only. I cannot quite tell how many > window manager systems may be impacted by this issue. > In Windows, creating a full screen window will get a window to cover one of > several monitors, the width and height are just the size of the monitor. > On unity (default ubuntu desktop environment), the window size is always the > combination of all monitors. But indeed if the window is created on the first > monitor, the half on the second monitor will be covered by the root window. i.e. > You can only see the area on the first monitor. If the window is created on the > second monitor, though the second half is out of display area, we can still get > a correct DrawableRegion(), since left & width indicate a correct area. (0 to > 3840 vs 1920 to 3840). > > I searched around, but I have not found a simple way to indicate which screen > current window is on. So on two-monitor linux based system, this could cause > errors if you run the test case on the first monitor with a less than 512 > horizontal resolution. Though this is not likely to happen (512 horizontal > resolution), it's still a potential issue. If I'm understanding the code correctly, I think the problem is both that the comment is unclear and the code is wrong. If (x,y) might not be (0,0) (presumably because the WM might not map the window where you asked it to) then you need to adjust width and height accordingly since if the top-left of the window is not at (0,0) then there's a region at the bottom right that isn't drawable. With that fix, you can move the comment above the call to XTranslateCoordinates and make it more high-level; something like: Find the actual bounding-rectangle for the window, since the window-manager is free to ignore the proposed size and position. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer_win.cc (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_win.cc:93: // Rectangle function cannot draw a 1 pixel rectangle. On 2016/08/29 21:57:28, Hzj_jie wrote: > On 2016/08/26 22:29:10, Jamie wrote: > > These special cases don't look right. The documentation for Rectangle states: > > > > The rectangle that is drawn excludes the bottom and right edges. > > > > In other words, if a single-pixel rectangle doesn't work, it suggests that an > > arbitrary rectangle will be 1px smaller that you asked for in each direction. > > Maybe you just need to add 1 to both dimensions? > > The ScreenDrawer::DrawRectangle has exactly the same behavior, i.e drawing a > rectangle from (a, b) to (c, d) will cover only (a, b) to (c - 1, d - 1). > (Excludes the bottom and right edges.) So if c == a + 1, i.e. 1 pixel width, > it's only a line from (a, b) to (a, d - 1). > > And since I am not using PS_NULL, (AFAICT, it's BLACK_PEN with a different > color.) the border is also painted by |color|. > > I believe this is a limitation of Windows Rectangle function, i.e. 1px height or > width rectangle is not existing in Windows GDI world. Thanks for the clarification. https://codereview.chromium.org/2268093002/diff/100001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/color.h (right): https://codereview.chromium.org/2268093002/diff/100001/webrtc/modules/desktop... webrtc/modules/desktop_capture/color.h:52: uint32_t ToUInt32() const; I don't think you're using this method. Please remove it. https://codereview.chromium.org/2268093002/diff/100001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_unittest.cc (right): https://codereview.chromium.org/2268093002/diff/100001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:96: std::initializer_list<ScreenCapturer*> capturers) { Nice :) I didn't know about initializer_list (or that we were allowed to use it, though it seems that we are). https://codereview.chromium.org/2268093002/diff/100001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_drawer_unittest.cc (right): https://codereview.chromium.org/2268093002/diff/100001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_drawer_unittest.cc:30: LOG(LS_WARNING) << "No ScreenDrawer implementation for current platform."; Use different logs for the two failure cases; DrawableRegion being empty does not mean that there's no ScreenDrawer implementation for the platform.
Description was changed from ========== [WebRTC] A real ScreenCapturer test We do not have a real ScreenCapturer test before. And after CL 2210443002, a new ScreenDrawer interface is added to the code base to draw various shapes on the screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides test cases, some other changes are included, 1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a ScreenDrawer to finish all the pending paintins. This function now only sleeps 50 milliseconds on X11 and 100 milliseconds on Windows. 2. A Color structure to help handle a big-endian or little-endian safe color and provide functions to compare with DesktopFrame::data(). Both ScreenDrawer and DesktopFrameGenerator (in change 2202443002) can use this class to create colors and compare with or paint to a DesktopFrame. 3. ScreenDrawer now uses Color structure instead of uint32_t. 4. A bug in DirectX capturer to cause the second ScreenCapturerWinDirectx instance to capture correct DesktopFrame. The change also makes DxgiDuplicatorController work with 2+ DesktopFrame queue. Now TwoDirectxCapturers test can pass. BUG=314516 ========== to ========== [WebRTC] A real ScreenCapturer test We do not have a real ScreenCapturer test before. And after CL 2210443002, a new ScreenDrawer interface is added to the code base to draw various shapes on the screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides test cases, some other changes are included, 1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a ScreenDrawer to finish all the pending paintins. This function now only sleeps 50 milliseconds on X11 and 100 milliseconds on Windows. 2. A Color structure to help handle a big-endian or little-endian safe color and provide functions to compare with DesktopFrame::data(). Both ScreenDrawer and DesktopFrameGenerator (in change 2202443002) can use this class to create colors and compare with or paint to a DesktopFrame. 3. ScreenDrawer now uses Color structure instead of uint32_t. 4. DirectX capturer won't be able to update the DesktopFrame for the second ScreenCapturerWinDirectx instance, if AcquireNextFrame() returns timeout or unchanged error. Current solution uses |last_frame| of the second ScreenCapturerWinDirectx instance, which also does not contain the updated frame between this and last AcquireNextFrame() calls. Considering following situation, (C1: capturer 1, C2: capturer 2, update: screen updated, next AcquireNextFrame() call will return a new frame, Fx: a frame x) update -> C2.capture returns F1 -> update -> C1.capture returns F2 -> C2.capture unchanged So using F1 to update the last capture frame is not correct, we need to use F2. The change also makes DxgiDuplicatorController work with 2+ DesktopFrame queue. Now TwoDirectxCapturers test can pass. BUG=314516 ==========
Patchset #5 (id:140001) has been deleted
Description was changed from ========== [WebRTC] A real ScreenCapturer test We do not have a real ScreenCapturer test before. And after CL 2210443002, a new ScreenDrawer interface is added to the code base to draw various shapes on the screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides test cases, some other changes are included, 1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a ScreenDrawer to finish all the pending paintins. This function now only sleeps 50 milliseconds on X11 and 100 milliseconds on Windows. 2. A Color structure to help handle a big-endian or little-endian safe color and provide functions to compare with DesktopFrame::data(). Both ScreenDrawer and DesktopFrameGenerator (in change 2202443002) can use this class to create colors and compare with or paint to a DesktopFrame. 3. ScreenDrawer now uses Color structure instead of uint32_t. 4. DirectX capturer won't be able to update the DesktopFrame for the second ScreenCapturerWinDirectx instance, if AcquireNextFrame() returns timeout or unchanged error. Current solution uses |last_frame| of the second ScreenCapturerWinDirectx instance, which also does not contain the updated frame between this and last AcquireNextFrame() calls. Considering following situation, (C1: capturer 1, C2: capturer 2, update: screen updated, next AcquireNextFrame() call will return a new frame, Fx: a frame x) update -> C2.capture returns F1 -> update -> C1.capture returns F2 -> C2.capture unchanged So using F1 to update the last capture frame is not correct, we need to use F2. The change also makes DxgiDuplicatorController work with 2+ DesktopFrame queue. Now TwoDirectxCapturers test can pass. BUG=314516 ========== to ========== [WebRTC] A real ScreenCapturer test We do not have a real ScreenCapturer test before. And after CL 2210443002, a new ScreenDrawer interface is added to the code base to draw various shapes on the screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides test cases, some other changes are included, 1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a ScreenDrawer to finish all the pending paintins. This function now only sleeps 50 milliseconds on X11 and 100 milliseconds on Windows. 2. A Color structure to help handle a big-endian or little-endian safe color and provide functions to compare with DesktopFrame::data(). Both ScreenDrawer and DesktopFrameGenerator (in change 2202443002) can use this class to create colors and compare with or paint to a DesktopFrame. 3. ScreenDrawer now uses Color structure instead of uint32_t. 4. DirectX capturer won't be able to update the DesktopFrame for the second ScreenCapturerWinDirectx instance, if AcquireNextFrame() returns timeout or unchanged error. Current solution uses |last_frame| of the second ScreenCapturerWinDirectx instance, which also does not contain the updated frame between this and last AcquireNextFrame() calls. Considering following situation, (C1: capturer 1, C2: capturer 2, update: screen updated, next AcquireNextFrame() call will return a new frame, Fx: a frame x) update -> C2.capture returns F1 -> update -> C1.capture returns F2 -> C2.capture unchanged So using F1 to update the last capture frame is not correct, we need to use F2. Refer to design doc https://goo.gl/hU1ifG for a detail description. The change also makes DxgiDuplicatorController work with 2+ DesktopFrame queue. Now TwoDirectxCapturers test can pass. BUG=314516 ==========
https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/color.cc (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/color.cc:22: } On 2016/08/31 17:39:39, Jamie wrote: > On 2016/08/29 21:57:27, Hzj_jie wrote: > > On 2016/08/26 22:29:09, Jamie wrote: > > > Do we have any capturers that return a meaningful alpha? I think they all > > return > > > a post-compositing pixel value, in which case alpha is meaningless. If so, > > then > > > it's better simply to ignore it when comparing (in principle, it still makes > > > sense when rendering, but I can't think of a useful test that would require > > it). > > > > This class will also be used by FrameGenerator (in change 2202443002). The > alpha > > is useful in its scenario. (Though I do not think we can capture an alpha > > channel for a screen, but a window may be partially transparent.) > > Assuming that this class will only be used by tests, that's my point; if the > capturer cannot capture alpha values, then what are we testing by specifying an > alpha != 255 when painting? Either we know what colour we're blending with (in > which case why not just render the resultant colour fully-opaque?) or we don't > (in which case the test will be flaky). Oh, I should explain more about the scenario in change 2202443002. That change uses a FrameGenerator and a FakeScreenCapturer / FakeDesktopCapturer, i.e. they can generate frames with meaningful alpha channel. And we use the FakeScreenCapturer / FakeDesktopCapturer to test wrapper layers, ex. ScreenCapturerDifferWrapper. Though it's not likely to happen, we do not expect the wrapper layers to change the alpha channel. In that case, the alpha channel is useful. I used to add a parameter for Color constructor, but it looks over-engineering. We do not have to use 0 or 255 in FrameGenerator for alpha channel. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_unittest.cc (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:70: ASSERT_TRUE(updated_region.Equals(DesktopRegion(rect))); On 2016/08/31 17:39:39, Jamie wrote: > On 2016/08/29 21:57:28, Hzj_jie wrote: > > On 2016/08/26 22:29:09, Jamie wrote: > > > It looks like this test will pass if updated_region is larger than expected. > > If > > > that's deliberate, it should be called out explicitly, though I think a > better > > > goal long-term would to have capturers return only the region that has > changed > > > and control the test environment tightly enough that other on-screen changes > > > such as a clock are not possible. > > > > Yes, currently it's expected. Before change 2202443002 has been submitted, we > > cannot guarantee all ScreenCapturer implementations return only the updated > > region. (At least DirectX capturer won't.) > > > > After all ScreenCapturer implementations are wrapped by > > ScreenCapturerDifferWrapper (in 2202443002), we can expect the changes in > > ScreenDrawer::DrawableRegion are all controllable. Then we won't need to take > > care about clock or other areas, they should cover only a very small part of > the > > screen. From the point of view of probability, it's not likely the bug happens > > only in these small areas. > > So will you update this test to be more exact once CL 2202443002 has landed, or > will it still be too difficult to ensure that no genuine changes occur outside > the expected area? Yes, all the area covered by ScreenDrawer::DrawableRegion() should be fully controllable. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer.h (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer.h:40: // will be captured at (100, 100) by a ScreenCapturer. On 2016/08/31 17:39:39, Jamie wrote: > On 2016/08/29 21:57:28, Hzj_jie wrote: > > On 2016/08/26 22:29:10, Jamie wrote: > > > Assuming it won't break existing code, let's try to stick to a single > > > frame-of-reference (the one used by the capturer). Then this comment should > be > > > something like: > > > > > > Returns the region inside which DrawRectangle is expected to work, in > capturer > > > coordinates (assuming screenCapturer::SelectScreen has not been called). > This > > > region excludes regions of the screen reserved by the OS for things like > menu > > > bars or app launchers. > > > > Done. > > One minor change, 'This region _may_ excludes regions of the screen ...'. > > ScreenDrawerWin can cover entire screen. > > That also works (but if you add "may" you also need to use "exclude", not > "excludes"). :( https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer_linux.cc (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.cc:76: // DrawableRegion() may be covered by another window. On 2016/08/31 17:39:39, Jamie wrote: > On 2016/08/29 21:57:28, Hzj_jie wrote: > > On 2016/08/26 22:29:10, Jamie wrote: > > > I don't understand this comment. If you're explaining that the presence of > > > toolbars or app launchers may affect the position of this window then I > think > > > that's better done in the header, as it's not specific to UNIX and is the > > entire > > > reason for the DrawableRegion method existing. > > > > Not really, AFAICT, this comment is for unity only. I cannot quite tell how > many > > window manager systems may be impacted by this issue. > > In Windows, creating a full screen window will get a window to cover one of > > several monitors, the width and height are just the size of the monitor. > > On unity (default ubuntu desktop environment), the window size is always the > > combination of all monitors. But indeed if the window is created on the first > > monitor, the half on the second monitor will be covered by the root window. > i.e. > > You can only see the area on the first monitor. If the window is created on > the > > second monitor, though the second half is out of display area, we can still > get > > a correct DrawableRegion(), since left & width indicate a correct area. (0 to > > 3840 vs 1920 to 3840). > > > > I searched around, but I have not found a simple way to indicate which screen > > current window is on. So on two-monitor linux based system, this could cause > > errors if you run the test case on the first monitor with a less than 512 > > horizontal resolution. Though this is not likely to happen (512 horizontal > > resolution), it's still a potential issue. > > If I'm understanding the code correctly, I think the problem is both that the > comment is unclear and the code is wrong. If (x,y) might not be (0,0) > (presumably because the WM might not map the window where you asked it to) then > you need to adjust width and height accordingly since if the top-left of the > window is not at (0,0) then there's a region at the bottom right that isn't > drawable. No, the size of window does not need to be adjusted, as the consumers use DrawableRegion(). i.e. A window is 1024 * 768, but is placed at (512, 512), the DrawableRegion() is (512, 512) - (1024, 768). > > With that fix, you can move the comment above the call to XTranslateCoordinates > and make it more high-level; something like: > > Find the actual bounding-rectangle for the window, since the window-manager is > free to ignore the proposed size and position. But on unity with two or more monitors, things become complex. Say two monitors are both 1024 * 768, the window is on the first monitor, with size 2048 * 768. But the second half, (1024, 0) - (2048, 768) won't be able to show up, as unity does not allow a window to cover both monitors. (A real full screen window). But if the window is on the second monitor, nothing goes wrong, i.e. window is placed at (1024, 0), with size (2048, 768), so DrawableRegion() is (1024, 0) - (2048, 768), which is expected. So, yes, I have not explained it correctly, the real problem is not because of the location which WM places the window, but whether the WM allows a real full screen window. I updated the comment to explain this issue more clearly. https://codereview.chromium.org/2268093002/diff/100001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/color.h (right): https://codereview.chromium.org/2268093002/diff/100001/webrtc/modules/desktop... webrtc/modules/desktop_capture/color.h:52: uint32_t ToUInt32() const; On 2016/08/31 17:39:39, Jamie wrote: > I don't think you're using this method. Please remove it. This will be used in change 2202443002. I can surely add it later. https://codereview.chromium.org/2268093002/diff/100001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_unittest.cc (right): https://codereview.chromium.org/2268093002/diff/100001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:96: std::initializer_list<ScreenCapturer*> capturers) { On 2016/08/31 17:39:39, Jamie wrote: > Nice :) I didn't know about initializer_list (or that we were allowed to use it, > though it seems that we are). That's a good question. I have searched around in chromium code base, some comments show initializer_list was not banned before. But I have not seen it would trigger an error in cpplint (https://cs.chromium.org/chromium/tools/depot_tools/cpplint.py?q=initializer_l...). And there are only comments regarding to use initializer_list in constructor, (https://cs.chromium.org/chromium/src/styleguide/c%2B%2B/c%2B%2B11.html?q=init...). I have also found several using cases in chromium. I believe it should be fine for me to use it here and in this way. https://codereview.chromium.org/2268093002/diff/100001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_drawer_unittest.cc (right): https://codereview.chromium.org/2268093002/diff/100001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_drawer_unittest.cc:30: LOG(LS_WARNING) << "No ScreenDrawer implementation for current platform."; On 2016/08/31 17:39:39, Jamie wrote: > Use different logs for the two failure cases; DrawableRegion being empty does > not mean that there's no ScreenDrawer implementation for the platform. Done.
https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/color.cc (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/color.cc:22: } On 2016/08/31 17:39:39, Jamie wrote: > On 2016/08/29 21:57:27, Hzj_jie wrote: > > On 2016/08/26 22:29:09, Jamie wrote: > > > Do we have any capturers that return a meaningful alpha? I think they all > > return > > > a post-compositing pixel value, in which case alpha is meaningless. If so, > > then > > > it's better simply to ignore it when comparing (in principle, it still makes > > > sense when rendering, but I can't think of a useful test that would require > > it). > > > > This class will also be used by FrameGenerator (in change 2202443002). The > alpha > > is useful in its scenario. (Though I do not think we can capture an alpha > > channel for a screen, but a window may be partially transparent.) > > Assuming that this class will only be used by tests, that's my point; if the > capturer cannot capture alpha values, then what are we testing by specifying an > alpha != 255 when painting? Either we know what colour we're blending with (in > which case why not just render the resultant colour fully-opaque?) or we don't > (in which case the test will be flaky). Oh, I should explain more about the scenario in change 2202443002. That change uses a FrameGenerator and a FakeScreenCapturer / FakeDesktopCapturer, i.e. they can generate frames with meaningful alpha channel. And we use the FakeScreenCapturer / FakeDesktopCapturer to test wrapper layers, ex. ScreenCapturerDifferWrapper. Though it's not likely to happen, we do not expect the wrapper layers to change the alpha channel. In that case, the alpha channel is useful. I used to add a parameter for Color constructor, but it looks over-engineering. We do not have to use 0 or 255 in FrameGenerator for alpha channel. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_capturer_unittest.cc (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:70: ASSERT_TRUE(updated_region.Equals(DesktopRegion(rect))); On 2016/08/31 17:39:39, Jamie wrote: > On 2016/08/29 21:57:28, Hzj_jie wrote: > > On 2016/08/26 22:29:09, Jamie wrote: > > > It looks like this test will pass if updated_region is larger than expected. > > If > > > that's deliberate, it should be called out explicitly, though I think a > better > > > goal long-term would to have capturers return only the region that has > changed > > > and control the test environment tightly enough that other on-screen changes > > > such as a clock are not possible. > > > > Yes, currently it's expected. Before change 2202443002 has been submitted, we > > cannot guarantee all ScreenCapturer implementations return only the updated > > region. (At least DirectX capturer won't.) > > > > After all ScreenCapturer implementations are wrapped by > > ScreenCapturerDifferWrapper (in 2202443002), we can expect the changes in > > ScreenDrawer::DrawableRegion are all controllable. Then we won't need to take > > care about clock or other areas, they should cover only a very small part of > the > > screen. From the point of view of probability, it's not likely the bug happens > > only in these small areas. > > So will you update this test to be more exact once CL 2202443002 has landed, or > will it still be too difficult to ensure that no genuine changes occur outside > the expected area? Yes, all the area covered by ScreenDrawer::DrawableRegion() should be fully controllable. https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer.h (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer.h:40: // will be captured at (100, 100) by a ScreenCapturer. On 2016/08/31 17:39:39, Jamie wrote: > On 2016/08/29 21:57:28, Hzj_jie wrote: > > On 2016/08/26 22:29:10, Jamie wrote: > > > Assuming it won't break existing code, let's try to stick to a single > > > frame-of-reference (the one used by the capturer). Then this comment should > be > > > something like: > > > > > > Returns the region inside which DrawRectangle is expected to work, in > capturer > > > coordinates (assuming screenCapturer::SelectScreen has not been called). > This > > > region excludes regions of the screen reserved by the OS for things like > menu > > > bars or app launchers. > > > > Done. > > One minor change, 'This region _may_ excludes regions of the screen ...'. > > ScreenDrawerWin can cover entire screen. > > That also works (but if you add "may" you also need to use "exclude", not > "excludes"). :( https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... File webrtc/modules/desktop_capture/screen_drawer_linux.cc (right): https://codereview.chromium.org/2268093002/diff/40001/webrtc/modules/desktop_... webrtc/modules/desktop_capture/screen_drawer_linux.cc:76: // DrawableRegion() may be covered by another window. On 2016/08/31 17:39:39, Jamie wrote: > On 2016/08/29 21:57:28, Hzj_jie wrote: > > On 2016/08/26 22:29:10, Jamie wrote: > > > I don't understand this comment. If you're explaining that the presence of > > > toolbars or app launchers may affect the position of this window then I > think > > > that's better done in the header, as it's not specific to UNIX and is the > > entire > > > reason for the DrawableRegion method existing. > > > > Not really, AFAICT, this comment is for unity only. I cannot quite tell how > many > > window manager systems may be impacted by this issue. > > In Windows, creating a full screen window will get a window to cover one of > > several monitors, the width and height are just the size of the monitor. > > On unity (default ubuntu desktop environment), the window size is always the > > combination of all monitors. But indeed if the window is created on the first > > monitor, the half on the second monitor will be covered by the root window. > i.e. > > You can only see the area on the first monitor. If the window is created on > the > > second monitor, though the second half is out of display area, we can still > get > > a correct DrawableRegion(), since left & width indicate a correct area. (0 to > > 3840 vs 1920 to 3840). > > > > I searched around, but I have not found a simple way to indicate which screen > > current window is on. So on two-monitor linux based system, this could cause > > errors if you run the test case on the first monitor with a less than 512 > > horizontal resolution. Though this is not likely to happen (512 horizontal > > resolution), it's still a potential issue. > > If I'm understanding the code correctly, I think the problem is both that the > comment is unclear and the code is wrong. If (x,y) might not be (0,0) > (presumably because the WM might not map the window where you asked it to) then > you need to adjust width and height accordingly since if the top-left of the > window is not at (0,0) then there's a region at the bottom right that isn't > drawable. No, the size of window does not need to be adjusted, as the consumers use DrawableRegion(). i.e. A window is 1024 * 768, but is placed at (512, 512), the DrawableRegion() is (512, 512) - (1024, 768). > > With that fix, you can move the comment above the call to XTranslateCoordinates > and make it more high-level; something like: > > Find the actual bounding-rectangle for the window, since the window-manager is > free to ignore the proposed size and position. But on unity with two or more monitors, things become complex. Say two monitors are both 1024 * 768, the window is on the first monitor, with size 2048 * 768. But the second half, (1024, 0) - (2048, 768) won't be able to show up, as unity does not allow a window to cover both monitors. (A real full screen window). But if the window is on the second monitor, nothing goes wrong, i.e. window is placed at (1024, 0), with size (2048, 768), so DrawableRegion() is (1024, 0) - (2048, 768), which is expected. So, yes, I have not explained it correctly, the real problem is not because of the location which WM places the window, but whether the WM allows a real full screen window. I updated the comment to explain this issue more clearly. https://codereview.chromium.org/2268093002/diff/100001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/color.h (right): https://codereview.chromium.org/2268093002/diff/100001/webrtc/modules/desktop... webrtc/modules/desktop_capture/color.h:52: uint32_t ToUInt32() const; On 2016/08/31 17:39:39, Jamie wrote: > I don't think you're using this method. Please remove it. This will be used in change 2202443002. I can surely add it later. https://codereview.chromium.org/2268093002/diff/100001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_unittest.cc (right): https://codereview.chromium.org/2268093002/diff/100001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:96: std::initializer_list<ScreenCapturer*> capturers) { On 2016/08/31 17:39:39, Jamie wrote: > Nice :) I didn't know about initializer_list (or that we were allowed to use it, > though it seems that we are). That's a good question. I have searched around in chromium code base, some comments show initializer_list was not banned before. But I have not seen it would trigger an error in cpplint (https://cs.chromium.org/chromium/tools/depot_tools/cpplint.py?q=initializer_l...). And there are only comments regarding to use initializer_list in constructor, (https://cs.chromium.org/chromium/src/styleguide/c%2B%2B/c%2B%2B11.html?q=init...). I have also found several using cases in chromium. I believe it should be fine for me to use it here and in this way. https://codereview.chromium.org/2268093002/diff/100001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_drawer_unittest.cc (right): https://codereview.chromium.org/2268093002/diff/100001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_drawer_unittest.cc:30: LOG(LS_WARNING) << "No ScreenDrawer implementation for current platform."; On 2016/08/31 17:39:39, Jamie wrote: > Use different logs for the two failure cases; DrawableRegion being empty does > not mean that there's no ScreenDrawer implementation for the platform. Done.
Description was changed from ========== [WebRTC] A real ScreenCapturer test We do not have a real ScreenCapturer test before. And after CL 2210443002, a new ScreenDrawer interface is added to the code base to draw various shapes on the screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides test cases, some other changes are included, 1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a ScreenDrawer to finish all the pending paintins. This function now only sleeps 50 milliseconds on X11 and 100 milliseconds on Windows. 2. A Color structure to help handle a big-endian or little-endian safe color and provide functions to compare with DesktopFrame::data(). Both ScreenDrawer and DesktopFrameGenerator (in change 2202443002) can use this class to create colors and compare with or paint to a DesktopFrame. 3. ScreenDrawer now uses Color structure instead of uint32_t. 4. DirectX capturer won't be able to update the DesktopFrame for the second ScreenCapturerWinDirectx instance, if AcquireNextFrame() returns timeout or unchanged error. Current solution uses |last_frame| of the second ScreenCapturerWinDirectx instance, which also does not contain the updated frame between this and last AcquireNextFrame() calls. Considering following situation, (C1: capturer 1, C2: capturer 2, update: screen updated, next AcquireNextFrame() call will return a new frame, Fx: a frame x) update -> C2.capture returns F1 -> update -> C1.capture returns F2 -> C2.capture unchanged So using F1 to update the last capture frame is not correct, we need to use F2. Refer to design doc https://goo.gl/hU1ifG for a detail description. The change also makes DxgiDuplicatorController work with 2+ DesktopFrame queue. Now TwoDirectxCapturers test can pass. BUG=314516 ========== to ========== [WebRTC] A real ScreenCapturer test We do not have a real ScreenCapturer test before. And after CL 2210443002, a new ScreenDrawer interface is added to the code base to draw various shapes on the screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides test cases, some other changes are included, 1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a ScreenDrawer to finish all the pending paintins. This function now only sleeps 50 milliseconds on X11 and 100 milliseconds on Windows. 2. A Color structure to help handle a big-endian or little-endian safe color and provide functions to compare with DesktopFrame::data(). Both ScreenDrawer and DesktopFrameGenerator (in change 2202443002) can use this class to create colors and compare with or paint to a DesktopFrame. 3. ScreenDrawer now uses Color structure instead of uint32_t. BUG=314516 ==========
I have not removed the TwoDirectxCapturers test, but mark it as DISABLED. We can enable it once change 2299663003 has been submitted.
LGTM
https://codereview.chromium.org/2268093002/diff/180001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/color.h (right): https://codereview.chromium.org/2268093002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/color.h:47: uint8_t blue() const; since these are lower-case accessors they can be inlined here. https://codereview.chromium.org/2268093002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/color.h:55: uint8_t bgra[DesktopFrame::kBytesPerPixel]; do you really need this as an array? why not just 4 uint8_t fields and then you wouldn't need the 4 methods above. https://codereview.chromium.org/2268093002/diff/180001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_drawer_linux.cc (right): https://codereview.chromium.org/2268093002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_drawer_linux.cc:101: xcolor.red = color.red() * 256 + color.red(); Isn't it the same as color.red() * 257? Same for green and blue https://codereview.chromium.org/2268093002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_drawer_linux.cc:128: } else { no else after return please.
https://codereview.chromium.org/2268093002/diff/180001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_drawer_linux.cc (right): https://codereview.chromium.org/2268093002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_drawer_linux.cc:101: xcolor.red = color.red() * 256 + color.red(); On 2016/09/01 19:26:38, Sergey Ulanov wrote: > Isn't it the same as color.red() * 257? > Same for green and blue It is the same, but I think the explicit addition makes it clearer that we're trying to map a 8-bit to a 16-bit colour space. It might be even more obvious written as color.red() << 8 + color.red()
Patchset #7 (id:200001) has been deleted
Description was changed from ========== [WebRTC] A real ScreenCapturer test We do not have a real ScreenCapturer test before. And after CL 2210443002, a new ScreenDrawer interface is added to the code base to draw various shapes on the screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides test cases, some other changes are included, 1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a ScreenDrawer to finish all the pending paintins. This function now only sleeps 50 milliseconds on X11 and 100 milliseconds on Windows. 2. A Color structure to help handle a big-endian or little-endian safe color and provide functions to compare with DesktopFrame::data(). Both ScreenDrawer and DesktopFrameGenerator (in change 2202443002) can use this class to create colors and compare with or paint to a DesktopFrame. 3. ScreenDrawer now uses Color structure instead of uint32_t. BUG=314516 ========== to ========== [WebRTC] A real ScreenCapturer test We do not have a real ScreenCapturer test before. And after CL 2210443002, a new ScreenDrawer interface is added to the code base to draw various shapes on the screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides test cases, some other changes are included, 1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a ScreenDrawer to finish all the pending draws. This function now only sleeps 50 milliseconds on X11 and 100 milliseconds on Windows. 2. A Color structure to help handle a big-endian or little-endian safe color and provide functions to compare with DesktopFrame::data(). Both ScreenDrawer and DesktopFrameGenerator (in change 2202443002) can use this class to create colors and compare with or paint to a DesktopFrame. 3. ScreenDrawer now uses Color structure instead of uint32_t. BUG=314516 ==========
https://codereview.chromium.org/2268093002/diff/180001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/color.h (right): https://codereview.chromium.org/2268093002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/color.h:47: uint8_t blue() const; On 2016/09/01 19:26:38, Sergey Ulanov wrote: > since these are lower-case accessors they can be inlined here. Done. https://codereview.chromium.org/2268093002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/color.h:55: uint8_t bgra[DesktopFrame::kBytesPerPixel]; On 2016/09/01 19:26:38, Sergey Ulanov wrote: > do you really need this as an array? why not just 4 uint8_t fields and then you > wouldn't need the 4 methods above. Done. https://codereview.chromium.org/2268093002/diff/180001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_drawer_linux.cc (right): https://codereview.chromium.org/2268093002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_drawer_linux.cc:101: xcolor.red = color.red() * 256 + color.red(); On 2016/09/01 19:26:38, Sergey Ulanov wrote: > Isn't it the same as color.red() * 257? > Same for green and blue My initial thought is, "color.red() * 257" looks like a typo. But, yes, I can add some comment to explain. https://codereview.chromium.org/2268093002/diff/180001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_drawer_linux.cc:128: } else { On 2016/09/01 19:26:38, Sergey Ulanov wrote: > no else after return please. Done.
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamiewalch@chromium.org Link to the patchset: https://codereview.chromium.org/2268093002/#ps220001 (title: "Resolve review comments")
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-webrtc-committers". Note that this has nothing to do with OWNERS files.
Sergey, do you have other comments?
several more style nits. LGTM when they are addressed. https://codereview.chromium.org/2268093002/diff/220001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/color.h (right): https://codereview.chromium.org/2268093002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/color.h:26: struct Color final { Maybe call this RgbColor to avoid confusion as WebRTC uses YUV in many places. https://codereview.chromium.org/2268093002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/color.h:35: bool operator==(const uint8_t* const bgra) const; Style guide requires overloaded operators behavior to be obvious: https://google.github.io/styleguide/cppguide.html#Operator_Overloading IMO this operator doesn't fit that requirement because it's comparing Color with uint8_t pointer. Maybe just add another constructor that creates Color from uint8_t array and use it when you need it? https://codereview.chromium.org/2268093002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/color.h:39: bool operator!=(const uint8_t* const bgra) const; I don't think you need the second const here. We normally don't use const for parameters themselves i.e. foo(int i) instead of foo(const int i). https://codereview.chromium.org/2268093002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/color.h:53: DesktopFrame::kBytesPerPixel == sizeof(uint32_t), What about sizeof(Color)?
https://codereview.chromium.org/2268093002/diff/220001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/color.h (right): https://codereview.chromium.org/2268093002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/color.h:26: struct Color final { On 2016/09/02 19:35:00, Sergey Ulanov wrote: > Maybe call this RgbColor to avoid confusion as WebRTC uses YUV in many places. RgbaColor may be better. https://codereview.chromium.org/2268093002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/color.h:35: bool operator==(const uint8_t* const bgra) const; On 2016/09/02 19:35:00, Sergey Ulanov wrote: > Style guide requires overloaded operators behavior to be obvious: > https://google.github.io/styleguide/cppguide.html#Operator_Overloading > IMO this operator doesn't fit that requirement because it's comparing Color with > uint8_t pointer. Maybe just add another constructor that creates Color from > uint8_t array and use it when you need it? Done. https://codereview.chromium.org/2268093002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/color.h:39: bool operator!=(const uint8_t* const bgra) const; On 2016/09/02 19:35:00, Sergey Ulanov wrote: > I don't think you need the second const here. We normally don't use const for > parameters themselves i.e. foo(int i) instead of foo(const int i). Done. https://codereview.chromium.org/2268093002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/color.h:53: DesktopFrame::kBytesPerPixel == sizeof(uint32_t), On 2016/09/02 19:35:00, Sergey Ulanov wrote: > What about sizeof(Color)? It used to be sizeof(Color), but Jamie has a different opinion. (This class used to be able to memcpy & memcmp with uint8_t*.) But since we are 2:1 now, I am happy to change it into sizeof(Color). :)
https://codereview.chromium.org/2268093002/diff/220001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/color.h (right): https://codereview.chromium.org/2268093002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/color.h:26: struct Color final { On 2016/09/02 19:35:00, Sergey Ulanov wrote: > Maybe call this RgbColor to avoid confusion as WebRTC uses YUV in many places. RgbaColor may be better. https://codereview.chromium.org/2268093002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/color.h:35: bool operator==(const uint8_t* const bgra) const; On 2016/09/02 19:35:00, Sergey Ulanov wrote: > Style guide requires overloaded operators behavior to be obvious: > https://google.github.io/styleguide/cppguide.html#Operator_Overloading > IMO this operator doesn't fit that requirement because it's comparing Color with > uint8_t pointer. Maybe just add another constructor that creates Color from > uint8_t array and use it when you need it? Done. https://codereview.chromium.org/2268093002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/color.h:39: bool operator!=(const uint8_t* const bgra) const; On 2016/09/02 19:35:00, Sergey Ulanov wrote: > I don't think you need the second const here. We normally don't use const for > parameters themselves i.e. foo(int i) instead of foo(const int i). Done. https://codereview.chromium.org/2268093002/diff/220001/webrtc/modules/desktop... webrtc/modules/desktop_capture/color.h:53: DesktopFrame::kBytesPerPixel == sizeof(uint32_t), On 2016/09/02 19:35:00, Sergey Ulanov wrote: > What about sizeof(Color)? It used to be sizeof(Color), but Jamie has a different opinion. (This class used to be able to memcpy & memcmp with uint8_t*.) But since we are 2:1 now, I am happy to change it into sizeof(Color). :)
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, jamiewalch@chromium.org Link to the patchset: https://codereview.chromium.org/2268093002/#ps240001 (title: "Resolve review comments")
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/8028)
zijiehe@chromium.org changed reviewers: + kjellander@chromium.org
Hi, Henrik, PTAL. I have added several real ScreenCapturer tests in this change, which impacts webrtc/modules/BUILD.gn.
Description was changed from ========== [WebRTC] A real ScreenCapturer test We do not have a real ScreenCapturer test before. And after CL 2210443002, a new ScreenDrawer interface is added to the code base to draw various shapes on the screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides test cases, some other changes are included, 1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a ScreenDrawer to finish all the pending draws. This function now only sleeps 50 milliseconds on X11 and 100 milliseconds on Windows. 2. A Color structure to help handle a big-endian or little-endian safe color and provide functions to compare with DesktopFrame::data(). Both ScreenDrawer and DesktopFrameGenerator (in change 2202443002) can use this class to create colors and compare with or paint to a DesktopFrame. 3. ScreenDrawer now uses Color structure instead of uint32_t. BUG=314516 ========== to ========== [WebRTC] A real ScreenCapturer test We do not have a real ScreenCapturer test before. And after CL 2210443002, a new ScreenDrawer interface is added to the code base to draw various shapes on the screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides test cases, some other changes are included, 1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a ScreenDrawer to finish all the pending draws. This function now only sleeps 50 milliseconds on X11 and 100 milliseconds on Windows. 2. A Color structure to help handle a big-endian or little-endian safe color and provide functions to compare with DesktopFrame::data(). Both ScreenDrawer and DesktopFrameGenerator (in change 2202443002) can use this class to create colors and compare with or paint to a DesktopFrame. 3. ScreenDrawer now uses Color structure instead of uint32_t. BUG=314516 TBR=kjellander@chromium.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 ========== [WebRTC] A real ScreenCapturer test We do not have a real ScreenCapturer test before. And after CL 2210443002, a new ScreenDrawer interface is added to the code base to draw various shapes on the screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides test cases, some other changes are included, 1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a ScreenDrawer to finish all the pending draws. This function now only sleeps 50 milliseconds on X11 and 100 milliseconds on Windows. 2. A Color structure to help handle a big-endian or little-endian safe color and provide functions to compare with DesktopFrame::data(). Both ScreenDrawer and DesktopFrameGenerator (in change 2202443002) can use this class to create colors and compare with or paint to a DesktopFrame. 3. ScreenDrawer now uses Color structure instead of uint32_t. BUG=314516 TBR=kjellander@chromium.org ========== to ========== [WebRTC] A real ScreenCapturer test We do not have a real ScreenCapturer test before. And after CL 2210443002, a new ScreenDrawer interface is added to the code base to draw various shapes on the screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides test cases, some other changes are included, 1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a ScreenDrawer to finish all the pending draws. This function now only sleeps 50 milliseconds on X11 and 100 milliseconds on Windows. 2. A Color structure to help handle a big-endian or little-endian safe color and provide functions to compare with DesktopFrame::data(). Both ScreenDrawer and DesktopFrameGenerator (in change 2202443002) can use this class to create colors and compare with or paint to a DesktopFrame. 3. ScreenDrawer now uses Color structure instead of uint32_t. BUG=314516 TBR=kjellander@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #8 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== [WebRTC] A real ScreenCapturer test We do not have a real ScreenCapturer test before. And after CL 2210443002, a new ScreenDrawer interface is added to the code base to draw various shapes on the screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides test cases, some other changes are included, 1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a ScreenDrawer to finish all the pending draws. This function now only sleeps 50 milliseconds on X11 and 100 milliseconds on Windows. 2. A Color structure to help handle a big-endian or little-endian safe color and provide functions to compare with DesktopFrame::data(). Both ScreenDrawer and DesktopFrameGenerator (in change 2202443002) can use this class to create colors and compare with or paint to a DesktopFrame. 3. ScreenDrawer now uses Color structure instead of uint32_t. BUG=314516 TBR=kjellander@chromium.org ========== to ========== [WebRTC] A real ScreenCapturer test We do not have a real ScreenCapturer test before. And after CL 2210443002, a new ScreenDrawer interface is added to the code base to draw various shapes on the screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides test cases, some other changes are included, 1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a ScreenDrawer to finish all the pending draws. This function now only sleeps 50 milliseconds on X11 and 100 milliseconds on Windows. 2. A Color structure to help handle a big-endian or little-endian safe color and provide functions to compare with DesktopFrame::data(). Both ScreenDrawer and DesktopFrameGenerator (in change 2202443002) can use this class to create colors and compare with or paint to a DesktopFrame. 3. ScreenDrawer now uses Color structure instead of uint32_t. BUG=314516 TBR=kjellander@chromium.org Committed: https://crrev.com/9d1c54ace0dc9f68da0152aa1ded2a8dba0a43ae Cr-Commit-Position: refs/heads/master@{#14058} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/9d1c54ace0dc9f68da0152aa1ded2a8dba0a43ae Cr-Commit-Position: refs/heads/master@{#14058}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:240001) has been created in https://codereview.webrtc.org/2310953002/ by asapersson@webrtc.org. The reason for reverting is: ScreenCapturerTest.CaptureUpdatedRegion fails on Win DrMemory Full..
Message was sent while issue was closed.
Patchset #9 (id:260001) has been deleted
Message was sent while issue was closed.
Description was changed from ========== [WebRTC] A real ScreenCapturer test We do not have a real ScreenCapturer test before. And after CL 2210443002, a new ScreenDrawer interface is added to the code base to draw various shapes on the screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides test cases, some other changes are included, 1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a ScreenDrawer to finish all the pending draws. This function now only sleeps 50 milliseconds on X11 and 100 milliseconds on Windows. 2. A Color structure to help handle a big-endian or little-endian safe color and provide functions to compare with DesktopFrame::data(). Both ScreenDrawer and DesktopFrameGenerator (in change 2202443002) can use this class to create colors and compare with or paint to a DesktopFrame. 3. ScreenDrawer now uses Color structure instead of uint32_t. BUG=314516 TBR=kjellander@chromium.org Committed: https://crrev.com/9d1c54ace0dc9f68da0152aa1ded2a8dba0a43ae Cr-Commit-Position: refs/heads/master@{#14058} ========== to ========== [WebRTC] A real ScreenCapturer test We do not have a real ScreenCapturer test before. And after CL 2210443002, a new ScreenDrawer interface is added to the code base to draw various shapes on the screen. This change is to use ScreenDrawer to test ScreenCapturer. Besides test cases, some other changes are included, 1. A WaitForPendingPaintings() function in ScreenDrawer, to wait for a ScreenDrawer to finish all the pending draws. This function now only sleeps 50 milliseconds on X11 and 100 milliseconds on Windows. 2. A Color structure to help handle a big-endian or little-endian safe color and provide functions to compare with DesktopFrame::data(). Both ScreenDrawer and DesktopFrameGenerator (in change 2202443002) can use this class to create colors and compare with or paint to a DesktopFrame. 3. ScreenDrawer now uses Color structure instead of uint32_t. BUG=314516 TBR=kjellander@chromium.org Committed: https://crrev.com/9d1c54ace0dc9f68da0152aa1ded2a8dba0a43ae Cr-Commit-Position: refs/heads/master@{#14058} ==========
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, jamiewalch@chromium.org Link to the patchset: https://codereview.webrtc.org/2268093002/#ps280001 (title: "Merge from 2313653003")
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: linux_baremetal on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Message was sent while issue was closed.
An exactly same change 2313653003 has been submitted. So this one should be closed. |
