Mir

Merge lp:~alan-griffiths/mir/fix-1701308 into lp:mir

Proposed by Alan Griffiths on 2017-06-30
Status: Merged
Approved by: Alan Griffiths on 2017-06-30
Approved revision: 4206
Merged at revision: 4202
Proposed branch: lp:~alan-griffiths/mir/fix-1701308
Merge into: lp:mir
Diff against target: 123 lines (+27/-40)
2 files modified
examples/eglapp.c (+12/-23)
src/client/mir_connection.cpp (+15/-17)
To merge this branch: bzr merge lp:~alan-griffiths/mir/fix-1701308
Reviewer Review Type Date Requested Status
Mir CI Bot continuous-integration Approve on 2017-06-30
Gerry Boland Approve on 2017-06-30
Daniel van Vugt 2017-06-30 Abstain on 2017-06-30
Review via email: mp+326582@code.launchpad.net

Commit message

Handle initial window size being imposed by shell in eglapps.

To post a comment you must log in.
Daniel van Vugt (vanvugt) wrote :

Is this reliable?

I had previously discovered that you can't rely on the resize callback occurring before the app starts rendering its first frame:

https://code.launchpad.net/~vanvugt/mir/fix-1686620/+merge/323378

So I guess if you are relying on the callback then the app could render two frames on startup... First at the size it requested, and very soon again at the size the shell requested.

It won't look pretty rendering that first frame at the wrong size so I suggest using the EGL_WIDTH/HEIGHT approach shown above. Sounds like it just got broken by the Mesa-RS changes. So if Mesa is now reporting a bogus EGL_WIDTH/HEIGHT that really needs to be fixed (as a matter of higher priority and also would supersede this proposal).

review: Needs Information
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:4202
https://mir-jenkins.ubuntu.com/job/mir-ci/3468/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/4738/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4896
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/4885
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/4885
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4885
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4775/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4775/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4775/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4775/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4775/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4775
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4775/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4775
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4775/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4775/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3468/rebuild

review: Needs Fixing (continuous-integration)
Alan Griffiths (alan-griffiths) wrote :

> Is this reliable?
>
> I had previously discovered that you can't rely on the resize callback
> occurring before the app starts rendering its first frame:

It is possible for a frame to be rendered at the requested (not actual) size: the "created" callback is, necessarily, called before the "resize" callback.

With our undeprecated the client cannot query the window size in the "created" callback.

> I suggest using the EGL_WIDTH/HEIGHT approach shown above

Remember the server has only sized the window, not the surface. The current RS design relies on the client to implement the surface resize. As discussed in the bug comments there's no quick fix for that.

Daniel van Vugt (vanvugt) wrote :

OK. I don't understand all of that but the last paragraph makes sense at least.

review: Abstain
Gerry Boland (gerboland) wrote :

I'm confused, as in eglapp.c after surface spec creation, it uses eglQuerySurface to fetch surface width & height, and has a big comment

 * Mir reserves the right to ignore our initial window dimensions and
 * resize to whatever it likes. Usually that resize callback has
 * occurred by now (see r4150), but libmirclient does not provide a
 * solid guarantee of it. Luckily, we don't care... by querying EGL
 * buffer dimensions we can get the correct answer without being
 * victim to the callback race that's going on in the background...
 * If the server has given us dimensions other than what we requested
 * then EGL will already know about it, possibly before the initial
 * resize event (!).

This patch does fix the delivery of the resize event, but as this comment says, surely the egl surface should have had the correct dimensions anyway?

review: Needs Information
Alan Griffiths (alan-griffiths) wrote :

> I'm confused, as in eglapp.c after surface spec creation, it uses
> eglQuerySurface to fetch surface width & height, and has a big comment
>
> * Mir reserves the right to ignore our initial window dimensions and
> * resize to whatever it likes. Usually that resize callback has
> * occurred by now (see r4150), but libmirclient does not provide a
> * solid guarantee of it. Luckily, we don't care... by querying EGL
> * buffer dimensions we can get the correct answer without being
> * victim to the callback race that's going on in the background...
> * If the server has given us dimensions other than what we requested
> * then EGL will already know about it, possibly before the initial
> * resize event (!).
>
> This patch does fix the delivery of the resize event, but as this comment
> says, surely the egl surface should have had the correct dimensions anyway?

That comment was obsolete: EGL has the size of the surface (not the window) which is controlled by the client.

Gerry Boland (gerboland) wrote :

Ok, I follow what is happening better now. And testing, it works ok

review: Approve
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:4203
https://mir-jenkins.ubuntu.com/job/mir-ci/3469/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4739
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4897
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/4886
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/4886
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4886
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4776
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4776/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4776
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4776/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4776
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4776/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4776
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4776/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4776
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4776/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4776
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4776/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4776
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4776/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4776
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4776/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3469/rebuild

review: Approve (continuous-integration)
Gerry Boland (gerboland) :
review: Approve
lp:~alan-griffiths/mir/fix-1701308 updated on 2017-06-30
4206. By Alan Griffiths on 2017-06-30

