mir_render_surface_get_buffer_stream

[Kevin DuBois]

On Thu, Dec 1, 2016 at 8:18 AM, Kevin DuBois <kevin.dubois at canonical.com>
wrote:

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

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")


>
>>
>> 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/b07b2cfb/attachment.html>