Revert 127257 - Use XDamageSubtract to fetch the damaged region from the X server.

(CL is missing the necessary linker flag for Xfixes)

Previously the damaged area was accumulated based on received XDamage events, and the entire desktop then "repaired" with an XDamageSubtract() call, leading to a race-condition between damage notifications and the repair.

This CL registers only for non-empty damage notifications, and atomically fetches and clears the damage region using XDamageSubtract(), to ensure that no damage is missed.

BUG=116283,117485
TEST=Change the resolution of a Chromoting Me2Me host between two sizes and verify that it is always displayed correctly at the client.


Review URL: http://codereview.chromium.org/9700084

TBR=wez@chromium.org
Review URL: https://chromiumcodereview.appspot.com/9719006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@127267 0039d316-1c4b-4281-b951-d872f2087c98

1 files changed, 38 insertions, 61 deletions

diff --git a/remoting/host/capturer_linux.cc b/remoting/host/capturer_linux.cc
index ec7dbf3..85e03ab 100644
--- a/remoting/host/capturer_linux.cc
+++ b/remoting/host/capturer_linux.cc
@@ -132,7 +132,6 @@ class CapturerLinux : public Capturer {
   Damage damage_handle_;
   int damage_event_base_;
   int damage_error_base_;
-  XserverRegion damage_region_;
 
   // Access to the X Server's pixel buffer.
   XServerPixelBuffer x_server_pixel_buffer_;
@@ -167,10 +166,9 @@ CapturerLinux::CapturerLinux()
       gc_(NULL),
       root_window_(BadValue),
       use_damage_(false),
-      damage_handle_(0),
+      damage_handle_(BadValue),
       damage_event_base_(-1),
       damage_error_base_(-1),
-      damage_region_(0),
       current_buffer_(0),
       pixel_format_(media::VideoFrame::RGB32),
       last_buffer_(NULL) {
@@ -217,43 +215,25 @@ bool CapturerLinux::Init() {
 }
 
 void CapturerLinux::InitXDamage() {
-  // Check for XFixes and XDamage extensions.  If both are found then use
-  // XDamage to get explicit notifications of on-screen changes.
-  int xfixes_event_base;
-  int xfixes_error_base;
-  if (!XFixesQueryExtension(display_, &xfixes_event_base, &xfixes_error_base)) {
-    LOG(INFO) << "X server does not support XFixes.";
-    return;
-  }
-  if (!XDamageQueryExtension(display_, &damage_event_base_,
-                             &damage_error_base_)) {
-    LOG(INFO) << "X server does not support XDamage.";
-    return;
-  }
-
-  // TODO(lambroslambrou): Disable DAMAGE in situations where it is known
-  // to fail, such as when Desktop Effects are enabled, with graphics
-  // drivers (nVidia, ATI) that fail to report DAMAGE notifications
-  // properly.
-
-  // Request notifications every time the screen becomes damaged.
-  damage_handle_ = XDamageCreate(display_, root_window_,
-                                 XDamageReportNonEmpty);
-  if (!damage_handle_) {
-    LOG(ERROR) << "Unable to initialize XDamage.";
-    return;
-  }
-
-  // Create an XFixes server-side region to collate damage into.
-  damage_region_ = XFixesCreateRegion(display_, 0, 0);
-  if (!damage_region_) {
-    XDamageDestroy(display_, damage_handle_);
-    LOG(ERROR) << "Unable to create XFixes region.";
-    return;
+  // Setup XDamage to report changes in the damage window.  Mark the whole
+  // window as invalid.
+  if (XDamageQueryExtension(display_, &damage_event_base_,
+                            &damage_error_base_)) {
+    damage_handle_ = XDamageCreate(display_, root_window_,
+                                   XDamageReportDeltaRectangles);
+    if (damage_handle_ == BadValue) {
+      LOG(ERROR) << "Unable to create damage handle.";
+    } else {
+      // TODO(lambroslambrou): Disable DAMAGE in situations where it is known
+      // to fail, such as when Desktop Effects are enabled, with graphics
+      // drivers (nVidia, ATI) that fail to report DAMAGE notifications
+      // properly.
+      use_damage_ = true;
+      LOG(INFO) << "Using XDamage extension.";
+    }
+  } else {
+    LOG(INFO) << "Server does not support XDamage.";
   }
-
-  use_damage_ = true;
-  LOG(INFO) << "Using XDamage extension.";
 }
 
 void CapturerLinux::ScreenConfigurationChanged() {
@@ -314,18 +294,34 @@ void CapturerLinux::ProcessPendingXEvents() {
   // on XPending because we want to guarantee this terminates.
   int events_to_process = XPending(display_);
   XEvent e;
+  SkRegion invalid_region;
 
   for (int i = 0; i < events_to_process; i++) {
     XNextEvent(display_, &e);
     if (use_damage_ && (e.type == damage_event_base_ + XDamageNotify)) {
       XDamageNotifyEvent *event = reinterpret_cast<XDamageNotifyEvent*>(&e);
-      DCHECK(event->level == XDamageReportNonEmpty);
+
+      // TODO(hclam): Perform more checks on the rect.
+      if (event->area.width <= 0 || event->area.height <= 0)
+        continue;
+
+      SkIRect damage_rect = SkIRect::MakeXYWH(event->area.x,
+                                              event->area.y,
+                                              event->area.width,
+                                              event->area.height);
+      invalid_region.op(damage_rect, SkRegion::kUnion_Op);
+      DVLOG(3) << "Damage received for rect at ("
+               << damage_rect.fLeft << "," << damage_rect.fTop << ") size ("
+               << damage_rect.width() << "," << damage_rect.height() << ")";
     } else if (e.type == ConfigureNotify) {
       ScreenConfigurationChanged();
+      invalid_region.setEmpty();
     } else {
       LOG(WARNING) << "Got unknown event type: " << e.type;
     }
   }
+
+  helper_.InvalidateRegion(invalid_region);
 }
 
 CaptureData* CapturerLinux::CaptureFrame() {
@@ -352,25 +348,12 @@ CaptureData* CapturerLinux::CaptureFrame() {
 
   x_server_pixel_buffer_.Synchronize();
   if (use_damage_ && last_buffer_) {
-    // Atomically fetch and clear the damage region.
-    XDamageSubtract(display_, damage_handle_, None, damage_region_);
-    int nRects = 0;
-    XRectangle bounds;
-    XRectangle* rects = XFixesFetchRegionAndBounds(display_, damage_region_,
-                                                   &nRects, &bounds);
-    for (int i=0; i<nRects; ++i) {
-      invalid_region.op(SkIRect::MakeXYWH(rects[i].x, rects[i].y,
-                                          rects[i].width, rects[i].height),
-                        SkRegion::kUnion_Op);
-    }
-    XFree(rects);
-    helper_.InvalidateRegion(invalid_region);
-
-    // Capture the damaged portions of the desktop.
     helper_.SwapInvalidRegion(&invalid_region);
     for (SkRegion::Iterator it(invalid_region); !it.done(); it.next()) {
       CaptureRect(it.rect(), capture_data);
     }
+    // TODO(ajwong): We should only repair the rects that were copied!
+    XDamageSubtract(display_, damage_handle_, None, None);
   } else {
     // Doing full-screen polling, or this is the first capture after a
     // screen-resolution change.  In either case, need a full-screen capture.
@@ -427,14 +410,8 @@ void CapturerLinux::DeinitXlib() {
   }
 
   if (display_) {
-    if (damage_handle_)
-      XDamageDestroy(display_, damage_handle_);
-    if (damage_region_)
-      XFixesDestroyRegion(display_, damage_region_);
     XCloseDisplay(display_);
     display_ = NULL;
-    damage_handle_ = 0;
-    damage_region_ = 0;
   }
 }