Skip to content

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

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.

Sign up for GitHub

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

Open
davidjiagoogle wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion core/camera/src/main/java/com/google/jetpackcamera/core/camera/CameraSession.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -667,9 +667,17 @@ internal fun createUseCaseGroup(
TAG,
"Setting initial device rotation to ${initialTransientSettings.deviceRotation}"
)
val isSensorRotationLandscape =
(cameraInfo.getSensorRotationDegrees(previewUseCase.targetRotation) % 180 == 0) ==
(cameraInfo.sensorRect.width() >= cameraInfo.sensorRect.height())
Comment on lines +670 to +672

@gemini-code-assist gemini-code-assist Bot Oct 7, 2026

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)

val viewPortAspectRatio = if (isSensorRotationLandscape) {
Rational(aspectRatio.denominator, aspectRatio.numerator)
} else {
Rational(aspectRatio.numerator, aspectRatio.denominator)
}
Comment on lines +670 to +677

@gemini-code-assist gemini-code-assist Bot Oct 7, 2026

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)

setViewPort(
ViewPort.Builder(
Rational(aspectRatio.numerator, aspectRatio.denominator),
viewPortAspectRatio,
// Initialize rotation to Preview's rotation, which comes from Display rotation
previewUseCase.targetRotation
).build()
Expand Down
6 changes: 5 additions & 1 deletion ...e/src/main/java/com/google/jetpackcamera/ui/components/capture/CaptureScreenComponents.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -797,7 +797,11 @@ fun PreviewDisplay(
val aspectRatio =
aspectRatioUiState.selectedAspectRatio
val maxAspectRatio: Float = maxWidth / maxHeight
val aspectRatioFloat: Float = aspectRatio.toFloat()
val aspectRatioFloat: Float = if (maxWidth > maxHeight) {
aspectRatio.toLandscapeFloat()
} else {
aspectRatio.toFloat()
}
val shouldUseMaxWidth = maxAspectRatio <= aspectRatioFloat
val width = if (shouldUseMaxWidth) maxWidth else maxHeight * aspectRatioFloat
val height = if (!shouldUseMaxWidth) maxHeight else maxWidth / aspectRatioFloat
Expand Down
Loading