Skip to content

Fix ViewPort and PreviewDisplay aspect ratio on landscape-natural devices - #592

Open
davidjiagoogle wants to merge 1 commit into
mainfrom
david/videoStabilizationCropFix
Open

davidjiagoogle wants to merge 1 commit into
mainfrom
david/videoStabilizationCropFix

Conversation

@davidjiagoogle

Copy link
Copy Markdown
Collaborator

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) causes ViewPort and PreviewDisplay to crop landscape camera streams into vertical portrait strips (e.g., cropping 1920x1080 down to 608x1080 and 1600x1200 down to 900x1200).

Changes

  1. CameraSession.kt (createUseCaseGroup):
    • Orient the Rational passed to ViewPort.Builder based on whether the sensor output in previewUseCase.targetRotation is landscape ((cameraInfo.getSensorRotationDegrees(previewUseCase.targetRotation) % 180 == 0) == (cameraInfo.sensorRect.width() >= cameraInfo.sensorRect.height())), using Rational(aspectRatio.denominator, aspectRatio.numerator) when landscape and Rational(aspectRatio.numerator, aspectRatio.denominator) when portrait.
  2. CaptureScreenComponents.kt (PreviewDisplay):
    • Use aspectRatio.toLandscapeFloat() when the window is landscape (maxWidth > maxHeight) instead of unconditionally using aspectRatio.toFloat().

Test Plan

  • ./gradlew spotlessCheck :core:camera:testDebugUnitTest :ui:components:capture:testDebugUnitTest --init-script gradle/init.gradle.kts

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +670 to +672
val isSensorRotationLandscape =
(cameraInfo.getSensorRotationDegrees(previewUseCase.targetRotation) % 180 == 0) ==
(cameraInfo.sensorRect.width() >= cameraInfo.sensorRect.height())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
  1. For complex or non-obvious logic, suggest adding explanatory comments to maintain code clarity and readability. (link)

Comment on lines +670 to +677
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)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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:

  1. 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).
  2. 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.

References
  1. If a PR introduces a significant feature or modifies logic without corresponding tests, flag this omission and suggest a test class name and outline what it should verify. (link)
  2. Always prefer Truth assertions (assertThat(...)) over JUnit assertions. (link)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant