This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Use a host interface for CustomLayer instead of function pointers #11553

Merged

asheemmamoowala merged 6 commits into release-boba from ahm-custom-layer-api-change

Apr 2, 2018

Contributor

asheemmamoowala commented Mar 29, 2018 •

edited

Loading

Fixes #11143.
Fixes #10771.

CustomLayers are a special class of style layer where the core API delegates rendering of the layer to external code.

With the change to asynchronous rendering, the RenderCustomLayer may call out to the external code to initialize, render, or clean-up outside of the CustomLayer's lifecycle. For example, a custom layer added to the current map style, followed by a different style being set on the map.

In addition to #11143, a number of other issues related to custom layers have been opened/addressed recently: #10771, #10755, #10765, #11343.

The existing API using avoid * context object and function pointers, doesn't provide any lifecycle information. The context object needs to exist for the lifetime of the CustomLayer (CustomLayer::Impl to be more specific), and the RenderCustomLayer. This PR proposes using a shared_ptr<> to ensure that the context object has the required lifetime.

Since a shared_ptr<> cannot be used with void *, a host interface CustomLayerHost with the former function pointers converted to equivalent events.

On iOS/macOS, the SDK uses the peer style layer MGLOpenGLStyleLayer as the context object, creating a tenuous chain of ownership. The implementation of CustomLayerHost addresses the retain/release issue described in this comment.

RenderCustomLayer calls CustomLayerHost::initialize() in its constructor to ensure that the peer style layer is retained, even if the style (and CustomLayer::Impl) are changed prematurely.

TODO

Pull in integration tests spread across a couple other branches
Document and communicate API changes

cc @julianrex @lilykaiser @jfirebaugh @akitchen @1ec5

jfirebaugh reviewed

View reviewed changes

Contributor

jfirebaugh left a comment

I like this idea, but doesn't the shared_ptr< CustomLayerHost> need to be used throughout the whole stack -- CustomLayer, CustomLayer::Impl -- not just RenderCustomLayer?

Contributor Author

asheemmamoowala commented Mar 29, 2018

The shared_ptr<> is used in CustomLayer::Impl as well as the RenderCustomLayer - they are the shared owners. CustomLayer passes the raw pointer down to the Impl and doesn't hold on to it.

Contributor

jfirebaugh commented Mar 29, 2018

Constructing a shared_ptr<> from a raw pointer is almost always incorrect. If ownership is being transferred into CustomLayer::Impl/RenderCustomLayer from CustomLayer, then use an ownership type (unique_ptr or shared_ptr) to indicate that.

asheemmamoowala force-pushed the ahm-custom-layer-api-change branch 3 times, most recently from f198d30 to b0f68d3 Compare

March 29, 2018 23:50

Andrew Kitchen and others added 5 commits

March 29, 2018 17:22


          Adds failing tests for #10771 and #11143

30ecd0c

Unfortunately a brief spin of the run loop is required for the layer reuse test. I've tried to tune this down for my machine; YMMV


          [core] Change CustomLayer to use a host interface instead of function…

… pointers

Co-authored-by: Julian Rex <julian.rex@mapbox.com>


          [ios, macos] Implement CustomLayerHost. Save a weak reference to the …

efb241f

…style layer, and upgrade to a strong layer when initialized by the renderer

Co-authored-by: Julian Rex <julian.rex@mapbox.com>


          [android] Implement CustomLayerHost

6bca0f0


          [qt] Implement CustomLayerHost using a forwarding wrapper.

46383bb

asheemmamoowala force-pushed the ahm-custom-layer-api-change branch from b0f68d3 to 46383bb Compare

March 30, 2018 00:26

asheemmamoowala changed the base branch from master to release-boba

March 30, 2018 00:27

julianrex requested review from akitchen, fabian-guerra, jmkiley, JThramer and tobrun

March 30, 2018 14:02

tobrun reviewed

View reviewed changes