Fix whitespace

Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:4206
https://mir-jenkins.ubuntu.com/job/mir-ci/3471/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4742
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4900
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/4889
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/4889
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4889
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4779
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4779/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4779
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4779/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4779
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4779/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4779
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4779/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4779
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4779/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4779
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4779/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4779
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4779/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4779
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4779/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3471/rebuild

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'examples/eglapp.c'
2--- examples/eglapp.c 2017-06-06 15:41:28 +0000
3+++ examples/eglapp.c 2017-06-30 11:36:00 +0000
4@@ -161,7 +161,6 @@
5
6 void mir_eglapp_handle_event(MirWindow* window, MirEvent const* ev, void* unused)
7 {
8- (void) window;
9 (void) unused;
10
11 switch (mir_event_get_type(ev))
12@@ -201,13 +200,13 @@
13
14 printf("Resized to %dx%d\n", new_width, new_height);
15 if (surface)
16- {
17- mir_render_surface_set_size(surface, new_width, new_height);
18- MirWindowSpec* spec = mir_create_window_spec(connection);
19- mir_window_spec_add_render_surface(spec, surface, new_width, new_height, 0, 0);
20- mir_window_apply_spec(window, spec);
21- mir_window_spec_release(spec);
22- }
23+ {
24+ mir_render_surface_set_size(surface, new_width, new_height);
25+ MirWindowSpec* spec = mir_create_window_spec(connection);
26+ mir_window_spec_add_render_surface(spec, surface, new_width, new_height, 0, 0);
27+ mir_window_apply_spec(window, spec);
28+ mir_window_spec_release(spec);
29+ }
30 }
31
32 static void show_help(struct mir_eglapp_arg const* const* arg_lists)
33@@ -462,6 +461,8 @@
34 name = p + 1;
35 }
36 mir_window_spec_set_name(spec, name);
37+ mir_window_spec_set_event_handler(spec, mir_eglapp_handle_event, NULL);
38+ mir_window_spec_set_cursor_name(spec, cursor_name);
39
40 if (output_id != mir_display_output_id_invalid)
41 mir_window_spec_set_fullscreen_on_output(spec, output_id);
42@@ -471,13 +472,6 @@
43
44 CHECK(mir_window_is_valid(window), "Can't create a window");
45
46- mir_window_set_event_handler(window, mir_eglapp_handle_event, NULL);
47-
48- spec = mir_create_window_spec(connection);
49- mir_window_spec_set_cursor_name(spec, cursor_name);
50- mir_window_apply_spec(window, spec);
51- mir_window_spec_release(spec);
52-
53 eglsurface = eglCreateWindowSurface(egldisplay,
54 eglconfig,
55 (EGLNativeWindowType)surface,
56@@ -498,14 +492,9 @@
57 {
58 /*
59 * Mir reserves the right to ignore our initial window dimensions and
60- * resize to whatever it likes. Usually that resize callback has
61- * occurred by now (see r4150), but libmirclient does not provide a
62- * solid guarantee of it. Luckily, we don't care... by querying EGL
63- * buffer dimensions we can get the correct answer without being
64- * victim to the callback race that's going on in the background...
65- * If the server has given us dimensions other than what we requested
66- * then EGL will already know about it, possibly before the initial
67- * resize event (!).
68+ * resize to whatever it likes. In that case, a resize callback to
69+ * mir_eglapp_handle_event() has occurred by now and we have resized
70+ * the eglsurface.
71 */
72 *width = buf_width;
73 *height = buf_height;
74
75=== modified file 'src/client/mir_connection.cpp'
76--- src/client/mir_connection.cpp 2017-06-09 14:16:54 +0000
77+++ src/client/mir_connection.cpp 2017-06-30 11:36:00 +0000
78@@ -276,21 +276,6 @@
79 MIR_LOG_UNCAUGHT_EXCEPTION(ex);
80 }
81
82-void send_resize_event_if_needed(MirWindow *window, MirWindowSpec const& spec, mir::protobuf::Surface const& surface_proto)
83-{
84- if (spec.width.is_set() && spec.height.is_set() && spec.event_handler.is_set())
85- {
86- mir::geometry::Size requested_size{spec.width.value(), spec.height.value()};
87- mir::geometry::Size actual_size{surface_proto.width(), surface_proto.height()};
88- auto event_handler = spec.event_handler.value();
89- if (requested_size != actual_size)
90- {
91- auto event = mev::make_event(mf::SurfaceId{surface_proto.id().value()}, actual_size);
92- event_handler.callback(window, event.get(), event_handler.context);
93- }
94- }
95-}
96-
97 std::mutex connection_guard;
98 MirConnection* valid_connections{nullptr};
99 }
100@@ -485,9 +470,22 @@
101 }
102
103 callback(surf.get(), context);
104+
105+ if (spec.width.is_set() && spec.height.is_set() && spec.event_handler.is_set())
106+ {
107+ mir::geometry::Size requested_size{spec.width.value(), spec.height.value()};
108+ mir::geometry::Size actual_size{(*surface_proto).width(), (*surface_proto).height()};
109+ auto event_handler = spec.event_handler.value();
110+ if (requested_size != actual_size)
111+ {
112+ auto event = mir::events::make_event(mir::frontend::SurfaceId{(*surface_proto).id().value()}, actual_size);
113+ lock.unlock();
114+ event_handler.callback(surf.get(), event.get(), event_handler.context);
115+ lock.lock();
116+ }
117+ }
118+
119 request->wh->result_received();
120-
121- send_resize_event_if_needed(surf.get(), spec, *surface_proto);
122 surface_requests.erase(request_it);
123 }
124

Subscribers

People subscribed via source and target branches