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 6 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,38 @@
/*
* 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

@interface CMPMetalDrawablesHandler : NSObject
MatkovIvan marked this conversation as resolved.
Show resolved Hide resolved

- (instancetype)initWithMetalLayer:(CAMetalLayer *)metalLayer;

- (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?

- (void)releaseDrawable:(void * CMP_CONSUMED)drawablePtr;
- (void * CMP_BORROWED)drawableTexture:(void * CMP_BORROWED)drawablePtr;
- (void)presentDrawable:(void * CMP_CONSUMED)drawablePtr;
- (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,7 @@ internal class MetalRedrawer(
private val metalLayer: CAMetalLayer,
private val callbacks: MetalRedrawerCallbacks
) {
private val metalDrawableHandler = CMPMetalDrawablesHandler(metalLayer)
MatkovIvan marked this conversation as resolved.
Show resolved Hide resolved
// 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 +318,7 @@ internal class MetalRedrawer(
}

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

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

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

val surface = Surface.makeFromBackendRenderTarget(
context,
Expand All @@ -344,6 +346,7 @@ internal class MetalRedrawer(
// Logger.warn { "'Surface.makeFromBackendRenderTarget' returned null. Skipping the frame." }
picture.close()
renderTarget.close()
metalDrawableHandler.releaseDrawable(metalDrawable)
dispatch_semaphore_signal(inflightSemaphore)
return@autoreleasepool
}
Expand Down Expand Up @@ -372,7 +375,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

metalDrawableHandler.scheduleDrawablePresentation(metalDrawable, commandBuffer)
}

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

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

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