mir_render_surface_get_buffer_stream

[Kevin DuBois]

On Wed, Nov 30, 2016 at 9:40 PM, Daniel van Vugt <
daniel.van.vugt at canonical.com> wrote:

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


>
>
> On 01/12/16 10:35, Christopher James Halse Rogers 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.
>>
>> 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?
>>
>
+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.


>> ¹: If you use the buffer stream at the wrong time you'll deadlock.
>> ²: For the same RenderSurface, obviously.
>>
>>
>>
> --
> Mir-devel mailing list
> Mir-devel at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailm
> an/listinfo/mir-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/mir-devel/attachments/20161201/836cb715/attachment.html>