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

Fix CustomLayer context retain count #10765

Merged
merged 2 commits into from
Dec 20, 2017
Merged
Show file tree
Hide file tree
Changes from all 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
6 changes: 2 additions & 4 deletions platform/darwin/src/MGLOpenGLStyleLayer.mm
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ void MGLDrawCustomStyleLayer(void *context, const mbgl::style::CustomLayerRender
when creating an OpenGL style layer.
*/
void MGLFinishCustomStyleLayer(void *context) {
MGLOpenGLStyleLayer *layer = (__bridge MGLOpenGLStyleLayer *)context;
MGLOpenGLStyleLayer *layer = (__bridge_transfer MGLOpenGLStyleLayer *)context;
[layer willMoveFromMapView:layer.style.mapView];
}

Expand Down Expand Up @@ -101,7 +101,7 @@ - (instancetype)initWithIdentifier:(NSString *)identifier {
MGLPrepareCustomStyleLayer,
MGLDrawCustomStyleLayer,
MGLFinishCustomStyleLayer,
(__bridge void *)self);
(__bridge_retained void *)self);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a corner case, but: this will leak a MGLOpenGLStyleLayer that's init'd but never added to a map.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😞 I knew I was missing something! Moved to : #10771

return self = [super initWithPendingLayer:std::move(layer)];
}

Expand All @@ -116,9 +116,7 @@ - (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;
_style = style;
_style.openGLLayers[self.identifier] = self;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only place where this dictionary was being used @1ec5 ; transferring the ownership is a better solution (and fixes the crash)

}

- (void)addToStyle:(MGLStyle *)style belowLayer:(MGLStyleLayer *)otherLayer {
Expand Down
2 changes: 0 additions & 2 deletions platform/darwin/src/MGLStyle.mm
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ @interface MGLStyle()
@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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this dictionary wound up unused after #8626. The original purpose of this dictionary was to allow -layerWithIdentifier: to return a fully initialized MGLOpenGLStyleLayer object that still had its callback implementations intact. As long as that’s still working correctly, this change looks good.

Copy link
Contributor

Choose a reason for hiding this comment

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

@1ec5 verified

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@1ec5 [MGLStlye layerFromMBGLLayer] already takes care of this for all layer types.

@property (nonatomic) NS_MUTABLE_DICTIONARY_OF(NSString *, NS_DICTIONARY_OF(NSObject *, MGLTextLanguage *) *) *localizedLayersByIdentifier;

@end
Expand Down Expand Up @@ -169,7 +168,6 @@ - (instancetype)initWithRawStyle:(mbgl::style::Style *)rawStyle mapView:(MGLMapV
if (self = [super init]) {
_mapView = mapView;
_rawStyle = rawStyle;
_openGLLayers = [NSMutableDictionary dictionary];
_localizedLayersByIdentifier = [NSMutableDictionary dictionary];
}
return self;
Expand Down
2 changes: 0 additions & 2 deletions platform/darwin/src/MGLStyle_Private.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ namespace mbgl {

- (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
Expand Down
21 changes: 13 additions & 8 deletions src/mbgl/renderer/layers/render_custom_layer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,20 +43,25 @@ std::unique_ptr<Bucket> RenderCustomLayer::createBucket(const BucketParameters&,
}

void RenderCustomLayer::render(PaintParameters& paintParameters, RenderSource*) {
if (!initialized) {
if (context != impl().context || !initialized) {
//If the context changed, deinitialize the previous one before initializing the new one.
if (context && !contextDestroyed && impl().deinitializeFn) {
impl().deinitializeFn(context);
}
context = impl().context;
assert(impl().initializeFn);
impl().initializeFn(impl().context);
initialized = true;
}

gl::Context& context = paintParameters.context;
gl::Context& glContext = paintParameters.context;
const TransformState& state = paintParameters.state;

// Reset GL state to a known state so the CustomLayer always has a clean slate.
context.bindVertexArray = 0;
context.setDepthMode(paintParameters.depthModeForSublayer(0, gl::DepthMode::ReadOnly));
context.setStencilMode(gl::StencilMode::disabled());
context.setColorMode(paintParameters.colorModeForRenderPass());
glContext.bindVertexArray = 0;
glContext.setDepthMode(paintParameters.depthModeForSublayer(0, gl::DepthMode::ReadOnly));
glContext.setStencilMode(gl::StencilMode::disabled());
glContext.setColorMode(paintParameters.colorModeForRenderPass());

CustomLayerRenderParameters parameters;

Expand All @@ -70,12 +75,12 @@ void RenderCustomLayer::render(PaintParameters& paintParameters, RenderSource*)
parameters.fieldOfView = state.getFieldOfView();

assert(impl().renderFn);
impl().renderFn(impl().context, parameters);
impl().renderFn(context, parameters);

// Reset the view back to our original one, just in case the CustomLayer changed
// the viewport or Framebuffer.
paintParameters.backend.bind();
context.setDirtyState();
glContext.setDirtyState();
}

} // namespace mbgl
1 change: 1 addition & 0 deletions src/mbgl/renderer/layers/render_custom_layer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ class RenderCustomLayer: public RenderLayer {
private:
bool initialized = false;
bool contextDestroyed = false;
void * context = nullptr;
};

template <>
Expand Down