Mir

Merge lp:~raof/mir/release-rendersurface-may-require-RPC into lp:mir

  1. release-rendersurface-may-require-RPC
  2. Merge into development-branch
Proposed by Chris Halse Rogers
Status: Work in progress
Proposed branch: lp:~raof/mir/release-rendersurface-may-require-RPC
Merge into: lp:mir
Diff against target: 601 lines (+262/-40)
14 files modified
include/test/mir/test/validity_matchers.h (+8/-0)
playground/egldiamond_render_surface.c (+1/-1)
playground/mir_demo_client_chain_jumping_buffers.c (+2/-1)
playground/mir_demo_client_prerendered_frames.c (+1/-1)
playground/render_surface.cpp (+1/-1)
src/client/mir_connection.cpp (+14/-12)
src/client/mir_connection.h (+3/-1)
src/client/mir_render_surface_api.cpp (+31/-2)
src/client/symbols.map (+1/-0)
src/include/client/mir_toolkit/mir_render_surface.h (+13/-1)
tests/acceptance-tests/staging/test_render_surface.cpp (+147/-16)
tests/mir_test/CMakeLists.txt (+4/-0)
tests/mir_test/validity_matchers.cpp (+31/-0)
tests/unit-tests/client/test_mir_render_surface.cpp (+5/-4)
To merge this branch: bzr merge lp:~raof/mir/release-rendersurface-may-require-RPC
Reviewer Review Type Date Requested Status
Cemil Azizoglu (community) Needs Fixing
Mir CI Bot continuous-integration Approve
Review via email: mp+312761@code.launchpad.net

Commit message

Rework mir_render_surface_release() to explicitly identify the RPC wait involved.

In the case that the MirRenderSurface is backed by a MirBufferStream, releasing the MirRenderSurface
implies releasing the MirBufferStream, and releasing the MirBufferStream requires an RPC wait.

Other, future, MirRenderSurface backing objects might require RPC waits to release, too.

Split into the traditional mir_render_surface_release(rs, callback, ctx)/_sync() pairs.

To post a comment you must log in.
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:3871
https://mir-jenkins.ubuntu.com/job/mir-ci/2336/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/3050/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/3116
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/3108
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/3108
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/3108
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3079/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3079/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3079/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3079/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3079/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/3079/console

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

review: Needs Fixing (continuous-integration)
Reply
lp:~raof/mir/release-rendersurface-may-require-RPC updated
3872. By Chris Halse Rogers

Fix MirRenderSurfaceTest that make was mysteriously failing to rebuild

Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:3872
https://mir-jenkins.ubuntu.com/job/mir-ci/2337/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/3051/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/3117
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/3109
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/3109
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/3109
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3080
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3080/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3080
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3080/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3080
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3080/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3080
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3080/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3080
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3080/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/3080/console

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

review: Needs Fixing (continuous-integration)
Reply
Revision history for this message
Chris Halse Rogers (raof) wrote :

Bug #1647573, retriggering.

Reply
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:3872
https://mir-jenkins.ubuntu.com/job/mir-ci/2342/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/3056/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/3122
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/3114
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/3114
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/3114
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3085
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3085/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3085
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3085/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3085
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3085/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3085
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3085/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3085
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3085/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/3085/console

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

review: Needs Fixing (continuous-integration)
Reply
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3872
https://mir-jenkins.ubuntu.com/job/mir-ci/2348/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/3063
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/3129
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/3121
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/3121
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/3121
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3092
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3092/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3092
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3092/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3092
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3092/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3092
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3092/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3092
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3092/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/3092
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/3092/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Reply
Revision history for this message
Cemil Azizoglu (cemil-azizoglu) wrote :

123 + StreamRelease stream_release{reinterpret_cast<MirBufferStream*>(render_surface),
124 + nullptr,
125 + reinterpret_cast<mir_buffer_stream_callback>(callback),
126 + ctx,
127 rs->stream_id().as_value(),
128 render_surface};

This is actually executed for PCs as well.

review: Needs Fixing
Reply
Revision history for this message
Cemil Azizoglu (cemil-azizoglu) wrote :

I guess I don't understand why releasing of streams requires an RPC wait while that of chains doesn't, when the operation is the same (i.e. server.release_buffer_stream(...)).

Reply
Revision history for this message
Chris Halse Rogers (raof) wrote :

