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

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

Closed
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions platform/darwin/src/MGLOpenGLStyleLayer.mm
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,9 @@
when creating an OpenGL style layer.
*/
void MGLPrepareCustomStyleLayer(void *context) {

// Pair retain/release during rendering (see MGLFinishCustomStyleLayer)
id retaineee = (__bridge id)context;
MGLOpenGLStyleLayer *layer = (__bridge MGLOpenGLStyleLayer*)CFBridgingRetain(retaineee);
// Note, that the layer is retained/released by MGLStyle, ensuring that the layer
// is alive during rendering
MGLOpenGLStyleLayer *layer = (__bridge MGLOpenGLStyleLayer*)context;

[layer didMoveToMapView:layer.style.mapView];
}
Expand Down Expand Up @@ -51,8 +50,10 @@ void MGLDrawCustomStyleLayer(void *context, const mbgl::style::CustomLayerRender
when creating an OpenGL style layer.
*/
void MGLFinishCustomStyleLayer(void *context) {
// Release the layer (since we retained it in the initialization)
MGLOpenGLStyleLayer *layer = CFBridgingRelease(context);

// Note, that the layer is retained/released by MGLStyle, ensuring that the layer
// is alive during rendering
MGLOpenGLStyleLayer *layer = (__bridge MGLOpenGLStyleLayer*)context;
[layer willMoveFromMapView:layer.style.mapView];
}

Expand Down
25 changes: 25 additions & 0 deletions platform/darwin/src/MGLStyle.mm
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,10 @@ @interface MGLStyle()
@property (readonly, copy, nullable) NSURL *URL;
@property (nonatomic) NS_MUTABLE_DICTIONARY_OF(NSString *, NS_DICTIONARY_OF(NSObject *, MGLTextLanguage *) *) *localizedLayersByIdentifier;

// Used for retain/release management
@property (nonatomic) NSMutableSet *layersForUpdating;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the use of these properties remove the need for tracking the peer object in core for style layers? For layers created in the SDK, [MGLStlye layerFromMBGLLayer] could be used to lookup this set instead of querying core

@property (nonatomic) NSSet *layersForRendering;

@end

@implementation MGLStyle
Expand Down Expand Up @@ -172,6 +176,8 @@ + (NSURL *)trafficNightStyleURLWithVersion:(NSInteger)version {

- (instancetype)initWithRawStyle:(mbgl::style::Style *)rawStyle mapView:(MGLMapView *)mapView {
if (self = [super init]) {
_layersForUpdating = [NSMutableSet set];

_mapView = mapView;
_rawStyle = rawStyle;
_localizedLayersByIdentifier = [NSMutableDictionary dictionary];
Expand Down Expand Up @@ -534,6 +540,25 @@ - (void)insertLayer:(MGLStyleLayer *)layer aboveLayer:(MGLStyleLayer *)sibling {
[self didChangeValueForKey:@"layers"];
}

#pragma mark - Layer retain/release management

- (void)addToManagedLayers:(MGLStyleLayer*)layer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be invoked from [MGLStyle layerFromMBGLLayer] as well, to capture internally created style layers.

[self.layersForUpdating addObject:layer];
}

- (void)removeFromManagedLayers:(MGLStyleLayer*)layer {
[self.layersForUpdating removeObject:layer];
}

- (void)retainLayersUsedDuringRendering {
self.layersForRendering = [self.layersForUpdating copy];
}

- (void)releaseLayersUsedDuringRendering {
self.layersForRendering = nil;
}


#pragma mark Style classes

- (NS_ARRAY_OF(NSString *) *)styleClasses
Expand Down
12 changes: 6 additions & 6 deletions platform/darwin/src/MGLStyleLayer.mm
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ - (void)addToStyle:(MGLStyle *)style belowLayer:(MGLStyleLayer *)otherLayer
"to the style more than once is invalid.", self, style];
}

// Since we're adding self to a C++ collection, we need to retain ourselves, so that we don't
// end up with a dangling pointer
CFBridgingRetain(self);
// We need to ensure that this layer is retained, so that any references from layer impl's
// e.g. contexts) are still valid
[style addToManagedLayers:self];

if (otherLayer) {
const mbgl::optional<std::string> belowLayerId{otherLayer.identifier.UTF8String};
Expand All @@ -55,9 +55,9 @@ - (void)removeFromStyle:(MGLStyle *)style
if (self.rawLayer == style.rawStyle->getLayer(self.identifier.UTF8String)) {
_pendingLayer = style.rawStyle->removeLayer(self.identifier.UTF8String);

// Pair the retain above, and release self, since we're now removed from the collection
CFTypeRef toRelease = (__bridge CFTypeRef)self;
CFBridgingRelease(toRelease);
// We need to ensure that this layer is now released (however, if this layer is about to be
// used by the renderer then it will released once rendering is complete)
[style removeFromManagedLayers:self];
}
}

Expand Down
4 changes: 4 additions & 0 deletions platform/darwin/src/MGLStyle_Private.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ namespace mbgl {

- (void)setStyleClasses:(NS_ARRAY_OF(NSString *) *)appliedClasses transitionDuration:(NSTimeInterval)transitionDuration;

- (void)addToManagedLayers:(MGLStyleLayer*)layer;
- (void)removeFromManagedLayers:(MGLStyleLayer*)layer;
- (void)retainLayersUsedDuringRendering;
- (void)releaseLayersUsedDuringRendering;
@end

@interface MGLStyle (MGLStreetsAdditions)
Expand Down
33 changes: 33 additions & 0 deletions platform/ios/Integration Tests/MBGLIntegrationTests.m
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,39 @@ - (void)testOpenGLLayerDoesNotLeakWhenCreatedAndDestroyedWithoutAddingToStyle {
XCTAssertNil(weakLayer);
}

- (void)testOpenGLLayerDoesNotLeakWhenMapViewDeallocs {
NSTimeInterval waitInterval = 0.02;
__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];

// UIView *superView = [[UIView alloc] initWithFrame:UIScreen.mainScreen.bounds];
// [superView addSubview:self.mapView];
// UIWindow *window = [[UIWindow alloc] initWithFrame:UIScreen.mainScreen.bounds];
// [window addSubview:superView];
// [window makeKeyAndVisible];

MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"];
weakLayer = layer;
[mapView2.style insertLayer:layer atIndex:0];
layer = nil;

[[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]];
[self.mapView setNeedsDisplay];
[[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]];
}
XCTAssertNil(weakLayer);
}

