-
-
Notifications
You must be signed in to change notification settings - Fork 77
Fix: image orientation #209
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
base: main
Are you sure you want to change the base?
Conversation
|
👋 Hello @asabri97, thank you for submitting a
For more guidance, please refer to our Contributing Guide. Don’t hesitate to leave a comment if you have any questions. Thank you for contributing to Ultralytics! 🚀 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
UltralyticsAssistant
left a comment
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.
🔍 PR Review
Made with ❤️ by Ultralytics Actions
The normalization helpers look solid; just add defer { UIGraphicsEndImageContext() } in both implementations so the graphics context is always released. Also, pruning the never-failing nil expectations in the new tests will keep the suite meaningful.
💬 Posted 4 inline comments
| newImage = uiImage | ||
| } | ||
| return newImage | ||
| UIGraphicsBeginImageContextWithOptions(uiImage.size, false, uiImage.scale) |
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.
💡 MEDIUM: Nice simplification! One small request: could we wrap the drawing block with defer { UIGraphicsEndImageContext() }? That guarantees the context is torn down even if we add another exit path or the draw routine starts returning early, and it keeps us safe from leaks if this helper evolves.
| newImage = uiImage | ||
| } | ||
| return newImage | ||
| UIGraphicsBeginImageContextWithOptions(uiImage.size, false, uiImage.scale) |
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.
💡 MEDIUM: Same feedback here—using defer { UIGraphicsEndImageContext() } around the render block would make sure the graphics context is always cleaned up, which protects us if the method later grows additional return points.
|
|
||
| // Check if the function returns a valid image | ||
| #expect(correctedImageDown != nil, "Corrected image should not be nil") | ||
| #expect(correctedImageDown.imageOrientation == .up, "Normalized image should have .up 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.
📝 LOW: getCorrectOrientationUIImage returns a non-optional UIImage, so this nil check will never fail. Dropping the assertion keeps the test focused on behaviors that can actually break.
|
|
||
| // Check if the function returns a valid image | ||
| #expect(correctedImageDown != nil, "Corrected image should not be nil") | ||
| #expect(correctedImageDown.imageOrientation == .up, "Normalized image should have .up 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.
📝 LOW: Same as the SwiftUI tests—since the helper returns a non-optional UIImage, this nil check cannot fail. Removing it will avoid a tautological assertion.
🛠️ PR Summary
Made with ❤️ by Ultralytics Actions
🌟 Summary
Normalize UIImage orientation across SwiftUI and UIKit example apps to ensure consistent .up orientation for inference and UI rendering.
📊 Key Changes
🎯 Purpose & Impact