Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide Metal drawables from Kotlin runtime #1390

Merged
merged 8 commits into from
Jun 10, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -38,5 +38,6 @@ val IosBugs = Screen.Selection(
MeasureAndLayoutCrash,
AnimationFreezeBug,
ModalMemoryLeak,
ModalCrash
ModalCrash,
PopupStretching
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
/*
* Copyright 2024 The Android Open Source Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package androidx.compose.mpp.demo.bugs

import androidx.compose.foundation.background
import androidx.compose.foundation.horizontalScroll
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.Row
import androidx.compose.foundation.layout.Spacer
import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.layout.height
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.size
import androidx.compose.foundation.rememberScrollState
import androidx.compose.foundation.verticalScroll
import androidx.compose.material.Text
import androidx.compose.material3.Button
import androidx.compose.mpp.demo.Screen
import androidx.compose.runtime.Composable
import androidx.compose.ui.Modifier
import androidx.compose.ui.graphics.Color
import androidx.compose.ui.interop.LocalUIViewController
import androidx.compose.ui.unit.dp
import androidx.compose.ui.window.ComposeUIViewController
import platform.UIKit.UIModalPresentationFormSheet
import platform.UIKit.UISheetPresentationControllerDetent
import platform.UIKit.sheetPresentationController

val PopupStretching = Screen.Example("Popup stretching") {
val viewController = LocalUIViewController.current

Button(onClick = {
val bottomSheetController = ComposeUIViewController {
VerticalScrollWithIndependentHorizontalRows()
}

bottomSheetController.modalPresentationStyle = UIModalPresentationFormSheet
bottomSheetController.sheetPresentationController?.setDetents(
listOf(
UISheetPresentationControllerDetent.mediumDetent(),
UISheetPresentationControllerDetent.largeDetent(),
)
)

viewController.presentViewController(bottomSheetController, animated = true, completion = {})
}) {
Text("Show popup")
}
}


@Composable
fun VerticalScrollWithIndependentHorizontalRows() {
Column(
modifier =
Modifier.fillMaxSize().verticalScroll(rememberScrollState(), enabled = true),
) {
repeat(10) { rowIndex ->
val horizontalScrollState = rememberScrollState()

Spacer(Modifier.height(30.dp).background(Color.DarkGray))
Row(
modifier =
Modifier
.padding(start = 16.dp, end = 16.dp)
.horizontalScroll(horizontalScrollState),
) {
repeat(5) {
Box(
modifier =
Modifier
.size(100.dp)
.background(Color.Gray),
) {
Text("Item $it in row $rowIndex")
}
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
EA70A7EC2B27106100300068 /* CMPAccessibilityContainer.m in Sources */ = {isa = PBXBuildFile; fileRef = EA70A7E92B27106100300068 /* CMPAccessibilityContainer.m */; };
EA82F4F92B86144E00465418 /* CMPOSLogger.m in Sources */ = {isa = PBXBuildFile; fileRef = EA82F4F82B86144E00465418 /* CMPOSLogger.m */; };
EA82F4FC2B86184F00465418 /* CMPOSLoggerInterval.m in Sources */ = {isa = PBXBuildFile; fileRef = EA82F4FB2B86184F00465418 /* CMPOSLoggerInterval.m */; };
EAB33E182C12E746002CFF44 /* CMPMetalDrawablesHandler.m in Sources */ = {isa = PBXBuildFile; fileRef = EAB33E172C12E746002CFF44 /* CMPMetalDrawablesHandler.m */; };
EABD912B2BC02B5F00455279 /* CMPInteropWrappingView.m in Sources */ = {isa = PBXBuildFile; fileRef = EABD912A2BC02B5F00455279 /* CMPInteropWrappingView.m */; };
EAC703E32B8C826E001ECDA6 /* CMPAccessibilityElement.m in Sources */ = {isa = PBXBuildFile; fileRef = EA70A7E82B27106100300068 /* CMPAccessibilityElement.m */; };
EAC703E42B8C826E001ECDA6 /* CMPViewController.m in Sources */ = {isa = PBXBuildFile; fileRef = 997DFCDD2B18D135000B56B5 /* CMPViewController.m */; };
Expand Down Expand Up @@ -82,6 +83,8 @@
EA82F4F82B86144E00465418 /* CMPOSLogger.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = CMPOSLogger.m; sourceTree = "<group>"; };
EA82F4FA2B86184F00465418 /* CMPOSLoggerInterval.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = CMPOSLoggerInterval.h; sourceTree = "<group>"; };
EA82F4FB2B86184F00465418 /* CMPOSLoggerInterval.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = CMPOSLoggerInterval.m; sourceTree = "<group>"; };
EAB33E162C12E746002CFF44 /* CMPMetalDrawablesHandler.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = CMPMetalDrawablesHandler.h; sourceTree = "<group>"; };
EAB33E172C12E746002CFF44 /* CMPMetalDrawablesHandler.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = CMPMetalDrawablesHandler.m; sourceTree = "<group>"; };
EABD91292BC02B5F00455279 /* CMPInteropWrappingView.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = CMPInteropWrappingView.h; sourceTree = "<group>"; };
EABD912A2BC02B5F00455279 /* CMPInteropWrappingView.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = CMPInteropWrappingView.m; sourceTree = "<group>"; };
EAC703DF2B8C8154001ECDA6 /* CMPUIKitUtilsTestApp-Bridging-Header.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = "CMPUIKitUtilsTestApp-Bridging-Header.h"; sourceTree = "<group>"; };
Expand Down Expand Up @@ -134,6 +137,8 @@
99DCAB0D2BD00F5C002E6AC7 /* CMPTextLoupeSession.m */,
99D97A862BF73A9B0035552B /* CMPEditMenuView.h */,
99D97A872BF73A9B0035552B /* CMPEditMenuView.m */,
EAB33E162C12E746002CFF44 /* CMPMetalDrawablesHandler.h */,
EAB33E172C12E746002CFF44 /* CMPMetalDrawablesHandler.m */,
);
path = CMPUIKitUtils;
sourceTree = "<group>";
Expand Down Expand Up @@ -308,6 +313,7 @@
buildActionMask = 2147483647;
files = (
997DFCDE2B18D135000B56B5 /* CMPViewController.m in Sources */,
EAB33E182C12E746002CFF44 /* CMPMetalDrawablesHandler.m in Sources */,
99D97A882BF73A9B0035552B /* CMPEditMenuView.m in Sources */,
EABD912B2BC02B5F00455279 /* CMPInteropWrappingView.m in Sources */,
EA70A7EC2B27106100300068 /* CMPAccessibilityContainer.m in Sources */,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,12 @@

#define CMP_MUST_BE_OVERRIDED
#define CMP_MUST_BE_OVERRIDED_INVARIANT_VIOLATION assert(false && "MUST_OVERRIDE");

// Marker for indicating that raw pointer returned from a function is owned by the caller
#define CMP_OWNED

// Marker for indicating that raw pointer is consumed when passed as an argument
#define CMP_CONSUMED

// Marker for indicating that raw pointer is implied as borrowed when returned from a function or passed as an argument
#define CMP_BORROWED
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
* Copyright 2023 The Android Open Source Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#import <Foundation/Foundation.h>
#import <QuartzCore/QuartzCore.h>
#import <Metal/Metal.h>

#import "CMPMacros.h"

NS_ASSUME_NONNULL_BEGIN

/**
A handler class for managing Metal drawables explicitly using raw pointers.
This class encapsulates the lifecycle management of drawable objects,
facilitating the use in environments where automatic reference counting (ARC)
mixed with Kotlin/Native memory model that leads to violation of practices enstated by Apple (namely, not releasing drawables as soon as possible), which lead to inadequate memory spikes during drawable size updates across consequent frames.
@see https://developer.apple.com/library/archive/documentation/3DDrawing/Conceptual/MTLBestPracticesGuide/Drawables.html

The class methods handle the acquisition, release, and presentation of
drawable objects associated with a given CAMetalLayer. Usage of raw pointers
helps in explicitly controlling the drawable lifecycle, preventing application from keeping drawables and their pools alive longer, than needed, due to awaiting deallocation by GC.
*/
@interface CMPMetalDrawablesHandler : NSObject
MatkovIvan marked this conversation as resolved.
Show resolved Hide resolved

/// Initializes the handler with a given Metal layer.
/// @param metalLayer The CAMetalLayer instance to be associated with this handler.
- (instancetype)initWithMetalLayer:(CAMetalLayer *)metalLayer;

/// Retrieves the next drawable object from the associated Metal layer.
/// @return A raw pointer to the next drawable object, ownership is transferred to the caller.
- (void * CMP_OWNED)nextDrawable;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just thinking about slightly more generic solution.
What do you think about creating analog of "Manual Reference Counter" that can wrap various classes and hide them from Kotlin GC?
Or maybe even play a bit with NSProxy to provide sufficient interface for Kotlin.

Copy link
Author

Choose a reason for hiding this comment

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

I tried to model this approach in my head, but didn't succeed. Do you have proof-of-concept available?


/// Releases a drawable object, indicating that it is no longer in use by the caller.
/// @param drawablePtr A raw pointer to the drawable to be released, indicating transfer of ownership back to the handler.
- (void)releaseDrawable:(void * CMP_CONSUMED)drawablePtr;

/// Retrieves the texture of a drawable without transferring ownership.
/// @param drawablePtr A raw pointer to the drawable from which to get the texture.
/// @return A raw pointer to the texture of the drawable, ownership is not transferred.
- (void * CMP_BORROWED)drawableTexture:(void * CMP_BORROWED)drawablePtr;

/// Presents a drawable to the screen immediately.
/// @param drawablePtr A raw pointer to the drawable to be presented, indicating transfer of ownership.
- (void)presentDrawable:(void * CMP_CONSUMED)drawablePtr;

/// Schedules the presentation of a drawable on a specific command buffer.
/// @param drawablePtr A raw pointer to the drawable to be presented, indicating transfer of ownership.
/// @param commandBuffer The command buffer on which the drawable will be scheduled for presentation.
- (void)scheduleDrawablePresentation:(void * CMP_CONSUMED)drawablePtr onCommandBuffer:(id <MTLCommandBuffer>)commandBuffer;

@end

NS_ASSUME_NONNULL_END
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/*
* Copyright 2023 The Android Open Source Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#import "CMPMetalDrawablesHandler.h"

@implementation CMPMetalDrawablesHandler {
CAMetalLayer *_metalLayer;
}

- (instancetype)initWithMetalLayer:(CAMetalLayer *)metalLayer {
self = [super init];
if (self) {
_metalLayer = metalLayer;
}
return self;
}

- (void * CMP_OWNED)nextDrawable {
id <CAMetalDrawable> drawable = [_metalLayer nextDrawable];

if (drawable) {
void *ptr = (__bridge_retained void *)drawable;
return ptr;
} else {
return NULL;
}
}

- (void)releaseDrawable:(void * CMP_CONSUMED)drawablePtr {
assert(drawablePtr != NULL);

id <CAMetalDrawable> drawable __unused = (__bridge_transfer id <CAMetalDrawable>)drawablePtr;
// drawable will be released by ARC
}

- (void * CMP_BORROWED)drawableTexture:(void * CMP_BORROWED)drawablePtr {
assert(drawablePtr != NULL);

id <CAMetalDrawable> drawable = (__bridge id <CAMetalDrawable>)drawablePtr;
id <MTLTexture> texture = drawable.texture;
void *texturePtr = (__bridge void *)texture;
return texturePtr;
}

- (void)presentDrawable:(void * CMP_CONSUMED)drawablePtr {
assert(drawablePtr != NULL);

id <CAMetalDrawable> drawable = (__bridge_transfer id <CAMetalDrawable>)drawablePtr;
[drawable present];
}

- (void)scheduleDrawablePresentation:(void * CMP_CONSUMED)drawablePtr onCommandBuffer:(id <MTLCommandBuffer>)commandBuffer {
assert(drawablePtr != NULL);

id <CAMetalDrawable> drawable = (__bridge_transfer id <CAMetalDrawable>)drawablePtr;
[commandBuffer presentDrawable:drawable];
}

@end
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,4 @@ FOUNDATION_EXPORT const unsigned char CMPUIKitUtilsVersionString[];
#import "CMPAccessibilityContainer.h"
#import "CMPOSLogger.h"
#import "CMPTextLoupeSession.h"
#import "CMPMetalDrawablesHandler.h"
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import androidx.compose.ui.interop.UIKitInteropState
import androidx.compose.ui.interop.UIKitInteropTransaction
import androidx.compose.ui.interop.doLocked
import androidx.compose.ui.interop.isNotEmpty
import androidx.compose.ui.uikit.utils.CMPMetalDrawablesHandler
import androidx.compose.ui.util.fastForEach
import androidx.compose.ui.util.trace
import kotlin.math.roundToInt
Expand Down Expand Up @@ -145,6 +146,13 @@ internal class MetalRedrawer(
private val metalLayer: CAMetalLayer,
private val callbacks: MetalRedrawerCallbacks
) {
/**
* A wrapper around CAMetalLayer that allows to perform operations on its drawables without
* exposing the objects to Kotlin/Native runtime and thus allowing explicit lifetime control of them.
*
* See ObjC implementation of [CMPMetalDrawablesHandler] for more details.
*/
private val metalDrawablesHandler = CMPMetalDrawablesHandler(metalLayer)
// Workaround for KN compiler bug
// Type mismatch: inferred type is objcnames.protocols.MTLDeviceProtocol but platform.Metal.MTLDeviceProtocol was expected
@Suppress("USELESS_CAST")
Expand Down Expand Up @@ -316,7 +324,7 @@ internal class MetalRedrawer(
}

val metalDrawable = trace("MetalRedrawer:draw:nextDrawable") {
metalLayer.nextDrawable()
metalDrawablesHandler.nextDrawable()
}

if (metalDrawable == null) {
Expand All @@ -328,7 +336,7 @@ internal class MetalRedrawer(
}

val renderTarget =
BackendRenderTarget.makeMetal(width, height, metalDrawable.texture.objcPtr())
BackendRenderTarget.makeMetal(width, height, metalDrawablesHandler.drawableTexture(metalDrawable).rawValue)

val surface = Surface.makeFromBackendRenderTarget(
context,
Expand All @@ -344,6 +352,7 @@ internal class MetalRedrawer(
// Logger.warn { "'Surface.makeFromBackendRenderTarget' returned null. Skipping the frame." }
picture.close()
renderTarget.close()
metalDrawablesHandler.releaseDrawable(metalDrawable)
dispatch_semaphore_signal(inflightSemaphore)
return@autoreleasepool
}
Expand Down Expand Up @@ -372,7 +381,9 @@ internal class MetalRedrawer(
commandBuffer.label = "Present"

if (!presentsWithTransaction) {
commandBuffer.presentDrawable(metalDrawable)
// scheduleDrawablePresentation consumes metalDrawable
// don't use metalDrawable after this call
Comment on lines +384 to +385
Copy link
Member

Choose a reason for hiding this comment

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

Can we just nullify it?

Copy link
Author

Choose a reason for hiding this comment

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

It's a smart cast val, doing so will require to put force-unwraps everywhere

metalDrawablesHandler.scheduleDrawablePresentation(metalDrawable, commandBuffer)
}

commandBuffer.addCompletedHandler {
Expand All @@ -388,7 +399,9 @@ internal class MetalRedrawer(
commandBuffer.waitUntilScheduled()
}

metalDrawable.present()
// presentDrawable consumes metalDrawable
// don't use metalDrawable after this call
metalDrawablesHandler.presentDrawable(metalDrawable)

interopTransaction.actions.fastForEach {
it.invoke()
Expand Down
Loading