Subject: mir_render_surface_get_buffer_stream

From: Christopher James Halse Rogers <[email-masked]>
Date: Thu, 01 Dec 2016 13:35:01 +1100

So, client-eventloop-driven MirConnection is getting really close to 
working properly.

The latest problem I've run into is 
mir_render_surface_get_buffer_stream - this purports to be a no-RPC 
call and so doesn't take a callback/have a _sync version, but the 
mcl::BufferStream constructor *does* perform hidden (poorly¹) RPC.

That's easy enough; just change it to take a callback & add a _sync 
version.

But looking at it I've got a second concern. 
mir_render_surface_get_buffer_stream takes a whole bunch of parameters 
- width, height, pixel format, buffer_usage. However, these are only 
used in the *first* call to get_buffer_stream; they're silently ignored 
in any subsequent call.

This seems like an API with an unnecessary hidden razorblade - the 
specifics of the buffer stream returned from this depend on whether or 
not the function had previously been called, which seems likely to lead 
to difficult-to-debug problems for clients.

I think we should disallow multiple calls to 
mir_render_surface_get_buffer_stream². It means that client code needs 
to be slightly more entangled - any time code might need to modify both 
the RenderSurface and the BufferStream it'll need to be provided with 
both rather than just the RenderSurface. This won't result in tighter 
coupling of the client code, though, because the code is *currently* 
coupled. The API just doesn't make it clear.

Thoughts?

¹: If you use the buffer stream at the wrong time you'll deadlock.
²: For the same RenderSurface, obviously.

---

From: Daniel van Vugt <[email-masked]>
Date: Thu, 1 Dec 2016 10:40:33 +0800

Tangentially, I'm still hanging out for a rename before 
mir_render_surface_* becomes a public API.

MirRenderSurface I hope will get a shorter name that closer to a one 
word noun. Although the ideal of making it MirSurface after the existing 
MirSurface becomes MirWindow is quite ambitious.

On 01/12/16 10:35, Christopher James Halse Rogers wrote:
> The latest problem I've run into is mir_render_surface_get_buffer_stream
> - this purports to be a no-RPC call and so doesn't take a callback/have
> a _sync version, but the mcl::BufferStream constructor *does* perform
> hidden (poorly¹) RPC.
>
> That's easy enough; just change it to take a callback & add a _sync version.
>
> mir_render_surface_get_buffer_stream takes a whole bunch of parameters -
> width, height, pixel format, buffer_usage. However, these are only used
> in the *first* call to get_buffer_stream; they're silently ignored in
> any subsequent call.
>

---

From: Kevin DuBois <[email-masked]>
Date: Thu, 1 Dec 2016 08:18:09 -0500

On Wed, Nov 30, 2016 at 9:40 PM, Daniel van Vugt <
[email-masked]> wrote:

> MirRenderSurface I hope will get a shorter name that closer to a one word
> noun. Although the ideal of making it MirSurface after the existing
Agree. I think it needs to be done before 1.0, and would be nice to do
before making the header public.
Another part that's proving hard to transition to the 1.0 goals is the
removal of MirWaitHandle.
It makes sense to me to place vetted and bikeshedded headers into a 1.0 api
directory to ease the transition.

>> coupled. The API just doesn't make it clear.
+1 in general. We're trying to hide too much from the user by saying that a
MirRenderSurface already has the resources that all the subsequent usages
of the MirRenderSurface will need. We should allocate memory for the types
the user wants when the user asks, and free it when the user asks.

Disallowing multiple associations is acceptable, although, this makes sense
to me as well:

MirConnection* conn = ...;
//current form:
auto rs = mir_connection_create_render_surface_sync(connection,
logical_width, logical_height);
//note name change and loss of MirBufferUsage
auto bs = mir_render_surface_create_buffer_stream(rs, physical_width,
physical_height, format);

//the rs is now associated with the bs. doing
//auto bs2 = mir_render_surface_create_buffer_stream(rs, physical_width,
physical_height, format);
//will give an 'error buffer stream', or fail. (similarly with PC/egl types
[1])
//however this will still work, as we're arranging the scene, not trying to
mix the way that RS is being used.
mir_surface_spec_add_render_surface(spec, rs, ...);

//new function, after which user can reuse rs
mir_buffer_stream_destroy(bs);

[1]
We probably need a:
bool mir_render_surface_associated(MirRenderSurface* rs);
so that the EGL driver with its limited context of what the client has done
before eglCreateWindowSurface()was called can fail sensibly the
MirRenderSurface is still under use.

> --
> Mir-devel mailing list
> [email-masked]
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailm
> an/listinfo/mir-devel
>

---

From: Kevin DuBois <[email-masked]>
Date: Thu, 1 Dec 2016 08:21:13 -0500

