Merge lp://qastaging/~afrantzis/mir/buffer-bind-to-render-image into lp://qastaging/mir

FAILED: Continuous integration, rev:2852
http://jenkins.qa.ubuntu.com/job/mir-ci/4603/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3567
    FAILURE: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/429/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/1101/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3515/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/752
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/752/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3515
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3515/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6290/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22652

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4603/rebuild

AFAIK, the verb "bind" is very much a GL specific concept. So the "gl_" prefix made sense in communicating that it must be called in a GL context. Not yet sure how much it's been generalized now and if this yet warrants removal of the "gl_" prefix...

Hm. Does it not work to have using RenderImage = void; ?

Oh, right. Because other renderers are going to shove some actual data in there...

Thinking about it, I kinda wonder to what extent Buffer is a sensible abstraction, or whether we'd be better off with three different ShmBuffer, GLBuffer, VulkanBuffer interfaces on top of a smaller Buffer that contains the common elements - id, size, stride, buffer format. We already don't support read/write on GL buffers, and NativeHandle is purely an IPC thing which would probably be better if implemented in the platform? Maybe?

The code itself looks fine. I don't know if there's a better architecture.

> AFAIK, the verb "bind" is very much a GL specific concept. So the "gl_" prefix
> made sense in communicating that it must be called in a GL context. Not yet sure
> how much it's been generalized now and if this yet warrants removal of the "gl_" prefix.

The main difference is that this method can return information that the renderer can use. For example for software rendering it could return a struct describing a memory area. At the same time, however, the API needs to support the GL way of doing things (implicit context).

> Thinking about it, I kinda wonder to what extent Buffer is a sensible abstraction,
> or whether we'd be better off with three different ShmBuffer, GLBuffer, VulkanBuffer
> interfaces on top of a smaller Buffer that contains the common elements - id, size,
> stride, buffer format. We already don't support read/write on GL buffers, and
> NativeHandle is purely an IPC thing which would probably be better if implemented
> in the platform? Maybe?

So, if I understand correctly, the buffer allocator will create a Shm/GL/Vulkan buffer (an interface specified by the corresponding renderer) which will be returned as an mg::Buffer and then each renderer can dynamically cast it back to its original type (since it knows what type to expect). This could work, as long as all the operations that extract rendering-specific information happen in code that is specific for each renderer type.

The intention is to offer something similar for the DisplayBuffer interface (since renderers need a render target). There are some complications in this case, since some renderer-related operations, e.g. make_current(), are currently called in rendering-agnostic code due to the nature of per-thread GL contexts, but perhaps we can make this work.

That's an excellent point. I also wonder if Buffer (or RenderImage) is a sensible abstraction. Maybe multiple implementations instead, each with their own semantics, would be better.

Tangentially I've been thinking we have needed for a long time to be able to tell the difference between a software buffer uploaded (copied) to GL and a platform buffer uploaded (shared) to GL. For performance so we know the earliest safe time to release the buffer associated with the resulting texture. Maybe the whole generic Buffer (or RenderImage) abstraction is the problem.

Similarly, for bypass that feature only works specifically with GBM buffers. It would be nice if these distinctions were more explicit...

The buffer allocator is therefore also a faulty abstraction. :)

It wouldn't hurt to keep Buffer as a base class and used in high-level code like BufferQueue and its replacement. But in the platform and rendering code, we'd be better off to not ever deal with the generic "Buffer".

> > Thinking about it, I kinda wonder to what extent Buffer is a sensible
> abstraction,
> > or whether we'd be better off with three different ShmBuffer, GLBuffer,
> VulkanBuffer
> > interfaces on top of a smaller Buffer that contains the common elements -
> id, size,
> > stride, buffer format. We already don't support read/write on GL buffers,
> and
> > NativeHandle is purely an IPC thing which would probably be better if
> implemented
> > in the platform? Maybe?
>
> So, if I understand correctly, the buffer allocator will create a
> Shm/GL/Vulkan buffer (an interface specified by the corresponding renderer)
> which will be returned as an mg::Buffer and then each renderer can dynamically
> cast it back to its original type (since it knows what type to expect).

