experiment: providing a gateway for writing your own renderer through MirAL #3338

mattkae · 2024-04-19T20:46:25Z

WARNING: Highly experimental, but it works

What's new?

Users can now provide CustomRenderer to miral which gives them the rites to entirely replace the existing renderer
Although this might not be 100% percent what we want, I would be VERY interested in seeing something like this go into an "internal" headers somewhere as it would enable a lot of interesting things

codecov · 2024-04-19T21:05:09Z

Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 77.33%. Comparing base (3293097) to head (d65e889).
Report is 3 commits behind head on main.

❗ Current head d65e889 differs from pull request most recent head 7b28349. Consider uploading reports for the commit 7b28349 to get more accurate results

Files	Patch %	Lines
src/miral/custom_renderer.cpp	0.00%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3338      +/-   ##
==========================================
- Coverage   77.33%   77.33%   -0.01%     
==========================================
  Files        1062     1063       +1     
  Lines       67817    67826       +9     
==========================================
+ Hits        52449    52451       +2     
- Misses      15368    15375       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AlanGriffiths

I'd tweak the MirAL API slightly to be more consistent with other MirAL APIs, but I don't think there's anything problematic about the semantics.

However, my question is whether it is possible to implement the CustomRender::Builder with the APIs we make available? (I suspect that might need "internal" APIs)

AlanGriffiths · 2024-04-22T08:58:31Z

include/miral/miral/custom_renderer.h

+class RendererFactory;
+namespace gl
+{
+class RenderTarget;
+}
+}
+}


Suggested change

class RendererFactory;

namespace gl

{

class RenderTarget;

}

}

}

}

}

AlanGriffiths · 2024-04-22T08:59:54Z

include/miral/miral/custom_renderer.h

+typedef std::function<std::unique_ptr<mir::renderer::Renderer>
+    (std::unique_ptr<mir::graphics::gl::OutputSurface>,
+     std::shared_ptr<mir::graphics::GLRenderingProvider>)> CreateRenderer;
+


Suggested change

typedef std::function<std::unique_ptr<mir::renderer::Renderer>

(std::unique_ptr<mir::graphics::gl::OutputSurface>,

std::shared_ptr<mir::graphics::GLRenderingProvider>)> CreateRenderer;

AlanGriffiths · 2024-04-22T09:00:29Z

include/miral/miral/custom_renderer.h

+class CustomRenderer
+{
+public:
+    explicit CustomRenderer(CreateRenderer const& renderer);


Suggested change

explicit CustomRenderer(CreateRenderer const& renderer);

using Builder = std::function<

std::unique_ptr<mir::renderer::Renderer>(

std::unique_ptr<mir::graphics::gl::OutputSurface>, std::shared_ptr<mir::graphics::GLRenderingProvider>)

>;

explicit CustomRenderer(Builder&& renderer);

AlanGriffiths · 2024-04-22T09:00:49Z

include/miral/miral/custom_renderer.h

+    explicit CustomRenderer(CreateRenderer const& renderer);
+    void operator()(mir::Server& server) const;
+private:
+    std::shared_ptr<mir::renderer::RendererFactory> factory;


Suggested change

std::shared_ptr<mir::renderer::RendererFactory> factory;

struct Self;

std::shared_ptr<Self> self;

mattkae · 2024-04-22T18:14:14Z

I'd tweak the MirAL API slightly to be more consistent with other MirAL APIs, but I don't think there's anything problematic about the semantics.

However, my question is whether it is possible to implement the CustomRender::Builder with the APIs we make available? (I suspect that might need "internal" APIs)

I was able to copy over the entire GL renderer into miracle-wm with a lot of success: miracle-wm-org/miracle-wm#103

It is a really rough copy, but I only required tesselation_helpers.h and primitive.h to be copied over. The rest was already exposed to me in MIRRENDERER. Those two headers aren't very fundamental in my opinion, so I'd say we're good with what we have now.

AlanGriffiths

I think miral::CustomRenderer should do one thing, not two

AlanGriffiths · 2024-04-23T08:57:39Z

include/miral/miral/custom_renderer.h

+        std::unique_ptr<mir::graphics::gl::OutputSurface>, std::shared_ptr<mir::graphics::GLRenderingProvider>)
+    >;
+
+    CustomRenderer(Builder&& renderer, std::shared_ptr<mir::graphics::GLConfig> const&);


Why are we passing a GLConfig here?