platform/android/src/example_custom_layer.cpp Outdated

		}

		void deinitialize() {

Member

tobrun Mar 30, 2018

nit additional return vs contextLost()

tobrun reviewed

View reviewed changes

Member

tobrun left a comment

tested on Android device and code is looking 👍

jfirebaugh reviewed

View reviewed changes

include/mbgl/style/layers/custom_layer.hpp Outdated

		@@ -2,18 +2,11 @@

		#include <mbgl/style/layer.hpp>

		#include <functional>

Contributor

jfirebaugh Mar 30, 2018

Unnecessary?

include/mbgl/style/layers/custom_layer.hpp Outdated

@@ @@ -28,6 +21,18 @@ struct CustomLayerRenderParameters { @@
                   double fieldOfView;
               };
+              class CustomLayerHost {
+              public:
+              virtual ~CustomLayerHost() = default;

Contributor

jfirebaugh Mar 30, 2018

Nit: indentation.

include/mbgl/style/layers/custom_layer.hpp Outdated

+               * main thread, at a point when the GL context is active but before rendering for the first
+               * time.
+               *
+               * Resources that are acquired in this method must be released in the UninitializeFunction.

Contributor

jfirebaugh Mar 30, 2018

"in the UninitializeFunction" → "in deinitialize"

include/mbgl/style/layers/custom_layer.hpp Outdated

    
            @@ -36,38 +41,29 @@ struct CustomLayerRenderParameters {
          
               * Make sure that you are drawing your fragments with a z value of 1 to take advantage of the

               * opaque fragment culling in case there are opaque layers above your custom layer.

               */

              using CustomLayerRenderFunction = void (*)(void* context, const CustomLayerRenderParameters&);

              virtual void render(const CustomLayerRenderParameters&) = 0;

              /**

               * Called when the system has destroyed the underlying GL context. The

               * `CustomLayerDeinitializeFunction` will not be called in this case, however

Contributor

jfirebaugh Mar 30, 2018

Update docs.

include/mbgl/style/layers/custom_layer.hpp Outdated

    
              /**

               * Called when the system has destroyed the underlying GL context. The

               * `CustomLayerDeinitializeFunction` will not be called in this case, however

               * `CustomLayerInitializeFunction` will be called instead to prepare for a new render.

               *

               */

              using CustomLayerContextLostFunction = void (*)(void* context);

              virtual void contextLost() = 0;

              /**

               * Destroy any GL state needed by the custom layer, and deallocate context, if necessary. This

               * method is called once, from the main thread, at a point when the GL context is active.

               *

               * Note that it may be called even when the InitializeFunction has not been called.

Contributor

jfirebaugh Mar 30, 2018

Update docs.

platform/darwin/src/MGLOpenGLStyleLayer.mm Outdated

@@ @@ -116,7 +114,9 @@ - (void)setStyle:(MGLStyle *)style { @@
                       [NSException raise:@"MGLLayerReuseException"
                                   format:@"%@ cannot be added to more than one MGLStyle at a time.", self];
                   }
+                  _style.openGLLayers[self.identifier] = nil;

Contributor

jfirebaugh Mar 30, 2018

What's this for?

Contributor Author

asheemmamoowala Mar 30, 2018

Clears the entry from the previous style when calling [styleLayer removeFromStyle:]. I had removed this in #10765, but I think it needs to be returned. In the Obj-C SDK, the style and the CustomLayerHost-implementation share ownership of the peer layer.

src/mbgl/renderer/layers/render_custom_layer.cpp Outdated

@@ @@ -12,17 +12,16 @@ namespace mbgl { @@
               using namespace style;
               RenderCustomLayer::RenderCustomLayer(Immutable<style::CustomLayer::Impl> _impl)
-                  : RenderLayer(LayerType::Custom, _impl) {
+                  : RenderLayer(LayerType::Custom, _impl), host(_impl->host) {
+                  host->initialize();

Contributor

jfirebaugh Mar 30, 2018

Is there a reason you're changing the timing of when the custom layer is initialized? Are you sure the GL context is guaranteed to be active at this point? If so, add assert(BackendScope::exists()); like in the destructor.

Contributor Author

asheemmamoowala Mar 30, 2018

I moved this to the constructor in an attempt to initialize and retain the Obj-C peer layer sooner. This was to address a race condition where a custom layer is added to a style, but before the next frame renders, the style is swapped. In this case, the peer layer was released too soon.
This fix didn't address that issue, but I decided to keep it in, since the render layers are created in Renderer::Impl and rendered in the same render pass. Adding the assert for protection.

src/mbgl/style/layers/custom_layer.cpp Outdated

		@@ -38,7 +26,6 @@ std::unique_ptr<Layer> CustomLayer::cloneRef(const std::string&) const {
		}

		// Visibility

Contributor

jfirebaugh Mar 30, 2018

Nit: revert whitespace change.

test/api/custom_layer.test.cpp Outdated

@@ @@ -34,7 +34,7 @@ void main() { @@
               // layer implementation because it is intended to reflect how someone using custom layers
               // might actually write their own implementation.
-              class TestLayer {
+              class TestLayer : public mbgl::style::CustomLayerHost {
               public:
                   ~TestLayer() {
                       if (program) {

Contributor

jfirebaugh Mar 30, 2018

This needs to move to deinitialize.

src/mbgl/renderer/layers/render_custom_layer.cpp Outdated

-                      }
+                  if (contextDestroyed) {
+                      host->contextLost();
+                  } else if (!contextDestroyed) {

Contributor

jfirebaugh Mar 30, 2018

The conditional is redundant.

asheemmamoowala mentioned this pull request

Adds tests and potential fixes for #10771 and #11143 #11291

Closed

julianrex mentioned this pull request

Use a pair of NSSets to manage the retain/release lifecycle of layers. #11343

Closed

friedbunny assigned asheemmamoowala

friedbunny added Core iOS macOS Android Qt labels

fabian-guerra approved these changes

View reviewed changes

Contributor

fabian-guerra left a comment •

edited

Loading

I tested the changes on macOS. Besides adding the blurb to the changelogs (iOS, macOS & Android) and ~~some nits~~ LGTM.

friedbunny mentioned this pull request

[ios] Release ios-v4.0.0-beta.3 #11580

Merged

asheemmamoowala force-pushed the ahm-custom-layer-api-change branch from adac588 to b2e7d7e Compare

April 2, 2018 22:16

jfirebaugh reviewed

View reviewed changes

platform/android/src/example_custom_layer.cpp Outdated

@@ @@ -172,6 +170,23 @@ class ExampleCustomLayer { @@
                   }
+                  void contextLost() {
+                      __android_log_write(ANDROID_LOG_INFO, LOG_TAG, "ContextLost");
+                      if (program) {

Contributor

jfirebaugh Apr 2, 2018

In the context lost case, the system has already destroyed all the GL resources, and glDeleteBuffers etc should not be called (IIRC, they'll actually produce errors). So all this should do is clear program.

asheemmamoowala force-pushed the ahm-custom-layer-api-change branch from b2e7d7e to c08c88c Compare

April 2, 2018 22:22


          Address review feedback and fix broken CI builds

c3cafe7

asheemmamoowala force-pushed the ahm-custom-layer-api-change branch from c08c88c to c3cafe7 Compare

April 2, 2018 22:39

asheemmamoowala merged commit 099fcc3 into release-boba

asheemmamoowala mentioned this pull request

MGLOpenGLStyleLayer leaks when not added to a map #10771

Closed

friedbunny mentioned this pull request

MGLOpenGLStyleLayer released when removed from style #11143

Closed

friedbunny deleted the ahm-custom-layer-api-change branch

April 4, 2018 17:18

julianrex mentioned this pull request

[ios] Crashes caused by dangling rawSource pointer after new style is parsed. #15333

Closed

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

tobrun tobrun left review comments

jfirebaugh jfirebaugh left review comments

fabian-guerra fabian-guerra approved these changes

akitchen Awaiting requested review from akitchen

jmkiley Awaiting requested review from jmkiley

JThramer Awaiting requested review from JThramer

Labels

Android Core iOS macOS Qt