From fd19db0fbeae321d241c90faec4ed100c2c7037b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Gr=C3=A4=C3=9Flin?= Date: Tue, 28 Jun 2016 08:38:30 +0200 Subject: [PATCH 1/6] Properly clip the damage region and windows in SceneQPainter Summary: On the framebuffer backend the software rendered cursor was creating rendering artifacts due to incorrect clipping. This change ensures that in a single screen setup the clipping is performed correctly. For multi screen a similar change might be required, but we currently don't have any a way to test that as framebuffer only supports one screen and other (visual) platforms don't support the software cursor. CCBUG: 356328 Reviewers: #kwin, #plasma_on_wayland Subscribers: plasma-devel, kwin Tags: #plasma_on_wayland, #kwin Differential Revision: https://phabricator.kde.org/D2024 --- scene_qpainter.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/scene_qpainter.cpp b/scene_qpainter.cpp index bd4b9dd961..0a73ed6315 100644 --- a/scene_qpainter.cpp +++ b/scene_qpainter.cpp @@ -171,6 +171,8 @@ qint64 SceneQPainter::paint(QRegion damage, ToplevelList toplevels) m_backend->present(mask, overallUpdate); } else { m_painter->begin(m_backend->buffer()); + m_painter->setClipping(true); + m_painter->setClipRegion(damage); if (m_backend->needsFullRepaint()) { mask |= Scene::PAINT_SCREEN_BACKGROUND_FIRST; damage = QRegion(0, 0, displayWidth(), displayHeight()); @@ -267,10 +269,10 @@ void SceneQPainter::Window::performPaint(int mask, QRegion region, WindowPaintDa QPainter *scenePainter = m_scene->painter(); QPainter *painter = scenePainter; + painter->save(); painter->setClipRegion(region); painter->setClipping(true); - painter->save(); painter->translate(x(), y()); if (mask & PAINT_WINDOW_TRANSFORMED) { painter->translate(data.xTranslation(), data.yTranslation()); @@ -317,9 +319,6 @@ void SceneQPainter::Window::performPaint(int mask, QRegion region, WindowPaintDa } painter->restore(); - - painter->setClipRegion(QRegion()); - painter->setClipping(false); } void SceneQPainter::Window::renderShadow(QPainter* painter) From feadcea6cea7b74acf4896c85dcb7d577615ff42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Gr=C3=A4=C3=9Flin?= Date: Tue, 28 Jun 2016 08:43:57 +0200 Subject: [PATCH 2/6] [platforms] Call setSoftWareCursor in init instead of ctor Summary: Platform::setSoftWareCursor creates connections to the Cursor in order to trigger repaints whenever the cursor position changes. The Cursor is created before Platform::init is called, but after the Platform is created. Thus the call needs to happen in init, otherwise the cursor is not rendered correctly. BUG: 356328 FIXED-IN: 5.7.0 Reviewers: #kwin, #plasma_on_wayland Subscribers: plasma-devel, kwin Tags: #plasma_on_wayland, #kwin Differential Revision: https://phabricator.kde.org/D2025 --- plugins/platforms/fbdev/fb_backend.cpp | 2 +- plugins/platforms/virtual/virtual_backend.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/platforms/fbdev/fb_backend.cpp b/plugins/platforms/fbdev/fb_backend.cpp index 95b0925e6c..fc7ed42c78 100644 --- a/plugins/platforms/fbdev/fb_backend.cpp +++ b/plugins/platforms/fbdev/fb_backend.cpp @@ -38,7 +38,6 @@ namespace KWin FramebufferBackend::FramebufferBackend(QObject *parent) : Platform(parent) { - setSoftWareCursor(true); } FramebufferBackend::~FramebufferBackend() @@ -61,6 +60,7 @@ QPainterBackend *FramebufferBackend::createQPainterBackend() void FramebufferBackend::init() { + setSoftWareCursor(true); LogindIntegration *logind = LogindIntegration::self(); auto takeControl = [logind, this]() { if (logind->hasSessionControl()) { diff --git a/plugins/platforms/virtual/virtual_backend.cpp b/plugins/platforms/virtual/virtual_backend.cpp index b231fbb5e5..396b56cd54 100644 --- a/plugins/platforms/virtual/virtual_backend.cpp +++ b/plugins/platforms/virtual/virtual_backend.cpp @@ -42,7 +42,6 @@ VirtualBackend::VirtualBackend(QObject *parent) qDebug() << "Screenshots saved to: " << m_screenshotDir->path(); } } - setSoftWareCursor(true); setSupportsPointerWarping(true); } @@ -50,6 +49,7 @@ VirtualBackend::~VirtualBackend() = default; void VirtualBackend::init() { + setSoftWareCursor(true); m_size = initialWindowSize(); setReady(true); waylandServer()->seat()->setHasPointer(true); From 7ea84cd346424161affbfddf653b16b6f90442f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Gr=C3=A4=C3=9Flin?= Date: Tue, 28 Jun 2016 10:20:27 +0200 Subject: [PATCH 3/6] [platforms/drm] If mapping a DrmBuffer for a cursor fails, fallback to software cursor Summary: So far the drm platform did not verify whether creating and mapping a DrmBuffer for a cursor works. This could result in a crash in the worst case. This change verfies whether mapping the two cursor buffers works, if not software cursor is enabled. The code is adjusted to ensure that none of the cursor buffers is accessed in case software cursor are enabled. Please note that right now the drm platform's rendering does not support software cursors. Thus currently this change results in no cursor at all. This will be addressed by following patches. BUG: 364740 FIXED-IN: 5.7 Test Plan: Verfied that it properly falls back to software cursor, but could not verify that the crash is actually fixed. Reviewers: #kwin, #plasma_on_wayland Subscribers: plasma-devel, kwin Tags: #plasma_on_wayland, #kwin Differential Revision: https://phabricator.kde.org/D2026 --- plugins/platforms/drm/drm_backend.cpp | 42 ++++++++++++++++++--------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/plugins/platforms/drm/drm_backend.cpp b/plugins/platforms/drm/drm_backend.cpp index 2467cce3dd..c15f3f4f44 100644 --- a/plugins/platforms/drm/drm_backend.cpp +++ b/plugins/platforms/drm/drm_backend.cpp @@ -152,14 +152,16 @@ void DrmBackend::reactivate() return; } m_active = true; - DrmBuffer *c = m_cursor[(m_cursorIndex + 1) % 2]; - const QPoint cp = Cursor::pos() - softwareCursorHotspot(); - for (auto it = m_outputs.constBegin(); it != m_outputs.constEnd(); ++it) { - DrmOutput *o = *it; - o->pageFlipped(); - o->blank(); - o->showCursor(c); - o->moveCursor(cp); + if (!usesSoftwareCursor()) { + DrmBuffer *c = m_cursor[(m_cursorIndex + 1) % 2]; + const QPoint cp = Cursor::pos() - softwareCursorHotspot(); + for (auto it = m_outputs.constBegin(); it != m_outputs.constEnd(); ++it) { + DrmOutput *o = *it; + o->pageFlipped(); + o->blank(); + o->showCursor(c); + o->moveCursor(cp); + } } // restart compositor m_pageFlipsPending = 0; @@ -473,6 +475,9 @@ void DrmBackend::initCursor() connect(waylandServer()->seat(), &KWayland::Server::SeatInterface::hasPointerChanged, this, [this] { m_cursorEnabled = waylandServer()->seat()->hasPointer(); + if (usesSoftwareCursor()) { + return; + } for (auto it = m_outputs.constBegin(); it != m_outputs.constEnd(); ++it) { if (m_cursorEnabled) { (*it)->showCursor(m_cursor[m_cursorIndex]); @@ -494,12 +499,18 @@ void DrmBackend::initCursor() } else { cursorSize.setHeight(64); } - m_cursor[0] = createBuffer(cursorSize); - m_cursor[0]->map(QImage::Format_ARGB32_Premultiplied); - m_cursor[0]->image()->fill(Qt::transparent); - m_cursor[1] = createBuffer(cursorSize); - m_cursor[1]->map(QImage::Format_ARGB32_Premultiplied); - m_cursor[1]->image()->fill(Qt::transparent); + auto createCursor = [this, cursorSize] (int index) { + m_cursor[index] = createBuffer(cursorSize); + if (!m_cursor[index]->map(QImage::Format_ARGB32_Premultiplied)) { + return false; + } + m_cursor[index]->image()->fill(Qt::transparent); + return true; + }; + if (!createCursor(0) || !createCursor(1)) { + setSoftWareCursor(true); + return; + } // now we have screens and can set cursors, so start tracking connect(this, &DrmBackend::cursorChanged, this, &DrmBackend::updateCursor); connect(Cursor::self(), &Cursor::posChanged, this, &DrmBackend::moveCursor); @@ -519,6 +530,9 @@ void DrmBackend::setCursor() void DrmBackend::updateCursor() { + if (usesSoftwareCursor()) { + return; + } const QImage &cursorImage = softwareCursor(); if (cursorImage.isNull()) { hideCursor(); From 23fb02cce20af7bb14103c3022d3d59bf55158be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Gr=C3=A4=C3=9Flin?= Date: Tue, 28 Jun 2016 10:27:52 +0200 Subject: [PATCH 4/6] Render cursor in multi-screen setup in QPainter Compositor Summary: The call to render the cursor was missing in the multi-screen code path of the QPainter Compositor. Reviewers: #kwin, #plasma_on_wayland Subscribers: plasma-devel, kwin Tags: #plasma_on_wayland, #kwin Differential Revision: https://phabricator.kde.org/D2027 --- scene_qpainter.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/scene_qpainter.cpp b/scene_qpainter.cpp index 0a73ed6315..d0d36a551f 100644 --- a/scene_qpainter.cpp +++ b/scene_qpainter.cpp @@ -163,6 +163,7 @@ qint64 SceneQPainter::paint(QRegion damage, ToplevelList toplevels) QRegion updateRegion, validRegion; paintScreen(&mask, damage.intersected(geometry), QRegion(), &updateRegion, &validRegion); overallUpdate = overallUpdate.united(updateRegion); + m_backend->renderCursor(m_painter.data()); m_painter->restore(); m_painter->end(); From 758d41d6bfa5b0c1b70c31b5a3de4ccd69becf7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Gr=C3=A4=C3=9Flin?= Date: Tue, 28 Jun 2016 10:37:02 +0200 Subject: [PATCH 5/6] Paint the software cursor directly in SceneQPainter Summary: No need to delegate the painting of the software cursor into the backend. The core has enough information to perform the rendering itself. This change means less code duplication and all platforms which might use a software cursor in QPainter compositor gain support for it without any further adjustments. Reviewers: #kwin, #plasma_on_wayland Subscribers: plasma-devel, kwin Tags: #plasma_on_wayland, #kwin Differential Revision: https://phabricator.kde.org/D2028 --- .../fbdev/scene_qpainter_fb_backend.cpp | 15 ------------ .../fbdev/scene_qpainter_fb_backend.h | 1 - .../scene_qpainter_virtual_backend.cpp | 15 ------------ .../virtual/scene_qpainter_virtual_backend.h | 1 - scene_qpainter.cpp | 24 +++++++++++++------ scene_qpainter.h | 2 +- 6 files changed, 18 insertions(+), 40 deletions(-) diff --git a/plugins/platforms/fbdev/scene_qpainter_fb_backend.cpp b/plugins/platforms/fbdev/scene_qpainter_fb_backend.cpp index 7399bad34c..732d5f31fe 100644 --- a/plugins/platforms/fbdev/scene_qpainter_fb_backend.cpp +++ b/plugins/platforms/fbdev/scene_qpainter_fb_backend.cpp @@ -87,19 +87,4 @@ bool FramebufferQPainterBackend::usesOverlayWindow() const return false; } -void FramebufferQPainterBackend::renderCursor(QPainter *painter) -{ - if (!m_backend->usesSoftwareCursor()) { - return; - } - const QImage img = m_backend->softwareCursor(); - if (img.isNull()) { - return; - } - const QPoint cursorPos = Cursor::pos(); - const QPoint hotspot = m_backend->softwareCursorHotspot(); - painter->drawImage(cursorPos - hotspot, img); - m_backend->markCursorAsRendered(); -} - } diff --git a/plugins/platforms/fbdev/scene_qpainter_fb_backend.h b/plugins/platforms/fbdev/scene_qpainter_fb_backend.h index f089533956..73d6268bb0 100644 --- a/plugins/platforms/fbdev/scene_qpainter_fb_backend.h +++ b/plugins/platforms/fbdev/scene_qpainter_fb_backend.h @@ -39,7 +39,6 @@ public: bool usesOverlayWindow() const override; void prepareRenderingFrame() override; void present(int mask, const QRegion &damage) override; - void renderCursor(QPainter *painter) override; private: QImage m_renderBuffer; diff --git a/plugins/platforms/virtual/scene_qpainter_virtual_backend.cpp b/plugins/platforms/virtual/scene_qpainter_virtual_backend.cpp index 9027538093..84ccecf5af 100644 --- a/plugins/platforms/virtual/scene_qpainter_virtual_backend.cpp +++ b/plugins/platforms/virtual/scene_qpainter_virtual_backend.cpp @@ -70,19 +70,4 @@ bool VirtualQPainterBackend::usesOverlayWindow() const return false; } -void VirtualQPainterBackend::renderCursor(QPainter *painter) -{ - if (!m_backend->usesSoftwareCursor()) { - return; - } - const QImage img = m_backend->softwareCursor(); - if (img.isNull()) { - return; - } - const QPoint cursorPos = Cursor::pos(); - const QPoint hotspot = m_backend->softwareCursorHotspot(); - painter->drawImage(cursorPos - hotspot, img); - m_backend->markCursorAsRendered(); -} - } diff --git a/plugins/platforms/virtual/scene_qpainter_virtual_backend.h b/plugins/platforms/virtual/scene_qpainter_virtual_backend.h index a81e9589a0..3c91195ae4 100644 --- a/plugins/platforms/virtual/scene_qpainter_virtual_backend.h +++ b/plugins/platforms/virtual/scene_qpainter_virtual_backend.h @@ -42,7 +42,6 @@ public: void prepareRenderingFrame() override; void present(int mask, const QRegion &damage) override; void screenGeometryChanged(const QSize &size) override; - void renderCursor(QPainter *painter) override; private: QImage m_backBuffer; diff --git a/scene_qpainter.cpp b/scene_qpainter.cpp index d0d36a551f..477aefd5f3 100644 --- a/scene_qpainter.cpp +++ b/scene_qpainter.cpp @@ -73,11 +73,6 @@ void QPainterBackend::setFailed(const QString &reason) m_failed = true; } -void QPainterBackend::renderCursor(QPainter *painter) -{ - Q_UNUSED(painter) -} - bool QPainterBackend::perScreenRendering() const { return false; @@ -163,7 +158,7 @@ qint64 SceneQPainter::paint(QRegion damage, ToplevelList toplevels) QRegion updateRegion, validRegion; paintScreen(&mask, damage.intersected(geometry), QRegion(), &updateRegion, &validRegion); overallUpdate = overallUpdate.united(updateRegion); - m_backend->renderCursor(m_painter.data()); + paintCursor(); m_painter->restore(); m_painter->end(); @@ -181,7 +176,7 @@ qint64 SceneQPainter::paint(QRegion damage, ToplevelList toplevels) QRegion updateRegion, validRegion; paintScreen(&mask, damage, QRegion(), &updateRegion, &validRegion); - m_backend->renderCursor(m_painter.data()); + paintCursor(); m_backend->showOverlay(); m_painter->end(); @@ -200,6 +195,21 @@ void SceneQPainter::paintBackground(QRegion region) m_painter->drawRects(region.rects()); } +void SceneQPainter::paintCursor() +{ + if (!kwinApp()->platform()->usesSoftwareCursor()) { + return; + } + const QImage img = kwinApp()->platform()->softwareCursor(); + if (img.isNull()) { + return; + } + const QPoint cursorPos = Cursor::pos(); + const QPoint hotspot = kwinApp()->platform()->softwareCursorHotspot(); + m_painter->drawImage(cursorPos - hotspot, img); + kwinApp()->platform()->markCursorAsRendered(); +} + Scene::Window *SceneQPainter::createWindow(Toplevel *toplevel) { return new SceneQPainter::Window(this, toplevel); diff --git a/scene_qpainter.h b/scene_qpainter.h index f5b3af0023..2292142662 100644 --- a/scene_qpainter.h +++ b/scene_qpainter.h @@ -80,7 +80,6 @@ public: **/ virtual QImage *bufferForScreen(int screenId); virtual bool needsFullRepaint() const = 0; - virtual void renderCursor(QPainter *painter); /** * Whether the rendering needs to be split per screen. * Default implementation returns @c false. @@ -130,6 +129,7 @@ protected: private: explicit SceneQPainter(QPainterBackend *backend, QObject *parent = nullptr); + void paintCursor(); QScopedPointer m_backend; QScopedPointer m_painter; class Window; From f79c3f244d5728c4243a8315f6ca33d2b5a1c72c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Gr=C3=A4=C3=9Flin?= Date: Sun, 26 Jun 2016 16:27:15 +0200 Subject: [PATCH 6/6] On Wayland never try to create a WindowPixmap from an X11 pixmap Summary: Our Wayland compositors cannot composite an X11 pixmap. Thus even if we succeed in creating the pixmap, the changes would not get to the scene. The code allowed for situation where a surface is not yet set to fall back to the X11 code path. This can happen for XWayland windows. Test Plan: Crash in xclipboardsynctest without the change Reviewers: #plasma_on_wayland, #kwin Subscribers: plasma-devel, kwin Tags: #plasma_on_wayland, #kwin Differential Revision: https://phabricator.kde.org/D2010 --- scene.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scene.cpp b/scene.cpp index 7d9f3d2dd1..834a731e60 100644 --- a/scene.cpp +++ b/scene.cpp @@ -964,7 +964,8 @@ void WindowPixmap::create() if (isValid() || toplevel()->isDeleted()) { return; } - if (toplevel()->surface()) { + // always update from Buffer on Wayland, don't try using XPixmap + if (kwinApp()->shouldUseWaylandForCompositing()) { // use Buffer updateBuffer(); if ((m_buffer || !m_fbo.isNull()) && m_subSurface.isNull()) {