[WebRTC] A real ScreenCapturer test

webrtc/modules/desktop_capture/screen_drawer_linux.cc

Index: webrtc/modules/desktop_capture/screen_drawer_linux.cc
diff --git a/webrtc/modules/desktop_capture/screen_drawer_linux.cc b/webrtc/modules/desktop_capture/screen_drawer_linux.cc
index 2aff80b6c7ce1259f48f32ef94b12121f0256ac5..f73271422e58175cb4d5b32eada6fe32faf5223e 100644
--- a/webrtc/modules/desktop_capture/screen_drawer_linux.cc
+++ b/webrtc/modules/desktop_capture/screen_drawer_linux.cc
@@ -13,6 +13,7 @@
 #include "webrtc/base/checks.h"
 #include "webrtc/modules/desktop_capture/screen_drawer.h"
 #include "webrtc/modules/desktop_capture/x11/shared_x_display.h"
+#include "webrtc/system_wrappers/include/sleep.h"
 
 namespace webrtc {
 
@@ -26,12 +27,12 @@ class ScreenDrawerLinux : public ScreenDrawer {
 
   // ScreenDrawer interface.
   DesktopRect DrawableRegion() override;
-  void DrawRectangle(DesktopRect rect, uint32_t rgba) override;
+  void DrawRectangle(DesktopRect rect, Color color) override;
   void Clear() override;
+  void WaitForPendingPaintings() override;
 
  private:
   rtc::scoped_refptr<SharedXDisplay> display_;
-  Screen* screen_;
   int screen_num_;
   DesktopRect rect_;
   Window window_;
@@ -42,15 +43,18 @@ class ScreenDrawerLinux : public ScreenDrawer {
 ScreenDrawerLinux::ScreenDrawerLinux() {
   display_ = SharedXDisplay::CreateDefault();
   RTC_CHECK(display_.get());
-  screen_ = DefaultScreenOfDisplay(display_->display());
-  RTC_CHECK(screen_);
   screen_num_ = DefaultScreen(display_->display());
-  rect_ = DesktopRect::MakeWH(screen_->width, screen_->height);
-  window_ = XCreateSimpleWindow(display_->display(),
-                                RootWindow(display_->display(), screen_num_), 0,
-                                0, rect_.width(), rect_.height(), 0,
-                                BlackPixel(display_->display(), screen_num_),
-                                BlackPixel(display_->display(), screen_num_));
+  XWindowAttributes root_attributes;
+  if (!XGetWindowAttributes(display_->display(),
+                            RootWindow(display_->display(), screen_num_),
+                            &root_attributes)) {
+    RTC_DCHECK(false) << "Failed to get root window size.";
+  }
+  window_ = XCreateSimpleWindow(
+      display_->display(), RootWindow(display_->display(), screen_num_), 0, 0,
+      root_attributes.width, root_attributes.height, 0,
+      BlackPixel(display_->display(), screen_num_),
+      BlackPixel(display_->display(), screen_num_));
   XSelectInput(display_->display(), window_, StructureNotifyMask);
   XMapWindow(display_->display(), window_);
   while (true) {
@@ -61,6 +65,17 @@ ScreenDrawerLinux::ScreenDrawerLinux() {
     }
   }
   XFlush(display_->display());
+  Window child;
+  int x, y;
+  if (!XTranslateCoordinates(display_->display(), window_,
+                             RootWindow(display_->display(), screen_num_), 0, 0,
+                             &x, &y, &child)) {
+    RTC_DCHECK(false) << "Failed to get window position.";
+  }
+  // In two monitor system, if this window is on the first monitor, part of the
+  // DrawableRegion() may be covered by another window.

[Jamie, 2016/08/26 22:29:10]

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.

[Hzj_jie, 2016/08/29 21:57:28]

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.

[Jamie, 2016/08/31 17:39:39]

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.

[Hzj_jie, 2016/08/31 21:22:39]

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).
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.

+  rect_ = DesktopRect::MakeLTRB(x, y, root_attributes.width,
+                                root_attributes.height);

[Jamie, 2016/08/26 22:29:10]

I think you need to add x and y to width and height, respectively to get right
and bottom.

[Hzj_jie, 2016/08/29 21:57:28]

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.

   context_ = DefaultGC(display_->display(), screen_num_);
   colormap_ = DefaultColormap(display_->display(), screen_num_);
 }
@@ -74,33 +89,42 @@ DesktopRect ScreenDrawerLinux::DrawableRegion() {
   return rect_;
 }
 
-void ScreenDrawerLinux::DrawRectangle(DesktopRect rect, uint32_t rgba) {
-  int r = (rgba & 0xff00) >> 8;
-  int g = (rgba & 0xff0000) >> 16;
-  int b = (rgba & 0xff000000) >> 24;
+void ScreenDrawerLinux::DrawRectangle(DesktopRect rect, Color color) {
+  XColor xcolor;
   // X11 does not support Alpha.
-  XColor color;
   // X11 uses 16 bits for each primary color.
-  color.red = r * 256;
-  color.green = g * 256;
-  color.blue = b * 256;
-  color.flags = DoRed | DoGreen | DoBlue;
-  XAllocColor(display_->display(), colormap_, &color);
-  XSetForeground(display_->display(), context_, color.pixel);
+  xcolor.red = color.red() * 256 + color.red();
+  xcolor.green = color.green() * 256 + color.green();
+  xcolor.blue = color.blue() * 256 + color.blue();
+  xcolor.flags = DoRed | DoGreen | DoBlue;
+  XAllocColor(display_->display(), colormap_, &xcolor);
+  XSetForeground(display_->display(), context_, xcolor.pixel);
   XFillRectangle(display_->display(), window_, context_, rect.left(),
                  rect.top(), rect.width(), rect.height());
   XFlush(display_->display());
 }
 
 void ScreenDrawerLinux::Clear() {
-  DrawRectangle(DrawableRegion(), 0);
+  DesktopRect rect =
+      DesktopRect::MakeWH(DrawableRegion().width(), DrawableRegion().height());
+  DrawRectangle(rect, Color::FromBGR(0));

[Jamie, 2016/08/26 22:29:10]

I think this method is simpler (doesn't need to be changed) if a single
coordinate space is used

[Hzj_jie, 2016/08/29 21:57:28]

Done.

+}
+
+// TODO(zijiehe): Find the right signal from X11 to indicate the finish of all
+// pending paintings.
+void ScreenDrawerLinux::WaitForPendingPaintings() {
+  SleepMs(50);
 }
 
 }  // namespace
 
 // static
 std::unique_ptr<ScreenDrawer> ScreenDrawer::Create() {
-  return std::unique_ptr<ScreenDrawer>(new ScreenDrawerLinux());
+  if (SharedXDisplay::CreateDefault().get()) {

[Jamie, 2016/08/26 22:29:10]

What is this needed for? Does it belong in this CL, or is it an unrelated
bugfix?

[Hzj_jie, 2016/08/29 21:57:28]

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.

+    return std::unique_ptr<ScreenDrawer>(new ScreenDrawerLinux());
+  } else {
+    return nullptr;
+  }
 }
 
 }  // namespace webrtc