Releasing streams requires an RPC wait for two reasons:
1) It's a necessary client synchronisation point: clients almost always want to have some data associated with a Mir object, and they can't free that associated data until we can guarantee they'll never get a callback on that Mir object again.
2) The code
mir_render_surface_release(rs)
mir_connection_release(connection)
is currently racy, and will frequently lead to client deadlocks.

Like so:
  Both the rs_release and connection_release client requests will get queued up
  to the socket, the server sends the responses and then closes the connection.
  .
  The rs_release response results in erasing the bufferstream from the connection
  map.
  .
  The bufferstream destructor calls release_buffer on all of its allocated buffers.
  (This is why the current acceptance tests don't notice the problem - we don't
   wait for the bufferstream to actually be valid, so its allocation requests
   are unfulfilled at destruction time, so it never tries to release them.)
  .
  The buffer_release call hits the RPC layer, which notices that the socket has
  been closed and fires on_disconnected.
  .
  on_disconnected runs all the pending completions. The first one is...
  release_connection, which immediately tries to grab the connection_map
  lock, which we already hold way up in rs_release, and deadlock.

I guess both of these could be solved instead by
a) Ensuring that mir_render_surface_release does not return until it's guaranteed that no more callbacks will be fired on the underlying bufferstream, and
b) Ensuring that we don't deadlock in this scenario.

I'll see how difficult this is to do.

Reply
Revision history for this message
Chris Halse Rogers (raof) wrote :

Hm. (2) is actually a flaw in my follow-up code, so it's only (1) that we're concerned about...

Reply

Unmerged revisions

3872. By Chris Halse Rogers

Fix MirRenderSurfaceTest that make was mysteriously failing to rebuild

3871. By Chris Halse Rogers

Rework mir_render_surface_release() to explicitly identify the RPC wait involved.

In the case that the MirRenderSurface is backed by a MirBufferStream, releasing the MirRenderSurface
implies releasing the MirBufferStream, and releasing the MirBufferStream requires an RPC wait.

Other, future, MirRenderSurface backing objects might require RPC waits to release, too.

Split into the traditional mir_render_surface_release(rs, callback, ctx)/_sync() pairs.

3870. By Chris Halse Rogers