There is prior art in miroil::OpenGLContext for doing this independently and that seems like a more flexible approach.

If we don't want to add a dependency on MirOil, then a miral::CustomGLContext seems like a cleaner approach. (What happens when there is another feature that wants to override the GL config?)

AlanGriffiths · 2024-04-23T09:00:58Z

src/miral/custom_renderer.cpp

+    }
+
+private:
+    miral::CustomRenderer::Builder const& renderer;


Should this really be a reference?

AlanGriffiths · 2024-04-23T09:06:16Z

src/miral/custom_renderer.cpp

+
+struct miral::CustomRenderer::Self
+{
+    Self(Builder const& renderer, std::shared_ptr<mir::graphics::GLConfig> const& config)


Suggested change

Self(Builder const& renderer, std::shared_ptr<mir::graphics::GLConfig> const& config)

Self(Builder&& renderer, std::shared_ptr<mir::graphics::GLConfig> const& config)

AlanGriffiths · 2024-04-23T09:06:37Z

src/miral/custom_renderer.cpp

+
+miral::CustomRenderer::CustomRenderer(
+    Builder&& renderer, std::shared_ptr<mir::graphics::GLConfig> const& config)
+    : self{std::make_shared<Self>(renderer, config)}


Suggested change

: self{std::make_shared<Self>(renderer, config)}

: self{std::make_shared<Self>(std::move(renderer), config)}

AlanGriffiths

IMO the code could be a little clearer. But otherwise fine.

(Approving, but not merging to allow tweak)

AlanGriffiths · 2024-04-24T08:24:30Z

src/miral/custom_renderer.cpp

+struct miral::CustomRenderer::Self
+{
+    explicit Self(Builder&& renderer)
+        : factory(std::make_shared<RendererFactory>(std::move(renderer)))
+    {
+    }
+
+    std::shared_ptr<mir::renderer::RendererFactory> factory;
+};


This would be simpler (fewer classes, less forwarding code) if miral::CustomRenderer::Self IsA mir::renderer::RendererFactory, not HasA. Vis:

Suggested change

struct miral::CustomRenderer::Self

{

explicit Self(Builder&& renderer)

: factory(std::make_shared<RendererFactory>(std::move(renderer)))

{

}

std::shared_ptr<mir::renderer::RendererFactory> factory;

};

struct miral::CustomRenderer::Self : public mir::renderer::RendererFactory

{

explicit Self(Builder&& renderer)

: factory(std::move(renderer))

{

}

[[nodiscard]] auto create_renderer_for(

std::unique_ptr<mir::graphics::gl::OutputSurface> output_surface,

std::shared_ptr<mir::graphics::GLRenderingProvider> gl_provider) const

-> std::unique_ptr<mir::renderer::Renderer> override

{

return renderer(std::move(output_surface), std::move(gl_provider));

}

private:

miral::CustomRenderer::Builder const renderer;

};

Alternatively, Self::factory should be const.

I agree with that change!

Signed-off-by: Matthew Kosarek <[email protected]>

AlanGriffiths

More nits

src/miral/custom_renderer.cpp

Co-authored-by: Alan Griffiths <[email protected]>

mattkae added 2 commits April 18, 2024 09:51

WIP

cccffa3

A basic, working renderer is exposed

1404488

mattkae requested a review from AlanGriffiths April 19, 2024 20:46

mattkae marked this pull request as ready for review April 19, 2024 20:46

mattkae requested a review from a team as a code owner April 19, 2024 20:46

mattkae changed the title ~~Providing a gateway for writing your own renderer through MirAL~~ experiment: providing a gateway for writing your own renderer through MirAL Apr 19, 2024

AlanGriffiths requested changes Apr 22, 2024

View reviewed changes

mattkae force-pushed the feature/expose_renderer_factory branch from b90d9d5 to 48789f6 Compare April 22, 2024 18:44

mattkae requested a review from AlanGriffiths April 22, 2024 19:05

Custom Renderer can also include a GL config

2191081

mattkae force-pushed the feature/expose_renderer_factory branch from 48789f6 to 2191081 Compare April 22, 2024 19:06

AlanGriffiths requested changes Apr 23, 2024

View reviewed changes

pr feedback: remove GLConfig

a51ddb6

mattkae requested a review from AlanGriffiths April 23, 2024 17:41

bugfix: remove reference to GLConfig

70f061d

AlanGriffiths approved these changes Apr 24, 2024

View reviewed changes

Merge branch 'main' into feature/expose_renderer_factory

b1e5220

Signed-off-by: Matthew Kosarek <[email protected]>

AlanGriffiths approved these changes Apr 24, 2024

View reviewed changes

src/miral/custom_renderer.cpp Outdated Show resolved Hide resolved

src/miral/custom_renderer.cpp Outdated Show resolved Hide resolved

mattkae and others added 3 commits April 24, 2024 10:12

regeneration libmiral7.symbols

d65e889

minor: take self by value

eabd7c0

Co-authored-by: Alan Griffiths <[email protected]>

minor: whitespace around Server&

7b28349

Co-authored-by: Alan Griffiths <[email protected]>

AlanGriffiths approved these changes Apr 24, 2024

View reviewed changes

AlanGriffiths enabled auto-merge April 24, 2024 15:02

AlanGriffiths added this pull request to the merge queue Apr 24, 2024

Merged via the queue into main with commit 989cebb Apr 24, 2024
33 of 36 checks passed

AlanGriffiths deleted the feature/expose_renderer_factory branch April 24, 2024 15:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

experiment: providing a gateway for writing your own renderer through MirAL #3338

experiment: providing a gateway for writing your own renderer through MirAL #3338

mattkae commented Apr 19, 2024

codecov bot commented Apr 19, 2024 •

edited

Loading

AlanGriffiths left a comment

AlanGriffiths Apr 22, 2024

AlanGriffiths Apr 22, 2024

AlanGriffiths Apr 22, 2024

AlanGriffiths Apr 22, 2024

mattkae commented Apr 22, 2024

AlanGriffiths left a comment

AlanGriffiths Apr 23, 2024

AlanGriffiths Apr 23, 2024

AlanGriffiths Apr 23, 2024

AlanGriffiths Apr 23, 2024

AlanGriffiths left a comment

AlanGriffiths Apr 24, 2024

AlanGriffiths Apr 24, 2024

mattkae Apr 24, 2024

AlanGriffiths left a comment

	typedef std::function<std::unique_ptr<mir::renderer::Renderer>
	(std::unique_ptr<mir::graphics::gl::OutputSurface>,
	std::shared_ptr<mir::graphics::GLRenderingProvider>)> CreateRenderer;

-    explicit CustomRenderer(CreateRenderer const& renderer);
+    using Builder = std::function<
+        std::unique_ptr<mir::renderer::Renderer>(
+            std::unique_ptr<mir::graphics::gl::OutputSurface>, std::shared_ptr<mir::graphics::GLRenderingProvider>)
+    >;
+    explicit CustomRenderer(Builder&& renderer);

	std::shared_ptr<mir::renderer::RendererFactory> factory;
	struct Self;
	std::shared_ptr<Self> self;

	Self(Builder const& renderer, std::shared_ptr<mir::graphics::GLConfig> const& config)
	Self(Builder&& renderer, std::shared_ptr<mir::graphics::GLConfig> const& config)

	: self{std::make_shared<Self>(renderer, config)}
	: self{std::make_shared<Self>(std::move(renderer), config)}

-struct miral::CustomRenderer::Self
-{
-    explicit Self(Builder&& renderer)
-        : factory(std::make_shared<RendererFactory>(std::move(renderer)))
-    {
-    }
-    std::shared_ptr<mir::renderer::RendererFactory> factory;
-};
+struct miral::CustomRenderer::Self : public mir::renderer::RendererFactory
+{
+    explicit Self(Builder&& renderer)
+        : factory(std::move(renderer))
+    {
+    }
+    [[nodiscard]] auto create_renderer_for(
+        std::unique_ptr<mir::graphics::gl::OutputSurface> output_surface,
+        std::shared_ptr<mir::graphics::GLRenderingProvider> gl_provider) const
+        -> std::unique_ptr<mir::renderer::Renderer> override
+    {
+        return renderer(std::move(output_surface), std::move(gl_provider));
+    }
+private:
+    miral::CustomRenderer::Builder const renderer;
+};

experiment: providing a gateway for writing your own renderer through MirAL #3338

experiment: providing a gateway for writing your own renderer through MirAL #3338

Conversation

mattkae commented Apr 19, 2024

What's new?

codecov bot commented Apr 19, 2024 • edited Loading

Codecov Report

AlanGriffiths left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkae commented Apr 22, 2024

AlanGriffiths left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanGriffiths left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanGriffiths left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 19, 2024 •

edited

Loading