-
Notifications
You must be signed in to change notification settings - Fork 2k
[net11.0] Make iOS image scaling deadlock-safe #37057
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
Changes from all commits
ade78c6
988f409
fa6a4ab
82a3d6e
7151b52
560bc61
160475b
63c54b1
476929f
0a1fc0c
611e859
a4c742b
a47c914
d11f043
25f54ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,6 +84,25 @@ public void ScaleImageUsesOneXBackingScale(float sourceScale) | |
| Assert.Equal(5, (int)scaled.CGImage.Height); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData(0, 10)] | ||
| [InlineData(10, 0)] | ||
| [InlineData(-1, 10)] | ||
| [InlineData(10, -1)] | ||
| [InlineData(double.NaN, 10)] | ||
| [InlineData(10, double.NaN)] | ||
| [InlineData(double.PositiveInfinity, 10)] | ||
| [InlineData(10, double.PositiveInfinity)] | ||
| public void ScaleImageReturnsOriginalForNonPositiveSize(double width, double height) | ||
|
Vignesh-SF3580 marked this conversation as resolved.
|
||
| { | ||
| using var source = CreatePatternImage(UIImageOrientation.Up); | ||
|
|
||
| var scaled = source.ScaleImage(new CGSize(width, height), disposeOriginal: true); | ||
|
|
||
| Assert.Same(source, scaled); | ||
|
Vignesh-SF3580 marked this conversation as resolved.
Vignesh-SF3580 marked this conversation as resolved.
|
||
| Assert.NotNull(scaled.CGImage); | ||
| } | ||
|
|
||
| [Fact] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Testing — Flagged by: 3/3 reviewers
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks — agreed that the stricter test also reaches |
||
| public async Task ScaleImageCanRunOnBackgroundThread() | ||
| { | ||
|
|
@@ -162,6 +181,10 @@ public void ScaleImageMatchesUIKitRendering(UIImageOrientation orientation) | |
|
|
||
| Assert.Equal(UIImageOrientation.Up, actual.Orientation); | ||
| Assert.Equal(GetPixelData(expected), GetPixelData(actual)); | ||
| Assert.Equal(expected.CGImage.Width, actual.CGImage.Width); | ||
| Assert.Equal(expected.CGImage.Height, actual.CGImage.Height); | ||
| Assert.Equal(13, (double)actual.Size.Width); | ||
| Assert.Equal(9, (double)actual.Size.Height); | ||
| } | ||
|
|
||
| private static UIImage CreatePatternImage(UIImageOrientation orientation) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] Public API Surface Design — This early return hands back the same instance and silently ignores
disposeOriginal: true. That is the right runtime behaviour (you cannot dispose what you are returning), and it matches the pre-existing shape at lines 11-14/29, but it is now frozen as intended behaviour by a new test, so it is worth making explicit.The ownership hazard is concrete one level up:
PlatformImage.Downsizewraps the result in a newPlatformImage(Platforms/iOS/PlatformImage.cs:25,31), so a caller passingdisposeOriginal: trueends up with twoIImageinstances aliasing one undisposedUIImagewhile believing the original was released — the exact expectationIssue21886asserts against (original.Widthmust throwObjectDisposedException). Unreachable fromDownsizetoday because thefloatoverload guards first, hence minor. An XML<remarks>on the public method stating thatdisposeOriginalis ignored when the original is returned unchanged would close the contract gap without any behaviour change.