Merge lp:~dandrader/qtubuntu/improve-resize into lp:qtubuntu

Proposed by Daniel d'Andrada on 2015-06-03
Status: Merged
Approved by: Gerry Boland on 2015-06-17
Approved revision: 260
Merged at revision: 266
Proposed branch: lp:~dandrader/qtubuntu/improve-resize
Merge into: lp:qtubuntu
Prerequisite: lp:~mir-team/qtubuntu/track-mir-deprecations
Diff against target: 204 lines (+34/-62)
3 files modified
src/ubuntumirclient/input.cpp (+7/-9)
src/ubuntumirclient/window.cpp (+27/-49)
src/ubuntumirclient/window.h (+0/-4)
To merge this branch: bzr merge lp:~dandrader/qtubuntu/improve-resize
Reviewer Review Type Date Requested Status
Gerry Boland 2015-06-03 Approve on 2015-06-18
PS Jenkins bot continuous-integration 2015-06-03 Approve on 2015-06-03
Review via email: mp+260950@code.launchpad.net

This proposal supersedes a proposal from 2015-05-12.

Commit message

Drastically improve surface resize responsiveness

QWindowSystemInterface::handle[...]() methods are thread safe, thus there's no need to have intermetiate methods between the rendering thread and such calls.

There was also some miscommunication between the now absent handleBufferResize() slot from the main thread and the swapbuffer handler from the render thread.

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Approve (continuous-integration)
Gerry Boland (gerboland) wrote : Posted in a previous version of this proposal

=== modified file 'src/ubuntumirclient/input.cpp'
+/*
+*/
necessary change?

In testing with mir_proving_server, I notice one problem: the surface size QML requests is not the size it gets. Think the geometry.setY() call is problematic now. This used to work before.

Remainder looks good. Am worried that there are 3+ threads (Qt GUI, Qt renderers, Mir input) all interacting with this class tho.

review: Needs Fixing
Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal

On 13/05/15 09:12, Gerry Boland wrote:
> === modified file 'src/ubuntumirclient/input.cpp'
> +/*
> +*/
> necessary change?

Yes, code won't build otherwise in a debug config. I've the same change
in another top-approved-but-not-yet-landed MP.

Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal

> In testing with mir_proving_server, I notice one problem: the surface size QML
> requests is not the size it gets.

This also happens with lp:qtubuntu.

I request (w=600,h=600) and get a geometry of (x=0,y=26,w=600,h=574). It's interesting that h+y=600. That's with mir_proving_server.

PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Gerry Boland (gerboland) wrote :

LGTM

review: Approve
261. By Daniel d'Andrada on 2015-06-17

Refactor optional logging and remove trailing whitespace

