Repository navigation
Fix ViewPort and PreviewDisplay aspect ratio on landscape-natural devices - #592
davidjiagoogle wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request adjusts the viewport aspect ratio calculation in CameraSession.kt to account for sensor rotation and dimensions, and updates PreviewDisplay in CaptureScreenComponents.kt to handle landscape orientation correctly. The feedback recommends adding explanatory comments to the non-obvious sensor rotation logic to improve maintainability, and requests adding unit tests to verify the viewport aspect ratio calculations under different sensor orientations and display rotations.
| val isSensorRotationLandscape = | ||
| (cameraInfo.getSensorRotationDegrees(previewUseCase.targetRotation) % 180 == 0) == | ||
| (cameraInfo.sensorRect.width() >= cameraInfo.sensorRect.height()) |
There was a problem hiding this comment.
The formula used to determine if the sensor output is landscape is highly elegant but non-obvious. To ensure long-term maintainability and prevent future developers from accidentally breaking or misunderstanding this logic, please add a brief explanatory comment explaining how the relative rotation and sensor dimensions are used to compute the final viewport orientation.
// Determine if the sensor output is landscape after applying the target rotation.
// If the relative rotation is 0 or 180, the orientation matches the sensor's natural orientation.
// If the relative rotation is 90 or 270, the orientation is rotated (landscape becomes portrait, and vice versa).
val isSensorRotationLandscape =
(cameraInfo.getSensorRotationDegrees(previewUseCase.targetRotation) % 180 == 0) ==
(cameraInfo.sensorRect.width() >= cameraInfo.sensorRect.height())References
- For complex or non-obvious logic, suggest adding explanatory comments to maintain code clarity and readability. (link)
| val isSensorRotationLandscape = | ||
| (cameraInfo.getSensorRotationDegrees(previewUseCase.targetRotation) % 180 == 0) == | ||
| (cameraInfo.sensorRect.width() >= cameraInfo.sensorRect.height()) | ||
| val viewPortAspectRatio = if (isSensorRotationLandscape) { | ||
| Rational(aspectRatio.denominator, aspectRatio.numerator) | ||
| } else { | ||
| Rational(aspectRatio.numerator, aspectRatio.denominator) | ||
| } |
There was a problem hiding this comment.
The new viewport aspect ratio calculation logic for landscape-natural devices is a critical change that should be covered by unit tests to prevent future regressions. Please add a unit test (e.g., in CameraSessionTest.kt or a new CameraSessionUseCaseTest.kt) to verify that createUseCaseGroup correctly computes the viewport aspect ratio under different sensor orientations and display rotations.
For example, the test should verify:
- A standard portrait-natural device with a landscape sensor rotated by 90 degrees held in portrait orientation (should result in a portrait viewport aspect ratio).
- A landscape-natural device with a landscape sensor rotated by 0 degrees held in landscape orientation (should result in a landscape viewport aspect ratio).
Ensure the test uses Robolectric and Truth assertions (assertThat(...)) in accordance with the repository testing guidelines.
Summary
Fixes an issue on landscape-natural devices where
android.sensor.orientation = 0(such as tablet/enterprise devices with a landscape natural display and landscape camera sensor) causesViewPortandPreviewDisplayto crop landscape camera streams into vertical portrait strips (e.g., cropping1920x1080down to608x1080and1600x1200down to900x1200).Changes
CameraSession.kt(createUseCaseGroup):Rationalpassed toViewPort.Builderbased on whether the sensor output inpreviewUseCase.targetRotationis landscape ((cameraInfo.getSensorRotationDegrees(previewUseCase.targetRotation) % 180 == 0) == (cameraInfo.sensorRect.width() >= cameraInfo.sensorRect.height())), usingRational(aspectRatio.denominator, aspectRatio.numerator)when landscape andRational(aspectRatio.numerator, aspectRatio.denominator)when portrait.CaptureScreenComponents.kt(PreviewDisplay):aspectRatio.toLandscapeFloat()when the window is landscape (maxWidth > maxHeight) instead of unconditionally usingaspectRatio.toFloat().Test Plan
./gradlew spotlessCheck :core:camera:testDebugUnitTest :ui:components:capture:testDebugUnitTest --init-script gradle/init.gradle.kts