Skip to content

Commit

Permalink
Fix clearing of mouse cursor if display uses a scale factor
Browse files Browse the repository at this point in the history
When a display uses a scale factor (different than 1.0) the previous
cursor position is not properly cleared during a CRD connection on
ChromeOS (see b/235191365).

The issue was that the fix for crbug.com/1323241 does not take device
scaling into account, so that fix would incorrectly not mark the
previous location of the mouse cursor as modified.

Adding proper boundary checks is hard and risky though, as the way the
position of the mouse cursor is reported seems to be platform dependent
(ChromeOS vs Linux vs ...).
So because crbug.com/1323241 only solves a theoretical crash that is rarely if
ever hit in the field, I decided to for now undo the fix for crbug.com/1323241.
A proper boundary check can then later be introduced without any pressure from
a looming release

(cherry picked from commit ff45105)

No-Try: True
Bug: chromium:1323241
Bug: b/235191365
Fixed: b/235191365
Test: Manually deployed
Change-Id: Ib09b6cc5e396bd52538332edfc4395ed80c6786e
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/265391
Reviewed-by: Alexander Cooper <alcooper@chromium.org>
Reviewed-by: Joe Downing <joedow@google.com>
Commit-Queue: Jeroen Dhollander <jeroendh@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#37274}
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/266491
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/branch-heads/5112@{#2}
Cr-Branched-From: a976a87-refs/heads/main@{#37168}
  • Loading branch information
Jeroen Dhollander authored and WebRTC LUCI CQ committed Jun 24, 2022
1 parent e083ccc commit 024e131
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 40 deletions.
11 changes: 6 additions & 5 deletions modules/desktop_capture/desktop_and_cursor_composer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,12 @@ DesktopFrameWithCursor::DesktopFrameWithCursor(

if (!previous_cursor_rect.equals(cursor_rect_)) {
mutable_updated_region()->AddRect(cursor_rect_);

// Only add the previous cursor if it is inside the frame.
// (it can be outside the frame if the desktop has been resized).
if (original_frame_->rect().ContainsRect(previous_cursor_rect))
mutable_updated_region()->AddRect(previous_cursor_rect);
// TODO(crbug:1323241) Update this code to properly handle the case where
// |previous_cursor_rect| is outside of the boundaries of |frame|.
// Any boundary check has to take into account the fact that
// |previous_cursor_rect| can be in DPI or in pixels, based on the platform
// we're running on.
mutable_updated_region()->AddRect(previous_cursor_rect);
} else if (cursor_changed) {
mutable_updated_region()->AddRect(cursor_rect_);
}
Expand Down
35 changes: 0 additions & 35 deletions modules/desktop_capture/desktop_and_cursor_composer_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -105,15 +105,6 @@ MouseCursor* CreateTestCursor(DesktopVector hotspot) {
return new MouseCursor(image.release(), hotspot);
}

std::vector<DesktopRect> GetUpdatedRegions(const DesktopFrame& frame) {
std::vector<DesktopRect> result;
for (webrtc::DesktopRegion::Iterator i(frame.updated_region()); !i.IsAtEnd();
i.Advance()) {
result.push_back(i.rect());
}
return result;
}

class FakeScreenCapturer : public DesktopCapturer {
public:
FakeScreenCapturer() {}
Expand Down Expand Up @@ -436,32 +427,6 @@ TEST_F(DesktopAndCursorComposerNoCursorMonitorTest,
EXPECT_TRUE(frame_->updated_region().Equals(expected_region));
}

TEST_F(DesktopAndCursorComposerNoCursorMonitorTest,
UpdatedRegionDoesNotIncludeOldCursorIfOutOfBounds) {
blender_.OnMouseCursor(CreateTestCursor(DesktopVector(0, 0)));

std::unique_ptr<SharedDesktopFrame> first_frame(
SharedDesktopFrame::Wrap(CreateTestFrame(1000, 1000)));
blender_.OnMouseCursorPosition(DesktopVector(900, 900));
fake_screen_->SetNextFrame(first_frame->Share());

blender_.CaptureFrame();

// Second frame is smaller than first frame, and the first cursor is outside
// of the bounds of the new frame, so it should not be in the updated region.
std::unique_ptr<SharedDesktopFrame> second_frame(
SharedDesktopFrame::Wrap(CreateTestFrame(500, 500)));
auto second_cursor_rect =
DesktopRect::MakeXYWH(400, 400, kCursorWidth, kCursorHeight);
blender_.OnMouseCursorPosition(DesktopVector(400, 400));
fake_screen_->SetNextFrame(second_frame->Share());
blender_.CaptureFrame();

DesktopRegion expected_region;
expected_region.AddRect(second_cursor_rect);
EXPECT_THAT(GetUpdatedRegions(*frame_), ElementsAre(second_cursor_rect));
}

TEST_F(DesktopAndCursorComposerNoCursorMonitorTest,
UpdatedRegionIncludesOldAndNewCursorRectsIfShapeChanged) {
std::unique_ptr<SharedDesktopFrame> frame(
Expand Down

0 comments on commit 024e131

Please sign in to comment.