I think I was thinking that the buffer allocator will create a Shm/GL/Vulkan buffer and *return* it as a Shm/GL/Vulkan buffer to the renderer.

Code that only cares about generic “bufferness” can still use mg::Buffer, but the renderer is intimately tied up with its buffer type.

In summary, I think we need:

Platform code:
  GLBuffer allocate_gl_buffer();
  ShmBuffer allocate_shm_buffer(); // Should work on all platforms

Rendering code:
  Switch behaviour based on the type of buffer. e.g. a ShmBuffer can be released immediately after glTexImage2D. However a GLBuffer must be held till after swapbuffers as the driver is likely sharing memory.

Generic buffer queuing code:
  Can refer to them all as "Buffer" which is the base class of the things being queued.

I'm not sure that BufferAllocator is a faulty abstraction; in (most - hello, Vulkan!) cases we'll still want to be able to allocate buffers, and width/height/pixel format are basically the interesting properties there, so there's a natural abstraction.

BufferAllocator can still exist. Just saying maybe it should not return the base class "Buffer".

Actually ShmBuffer should not be hidden behind an allocator at all. It would work and should be used on all platforms.

@Chris
> I think I was thinking that the buffer allocator will create a Shm/GL/Vulkan buffer
> and *return* it as a Shm/GL/Vulkan buffer to the renderer.
>
> Code that only cares about generic “bufferness” can still use mg::Buffer,
> but the renderer is intimately tied up with its buffer type.

@Daniel
> Platform code:
> GLBuffer allocate_gl_buffer();
> ShmBuffer allocate_shm_buffer(); // Should work on all platforms

The problem with the above proposals is that our core will be forced to become aware of specific buffer and renderer types, whereas it has no business caring about them. Only the platform and the renderer should care about rendering related implementation details.

With this scheme, adding a new buffer/renderer type is burdensome, and most importantly is not transparent to our core: if a vendor wants to add a platform with a new buffer/renderer type, the vendor will need to patch the Mir core code.

My proposal is that the platform creates a buffer with specific rendering support, passes it to the core as a generic mg::Buffer, and when a component needs to render it knows how to access the rendering capabilities (with either the RenderImage bind_to_render_image() approach in this MP, or by downcasting mg::Buffer to a GL/Vulkan/WhateverBuffer, which I also like).

To illustrate that renderers and core need not change for new drivers/platforms:

class GLBuffer : public Buffer
class LinearBuffer : public Buffer // addressable with a stride etc
class VkBuffer : public Buffer

class ShmBuffer : public LinearBuffer
class DirectFBBuffer : public LinearBuffer

class GBMBuffer : public GLBuffer
class NvidiaGLBuffer : public GLBuffer
class NvidiaVkBuffer : public VkBuffer
class AndroidBuffer : public GLBuffer

Mesa driver:
   GLBuffer allocate_gl_buffer(); // returns GBMBuffer

Android driver:
   GLBuffer allocate_gl_buffer(); // returns AndroidBuffer

Nvidia driver:
   GLBuffer allocate_gl_buffer(); // returns NvidiaGLBuffer
   VkBuffer allocate_vk_buffer(); // returns NvidiaVkBuffer

libmirplatform:
   LinearBuffer allocate_linear_buffer(); // returns ShmBuffer usually

Drivers could optionally override with their own:
   LinearBuffer allocate_linear_buffer();
but it's generally not required or even recommended in most cases.

DirectFB:
   LinearBuffer allocate_linear_buffer(); // might return a DirectFBBuffer

Renderers:
   GL/GLES: Understands GLBuffer and LinearBuffer
   Vulkan: Understands VkBuffer and LinearBuffer
   Joe's cool software blitter: Understands LinearBuffer

New renderers are only required for new graphics languages, which are quite rare.