- (void)testOpenGLLayerDoesNotLeakWhenRemovedFromStyle {
NSTimeInterval waitInterval = 0.02;

Expand Down
9 changes: 9 additions & 0 deletions platform/ios/src/MGLMapView.mm
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ @interface MGLMapView () <UIGestureRecognizerDelegate,

@property (nonatomic, readwrite) MGLStyle *style;


@property (nonatomic) UITapGestureRecognizer *singleTapGestureRecognizer;
@property (nonatomic) UITapGestureRecognizer *doubleTap;
@property (nonatomic) UITapGestureRecognizer *twoFingerTap;
Expand Down Expand Up @@ -359,7 +360,9 @@ - (void)setStyleURL:(nullable NSURL *)styleURL
}

styleURL = styleURL.mgl_URLByStandardizingScheme;

self.style = nil;

_mbglMap->getStyle().loadURL([[styleURL absoluteString] UTF8String]);
}

Expand Down Expand Up @@ -5549,10 +5552,13 @@ - (void)mapViewDidFailLoadingMapWithError:(NSError *)error {
}

- (void)mapViewWillStartRenderingFrame {

if (!_mbglMap) {
return;
}

[self.style retainLayersUsedDuringRendering];

if ([self.delegate respondsToSelector:@selector(mapViewWillStartRenderingFrame:)])
{
[self.delegate mapViewWillStartRenderingFrame:self];
Expand All @@ -5575,6 +5581,9 @@ - (void)mapViewDidFinishRenderingFrameFullyRendered:(BOOL)fullyRendered {
{
[self.delegate mapViewDidFinishRenderingFrame:self fullyRendered:fullyRendered];
}

[self.style releaseLayersUsedDuringRendering];

}

- (void)mapViewWillStartRenderingMap {
Expand Down
4 changes: 4 additions & 0 deletions platform/macos/src/MGLMapView.mm
Original file line number Diff line number Diff line change
Expand Up @@ -882,6 +882,8 @@ - (void)mapViewWillStartRenderingFrame {
return;
}

[self.style retainLayersUsedDuringRendering];

if ([self.delegate respondsToSelector:@selector(mapViewWillStartRenderingFrame:)]) {
[self.delegate mapViewWillStartRenderingFrame:self];
}
Expand All @@ -899,6 +901,8 @@ - (void)mapViewDidFinishRenderingFrameFullyRendered:(BOOL)fullyRendered {
if ([self.delegate respondsToSelector:@selector(mapViewDidFinishRenderingFrame:fullyRendered:)]) {
[self.delegate mapViewDidFinishRenderingFrame:self fullyRendered:fullyRendered];
}

[self.style releaseLayersUsedDuringRendering];
}

- (void)mapViewWillStartRenderingMap {
Expand Down