Merge lp://qastaging/~afrantzis/mir/buffer-native-type into lp://qastaging/mir

PASSED: Continuous integration, rev:2880
http://jenkins.qa.ubuntu.com/job/mir-ci/4684/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3682
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/103
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2593
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3632
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/833
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/833/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3632
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3632/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6401
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22844

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

Just to represent the mapper approach:

Like, what I had envisioned is (some pseudocode ahead):
mg::Mapper
{
    GLBinding bind_to_gl(std::shared_ptr<NativeBuffer> const&, GLContext&);
    vkImage vulkan_image_from(std::shared_ptr<NativeBuffer> const&);
    MemoryRegion map_to_cpu(std::shared_ptr<NativeBuffer> const&);
}

plus:
mg::Buffer
{
....
    std::shared_ptr<NativeBuffer> native_buffer_handle(); // existing
    mg::BufferUsage[1] usage();
}

and the renderer would look like:

GLRenderer(std::shared_ptr<mg::Mapper> const& mapper) : mapper(mapper) {}
void GLRenderer::render(RenderableList list)
{
   context.make_current();
   for(auto & r : list)
   {
       auto buffer = r->buffer();
       if (buffer->capabilites().has_gl_capability())
           mapper->bind_to_gl_context(buffer->native_buffer_handle(), context);
       //else we're in trouble
   }
   context.swap_buffers();
}

This approach gives us:
GLRenderer() {}
void GLRenderer::render(RenderableList list)
{
   context.make_current();
   for(auto & r : list)
   {
       auto buffer = r->buffer();
       auto bindable = dynamic_cast<TextureBindable>(buffer->native_buffer());
       if (bindable)
            bindable->gl_bind();
       //else we're in trouble
   }
   context.swap_buffers();
}

[1] This currently an enum class, should be more of a bitmask of the capabilities.

I guess though at the end of the day, we still have that "//else we're in trouble" when the capabilities don't match up. We're still stuck throwing when the user tries map an incapable object.

I'd rather communicate the capabilites of the object in an explicit way rather than have to figure out the inheritance structure of the object we have. With this way, the user has to know that a TextureBindable interface exists, and that some mg::Buffers inherit from it, rather than finding an enum, and seeing explicitly if the buffer is capable or not.

And, having said all that :) The approaches aren't all that different... and although I guess I'd prefer the bikeshed in the other color... ^_^

With the casting approach, I'd still prefer:

mg::NativeBufferBase* mg::Buffer::native_buffer_base()

that we can then cast to TextureBindable, as it doesn't force all mg::Buffer implementations to inherit from some sort of binding interface, and lets us separate the implementation of the buffers in the platform.

Hmm, what sort of review is that... I guess the above part was just rambling, then a 'needs fixing' just on the return type, which is a smaller change, and would give more flexibility in the structure of the platform Buffer objects.

There's also a bit of a 'needs info' about the plan for mg::Buffer::write, read, gl_bind_to_texture, as those seem redundant on mg::Buffer after the new plan now.

> I'd rather communicate the capabilites of the object in an explicit way rather
> than have to figure out the inheritance structure of the object we have. With
> this way, the user has to know that a TextureBindable interface exists, and
> that some mg::Buffers inherit from it, rather than finding an enum, and seeing
> explicitly if the buffer is capable or not.

Regardless of the implementation details, I think that our different solutions stem from making different trade-offs in the explicitness/coupling axes for renderer types in our core. I want our core interfaces to know absolutely nothing (minimal coupling) about the different rendering types, so that adding a new renderer type does not involve any Mir changes. A vendor could ship a platform and renderer plugin (with a new renderer type, e.g., "vendor-hw-blitter") and any compatible mir server would just use them transparently. For the common rendering types (for which we are going to provide some default renderer plugins) we are going to ship the interface definitions, so that 3rd party platforms can reuse them.

> With the casting approach, I'd still prefer:
> mg::NativeBufferBase* mg::Buffer::native_buffer_base()

As mentioned, I didn't see an immediate advantage using a new base type vs returning mg::Buffer*, especially since I consider this branch just a workaround for wrapping. However, I am fine with this approach, too (it's actually what I used when I spiked this branch). Let's get some more opinions and if there is a preference for this I will change it.

> There's also a bit of a 'needs info' about the plan for mg::Buffer::write, read,
> gl_bind_to_texture, as those seem redundant on mg::Buffer after the new plan now.

gl_bind_to_texture is moved to a renderer specific interface, used exactly as you have shown in the code example in a previous comment. The proposed branch is:

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

Other renderer specific methods will be handled similarly.

For the Buffer::native_buffer_base() variation as discussed in a previous comment see:

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

Cast your vote!

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