Maybe GLBuffer could be called "EGLImageBuffer". Because a LinearBuffer may contain drawing from a GL client too (software rendering via Mesa LLVMpipe). And all renderers should usually be capable of displaying a LinearBuffer.

> New renderers are only required for new graphics languages, which are quite rare.

Two examples:

* DirectFB works with its own image/surface types. The information provided by a LinearBuffer (starting address, width, height, stride etc) is not sufficient.

* Similarly 2D blitters may need more or different information than a LinearBuffer provides, thus requiring a new buffer and renderer type.

But even if we accept that GL/Vulkan/Linear are enough, what's the benefit of the core knowing about these types?

The benefit is that you can implement a couple of default renderers in core and then never touch those renders again. No matter how many new drivers/platforms are added over time.

I have indeed been making the assumption that DirectFB is linear underneath and you can somehow get the address of those pixels. If that was not true, then the answer is still simple: Then DirectFB is just another graphics language. Admittedly that would then require a new renderer, but new graphics languages are rare.

> The benefit is that you can implement a couple of default renderers in core
> and then never touch those renders again. No matter how many new drivers/platforms
> are added over time.

The ultimate plan (i.e. long-term goal, don't expect it to happen very soon) is to have some renderers in the mir code base as plugins, but not as part of our "core" code. The definitions needed to implement the renderer and platforms that support that renderer (i.e. the information returned by bind_to_render_image(), or the definitions of platform-specific Buffer interfaces, depending on how we move forward) will be controlled by the renderer and will be shipped in packages like:

mir-server-renderer-gles2-dev: /usr/include/mirserver/renderer/gles2/gles2_render_image.h
mir-server-renderer-vulkan-dev: /usr/include/mirserver/renderer/vulkan/vulkan_render_image.h

This way platforms/renderers can access and implement these interfaces for their own platforms, or they can use their own type (e.g. "vendor-bla-2d-blitter") which they don't have to publish in a package.

Of course, we will also provide default renderer implementations shipped in packages like:

mir-server-renderer-gles2-default: /usr/lib/.../mirserver/renderers/gles2(_default).so

A Buffer is a Generic Thing. Having a mg::Buffer that all buffers implement makes sense to a lot of the core code that doesn't have to access the content, it just has to pass the resource around, or figure out some of the basic concepts.

The parts on GraphicBufferAllocator that don't make sense is how mg::BufferProperties takes a {hardware/software} parameter. It should take a {gl/vulkan/cpu} (bitmask maybe?), so that platforms that have to allocate a bit differently can.

The parts on Buffer that don't make sense in the generic sense are mg::Buffer::gl_bind_to_texture(), mg::Buffer::read(), mg::Buffer::write(), because these are all functions that access the mg::Buffer::native_buffer_handle(), and do some platform-specific operations on that for the particular operation. It doesn't scale very well though, because of the number of different types of buffers, each which might not be able to service GL or cpu commands.

It makes the most sense to me to have some platform-created Mapper objects. The mapper objects manipulate the mg::NativeBuffers, and give out the correct resources for that rendering mode. A Vulkan renderer coder wouldn't be inclined to try to use the GLMapper. (OTOH, a Vulkan renderer wouldnt be inclined to call mg::Buffer::gl_bind_to_texture either). Objects that just care that the have a Buffer (like the frontend code and the swapping code) would be shielded from the binding/mapping/access functions, which is a bit nicer.

Now, having said all that... This MP is pretty similiar to that scheme, except that it has no Mappers that the renderers get, and the mg::Buffer functions will end up having to throw more.

So I guess I'm okay with this design, but:

sp<NativeHandle> mg::Buffer::native_buffer_handle();
and
sp<RenderImage> mg::Buffer::bind_to_render_image();

seem to be two different ways to say the same thing, with the RenderImage perhaps sneaking in some platform-specific logic eventually without having the Renderers have to have access to some sort of mapping logic outside of mg::Buffer.

If Renderer is not counted as "core" in future then I don't understand what part of Mir's core you think would be platform specific in any way. Although if a Renderer is bound to a graphics language only and not a platform, then I don't see any reason to want to separate it from the project. Given we will still require mentions of those same graphics languages in the Mir source code, separating the renderers would be detrimental to the Mir ecosystem and our productivity (increased coupling and lower cohesion).

I think we need to stop talking about DirectFB for starters... DirectFB is a toolkit, which me may not use for something in the distant future. Citing this hypothetical thing as part of our design decisions is probably going to take us down the wrong path. Especially given we're not intimately familiar with it, or even know if we need it for anything at all in future.

Let's focus on the real things for now: OpenGL, Vulkan, blitting
Those are our three primary graphics languages that our core interfaces (and renderers) need to know about.

There's a whole bunch of Mir code that talks about Buffers but is not either the BufferAllocator or the Renderer - BufferQueue, BufferBundle, Compositor, the frontend all have some interaction with Buffers; none of these care about the buffer content. Triplicating all of these - GL, Vulkan, blit - seems a bit excessive.

We *could* plausibly provide these as templates that the platform specialises, but that's a significant undertaking.

Hm. That might end up being a much better fit, actually. As I've mentioned before, I'm not sure that an EGLStream-based platform would fit our current abstractions.

> So I guess I'm okay with this design, but:
>
> sp<NativeHandle> mg::Buffer::native_buffer_handle();
> and

I'm not sure in what way this is different to sp<void> mg::Buffer::native_buffer_handle(), or, in fact, how it differs from auto VkBuffer = dynamic_pointer_cast<mg::VkBuffer>(generic_buffer)? Except, I guess, performance, which is a reasonable point. We don't build with RTTI disabled, though, for other reasons...

> There's a whole bunch of Mir code that talks about Buffers but is not either
> the BufferAllocator or the Renderer - BufferQueue, BufferBundle, Compositor,
> the frontend all have some interaction with Buffers; none of these care about
> the buffer content. Triplicating all of these - GL, Vulkan, blit - seems a bit
> excessive.

I agree and have suggested "Buffer" remains as the base class for those use cases.

I don't think assuming we will have only a few buffer/renderer types (GL, Vk, Shm) is the right way IMHO. E.g. 2D engines have proprietary (low-level) APIs (so it _is_ akin to having a new graphics API). Each graphics vendor would potentially have one. And they'd want to use their own allocators as only they would know what sort of buffers would work with the hardware (pf, alignment, etc). They would also want to do things like bind_to_render_image() just like gl would do.

So keeping things generic in core, and having the platforms be able to decode them would be preferable.

From IRC it looks like we've settled on the void* mg::Buffer::native_type() approach. I'm fine with that.

Internally the renderer will either have to use dynamic_cast<>, or for us to implement our own simple RTTI
(eg:
struct NativeBuffer
{
  intptr_t type_tag;
  void* data;
}

with type_tag being filled in with a pointer to a known tag, like NativeBuffer { &gl_buffer_tag, buffer_data })

I'd like to say I don't care and have expressed enough opinion already, but looking again I find the implementation still overly vague. RenderImage and...

21 - renderable->buffer()->gl_bind_to_texture();
22 + renderable->buffer()->bind_to_render_image();

makes the code harder to follow because we no longer explicitly state what's going on, or even what's being operated on under what semantics. For example if you're using OpenGL the semantics are very specific to OpenGL (prior to Vulkan existing), and even the type of buffer changes how your code calling "bind" should behave. Software buffers and bound quite differently to EGLImage and so should be treated differently for greatest efficiency (see LP: #1264934).

We may not have come across the right solution yet, but I feel what's proposed here is significantly worse than what it replaces. So more thought is required.

See:

https://code.launchpad.net/~afrantzis/mir/buffer-native-type/+merge/269226
https://code.launchpad.net/~afrantzis/mir/texture-bindable/+merge/269231

for an alternative approach based on casting mg::Buffers to their native types.

Rejected in favor of https://code.launchpad.net/~afrantzis/mir/buffer-native-buffer-base/+merge/269363