Add IsValid() implementations for MirBufferStream and MirRenderSurface

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'include/test/mir/test/validity_matchers.h'
--- include/test/mir/test/validity_matchers.h 2016-01-29 08:18:22 +0000
+++ include/test/mir/test/validity_matchers.h 2016-12-08 03:24:08 +0000
@@ -23,6 +23,8 @@
2323
24#include "mir_toolkit/mir_client_library.h"24#include "mir_toolkit/mir_client_library.h"
2525
26class MirRenderSurface;
27
26using ::testing::MakePolymorphicMatcher;28using ::testing::MakePolymorphicMatcher;
27using ::testing::MatchResultListener;29using ::testing::MatchResultListener;
28using ::testing::NotNull;30using ::testing::NotNull;
@@ -54,6 +56,12 @@
54template<>56template<>
55bool IsValidMatcher::MatchAndExplain(MirSurface* surface, MatchResultListener* listener) const;57bool IsValidMatcher::MatchAndExplain(MirSurface* surface, MatchResultListener* listener) const;
5658
59template<>
60bool IsValidMatcher::MatchAndExplain(MirBufferStream* stream, MatchResultListener* listener) const;
61
62template<>
63bool IsValidMatcher::MatchAndExplain(MirRenderSurface* stream, MatchResultListener* listener) const;
64
57// To construct a polymorphic matcher, pass an instance of the class65// To construct a polymorphic matcher, pass an instance of the class
58// to MakePolymorphicMatcher(). Note the return type.66// to MakePolymorphicMatcher(). Note the return type.
59inline PolymorphicMatcher<IsValidMatcher> IsValid()67inline PolymorphicMatcher<IsValidMatcher> IsValid()
6068
=== modified file 'playground/egldiamond_render_surface.c'
--- playground/egldiamond_render_surface.c 2016-11-30 04:19:26 +0000
+++ playground/egldiamond_render_surface.c 2016-12-08 03:24:08 +0000
@@ -312,7 +312,7 @@
312 future_driver_eglTerminate(egldisplay);312 future_driver_eglTerminate(egldisplay);
313 else313 else
314 eglTerminate(egldisplay);314 eglTerminate(egldisplay);
315 mir_render_surface_release(render_surface);315 mir_render_surface_release_sync(render_surface);
316 mir_surface_release_sync(surface);316 mir_surface_release_sync(surface);
317 mir_connection_release(connection);317 mir_connection_release(connection);
318 return 0;318 return 0;
319319
=== modified file 'playground/mir_demo_client_chain_jumping_buffers.c'
--- playground/mir_demo_client_chain_jumping_buffers.c 2016-12-01 16:01:55 +0000
+++ playground/mir_demo_client_chain_jumping_buffers.c 2016-12-08 03:24:08 +0000
@@ -265,7 +265,8 @@
265 for (unsigned int i = 0u; i < num_buffers; i++)265 for (unsigned int i = 0u; i < num_buffers; i++)
266 mir_buffer_release(buffer_available[i].buffer);266 mir_buffer_release(buffer_available[i].buffer);
267 for (unsigned int i = 0u; i < num_chains; i++)267 for (unsigned int i = 0u; i < num_chains; i++)
268 mir_render_surface_release(render_surface[i]);268 mir_render_surface_release_sync(render_surface[i]);
269
269 mir_surface_release_sync(surface);270 mir_surface_release_sync(surface);
270 mir_connection_release(connection);271 mir_connection_release(connection);
271 return 0;272 return 0;
272273
=== modified file 'playground/mir_demo_client_prerendered_frames.c'
--- playground/mir_demo_client_prerendered_frames.c 2016-12-01 16:01:55 +0000
+++ playground/mir_demo_client_prerendered_frames.c 2016-12-08 03:24:08 +0000
@@ -232,7 +232,7 @@
232232
233 for (i = 0u; i < num_prerendered_frames; i++)233 for (i = 0u; i < num_prerendered_frames; i++)
234 mir_buffer_release(buffer_available[i].buffer);234 mir_buffer_release(buffer_available[i].buffer);
235 mir_render_surface_release(render_surface);235 mir_render_surface_release_sync(render_surface);
236 mir_surface_release_sync(surface);236 mir_surface_release_sync(surface);
237 mir_connection_release(connection);237 mir_connection_release(connection);
238 return 0;238 return 0;
239239
=== modified file 'playground/render_surface.cpp'
--- playground/render_surface.cpp 2016-11-14 22:39:13 +0000
+++ playground/render_surface.cpp 2016-12-08 03:24:08 +0000
@@ -243,7 +243,7 @@
243 mir_buffer_stream_swap_buffers_sync(buffer_stream);243 mir_buffer_stream_swap_buffers_sync(buffer_stream);
244 }244 }
245245
246 mir_render_surface_release(render_surface);246 mir_render_surface_release_sync(render_surface);
247 mir_surface_release_sync(surface);247 mir_surface_release_sync(surface);
248 close(signal_watch);248 close(signal_watch);
249249
250250
=== modified file 'src/client/mir_connection.cpp'
--- src/client/mir_connection.cpp 2016-12-06 16:56:33 +0000
+++ src/client/mir_connection.cpp 2016-12-08 03:24:08 +0000
@@ -475,17 +475,17 @@
475475
476void MirConnection::released(StreamRelease data)476void MirConnection::released(StreamRelease data)
477{477{
478 {
479 std::unique_lock<decltype(mutex)> lk(mutex);
480 if (data.rs)
481 surface_map->erase(data.rs);
482 surface_map->erase(mf::BufferStreamId(data.rpc_id));
483 }
484
478 if (data.callback)485 if (data.callback)
479 data.callback(data.stream, data.context);486 data.callback(data.stream, data.context);
480 if (data.handle)487 if (data.handle)
481 data.handle->result_received();488 data.handle->result_received();
482
483 {
484 std::unique_lock<decltype(mutex)> lk(mutex);
485 if (data.rs)
486 surface_map->erase(data.rs);
487 surface_map->erase(mf::BufferStreamId(data.rpc_id));
488 }
489}489}
490490
491void MirConnection::released(SurfaceRelease data)491void MirConnection::released(SurfaceRelease data)
@@ -1331,14 +1331,16 @@
1331}1331}
13321332
1333void MirConnection::release_render_surface_with_content(1333void MirConnection::release_render_surface_with_content(
1334 void* render_surface)1334 void* render_surface,
1335 mir_render_surface_callback callback,
1336 void* ctx)
1335{1337{
1336 auto rs = surface_map->render_surface(render_surface);1338 auto rs = surface_map->render_surface(render_surface);
13371339
1338 StreamRelease stream_release{nullptr,1340 StreamRelease stream_release{reinterpret_cast<MirBufferStream*>(render_surface),
1339 nullptr,1341 nullptr,
1340 nullptr,1342 reinterpret_cast<mir_buffer_stream_callback>(callback),
1341 nullptr,1343 ctx,
1342 rs->stream_id().as_value(),1344 rs->stream_id().as_value(),
1343 render_surface};1345 render_surface};
13441346
13451347
=== modified file 'src/client/mir_connection.h'
--- src/client/mir_connection.h 2016-12-06 16:56:33 +0000
+++ src/client/mir_connection.h 2016-12-08 03:24:08 +0000
@@ -224,7 +224,9 @@
224 void* context,224 void* context,
225 void** native_window);225 void** native_window);
226 void release_render_surface_with_content(226 void release_render_surface_with_content(
227 void* render_surface);227 void* render_surface,
228 mir_render_surface_callback callback,
229 void* ctx);
228230
229 void* request_interface(char const* name, int version);231 void* request_interface(char const* name, int version);
230232
231233
=== modified file 'src/client/mir_render_surface_api.cpp'
--- src/client/mir_render_surface_api.cpp 2016-12-06 16:56:33 +0000
+++ src/client/mir_render_surface_api.cpp 2016-12-08 03:24:08 +0000
@@ -158,19 +158,48 @@
158}158}
159159
160void mir_render_surface_release(160void mir_render_surface_release(
161 MirRenderSurface* render_surface)161 MirRenderSurface* render_surface,
162 mir_render_surface_callback callback,
163 void* ctx)
162try164try
163{165{
164 mir::require(render_surface);166 mir::require(render_surface);
165 auto connection = connection_map.connection(static_cast<void*>(render_surface));167 auto connection = connection_map.connection(static_cast<void*>(render_surface));
166 connection_map.erase(static_cast<void*>(render_surface));168 connection_map.erase(static_cast<void*>(render_surface));
167 connection->release_render_surface_with_content(render_surface);169 connection->release_render_surface_with_content(render_surface, callback, ctx);
168}170}
169catch (std::exception const& ex)171catch (std::exception const& ex)
170{172{
173 callback(render_surface, ctx);
171 MIR_LOG_UNCAUGHT_EXCEPTION(ex);174 MIR_LOG_UNCAUGHT_EXCEPTION(ex);
172}175}
173176
177void mir_render_surface_release_sync(MirRenderSurface* render_surface)
178{
179 struct Waiter
180 {
181 std::condition_variable notifier;
182 std::mutex lock;
183 bool done{false};
184 } waiter;
185
186 mir_render_surface_release(
187 render_surface,
188 [](MirRenderSurface*, void* ctx)
189 {
190 auto waiter = reinterpret_cast<Waiter*>(ctx);
191 {
192 std::lock_guard<std::mutex> lock{waiter->lock};
193 waiter->done = true;
194 }
195 waiter->notifier.notify_all();
196 },
197 &waiter);
198
199 std::unique_lock<std::mutex> lock(waiter.lock);
200 waiter.notifier.wait(lock, [&waiter]() { return waiter.done; });
201}
202
174MirBufferStream* mir_render_surface_get_buffer_stream(203MirBufferStream* mir_render_surface_get_buffer_stream(
175 MirRenderSurface* render_surface,204 MirRenderSurface* render_surface,
176 int width, int height,205 int width, int height,
177206
=== modified file 'src/client/symbols.map'
--- src/client/symbols.map 2016-12-06 16:56:33 +0000
+++ src/client/symbols.map 2016-12-08 03:24:08 +0000
@@ -396,6 +396,7 @@
396 mir_render_surface_get_size;396 mir_render_surface_get_size;
397 mir_render_surface_set_size;397 mir_render_surface_set_size;
398 mir_render_surface_release;398 mir_render_surface_release;
399 mir_render_surface_release_sync;
399 mir_surface_spec_add_render_surface;400 mir_surface_spec_add_render_surface;
400401
401 #private functions needed temporarily by nested passthrough402 #private functions needed temporarily by nested passthrough
402403
=== modified file 'src/include/client/mir_toolkit/mir_render_surface.h'
--- src/include/client/mir_toolkit/mir_render_surface.h 2016-12-06 16:56:33 +0000
+++ src/include/client/mir_toolkit/mir_render_surface.h 2016-12-08 03:24:08 +0000
@@ -110,9 +110,21 @@
110/**110/**
111 * Release the specified render surface111 * Release the specified render surface
112 *112 *
113 * \param [in] render_surface The render surface to be released113 * \param [in] render_surface The render surface to be released
114 * \param [in] callback Callback to be run when the operation completes
115 * \param [in,out] ctx Context passed to callback
114 */116 */
115void mir_render_surface_release(117void mir_render_surface_release(
118 MirRenderSurface* render_surface,
119 mir_render_surface_callback callback,
120 void* ctx);
121
122/**
123 * Release the specified render surface and wait for the operation to complete
124 *
125 * \param [in] render_surface The render surface to be released
126 */
127void mir_render_surface_release_sync(
116 MirRenderSurface* render_surface);128 MirRenderSurface* render_surface);
117129
118/**130/**
119131
=== modified file 'tests/acceptance-tests/staging/test_render_surface.cpp'
--- tests/acceptance-tests/staging/test_render_surface.cpp 2016-12-06 16:56:33 +0000
+++ tests/acceptance-tests/staging/test_render_surface.cpp 2016-12-08 03:24:08 +0000
@@ -24,11 +24,13 @@
24#include "mir_test_framework/headless_in_process_server.h"24#include "mir_test_framework/headless_in_process_server.h"
25#include "mir_test_framework/using_stub_client_platform.h"25#include "mir_test_framework/using_stub_client_platform.h"
26#include "mir/test/validity_matchers.h"26#include "mir/test/validity_matchers.h"
27#include "mir/test/signal.h"
2728
28#include <gtest/gtest.h>29#include <gtest/gtest.h>
29#include <gmock/gmock.h>30#include <gmock/gmock.h>
3031
31namespace mtf = mir_test_framework;32namespace mtf = mir_test_framework;
33namespace mt = mir::test;
32namespace geom = mir::geometry;34namespace geom = mir::geometry;
3335
34namespace36namespace
@@ -54,7 +56,7 @@
54 EXPECT_TRUE(mir_render_surface_is_valid(rs));56 EXPECT_TRUE(mir_render_surface_is_valid(rs));
55 EXPECT_THAT(mir_render_surface_get_error_message(rs), StrEq(""));57 EXPECT_THAT(mir_render_surface_get_error_message(rs), StrEq(""));
5658
57 mir_render_surface_release(rs);59 mir_render_surface_release_sync(rs);
58 mir_connection_release(connection);60 mir_connection_release(connection);
59}61}
6062
@@ -83,7 +85,7 @@
83 EXPECT_TRUE(mir_buffer_stream_is_valid(bs));85 EXPECT_TRUE(mir_buffer_stream_is_valid(bs));
84 EXPECT_THAT(mir_buffer_stream_get_error_message(bs), StrEq(""));86 EXPECT_THAT(mir_buffer_stream_get_error_message(bs), StrEq(""));
8587
86 mir_render_surface_release(rs);88 mir_render_surface_release_sync(rs);
87 mir_connection_release(connection);89 mir_connection_release(connection);
88}90}
8991
@@ -113,7 +115,7 @@
113115
114 EXPECT_THAT(bs2, Eq(nullptr));116 EXPECT_THAT(bs2, Eq(nullptr));
115117
116 mir_render_surface_release(rs);118 mir_render_surface_release_sync(rs);
117 mir_connection_release(connection);119 mir_connection_release(connection);
118}120}
119121
@@ -134,7 +136,7 @@
134 EXPECT_TRUE(mir_buffer_stream_is_valid(bs));136 EXPECT_TRUE(mir_buffer_stream_is_valid(bs));
135 EXPECT_THAT(mir_buffer_stream_get_error_message(bs), StrEq(""));137 EXPECT_THAT(mir_buffer_stream_get_error_message(bs), StrEq(""));
136138
137 mir_render_surface_release(rs);139 mir_render_surface_release_sync(rs);
138 mir_connection_release(connection);140 mir_connection_release(connection);
139}141}
140142
@@ -157,7 +159,7 @@
157 EXPECT_THAT(surface, IsValid());159 EXPECT_THAT(surface, IsValid());
158 EXPECT_THAT(mir_surface_get_buffer_stream(surface), Eq(nullptr));160 EXPECT_THAT(mir_surface_get_buffer_stream(surface), Eq(nullptr));
159161
160 mir_render_surface_release(rs);162 mir_render_surface_release_sync(rs);
161 mir_surface_release_sync(surface);163 mir_surface_release_sync(surface);
162 mir_connection_release(connection);164 mir_connection_release(connection);
163}165}
@@ -184,10 +186,11 @@
184 usage);186 usage);
185187
186 EXPECT_THAT(surface, IsValid());188 EXPECT_THAT(surface, IsValid());
189
187 EXPECT_THAT(mir_surface_get_buffer_stream(surface), Eq(nullptr));190 EXPECT_THAT(mir_surface_get_buffer_stream(surface), Eq(nullptr));
188 EXPECT_TRUE(mir_buffer_stream_is_valid(bs));191 EXPECT_THAT(bs, IsValid());
189192
190 mir_render_surface_release(rs);193 mir_render_surface_release_sync(rs);
191 mir_surface_release_sync(surface);194 mir_surface_release_sync(surface);
192 mir_connection_release(connection);195 mir_connection_release(connection);
193}196}
@@ -204,7 +207,7 @@
204 ASSERT_THAT(pc, NotNull());207 ASSERT_THAT(pc, NotNull());
205 EXPECT_TRUE(mir_presentation_chain_is_valid(pc));208 EXPECT_TRUE(mir_presentation_chain_is_valid(pc));
206209
207 mir_render_surface_release(rs);210 mir_render_surface_release_sync(rs);
208 mir_connection_release(connection);211 mir_connection_release(connection);
209}212}
210213
@@ -224,7 +227,7 @@
224227
225 EXPECT_THAT(pc2, Eq(nullptr));228 EXPECT_THAT(pc2, Eq(nullptr));
226229
227 mir_render_surface_release(rs);230 mir_render_surface_release_sync(rs);
228 mir_connection_release(connection);231 mir_connection_release(connection);
229}232}
230233
@@ -249,7 +252,7 @@
249 EXPECT_THAT(mir_surface_get_buffer_stream(surface), Eq(nullptr));252 EXPECT_THAT(mir_surface_get_buffer_stream(surface), Eq(nullptr));
250 EXPECT_TRUE(mir_presentation_chain_is_valid(pc));253 EXPECT_TRUE(mir_presentation_chain_is_valid(pc));
251254
252 mir_render_surface_release(rs);255 mir_render_surface_release_sync(rs);
253 mir_surface_release_sync(surface);256 mir_surface_release_sync(surface);
254 mir_connection_release(connection);257 mir_connection_release(connection);
255}258}
@@ -265,7 +268,7 @@
265 ASSERT_THAT(pc, NotNull());268 ASSERT_THAT(pc, NotNull());
266 EXPECT_TRUE(mir_presentation_chain_is_valid(pc));269 EXPECT_TRUE(mir_presentation_chain_is_valid(pc));
267270
268 mir_render_surface_release(rs);271 mir_render_surface_release_sync(rs);
269 mir_connection_release(connection);272 mir_connection_release(connection);
270}273}
271274
@@ -289,7 +292,7 @@
289292
290 EXPECT_THAT(bs, Eq(nullptr));293 EXPECT_THAT(bs, Eq(nullptr));
291294
292 mir_render_surface_release(rs);295 mir_render_surface_release_sync(rs);
293 mir_connection_release(connection);296 mir_connection_release(connection);
294}297}
295298
@@ -306,13 +309,141 @@
306 mir_pixel_format_abgr_8888,309 mir_pixel_format_abgr_8888,
307 mir_buffer_usage_hardware);310 mir_buffer_usage_hardware);
308311
309 ASSERT_THAT(bs, NotNull());312 EXPECT_THAT(bs, IsValid());
310 EXPECT_TRUE(mir_buffer_stream_is_valid(bs));
311313
312 auto pc = mir_render_surface_get_presentation_chain(rs);314 auto pc = mir_render_surface_get_presentation_chain(rs);
313315
314 EXPECT_THAT(pc, Eq(nullptr));316 EXPECT_THAT(pc, Eq(nullptr));
315317
316 mir_render_surface_release(rs);318 mir_render_surface_release_sync(rs);
317 mir_connection_release(connection);319 mir_connection_release(connection);
320}
321
322TEST_F(RenderSurfaceTest, asynchronous_render_surface_destruction_works_with_realised_buffer_stream)
323{
324 auto physical_size = logical_size;
325 auto connection = mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__);
326
327 auto rs = mir_connection_create_render_surface_sync(
328 connection, logical_size.width.as_int(), logical_size.height.as_int());
329 mir_render_surface_get_buffer_stream(
330 rs,
331 physical_size.width.as_int(), physical_size.height.as_int(),
332 mir_pixel_format_abgr_8888,
333 mir_buffer_usage_hardware);
334
335 struct Context
336 {
337 MirRenderSurface* rs;
338 mt::Signal done;
339 } context;
340
341 context.rs = rs;
342
343 mir_render_surface_release(
344 rs,
345 [](MirRenderSurface* rs, void* ctx)
346 {
347 auto& context = *reinterpret_cast<Context*>(ctx);
348 EXPECT_THAT(rs, Eq(context.rs));
349 /*
350 * Should really test that the render surface isn't valid here, but that fails.
351 *
352 EXPECT_THAT(rs, Not(IsValid()));
353 */
354 context.done.raise();
355 },
356 &context);
357
358 ASSERT_TRUE(context.done.wait_for(std::chrono::seconds{20}));
359
360 mir_connection_release(connection);
361}
362
363TEST_F(RenderSurfaceTest, asynchronous_render_surface_destruction_works_with_realised_presentation_chain)
364{
365 auto connection = mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__);
366
367 auto rs = mir_connection_create_render_surface_sync(
368 connection, logical_size.width.as_int(), logical_size.height.as_int());
369 mir_render_surface_get_presentation_chain(rs);
370
371 struct Context
372 {
373 MirRenderSurface* rs;
374 mt::Signal done;
375 } context;
376
377 context.rs = rs;
378
379 mir_render_surface_release(
380 rs,
381 [](MirRenderSurface* rs, void* ctx)
382 {
383 auto& context = *reinterpret_cast<Context*>(ctx);
384 EXPECT_THAT(rs, Eq(context.rs));
385 /*
386 * Should really test that the render surface isn't valid here, but that fails.
387 *
388 EXPECT_THAT(rs, Not(IsValid()));
389 */
390 context.done.raise();
391 },
392 &context);
393
394 ASSERT_TRUE(context.done.wait_for(std::chrono::seconds{20}));
395
396 mir_connection_release(connection);
397}
398
399TEST_F(RenderSurfaceTest, asynchronous_render_surface_destruction_works_with_no_backing_object)
400{
401 auto connection = mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__);
402
403 auto rs = mir_connection_create_render_surface_sync(
404 connection, logical_size.width.as_int(), logical_size.height.as_int());
405
406 struct Context
407 {
408 MirRenderSurface* rs;
409 mt::Signal done;
410 } context;
411
412 context.rs = rs;
413
414 mir_render_surface_release(
415 rs,
416 [](MirRenderSurface* rs, void* ctx)
417 {
418 auto& context = *reinterpret_cast<Context*>(ctx);
419 EXPECT_THAT(rs, Eq(context.rs));
420 /*
421 * Should really test that the render surface isn't valid here, but that fails.
422 *
423 EXPECT_THAT(rs, Not(IsValid()));
424 */
425 context.done.raise();
426 },
427 &context);
428
429 ASSERT_TRUE(context.done.wait_for(std::chrono::seconds{20}));
430
431 mir_connection_release(connection);
432}
433
434TEST_F(RenderSurfaceTest, asynchronous_destruction_calls_callback_on_failure)
435{
436 auto not_a_valid_client_side_rs = reinterpret_cast<MirRenderSurface*>(0x0ff01bb1);
437
438 mt::Signal callback_called;
439
440 mir_render_surface_release(
441 not_a_valid_client_side_rs,
442 [](MirRenderSurface*, void* ctx)
443 {
444 reinterpret_cast<mt::Signal*>(ctx)->raise();
445 },
446 &callback_called);
447
448 EXPECT_TRUE(callback_called.wait_for(std::chrono::seconds{20}));
318}449}
319450
=== modified file 'tests/mir_test/CMakeLists.txt'
--- tests/mir_test/CMakeLists.txt 2016-11-09 02:30:14 +0000
+++ tests/mir_test/CMakeLists.txt 2016-12-08 03:24:08 +0000
@@ -2,6 +2,10 @@
2string (REPLACE " -flto " " " CMAKE_C_FLAGS ${CMAKE_C_FLAGS})2string (REPLACE " -flto " " " CMAKE_C_FLAGS ${CMAKE_C_FLAGS})
3string (REPLACE " -flto " " " CMAKE_CXX_FLAGS ${CMAKE_CXX_FLAGS})3string (REPLACE " -flto " " " CMAKE_CXX_FLAGS ${CMAKE_CXX_FLAGS})
44
5include_directories(
6 ${PROJECT_SOURCE_DIR}/src/include/client
7)
8
5add_library(mir-public-test OBJECT9add_library(mir-public-test OBJECT
6 cross_process_action.cpp10 cross_process_action.cpp
7 cross_process_sync.cpp11 cross_process_sync.cpp
812
=== modified file 'tests/mir_test/validity_matchers.cpp'
--- tests/mir_test/validity_matchers.cpp 2015-06-25 03:00:08 +0000
+++ tests/mir_test/validity_matchers.cpp 2016-12-08 03:24:08 +0000
@@ -1,5 +1,6 @@
1#include "mir/test/validity_matchers.h"1#include "mir/test/validity_matchers.h"
2#include "mir_toolkit/mir_client_library.h"2#include "mir_toolkit/mir_client_library.h"
3#include "mir_toolkit/mir_render_surface.h"
34
4template<>5template<>
5bool IsValidMatcher::MatchAndExplain(MirConnection* connection, MatchResultListener* listener) const6bool IsValidMatcher::MatchAndExplain(MirConnection* connection, MatchResultListener* listener) const
@@ -30,3 +31,33 @@
30 }31 }
31 return true;32 return true;
32}33}
34
35template<>
36bool IsValidMatcher::MatchAndExplain(MirBufferStream* stream, MatchResultListener* listener) const
37{
38 if (stream == nullptr)
39 {
40 return false;
41 }
42 if (!mir_buffer_stream_is_valid(stream))
43 {
44 *listener << "error is: " << mir_buffer_stream_get_error_message(stream);
45 return false;
46 }
47 return true;
48}
49
50template<>
51bool IsValidMatcher::MatchAndExplain(MirRenderSurface* rs, MatchResultListener* listener) const
52{
53 if (rs == nullptr)
54 {
55 return false;
56 }
57 if (!mir_render_surface_is_valid(rs))
58 {
59 *listener << "error is: " << mir_render_surface_get_error_message(rs);
60 return false;
61 }
62 return true;
63}
3364
=== modified file 'tests/unit-tests/client/test_mir_render_surface.cpp'
--- tests/unit-tests/client/test_mir_render_surface.cpp 2016-12-01 18:10:01 +0000
+++ tests/unit-tests/client/test_mir_render_surface.cpp 2016-12-08 03:24:08 +0000
@@ -44,10 +44,11 @@
4444
45namespace45namespace
46{46{
47void assign_result(void* result, void** context)47template<typename T>
48void assign_result(T* result, void* context)
48{49{
49 if (context)50 if (context)
50 *context = result;51 *reinterpret_cast<T**>(context) = result;
51}52}
5253
53struct RenderSurfaceCallback54struct RenderSurfaceCallback
@@ -208,14 +209,14 @@
208 MirRenderSurface* render_surface = nullptr;209 MirRenderSurface* render_surface = nullptr;
209 connection->create_render_surface_with_content(210 connection->create_render_surface_with_content(
210 {10, 10},211 {10, 10},
211 reinterpret_cast<mir_render_surface_callback>(assign_result),212 &assign_result,
212 &render_surface,213 &render_surface,
213 &nw);214 &nw);
214215
215 EXPECT_THAT(render_surface, NotNull());216 EXPECT_THAT(render_surface, NotNull());
216 EXPECT_THAT(nw, NotNull());217 EXPECT_THAT(nw, NotNull());
217 EXPECT_THAT(render_surface, Eq(nw));218 EXPECT_THAT(render_surface, Eq(nw));
218 EXPECT_NO_THROW(connection->release_render_surface_with_content(nw));219 EXPECT_NO_THROW(connection->release_render_surface_with_content(nw, &assign_result, nullptr));
219220
220 connection->disconnect();221 connection->disconnect();
221}222}

Subscribers

People subscribed via source and target branches