On Thu, Dec 1, 2016 at 8:18 AM, Kevin DuBois <[email-masked]>
wrote:

> It makes sense to me to place vetted and bikeshedded headers into a 1.0
> api directory to ease the transition.
>
>

Oh, also to strengthen the call for the transition,
http://vulkan-spec-chunked.ahcox.com/ch29s02.html#VkMirSurfaceCreateInfoKHR
seems to already have "MirSurface*" as the surface we're passing in, so it
seems more appropriate to have
MirSurface->MirWindow,
MirRenderSurface->MirSurface
and then the WSI would have the object that represents the place that the
buffers go. (and the user could integrate a multi "surface" scene in a
"window")

> +1 in general. We're trying to hide too much from the user by saying that
> a MirRenderSurface already has the resources that all the subsequent usages
> Disallowing multiple associations is acceptable, although, this makes
> sense to me as well:
>
> //will give an 'error buffer stream', or fail. (similarly with PC/egl
> types [1])
> //however this will still work, as we're arranging the scene, not trying
> to mix the way that RS is being used.
> so that the EGL driver with its limited context of what the client has
> done before eglCreateWindowSurface()was called can fail sensibly the

---

From: Alan Griffiths <[email-masked]>
Date: Thu, 1 Dec 2016 14:29:08 +0000

On 01/12/16 13:18, Kevin DuBois wrote:
>
>
>
>     MirRenderSurface I hope will get a shorter name that closer to a
>     one word noun. Although the ideal of making it MirSurface after
>     the existing MirSurface becomes MirWindow is quite ambitious.
>
>

Are we really intending to rework all the code "out there" that refers
to MirSurface?!

I agree the name is wrong, but I think the time to change it has passed[*].

Alan

[*] If only version control systems /1/ understood refactorings, and /2/
talked to each other.

---

From: Alan Griffiths <[email-masked]>
Date: Thu, 1 Dec 2016 14:35:58 +0000

On 01/12/16 14:29, Alan Griffiths wrote:
>> Agree. I think it needs to be done before 1.0, and would be nice to
>> do before making the header public.
>
>
I think we should instead to something like: Add MrSurface and MrWindow.

And for backwards compatibility:

    typedef MirSurface MrWindow;

---

From: Cemil Azizoglu <[email-masked]>
Date: Thu, 1 Dec 2016 12:13:47 -0600

On Wed, Nov 30, 2016 at 8:35 PM, Christopher James Halse Rogers
<[email-masked]> wrote:
> So, client-eventloop-driven MirConnection is getting really close to working
> properly.
>
> The latest problem I've run into is mir_render_surface_get_buffer_stream -
> this purports to be a no-RPC call and so doesn't take a callback/have a
> _sync version, but the mcl::BufferStream constructor *does* perform hidden
> (poorly¹) RPC.
>
> width, height, pixel format, buffer_usage. However, these are only used in
> the *first* call to get_buffer_stream; they're silently ignored in any
> subsequent call.
>
> This seems like an API with an unnecessary hidden razorblade - the specifics
> of the buffer stream returned from this depend on whether or not the
> function had previously been called, which seems likely to lead to
> difficult-to-debug problems for clients.
>
> mir_render_surface_get_buffer_stream². It means that client code needs to be

Implemented disallowing multiple calls to mir_render_surface_get_* and pushed to

lp:~cemil-azizoglu/mir/create-chain-from-render-surface

Also adjusted the tests to account for this case.

> slightly more entangled - any time code might need to modify both the
> RenderSurface and the BufferStream it'll need to be provided with both
> rather than just the RenderSurface. This won't result in tighter coupling of
> the client code, though, because the code is *currently* coupled. The API
> just doesn't make it clear.
>
> Thoughts?
>
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/mir-devel
>

---

From: Daniel van Vugt <[email-masked]>
Date: Fri, 2 Dec 2016 09:34:36 +0800

Indeed that was my position a couple of months ago. But I was swayed by 
the team's bullishness about ABI/API breaks before "1.0". I mentioned 
that was still only an ideal though...

More realistically I think we'll probably end up keeping MirSurface as 
is, because that nomenclature has permeated too far beyond a single 
codebase already. That just makes finding a better name for 
MirRenderSurface a bit more difficult.

---

From: Daniel van Vugt <[email-masked]>
Date: Fri, 2 Dec 2016 10:26:13 +0800

Completely forgot about MirBufferStream too. When it was first 
introduced some years ago I suggested more correct (and elegant) names were:
    MirSurface -> MirWindow
    MirBufferStream -> MirSurface

Now we've got a third object 'MirRenderSurface' coming I'm not even sure 
what the ideal naming scheme is.

Can we avoid having three objects and somehow keep it at two? Isn't 
MirBufferStream going to be just a specific case/mode of a MirRenderSurface?