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

Metal hal seems to accumulate CAMetalLayers in view on iOS #3190

Open
jimblandy opened this issue Nov 8, 2022 · 5 comments
Open

Metal hal seems to accumulate CAMetalLayers in view on iOS #3190

jimblandy opened this issue Nov 8, 2022 · 5 comments
Labels
backend: metal Issues with Metal

Comments

@jimblandy
Copy link
Member

On iOS, this code seems to be appending a new sublayer to the UIView's main layer each time the function is called:

#[cfg(target_os = "ios")]
{
// Unlike NSView, UIView does not allow to replace main layer.
let () = msg_send![main_layer, addSublayer: new_layer];
// On iOS, "from_view" may be called before the application initialization is complete,
// `msg_send![view, window]` and `msg_send![window, screen]` will get null.
let screen: *mut Object = msg_send![class!(UIScreen), mainScreen];
let scale_factor: CGFloat = msg_send![screen, nativeScale];
let () = msg_send![view, setContentScaleFactor: scale_factor];
};

If the view's layer is not a CAMetalLayer, we create one, and add to the end of view's layer's list of sublayers. But the view's layer itself doesn't change, so if we call this function again, we're just going to create and add a new CAMetalLayer, not return the one we created last time, which seems to be what the function is trying to do.

I'm not sure this is actually a bug; I just noticed it while looking at the code. Putting it here just in case.

@jimblandy jimblandy added the backend: metal Issues with Metal label Nov 8, 2022
@jimblandy
Copy link
Member Author

Cc @jinleili

@jinleili
Copy link
Contributor

jinleili commented Nov 8, 2022

get_metal_layer is actually only called when create_surface(), so is it enough to change visibility to pub(crate)?

@jimblandy
Copy link
Member Author

If that's the case, then is the code that checks for an existing CAMetalLayer needed? Will people ever pass in views that already have a CAMetalLayer?

@jimblandy
Copy link
Member Author

Yes, making it only callable from the crate would make sense.

It confused me because it looked like it was designed to be idempotent, but then it wasn't.

@jinleili
Copy link
Contributor

jinleili commented Nov 8, 2022

If that's the case, then is the code that checks for an existing CAMetalLayer needed? Will people ever pass in views that already have a CAMetalLayer?

It is indeed possible for a user to pass a UIView with CAMetalLayer, and I need to think about how to better check CAMetalLayer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: metal Issues with Metal
Projects
None yet
Development

No branches or pull requests

2 participants