Gerry Boland (gerboland) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/ubuntumirclient/input.cpp'
2--- src/ubuntumirclient/input.cpp 2015-06-11 09:10:55 +0000
3+++ src/ubuntumirclient/input.cpp 2015-06-17 13:08:26 +0000
4@@ -157,9 +157,7 @@
5 // Qt will take care of deleting mTouchDevice.
6 }
7
8-#ifndef QT_NO_DEBUG
9-
10-
11+#if (LOG_EVENTS != 0)
12 static const char* nativeEventTypeToStr(MirEventType t)
13 {
14 switch (t)
15@@ -185,9 +183,7 @@
16 return "invalid";
17 }
18 }
19-
20-
21-#endif
22+#endif // LOG_EVENTS != 0
23
24 void UbuntuInput::customEvent(QEvent* event)
25 {
26@@ -209,10 +205,12 @@
27 return;
28 }
29
30- //DLOG("UbuntuInput::customEvent(type=%s)", nativeEventTypeToStr(mir_event_get_type(nativeEvent)));
31+ #if (LOG_EVENTS != 0)
32+ LOG("UbuntuInput::customEvent(type=%s)", nativeEventTypeToStr(mir_event_get_type(nativeEvent)));
33+ #endif
34
35 // Event dispatching.
36- switch (mir_event_get_type(nativeEvent))
37+ switch (mir_event_get_type(nativeEvent))
38 {
39 case mir_event_type_input:
40 dispatchInputEvent(ubuntuEvent->window->window(), mir_event_get_input_event(nativeEvent));
41@@ -429,7 +427,7 @@
42
43 auto local_point = QPointF(mir_pointer_event_axis_value(pev, mir_pointer_axis_x),
44 mir_pointer_event_axis_value(pev, mir_pointer_axis_y));
45-
46+
47 QWindowSystemInterface::handleMouseEvent(window, timestamp, local_point, local_point /* Should we omit global point instead? */,
48 buttons, modifiers);
49 }
50
51=== modified file 'src/ubuntumirclient/window.cpp'
52--- src/ubuntumirclient/window.cpp 2015-05-26 21:11:43 +0000
53+++ src/ubuntumirclient/window.cpp 2015-06-17 13:08:26 +0000
54@@ -96,7 +96,6 @@
55 WId id;
56 UbuntuInput* input;
57 Qt::WindowState state;
58- QRect geometry;
59 MirConnection *connection;
60 MirSurface* surface;
61 QSize bufferSize;
62@@ -140,8 +139,8 @@
63 d->id = id++;
64
65 // Use client geometry if set explicitly, use available screen geometry otherwise.
66- d->geometry = window()->geometry() != screen->geometry() ?
67- window()->geometry() : screen->availableGeometry();
68+ QPlatformWindow::setGeometry(window()->geometry() != screen->geometry() ?
69+ window()->geometry() : screen->availableGeometry());
70 createWindow();
71 DLOG("UbuntuWindow::UbuntuWindow (this=%p, w=%p, screen=%p, input=%p)", this, w, screen, input);
72 }
73@@ -253,7 +252,7 @@
74 geometry.setY(panelHeight);
75 } else {
76 printf("UbuntuWindow - regular geometry\n");
77- geometry = d->geometry;
78+ geometry = this->geometry();
79 geometry.setY(panelHeight);
80 }
81
82@@ -314,22 +313,16 @@
83
84 void UbuntuWindow::handleSurfaceResize(int width, int height)
85 {
86+ QMutexLocker(&d->mutex);
87 LOG("UbuntuWindow::handleSurfaceResize(width=%d, height=%d)", width, height);
88
89 // The current buffer size hasn't actually changed. so just render on it and swap
90 // buffers until we render on a buffer with the target size.
91
92- bool shouldSwapBuffers;
93-
94- {
95- QMutexLocker(&d->mutex);
96- d->targetBufferSize.rwidth() = width;
97- d->targetBufferSize.rheight() = height;
98-
99- shouldSwapBuffers = d->bufferSize != d->targetBufferSize;
100- }
101-
102- if (shouldSwapBuffers) {
103+ d->targetBufferSize.rwidth() = width;
104+ d->targetBufferSize.rheight() = height;
105+
106+ if (d->bufferSize != d->targetBufferSize) {
107 QWindowSystemInterface::handleExposeEvent(window(), geometry());
108 } else {
109 qWarning("[ubuntumirclient QPA] UbuntuWindow::handleSurfaceResize"
110@@ -356,35 +349,6 @@
111 QWindowSystemInterface::handleWindowActivated(activatedWindow, Qt::ActiveWindowFocusReason);
112 }
113
114-void UbuntuWindow::handleBufferResize(int width, int height)
115-{
116- DLOG("UbuntuWindow::handleBufferResize(width=%d, height=%d)", width, height);
117-
118- QRect oldGeometry;
119- QRect newGeometry;
120-
121- {
122- QMutexLocker(&d->mutex);
123- oldGeometry = geometry();
124- newGeometry = oldGeometry;
125- newGeometry.setWidth(width);
126- newGeometry.setHeight(height);
127-
128- d->bufferSize.rwidth() = width;
129- d->bufferSize.rheight() = height;
130- d->geometry = newGeometry;
131- }
132-
133- QPlatformWindow::setGeometry(newGeometry);
134- QWindowSystemInterface::handleGeometryChange(window(), newGeometry, oldGeometry);
135- QWindowSystemInterface::handleExposeEvent(window(), newGeometry);
136-}
137-
138-void UbuntuWindow::forceRedraw()
139-{
140- QWindowSystemInterface::handleExposeEvent(window(), geometry());
141-}
142-
143 void UbuntuWindow::setWindowState(Qt::WindowState state)
144 {
145 QMutexLocker(&d->mutex);
146@@ -406,7 +370,7 @@
147
148 {
149 QMutexLocker(&d->mutex);
150- d->geometry = rect;
151+ QPlatformWindow::setGeometry(rect);
152 doMoveResize = d->state != Qt::WindowFullScreen && d->state != Qt::WindowMaximized;
153 }
154
155@@ -450,16 +414,30 @@
156
157 if (sizeKnown && (d->bufferSize.width() != newBufferWidth ||
158 d->bufferSize.height() != newBufferHeight)) {
159- QMetaObject::invokeMethod(this, "handleBufferResize",
160- Qt::QueuedConnection,
161- Q_ARG(int, newBufferWidth), Q_ARG(int, newBufferHeight));
162+
163+ DLOG("UbuntuWindow::onBuffersSwapped_threadSafe - buffer size changed from (%d,%d) to (%d,%d)",
164+ d->bufferSize.width(), d->bufferSize.height(), newBufferWidth, newBufferHeight);
165+
166+ d->bufferSize.rwidth() = newBufferWidth;
167+ d->bufferSize.rheight() = newBufferHeight;
168+
169+ QRect newGeometry;
170+
171+ newGeometry = geometry();
172+ newGeometry.setWidth(d->bufferSize.width());
173+ newGeometry.setHeight(d->bufferSize.height());
174+
175+ QPlatformWindow::setGeometry(newGeometry);
176+ QWindowSystemInterface::handleGeometryChange(window(), newGeometry, QRect());
177+ QWindowSystemInterface::handleExposeEvent(window(), newGeometry);
178+
179 } else {
180 // buffer size hasn't changed
181 if (d->targetBufferSize.isValid()) {
182 if (d->bufferSize != d->targetBufferSize) {
183 // but we still didn't reach the promised buffer size from the mir resize event.
184 // thus keep swapping buffers
185- QMetaObject::invokeMethod(this, "forceRedraw", Qt::QueuedConnection);
186+ QWindowSystemInterface::handleExposeEvent(window(), geometry());
187 } else {
188 // target met. we have just provided a render with the target size and
189 // can therefore finally rest.
190
191=== modified file 'src/ubuntumirclient/window.h'
192--- src/ubuntumirclient/window.h 2015-02-04 16:14:43 +0000
193+++ src/ubuntumirclient/window.h 2015-06-17 13:08:26 +0000
194@@ -49,10 +49,6 @@
195
196 UbuntuWindowPrivate* priv() { return d; }
197
198-public Q_SLOTS:
199- void handleBufferResize(int width, int height);
200- void forceRedraw();
201-
202 private:
203 void createWindow();
204 void moveResize(const QRect& rect);

Subscribers

People subscribed via source and target branches