cherrypick: Bring changes to improve our RCTUIGraphicsImageRenderer shim #2219
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…r` (facebook#46772) Summary: Because `UIGraphicsImageRenderer` doesn't exist on macOS, I need to shim it for React Native macOS (See microsoft#2209). I planned to use the name `RCTUIGraphicsImageRenderer`. However.. it seems that is used by a static helper function in `RCTBorderDrawing.m`. So.. let's rename it? The function is just a helper method to make an instance of the class, so I think the name `RCTMakeUIGraphicsImageRenderer` is slightly more idiomatic anyway. This method is not public, so it should not break the public API of React Native. [IOS] [CHANGED] - Rename `RCTUIGraphicsImageRenderer` to `RCTMakeUIGraphicsImageRenderer` Pull Request resolved: facebook#46772 Test Plan: CI should pass Reviewed By: joevilches Differential Revision: D63765490 Pulled By: cipolleschi fbshipit-source-id: de68dce0f92ec249ea8586dbf7b9ba34a8476074
…emory management (facebook#46847) Summary: Update React Native on iOS to do less manual memory management, by replacing uses of `CGColorRef` with `UIColor`, and only calling `UIColor.CGColor` when needed. This results in much less manual memory management, which is probably a good thing in 2024 :D. The downside is there is a breaking change: the signature of a method in `RCTBorderDrawing` changes. This is a followup to facebook#46797 . After that PR merged and I tested merging React Native macOS to the `0.76-stable` branch cut commit again, I saw even more places I needed to manually call `CGColorRetain` / `CGColorRelease`. The reason is due to React Native macOS specifics (explained below) and I could update React Native macOS to not need these changes, but I thought I would at least throw up a PR to propose the changes, as it may be good to move away from using Core Graphics' C base API as much as possible. With microsoft#2209 , I wrote a shim of [UIGraphicsImageRenderer](https://developer.apple.com/documentation/uikit/uigraphicsimagerenderer) for macOS. The main difference is my shim calls the NSImage API [imageWithSize:flipped:drawingHandler:](https://developer.apple.com/documentation/appkit/nsimage/1519860-imagewithsize?language=objc) to shim the UIGraphicsImageRenderer API [imageWithData:](https://developer.apple.com/documentation/uikit/uiimage/1624137-imagewithdata). The difference between the two is that the macOS API copies the block, and executes it when Appkit is about to draw the image, while the iOS API executes the block right away. Because of this, I think I am hitting way more places where `CGColorRef` variables needed to be retained / released. Additionally, I hit more of them when I merge to the 0.76 branch cut commit (this is why I didn't catch it with facebook#46797). Given this constraint, I have a couple of options: 1. Refactor my macOS shim to use the deprecated API [[NSImage lockFocus]](https://developer.apple.com/documentation/appkit/nsimage/1519891-lockfocus) - I am not a fan of this because `lockFocus` was deprecated for `imageWithSize:flipped:drawingHandler:` 2. Refactor my macOS shim to do what we used to do: Create a CGContext manually and write it to an image - This is probably OK. Relies on less Appkit specifics, and potentially gives more control to RN on the rendering layer, which I recall lenaic told me is better for Fabric) 3. Refactor React Native to avoid CGColorRef altogether, and use `UIColor` (which ARC will memory manage for us) as much as possible. - This is the approach of this PR and my preferred approach. The downside is this changes the signature of some of the methods in `RCTBorderDrawing` which is a breaking change. I've seen other PRs do this, so maybe its OK? [IOS] [BREAKING] - Replace uses of `CGColorRef` with UIColor to avoid manual memory management Pull Request resolved: facebook#46847 Test Plan: Launching RNTester's View example (which tests a lot of the border / outline / shadow rendering) does not crash for me on both iOS and macOS. Reviewed By: NickGerleman Differential Revision: D63989547 Pulled By: joevilches fbshipit-source-id: 5e85e17567e3dd8b4b0388452398954ad2e02464
acoates-ms
approved these changes
Oct 10, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Cherry pick facebook@6a09fc0 and facebook@b70709d to help prevent invalid memory access crashes with our
RCTUIGraphicsImageRenderershim.As a consequence, we can also remove a diff we had where we had added an extra
scaleFactorparameter toRCTGetBorderImage(), now that the shim doesn't take that argument.Test Plan:
CI should pass.