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

Proposed by Daniel d'Andrada on 2015-05-12
Status: Superseded
Proposed branch: lp:~dandrader/qtubuntu/improve-resize
Merge into: lp:qtubuntu
Diff against target: 324 lines (+58/-84)
4 files modified
debian/control (+1/-1)
src/ubuntumirclient/input.cpp (+28/-27)
src/ubuntumirclient/window.cpp (+29/-52)
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-05-12 Needs Fixing on 2015-05-13
PS Jenkins bot continuous-integration Approve on 2015-05-12
Review via email: mp+258912@code.launchpad.net

This proposal has been superseded by a proposal from 2015-06-03.

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 :
review: Approve (continuous-integration)
Gerry Boland (gerboland) wrote :

=== 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 :

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 :

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

260. By Daniel d'Andrada on 2015-06-02

Merge lp:~mir-team/qtubuntu/track-mir-deprecations

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

Refactor optional logging and remove trailing whitespace

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/control'
2--- debian/control 2015-03-05 19:54:57 +0000
3+++ debian/control 2015-06-03 13:24:47 +0000
4@@ -17,7 +17,7 @@
5 qtbase5-private-dev,
6 qtdeclarative5-dev,
7 libqt5sensors5-dev,
8- libmirclient-dev (>= 0.11.0),
9+ libmirclient-dev (>= 0.13.0),
10 # if you don't have have commit access to this branch but would like to upload
11 # directly to Ubuntu, don't worry: your changes will be merged back into the
12 # upstream branch
13
14=== modified file 'src/ubuntumirclient/input.cpp'
15--- src/ubuntumirclient/input.cpp 2015-02-17 17:39:47 +0000
16+++ src/ubuntumirclient/input.cpp 2015-06-03 13:24:47 +0000
17@@ -160,6 +160,7 @@
18 #ifndef QT_NO_DEBUG
19
20
21+/*
22 static const char* nativeEventTypeToStr(MirEventType t)
23 {
24 switch (t)
25@@ -185,7 +186,7 @@
26 return "invalid";
27 }
28 }
29-
30+*/
31
32 #endif
33
34@@ -275,7 +276,7 @@
35
36 void UbuntuInput::dispatchTouchEvent(QWindow *window, const MirInputEvent *ev)
37 {
38- const MirTouchInputEvent *tev = mir_input_event_get_touch_input_event(ev);
39+ const MirTouchEvent *tev = mir_input_event_get_touch_event(ev);
40
41 // FIXME(loicm) Max pressure is device specific. That one is for the Samsung Galaxy Nexus. That
42 // needs to be fixed as soon as the compat input lib adds query support.
43@@ -286,30 +287,30 @@
44
45 // TODO: Is it worth setting the Qt::TouchPointStationary ones? Currently they are left
46 // as Qt::TouchPointMoved
47- const unsigned int kPointerCount = mir_touch_input_event_get_touch_count(tev);
48+ const unsigned int kPointerCount = mir_touch_event_point_count(tev);
49 for (unsigned int i = 0; i < kPointerCount; ++i) {
50 QWindowSystemInterface::TouchPoint touchPoint;
51
52- const float kX = mir_touch_input_event_get_touch_axis_value(tev, i, mir_touch_input_axis_x) + kWindowGeometry.x();
53- const float kY = mir_touch_input_event_get_touch_axis_value(tev, i, mir_touch_input_axis_y) + kWindowGeometry.y(); // see bug lp:1346633 workaround comments elsewhere
54- const float kW = mir_touch_input_event_get_touch_axis_value(tev, i, mir_touch_input_axis_touch_major);
55- const float kH = mir_touch_input_event_get_touch_axis_value(tev, i, mir_touch_input_axis_touch_minor);
56- const float kP = mir_touch_input_event_get_touch_axis_value(tev, i, mir_touch_input_axis_pressure);
57- touchPoint.id = mir_touch_input_event_get_touch_id(tev, i);
58+ const float kX = mir_touch_event_axis_value(tev, i, mir_touch_axis_x) + kWindowGeometry.x();
59+ const float kY = mir_touch_event_axis_value(tev, i, mir_touch_axis_y) + kWindowGeometry.y(); // see bug lp:1346633 workaround comments elsewhere
60+ const float kW = mir_touch_event_axis_value(tev, i, mir_touch_axis_touch_major);
61+ const float kH = mir_touch_event_axis_value(tev, i, mir_touch_axis_touch_minor);
62+ const float kP = mir_touch_event_axis_value(tev, i, mir_touch_axis_pressure);
63+ touchPoint.id = mir_touch_event_id(tev, i);
64 touchPoint.normalPosition = QPointF(kX / kWindowGeometry.width(), kY / kWindowGeometry.height());
65 touchPoint.area = QRectF(kX - (kW / 2.0), kY - (kH / 2.0), kW, kH);
66 touchPoint.pressure = kP / kMaxPressure;
67
68- MirTouchInputEventTouchAction touch_action = mir_touch_input_event_get_touch_action(tev, i);
69+ MirTouchAction touch_action = mir_touch_event_action(tev, i);
70 switch (touch_action)
71 {
72- case mir_touch_input_event_action_down:
73+ case mir_touch_action_down:
74 touchPoint.state = Qt::TouchPointPressed;
75 break;
76- case mir_touch_input_event_action_up:
77+ case mir_touch_action_up:
78 touchPoint.state = Qt::TouchPointReleased;
79 break;
80- case mir_touch_input_event_action_change:
81+ case mir_touch_action_change:
82 default:
83 touchPoint.state = Qt::TouchPointMoved;
84 }
85@@ -363,23 +364,23 @@
86
87 void UbuntuInput::dispatchKeyEvent(QWindow *window, const MirInputEvent *event)
88 {
89- const MirKeyInputEvent *key_event = mir_input_event_get_key_input_event(event);
90+ const MirKeyboardEvent *key_event = mir_input_event_get_keyboard_event(event);
91
92 ulong timestamp = mir_input_event_get_event_time(event) / 1000000;
93- xkb_keysym_t xk_sym = mir_key_input_event_get_key_code(key_event);
94+ xkb_keysym_t xk_sym = mir_keyboard_event_key_code(key_event);
95
96 // Key modifier and unicode index mapping.
97- auto modifiers = qt_modifiers_from_mir(mir_key_input_event_get_modifiers(key_event));
98+ auto modifiers = qt_modifiers_from_mir(mir_keyboard_event_modifiers(key_event));
99
100- MirKeyInputEventAction action = mir_key_input_event_get_action(key_event);
101- QEvent::Type keyType = action == mir_key_input_event_action_up
102+ MirKeyboardAction action = mir_keyboard_event_action(key_event);
103+ QEvent::Type keyType = action == mir_keyboard_action_up
104 ? QEvent::KeyRelease : QEvent::KeyPress;
105
106 char s[2];
107 int sym = translateKeysym(xk_sym, s, sizeof(s));
108 QString text = QString::fromLatin1(s);
109
110- bool is_auto_rep = action == mir_key_input_event_action_repeat;
111+ bool is_auto_rep = action == mir_keyboard_action_repeat;
112
113 QPlatformInputContext *context = QGuiApplicationPrivate::platformIntegration()->inputContext();
114 if (context) {
115@@ -396,14 +397,14 @@
116
117 namespace
118 {
119-Qt::MouseButtons extract_buttons(const MirPointerInputEvent *pev)
120+Qt::MouseButtons extract_buttons(const MirPointerEvent *pev)
121 {
122 Qt::MouseButtons buttons = Qt::NoButton;
123- if (mir_pointer_input_event_get_button_state(pev, mir_pointer_input_button_primary))
124+ if (mir_pointer_event_button_state(pev, mir_pointer_button_primary))
125 buttons |= Qt::LeftButton;
126- if (mir_pointer_input_event_get_button_state(pev, mir_pointer_input_button_secondary))
127+ if (mir_pointer_event_button_state(pev, mir_pointer_button_secondary))
128 buttons |= Qt::RightButton;
129- if (mir_pointer_input_event_get_button_state(pev, mir_pointer_input_button_tertiary))
130+ if (mir_pointer_event_button_state(pev, mir_pointer_button_tertiary))
131 buttons |= Qt::MidButton;
132
133 // TODO: Should mir back and forward buttons exist?
134@@ -416,12 +417,12 @@
135 {
136 auto timestamp = mir_input_event_get_event_time(ev) / 1000000;
137
138- auto pev = mir_input_event_get_pointer_input_event(ev);
139- auto modifiers = qt_modifiers_from_mir(mir_pointer_input_event_get_modifiers(pev));
140+ auto pev = mir_input_event_get_pointer_event(ev);
141+ auto modifiers = qt_modifiers_from_mir(mir_pointer_event_modifiers(pev));
142 auto buttons = extract_buttons(pev);
143
144- auto local_point = QPointF(mir_pointer_input_event_get_axis_value(pev, mir_pointer_input_axis_x),
145- mir_pointer_input_event_get_axis_value(pev, mir_pointer_input_axis_y));
146+ auto local_point = QPointF(mir_pointer_event_axis_value(pev, mir_pointer_axis_x),
147+ mir_pointer_event_axis_value(pev, mir_pointer_axis_y));
148
149 QWindowSystemInterface::handleMouseEvent(window, timestamp, local_point, local_point /* Should we omit global point instead? */,
150 buttons, modifiers);
151
152=== modified file 'src/ubuntumirclient/window.cpp'
153--- src/ubuntumirclient/window.cpp 2015-02-23 19:53:45 +0000
154+++ src/ubuntumirclient/window.cpp 2015-06-03 13:24:47 +0000
155@@ -96,7 +96,6 @@
156 WId id;
157 UbuntuInput* input;
158 Qt::WindowState state;
159- QRect geometry;
160 MirConnection *connection;
161 MirSurface* surface;
162 QSize bufferSize;
163@@ -119,8 +118,7 @@
164 UbuntuWindow* platformWindow = static_cast<UbuntuWindow*>(context);
165 platformWindow->priv()->surface = surface;
166
167- MirEventDelegate handler = {eventCallback, context};
168- mir_surface_set_event_handler(surface, &handler);
169+ mir_surface_set_event_handler(surface, eventCallback, context);
170 }
171
172 UbuntuWindow::UbuntuWindow(QWindow* w, QSharedPointer<UbuntuClipboard> clipboard, UbuntuScreen* screen,
173@@ -141,8 +139,8 @@
174 d->id = id++;
175
176 // Use client geometry if set explicitly, use available screen geometry otherwise.
177- d->geometry = window()->geometry() != screen->geometry() ?
178- window()->geometry() : screen->availableGeometry();
179+ QPlatformWindow::setGeometry(window()->geometry() != screen->geometry() ?
180+ window()->geometry() : screen->availableGeometry());
181 createWindow();
182 DLOG("UbuntuWindow::UbuntuWindow (this=%p, w=%p, screen=%p, input=%p)", this, w, screen, input);
183 }
184@@ -254,7 +252,7 @@
185 geometry.setY(panelHeight);
186 } else {
187 printf("UbuntuWindow - regular geometry\n");
188- geometry = d->geometry;
189+ geometry = this->geometry();
190 geometry.setY(panelHeight);
191 }
192
193@@ -279,7 +277,7 @@
194 mir_surface_spec_release(spec);
195
196 DASSERT(d->surface != NULL);
197- d->createEGLSurface((EGLNativeWindowType)mir_surface_get_egl_native_window(d->surface));
198+ d->createEGLSurface((EGLNativeWindowType)mir_buffer_stream_get_egl_native_window(mir_surface_get_buffer_stream(d->surface)));
199
200 if (d->state == Qt::WindowFullScreen) {
201 // TODO: We could set this on creation once surface spec supports it (mps already up)
202@@ -315,22 +313,16 @@
203
204 void UbuntuWindow::handleSurfaceResize(int width, int height)
205 {
206+ QMutexLocker(&d->mutex);
207 LOG("UbuntuWindow::handleSurfaceResize(width=%d, height=%d)", width, height);
208
209 // The current buffer size hasn't actually changed. so just render on it and swap
210 // buffers until we render on a buffer with the target size.
211
212- bool shouldSwapBuffers;
213-
214- {
215- QMutexLocker(&d->mutex);
216- d->targetBufferSize.rwidth() = width;
217- d->targetBufferSize.rheight() = height;
218-
219- shouldSwapBuffers = d->bufferSize != d->targetBufferSize;
220- }
221-
222- if (shouldSwapBuffers) {
223+ d->targetBufferSize.rwidth() = width;
224+ d->targetBufferSize.rheight() = height;
225+
226+ if (d->bufferSize != d->targetBufferSize) {
227 QWindowSystemInterface::handleExposeEvent(window(), geometry());
228 } else {
229 qWarning("[ubuntumirclient QPA] UbuntuWindow::handleSurfaceResize"
230@@ -357,35 +349,6 @@
231 QWindowSystemInterface::handleWindowActivated(activatedWindow, Qt::ActiveWindowFocusReason);
232 }
233
234-void UbuntuWindow::handleBufferResize(int width, int height)
235-{
236- DLOG("UbuntuWindow::handleBufferResize(width=%d, height=%d)", width, height);
237-
238- QRect oldGeometry;
239- QRect newGeometry;
240-
241- {
242- QMutexLocker(&d->mutex);
243- oldGeometry = geometry();
244- newGeometry = oldGeometry;
245- newGeometry.setWidth(width);
246- newGeometry.setHeight(height);
247-
248- d->bufferSize.rwidth() = width;
249- d->bufferSize.rheight() = height;
250- d->geometry = newGeometry;
251- }
252-
253- QPlatformWindow::setGeometry(newGeometry);
254- QWindowSystemInterface::handleGeometryChange(window(), newGeometry, oldGeometry);
255- QWindowSystemInterface::handleExposeEvent(window(), newGeometry);
256-}
257-
258-void UbuntuWindow::forceRedraw()
259-{
260- QWindowSystemInterface::handleExposeEvent(window(), geometry());
261-}
262-
263 void UbuntuWindow::setWindowState(Qt::WindowState state)
264 {
265 QMutexLocker(&d->mutex);
266@@ -407,7 +370,7 @@
267
268 {
269 QMutexLocker(&d->mutex);
270- d->geometry = rect;
271+ QPlatformWindow::setGeometry(rect);
272 doMoveResize = d->state != Qt::WindowFullScreen && d->state != Qt::WindowMaximized;
273 }
274
275@@ -451,16 +414,30 @@
276
277 if (sizeKnown && (d->bufferSize.width() != newBufferWidth ||
278 d->bufferSize.height() != newBufferHeight)) {
279- QMetaObject::invokeMethod(this, "handleBufferResize",
280- Qt::QueuedConnection,
281- Q_ARG(int, newBufferWidth), Q_ARG(int, newBufferHeight));
282+
283+ DLOG("UbuntuWindow::onBuffersSwapped_threadSafe - buffer size changed from (%d,%d) to (%d,%d)",
284+ d->bufferSize.width(), d->bufferSize.height(), newBufferWidth, newBufferHeight);
285+
286+ d->bufferSize.rwidth() = newBufferWidth;
287+ d->bufferSize.rheight() = newBufferHeight;
288+
289+ QRect newGeometry;
290+
291+ newGeometry = geometry();
292+ newGeometry.setWidth(d->bufferSize.width());
293+ newGeometry.setHeight(d->bufferSize.height());
294+
295+ QPlatformWindow::setGeometry(newGeometry);
296+ QWindowSystemInterface::handleGeometryChange(window(), newGeometry, QRect());
297+ QWindowSystemInterface::handleExposeEvent(window(), newGeometry);
298+
299 } else {
300 // buffer size hasn't changed
301 if (d->targetBufferSize.isValid()) {
302 if (d->bufferSize != d->targetBufferSize) {
303 // but we still didn't reach the promised buffer size from the mir resize event.
304 // thus keep swapping buffers
305- QMetaObject::invokeMethod(this, "forceRedraw", Qt::QueuedConnection);
306+ QWindowSystemInterface::handleExposeEvent(window(), geometry());
307 } else {
308 // target met. we have just provided a render with the target size and
309 // can therefore finally rest.
310
311=== modified file 'src/ubuntumirclient/window.h'
312--- src/ubuntumirclient/window.h 2015-02-04 16:14:43 +0000
313+++ src/ubuntumirclient/window.h 2015-06-03 13:24:47 +0000
314@@ -49,10 +49,6 @@
315
316 UbuntuWindowPrivate* priv() { return d; }
317
318-public Q_SLOTS:
319- void handleBufferResize(int width, int height);
320- void forceRedraw();
321-
322 private:
323 void createWindow();
324 void moveResize(const QRect& rect);

Subscribers

People subscribed via source and target branches