Render interface

[ivovandongen]

Introduces a Renderer interface with a matching RendererFrontend interface to decouple the map from the actual rendering. This makes it possible to have different threading strategies per platform.

[ivovandongen]

@jfirebaugh Given your notes in #8820 I've started implementing a renderer abstraction and proxy/bridge interface for the SDK to implement so we can do either synchronous or asynchronous rendering.

As I understood your notes, we want something like:

Besides quite some details to work out I'm running into a issue with the sdk binding part you might have already thought about:

The proxy/bridge ideally doesn't know about the data passed to the renderer as it contains references to non-public headers (like the TransformState). I can find no way to pass it along with unique_ptrs though, without having the definition been known at the point where I move the data.
Atm it works when the renderer accepts const references (9f4c3ec#diff-9f4c7224e9bcf6a1be79b87e1591357dR47). But when using different threads we want to actually move the data and accept a unique_ptr in the renderer as well so that the data is not destroyed before it has been used in the renderer. This invariably leads to error: invalid application of 'sizeof' to an incomplete type. A way around this probably would be to use an Impl for each of the messages, but that will add quite a lot of code.

[jfirebaugh]

Current call chain for continuous rendering is:

1. style/camera mutation
2. onUpdate(Update::Repaint)
3. asyncInvalidate.send()
4. (event loop turn)
5. backend.invalidate()
6. (SDK code)
7. Map::render(view)
8. renderStyle->update, painter->render

With the introduction of RenderBridge, we should reduce the number of moving parts here.

• Replace onUpdate(Update::Repaint) with renderBridge.update(...)
• Remove Backend::invalidate
• Remove Map::Impl::asyncInvalidate; instead RenderBridge implementation is responsible for coalescing updates/repaints
• Merge View into Backend
• RenderBridge must provide Backend& to Renderer::render
• In Map constructor parameters, replace Backend& with RenderBridge&; remove createRenderBridge()

This will make a clean division:

• RenderBridge is the interface consumed by Map on the main thread.
• Backend is the interface consumed by Renderer on the orchestration/rendering thread.

For neatness and parallelism, we could rename them RendererFrontend and RendererBackend.

[ivovandongen]

@jfirebaugh I made some progress with reducing the number of moving parts. Leaving some notes here.

• Replace onUpdate(Update::Repaint) with renderBridge.update(...)
  As discussed, we need to measure if the additional creation of UpdateParameters impacts performance. I fixed up the node and mac os bindings. Render tests succeed, but the rendering on the macos app stutters. Need to investigate if this is the cause.

• Remove Backend::invalidate
  Removed it from Backend, but kept it intact in the way that a invalidate callback can be provided to the default render bridge so the UI system can be used to synchronise render calls with rendering the rest of the UI like before

• Remove Map::Impl::asyncInvalidate; instead RenderBridge implementation is responsible for coalescing updates/repaints
  I've implemented this in the DefaultRenderBridge since it is common to a couple of our platforms and I don't want to duplicate the code. ATM it provides either a constructor that takes a callback or the Backend and View so that Renderer::render may be called after coalescing updates

• RenderBridge must provide Backend& to Renderer::render
  Done through the invalidate callback / constructor arguments for now. Maybe inheritance is more suitable here.

• In Map constructor parameters, replace Backend& with RenderBridge&; remove createRenderBridge()
  We probably want to split off the MapObserver from the RenderBridge& argument so that we more easily use composition in the SDK bindings instead of inheritance. ATM the SDK view implementations are a combination of Backend, RenderBridge, View and MapObserver. Which makes re-use of, DefaultRenderBridge for example a bit clunky. For example, in the macos binding: _mbglView->render(*_mbglView, *_mbglView);

related: Cutting the ownership between Map and RenderStyle and providing it indirectly by reference to the RenderBridge led me to a bit of an awkward position. As the RenderStyle needs to be destructed before the AnnotationManager since the former uses the latter through the destructor of AnnotationTile we need to ensure that the Renderer/RenderBridge gets destructed before the Map, but since the Map needs the RenderBridge as a reference now we can't declare both on the stack anymore (because we need to explicitly delete them in the same order).
For example, the "fixed" Annotations test:

    std::unique_ptr<RenderBridge> renderBridge;
    std::unique_ptr<Map> map;

    AnnotationTest()
            : renderBridge(std::make_unique<DefaultRenderBridge>(backend, view))
            , map(std::make_unique<Map>(*renderBridge, view.getSize(), 1, fileSource, threadPool, MapMode::Still)) {

    }

    ~AnnotationTest() {
        renderBridge.reset();
        map.reset();
    }

When we use the renderer on a separate thread, we are going to have to assure that the destruction of the Renderer/RenderBridge still completes before destructing the map.

• Merge View into Backend
  Started on this, but don't know if this makes sense for all implementations, for example OffscreenTexture and OffscreenView. The View is often re-created as well in the node bindings whereas the Backend and associated Context can remain intact.

[jfirebaugh]

Removed it from Backend, but kept it intact in the way that a invalidate callback can be provided to the default render bridge so the UI system can be used to synchronise render calls with rendering the rest of the UI like before

I think instead we should have per-SDK implementations of RenderBackend, rather than trying to share code with DefaultRenderBackend.

I've implemented this in the DefaultRenderBridge since it is common to a couple of our platforms and I don't want to duplicate the code.

It's not that much duplication, and some SDKs probably don't even need it. E.g. [nativeView setNeedsGLDisplay] likely already does coalescing internally.

We probably want to split off the MapObserver from the RenderBridge& argument

Yes, good idea.

Cutting the ownership between Map and RenderStyle and providing it indirectly by reference to the RenderBridge led me to a bit of an awkward position.

Does this go away if ownership of RenderBridge is passed into Map, i.e. the parameter is std::unique_ptr<RenderBridge> rather than RenderBridge&, and Map releases it explicitly in ~Map?

[jfirebaugh]

Let's fix #8039 now so that we don't need this.

[ivovandongen]

Good idea. I'll start on that first.

[jfirebaugh]

Let's have an explicit pass-through in RenderBridge for this.

[ivovandongen]

👍

[jfirebaugh]

And this. And then remove setRendererState.

[ivovandongen]

I think instead we should have per-SDK implementations of RenderBackend, rather than trying to share code with DefaultRenderBackend.

Makes sense if coallescing is indeed a platform issue. I'll try it out on a couple of platforms. I know that Android also just sets a flag that isn't used until the next draw call.

Cutting the ownership between Map and RenderStyle and providing it indirectly by reference to the RenderBridge led me to a bit of an awkward position.

Does this go away if ownership of RenderBridge is passed into Map, i.e. the parameter is std::unique_ptr rather than RenderBridge&, and Map releases it explicitly in ~Map?

That would help a lot. I used a reference initially as moving ownership limits the use of the RenderBridge as a part of the platform View bindings by inheritance (MGLMapView for example). But the multiple-inheritance there doesn't make for a very clear abstraction anyway.

[ivovandongen]

@jfirebaugh I'm finishing up with the platform changes. Besides a bug specific to egl on linux, they seem to work well. Could you give it a quick check if it's moving in the right direction?

Major parts still on my todo list:

• Rename/move Backend
• Rename/move BackendScope
• Get rid of the renderer_impl.hpp import in map.cpp (move out some of the parameters definitions)
• Map::isFullyLoaded
• Map::triggerRepaint()

For reference the bug with egl/linux

[----------] 1 test from AnnotationTile
[ RUN      ] AnnotationTile.Issue8289
terminate called after throwing an instance of 'std::runtime_error'
  what():  Error creating the EGL context object.

[ivovandongen]

@jfirebaugh Noting additional changes from our conversation on chat:

• Extract the removal of MapObserver from Backend in a separate commit
• Renderer:
  • Let SDK's create the Renderer
  • Remove redundant parameters from Map constructor
  • Move BackendScope handling into the renderer (Obtain BackendScope from Backend / or at least the ScopeType)
  • Move Backend into constructor parameters as it can't be switched during the life-time of Renderer anyway
  • Get rid of TransformState from query parameters and re-use the one in the Renderer. This allows us to get rid of the *QueryParameters wrappers as well.
  • (maybe) Combine query* methods by using a co-variant argument
• RendererFrontend
  • Add resetRenderer() method to force a synchronous cleanup of the internals (to deal with the fact that AnnotationManager is shared between Map and RenderStyle ATM)
  • Go back to passing the RendererFrontend by reference to the Map. This enables us to implement the RendererFrontend interface as part of other (UI) objects on the SDK side.
  • Remove onLowMemory() and instead call directly from SDK bindings to Renderer where needed
  • Remove dumpDebugLogs() and instead call directly from SDK bindings (node) to Renderer where needed
• AnnotationManager
  • Remove reference to style and deal with Map::setStyle
• Map
  • Map::isFullyLoaded() Reuse and store state from RendererObserver::onDidFinishRenderingFrame.
• General
  • Clean up unused imports
  • Clean up comments

Plus the outstanding work for completeness:

• Rename/move Backend
• Rename/move BackendScope
• Get rid of the renderer_impl.hpp import in map.cpp
• Fix egl+linux builds

[ivovandongen]

@kkaefer Noticed that animations are choppier in this branch than on master. Seems like fewer frames are rendered than before.

[ivovandongen]

@frederoni It seems there is a slight difference between macOS and iOS when calling [nativeView setNeedsDisplay]. On macOS, when doing this while renderSync is in progress, it is not queued behind the current redraw, whereas it is on iOS. To make sure that animations work properly on both platforms I had to make sure it works with an AsyncTask on macOS where it is not needed on iOS.

I kept a single RendererFrontend implementation, with a switch to enable asynchronous behaviour in ce3538d. I rather not make two separate implementations. Let me know how this looks to you.

@kkaefer A similar problem existed on glfw where the dirty flag was cleared too late so another iteration could not be triggered from anination frame callbacks. I addressed this in f290c6d