On Thu, Dec 1, 2016 at 8:18 AM, Kevin DuBois <kevin.dub...@canonical.com> wrote:
> > > On Wed, Nov 30, 2016 at 9:40 PM, Daniel van Vugt < > daniel.van.v...@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@lists.ubuntu.com >> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailm >> an/listinfo/mir-devel >> > >
-- Mir-devel mailing list Mir-devel@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/mir-devel
