diff options
author | Asheem Mamoowala <asheem.mamoowala@mapbox.com> | 2018-03-29 17:10:26 -0700 |
---|---|---|
committer | Asheem Mamoowala <asheem.mamoowala@mapbox.com> | 2018-03-29 17:23:35 -0700 |
commit | efb241f4bc955eb8f81b6577db5938f96f7efa1c (patch) | |
tree | 1d2a2148dbf161367107b25028598104adb6e8d5 | |
parent | 11373787c7176b363cb1d4e4fcc473818300860d (diff) | |
download | qtlocation-mapboxgl-efb241f4bc955eb8f81b6577db5938f96f7efa1c.tar.gz |
[ios, macos] Implement CustomLayerHost. Save a weak reference to the style layer, and upgrade to a strong layer when initialized by the renderer
Co-authored-by: Julian Rex <julian.rex@mapbox.com>
-rw-r--r-- | include/mbgl/style/layers/custom_layer.hpp | 2 | ||||
-rw-r--r-- | platform/darwin/src/MGLOpenGLStyleLayer.mm | 86 | ||||
-rw-r--r-- | platform/darwin/src/MGLStyle.mm | 2 | ||||
-rw-r--r-- | platform/darwin/src/MGLStyle_Private.h | 2 | ||||
-rw-r--r-- | platform/ios/Integration Tests/MBGLIntegrationTests.m | 215 | ||||
-rw-r--r-- | src/mbgl/style/layers/custom_layer.cpp | 1 |
6 files changed, 235 insertions, 73 deletions
diff --git a/include/mbgl/style/layers/custom_layer.hpp b/include/mbgl/style/layers/custom_layer.hpp index be656b0e14..5aa80a3afe 100644 --- a/include/mbgl/style/layers/custom_layer.hpp +++ b/include/mbgl/style/layers/custom_layer.hpp @@ -2,6 +2,8 @@ #include <mbgl/style/layer.hpp> +#include <functional> + namespace mbgl { namespace style { diff --git a/platform/darwin/src/MGLOpenGLStyleLayer.mm b/platform/darwin/src/MGLOpenGLStyleLayer.mm index 8933a77382..d9aa4f2575 100644 --- a/platform/darwin/src/MGLOpenGLStyleLayer.mm +++ b/platform/darwin/src/MGLOpenGLStyleLayer.mm @@ -1,4 +1,4 @@ -#import "MGLOpenGLStyleLayer.h" + #import "MGLOpenGLStyleLayer.h" #import "MGLMapView_Private.h" #import "MGLStyle_Private.h" @@ -7,49 +7,50 @@ #include <mbgl/style/layers/custom_layer.hpp> #include <mbgl/math/wrap.hpp> -/** - Runs the preparation handler block contained in the given context, which is - implicitly an instance of `MGLOpenGLStyleLayer`. +class MGLOpenGLLayerHost : public mbgl::style::CustomLayerHost { +public: + MGLOpenGLLayerHost(MGLOpenGLStyleLayer *styleLayer) { + layerRef = styleLayer; + layer = nil; + } + ~MGLOpenGLLayerHost () { + layer = nil; + } - @param context An `MGLOpenGLStyleLayer` instance that was provided as context - when creating an OpenGL style layer. - */ -void MGLPrepareCustomStyleLayer(void *context) { - MGLOpenGLStyleLayer *layer = (__bridge MGLOpenGLStyleLayer *)context; - [layer didMoveToMapView:layer.style.mapView]; -} + void initialize() { + if (layerRef == nil) return; + else if (layer == nil) layer = layerRef; -/** - Runs the drawing handler block contained in the given context, which is - implicitly an instance of `MGLOpenGLStyleLayer`. + [layer didMoveToMapView:layer.style.mapView]; + } - @param context An `MGLOpenGLStyleLayer` instance that was provided as context - when creating an OpenGL style layer. - */ -void MGLDrawCustomStyleLayer(void *context, const mbgl::style::CustomLayerRenderParameters ¶ms) { - MGLOpenGLStyleLayer *layer = (__bridge MGLOpenGLStyleLayer *)context; - MGLStyleLayerDrawingContext drawingContext = { - .size = CGSizeMake(params.width, params.height), - .centerCoordinate = CLLocationCoordinate2DMake(params.latitude, params.longitude), - .zoomLevel = params.zoom, - .direction = mbgl::util::wrap(params.bearing, 0., 360.), - .pitch = static_cast<CGFloat>(params.pitch), - .fieldOfView = static_cast<CGFloat>(params.fieldOfView), - }; - [layer drawInMapView:layer.style.mapView withContext:drawingContext]; -} + void render(const mbgl::style::CustomLayerRenderParameters ¶ms) { + assert(layerRef); + + MGLStyleLayerDrawingContext drawingContext = { + .size = CGSizeMake(params.width, params.height), + .centerCoordinate = CLLocationCoordinate2DMake(params.latitude, params.longitude), + .zoomLevel = params.zoom, + .direction = mbgl::util::wrap(params.bearing, 0., 360.), + .pitch = static_cast<CGFloat>(params.pitch), + .fieldOfView = static_cast<CGFloat>(params.fieldOfView), + }; + [layer drawInMapView:layer.style.mapView withContext:drawingContext]; + } -/** - Runs the completion handler block contained in the given context, which is - implicitly an instance of `MGLOpenGLStyleLayer`. + void contextLost() {} - @param context An `MGLOpenGLStyleLayer` instance that was provided as context - when creating an OpenGL style layer. - */ -void MGLFinishCustomStyleLayer(void *context) { - MGLOpenGLStyleLayer *layer = (__bridge_transfer MGLOpenGLStyleLayer *)context; - [layer willMoveFromMapView:layer.style.mapView]; -} + void deinitialize() { + if (layer == nil) return; + + [layer willMoveFromMapView:layer.style.mapView]; + layerRef = layer; + layer = nil; + } +private: + __weak MGLOpenGLStyleLayer * layerRef; + MGLOpenGLStyleLayer * layer = nil; +}; /** An `MGLOpenGLStyleLayer` is a style layer that is rendered by OpenGL code that @@ -98,10 +99,7 @@ void MGLFinishCustomStyleLayer(void *context) { */ - (instancetype)initWithIdentifier:(NSString *)identifier { auto layer = std::make_unique<mbgl::style::CustomLayer>(identifier.UTF8String, - MGLPrepareCustomStyleLayer, - MGLDrawCustomStyleLayer, - MGLFinishCustomStyleLayer, - (__bridge_retained void *)self); + std::make_unique<MGLOpenGLLayerHost>(self)); return self = [super initWithPendingLayer:std::move(layer)]; } @@ -116,7 +114,9 @@ void MGLFinishCustomStyleLayer(void *context) { [NSException raise:@"MGLLayerReuseException" format:@"%@ cannot be added to more than one MGLStyle at a time.", self]; } + _style.openGLLayers[self.identifier] = nil; _style = style; + _style.openGLLayers[self.identifier] = self; } - (void)addToStyle:(MGLStyle *)style belowLayer:(MGLStyleLayer *)otherLayer { diff --git a/platform/darwin/src/MGLStyle.mm b/platform/darwin/src/MGLStyle.mm index 199f6c2e02..62b6b25747 100644 --- a/platform/darwin/src/MGLStyle.mm +++ b/platform/darwin/src/MGLStyle.mm @@ -83,6 +83,7 @@ @property (nonatomic, readonly, weak) MGLMapView *mapView; @property (nonatomic, readonly) mbgl::style::Style *rawStyle; @property (readonly, copy, nullable) NSURL *URL; +@property (nonatomic, readwrite, strong) NS_MUTABLE_DICTIONARY_OF(NSString *, MGLOpenGLStyleLayer *) *openGLLayers; @property (nonatomic) NS_MUTABLE_DICTIONARY_OF(NSString *, NS_DICTIONARY_OF(NSObject *, MGLTextLanguage *) *) *localizedLayersByIdentifier; @end @@ -125,6 +126,7 @@ static_assert(6 == mbgl::util::default_styles::numOrderedStyles, if (self = [super init]) { _mapView = mapView; _rawStyle = rawStyle; + _openGLLayers = [NSMutableDictionary dictionary]; _localizedLayersByIdentifier = [NSMutableDictionary dictionary]; } return self; diff --git a/platform/darwin/src/MGLStyle_Private.h b/platform/darwin/src/MGLStyle_Private.h index 4cbe953a44..3597a0a0a6 100644 --- a/platform/darwin/src/MGLStyle_Private.h +++ b/platform/darwin/src/MGLStyle_Private.h @@ -25,7 +25,7 @@ namespace mbgl { @property (nonatomic, readonly) mbgl::style::Style *rawStyle; - (nullable NS_ARRAY_OF(MGLAttributionInfo *) *)attributionInfosWithFontSize:(CGFloat)fontSize linkColor:(nullable MGLColor *)linkColor; - +@property (nonatomic, readonly, strong) NS_MUTABLE_DICTIONARY_OF(NSString *, MGLOpenGLStyleLayer *) *openGLLayers; - (void)setStyleClasses:(NS_ARRAY_OF(NSString *) *)appliedClasses transitionDuration:(NSTimeInterval)transitionDuration; @end diff --git a/platform/ios/Integration Tests/MBGLIntegrationTests.m b/platform/ios/Integration Tests/MBGLIntegrationTests.m index deb362cd64..7b82f41b57 100644 --- a/platform/ios/Integration Tests/MBGLIntegrationTests.m +++ b/platform/ios/Integration Tests/MBGLIntegrationTests.m @@ -1,6 +1,6 @@ #import <XCTest/XCTest.h> - -@import Mapbox; +#import <objc/message.h> +#import <Mapbox/Mapbox.h> @interface MBGLIntegrationTests : XCTestCase <MGLMapViewDelegate> @@ -11,8 +11,11 @@ @implementation MBGLIntegrationTests { XCTestExpectation *_styleLoadingExpectation; + XCTestExpectation *_renderFinishedExpectation; } +#pragma mark - Setup/Teardown + - (void)setUp { [super setUp]; @@ -32,6 +35,16 @@ [window makeKeyAndVisible]; } +- (void)tearDown { + _styleLoadingExpectation = nil; + self.mapView = nil; + self.style = nil; + + [super tearDown]; +} + +#pragma mark - MGLMapViewDelegate + - (void)mapView:(MGLMapView *)mapView didFinishLoadingStyle:(MGLStyle *)style { XCTAssertNotNil(mapView.style); XCTAssertEqual(mapView.style, style); @@ -39,30 +52,112 @@ [_styleLoadingExpectation fulfill]; } -- (void)tearDown { - _styleLoadingExpectation = nil; - self.mapView = nil; +- (void)mapViewDidFinishRenderingFrame:(MGLMapView *)mapView fullyRendered:(__unused BOOL)fullyRendered { + [_renderFinishedExpectation fulfill]; + _renderFinishedExpectation = nil; +} - [super tearDown]; +#pragma mark - Utilities + +- (void)waitForMapViewToBeRendered { + [self.mapView setNeedsDisplay]; + _renderFinishedExpectation = [self expectationWithDescription:@"Map view should be rendered"]; + [self waitForExpectations:@[_renderFinishedExpectation] timeout:1]; } - (MGLStyle *)style { return self.mapView.style; } +#pragma mark - Tests + +// This test does not strictly need to be in this test file/target. Including here for convenience. +- (void)testOpenGLLayerDoesNotLeakWhenCreatedAndDestroyedWithoutAddingToStyle { + MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"]; + __weak id weakLayer = layer; + layer = nil; + + XCTAssertNil(weakLayer); +} + +- (void)testAddingRemovingOpenGLLayerWithoutRendering { + XCTAssertNotNil(self.style); + + void(^addRemoveGLLayer)(void) = ^{ + __weak id weakLayer = nil; + + @autoreleasepool { + MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"]; + [self.style insertLayer:layer atIndex:0]; + weakLayer = layer; + + // Nil the layer prior to remove to ensure it's being retained + layer = nil; + [self.style removeLayer:weakLayer]; + } + + XCTAssertNil(weakLayer); + }; + + addRemoveGLLayer(); + addRemoveGLLayer(); + addRemoveGLLayer(); +} + +- (void)testReusingOpenGLLayerIdentifier { + __weak MGLOpenGLStyleLayer *weakLayer2; + + @autoreleasepool { + MGLOpenGLStyleLayer *layer1 = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"]; + [self.style insertLayer:layer1 atIndex:0]; + [self waitForMapViewToBeRendered]; + [self.style removeLayer:layer1]; + + MGLOpenGLStyleLayer *layer2 = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"]; + weakLayer2 = layer2; + + XCTAssertNotNil(layer2); + XCTAssert(layer1 != layer2); + + [self.style insertLayer:layer2 atIndex:0]; + [self waitForMapViewToBeRendered]; + [self.style removeLayer:layer2]; + + XCTAssertNil([layer1 style]); + XCTAssertNil([layer2 style]); + } + + // At this point, layer2 (and layer1) should still be around, + // since the render process needs to keep a reference to them. + XCTAssertNotNil(weakLayer2); + + // Let render loop run enough to release the layers + [self waitForMapViewToBeRendered]; + XCTAssertNil(weakLayer2); +} + - (void)testAddingRemovingOpenGLLayer { XCTAssertNotNil(self.style); void(^addRemoveGLLayer)(void) = ^{ - MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"]; - [self.style insertLayer:layer atIndex:0]; - layer = nil; - [[NSRunLoop currentRunLoop] runUntilDate:[NSDate date]]; + __weak id retrievedLayer = nil; + + @autoreleasepool { + MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"]; + [self.style insertLayer:layer atIndex:0]; + layer = nil; + + [self waitForMapViewToBeRendered]; - id retrievedLayer = [self.style layerWithIdentifier:@"gl-layer"]; - XCTAssertNotNil(retrievedLayer); - [self.style removeLayer:retrievedLayer]; + retrievedLayer = [self.style layerWithIdentifier:@"gl-layer"]; + XCTAssertNotNil(retrievedLayer); + + [self.style removeLayer:retrievedLayer]; + [self waitForMapViewToBeRendered]; + } + + XCTAssertNil(retrievedLayer); }; addRemoveGLLayer(); @@ -71,33 +166,97 @@ } - (void)testReusingOpenGLLayer { - NSTimeInterval waitInterval = 0.01; - MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"]; [self.style insertLayer:layer atIndex:0]; - - [[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]]; + [self waitForMapViewToBeRendered]; + + [self.style removeLayer:layer]; + [self waitForMapViewToBeRendered]; + + [self.style insertLayer:layer atIndex:0]; + [self waitForMapViewToBeRendered]; [self.style removeLayer:layer]; + [self waitForMapViewToBeRendered]; +} - [[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]]; +- (void)testOpenGLLayerDoesNotLeakWhenRemovedFromStyle { + __weak id weakLayer; + @autoreleasepool { + MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"]; + weakLayer = layer; + [self.style insertLayer:layer atIndex:0]; + layer = nil; - [self.style insertLayer:layer atIndex:0]; + [self waitForMapViewToBeRendered]; + [self.style removeLayer:[self.style layerWithIdentifier:@"gl-layer"]]; + } - [[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]]; + MGLStyleLayer *layer2 = weakLayer; - [self.style removeLayer:layer]; + XCTAssertNotNil(weakLayer); + [self waitForMapViewToBeRendered]; - [[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]]; + layer2 = nil; + XCTAssertNil(weakLayer); } -// This test does not strictly need to be in this test file/target. Including here for convenience. -- (void)testOpenGLLayerDoesNotLeakWhenCreatedAndDestroyedWithoutAddingToStyle { - MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"]; - __weak id weakLayer = layer; - layer = nil; - +- (void)testOpenGLLayerDoesNotLeakWhenStyleChanged { + __weak MGLOpenGLStyleLayer *weakLayer; + + @autoreleasepool { + { + MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"]; + weakLayer = layer; + [self.style insertLayer:layer atIndex:0]; + layer = nil; + } + } + + XCTAssertNotNil(weakLayer); + + [self waitForMapViewToBeRendered]; + + MGLStyleLayer *layer2 = [self.mapView.style layerWithIdentifier:@"gl-layer"]; + + NSURL *styleURL = [[NSBundle bundleForClass:[self class]] URLForResource:@"one-liner" withExtension:@"json"]; + _styleLoadingExpectation = [self expectationWithDescription:@"Map view should finish loading style."]; + [self.mapView setStyleURL:styleURL]; + [self waitForExpectations:@[_styleLoadingExpectation] timeout:10]; + + // At this point the C++ CustomLayer will have been destroyed, and the rawLayer pointer has been NULLed + XCTAssert(weakLayer == layer2); + XCTAssertNotNil(weakLayer); + + // Asking the style for the layer should return nil + MGLStyleLayer *layer3 = [self.mapView.style layerWithIdentifier:@"gl-layer"]; + XCTAssertNil(layer3); +} + + +- (void)testOpenGLLayerDoesNotLeakWhenMapViewDeallocs { + __weak id weakLayer; + + @autoreleasepool { + + NSURL *styleURL = [[NSBundle bundleForClass:[self class]] URLForResource:@"one-liner" withExtension:@"json"]; + MGLMapView *mapView2 = [[MGLMapView alloc] initWithFrame:UIScreen.mainScreen.bounds styleURL:styleURL]; + mapView2.delegate = self; + + XCTAssertNil(mapView2.style); + + _styleLoadingExpectation = [self expectationWithDescription:@"Map view should finish loading style."]; + [self waitForExpectationsWithTimeout:1 handler:nil]; + + MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"]; + weakLayer = layer; + [mapView2.style insertLayer:layer atIndex:0]; + layer = nil; + + [self waitForMapViewToBeRendered]; + } XCTAssertNil(weakLayer); } @end + diff --git a/src/mbgl/style/layers/custom_layer.cpp b/src/mbgl/style/layers/custom_layer.cpp index 0e51a70e50..f6749275ee 100644 --- a/src/mbgl/style/layers/custom_layer.cpp +++ b/src/mbgl/style/layers/custom_layer.cpp @@ -26,7 +26,6 @@ std::unique_ptr<Layer> CustomLayer::cloneRef(const std::string&) const { } // Visibility - void CustomLayer::setVisibility(VisibilityType value) { if (value == getVisibility()) return; |