Skip to content

Fix portrait video quality detection and fallback logging - #620

Open
davidjiagoogle wants to merge 5 commits into
mainfrom
david/videoQualityOrientationFix
Open

davidjiagoogle wants to merge 5 commits into
mainfrom
david/videoQualityOrientationFix

Conversation

@davidjiagoogle

@davidjiagoogle davidjiagoogle commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  1. Orientation-invariant video quality classification (getVideoQualityFromResolution):
    • When VideoCapture is wrapped in StreamSharing, VirtualCameraAdapter pre-rotates the child stream resolution into portrait (e.g., 720x1280 for HD or 1080x1920 for FHD).
    • Previously, getVideoQualityFromResolution() checked res.height against QUALITY_RANGE_MAP, which misclassified portrait 720x1280 (height = 1280) as VideoQuality.FHD and portrait 1080x1920 (height = 1920) as VideoQuality.UNSPECIFIED.
    • Use minOf(res.width, res.height) so quality detection uses the short edge regardless of orientation, and extend the FHD range in QUALITY_RANGE_MAP up to 2159 so intermediate resolutions like 1440p (QHD) map to FHD instead of UNSPECIFIED.
  2. Orientation-aware video resolution dimensions (getWidthFromCropRect / getHeightFromCropRect):
    • Account for ResolutionInfo.rotationDegrees when computing upright video width and height from cropRect, refresh dimensions when device rotation updates, and format width x height in DebugOverlayComponents so dimensions display accurately both with and without StreamSharing.
  3. Video quality fallback logging (runSingleCameraSession):
    • Guard the fallback log with resolutionInfo != null and sessionSettings.videoQuality != VideoQuality.UNSPECIFIED, fix string interpolation, and log at Log.w.
  4. FallbackStrategy comment (createVideoUseCase):
    • Update the comment above QualitySelector.from(quality, FallbackStrategy.lowerQualityOrHigherThan(quality)) to accurately reflect the fallback behavior.
  5. Unit Tests:
    • Add CameraSessionTest covering getVideoQualityFromResolution across landscape, portrait, 1440p (2560x1440 / 1440x2560), range boundaries, and null resolutions, as well as getWidthFromCropRect and getHeightFromCropRect across rotated and unrotated crop rects.

@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 makes video quality resolution mapping orientation-invariant by using the short edge of the resolution, adds corresponding unit tests, and prevents crashes by allowing fallback video qualities. The review feedback suggests simplifying a log statement's string concatenation into a single Kotlin string template and highlights a gap in the QUALITY_RANGE_MAP classification ranges that could cause 1440p resolutions to be incorrectly classified as unspecified.

Comment thread core/camera/src/main/java/com/google/jetpackcamera/core/camera/CameraSession.kt Outdated

@temcguir temcguir left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice fix, looks good overall! Just a few small things inline. Could you also mention the FHD range change in the PR description?

Comment thread core/camera/src/main/java/com/google/jetpackcamera/core/camera/CameraSession.kt Outdated
Comment thread core/camera/src/main/java/com/google/jetpackcamera/core/camera/CameraSession.kt Outdated
- Document short-edge matching and 1440p FHD range in QUALITY_RANGE_MAP
- Skip video quality fallback log when resolutionInfo is null and downgrade to Log.w
- Make getWidthFromCropRect/getHeightFromCropRect rotation-aware and format width x height in DebugOverlayComponents
- Update 1440p test resolutions to 2560x1440 / 1440x2560 and add range boundary and crop-rect rotation unit tests
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.

2 participants