Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[camera] Allow logical cameras to use all physical cameras via zoom on android 11+ #6150

Conversation

lucasoskorep
Copy link
Contributor

@lucasoskorep lucasoskorep commented Jul 27, 2022

Cameras in android versions greater than 11 are strongly pushed by the android team to use logical cameras to wrap multiple physical rear cameras into a single "logical camera."

The Flutter camera plugin has an issue where it calculates zoom incorrectly on these cameras and stops the user from being able to change the zoom appropriately to swap between the physical cameras wrapped by the logical camera exposed via the Android API

This change uses the new APIs implemented in Android 11 to correctly calculate zoom, and allow the camera app to swap between all the physical cameras contained within a single Logical camera in android. Users will still see a single Rear cam, but zooming should swap correctly.

Linked to #6091

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…amera preference. This preference is essential for supporting Logical cameras on the rear which after android 11 wrap multiple cameras (ultrawide, normal, superzoom, etc) into a single rear facing camera.

This updates change how zoom functions as well as how min and max zoom is set to be compliant with those updates as long as the user device is on android 11+.
Copy link
Contributor

@camsim99 camsim99 left a comment

Choose a reason for hiding this comment

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

Thank you for doing this! I see this method you used is preferred for Android 11+ so this is great! I just requested some changes and had some questions.

@@ -40,7 +40,7 @@ public void before() {
mockSensorArray = mock(Rect.class);

mockedStaticCameraZoom
.when(() -> ZoomUtils.computeZoom(anyFloat(), any(), anyFloat(), anyFloat()))
.when(() -> ZoomUtils.computeZoomRect(anyFloat(), any(), anyFloat(), anyFloat()))
.thenReturn(mockZoomArea);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we can add a test here that utilizes Robolectric to make sure the proper CaptureRequest is sent when updateBuilder is called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So Robolectric has some major issues mocking this call

mock(CaptureRequest.Builder.class);

Seems it can only be mocked using our standard test runner - checking the rest of the project nothing mocks this while using mockito.

I did however create a small method to help mock which version of the SDK_INT is running in our code and was able to write a few more tests using this to set our build SDK to R or Q depending on the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh gotcha. Okay thanks!

@@ -1,52 +0,0 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this and its corresponding test should be deleted, since this doesn't seem to be used anywhere. @stuartmorgan can you double check me on this? I'm not sure if you have more context.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it was replaced with ZoomLevelFeature during a refactor and the dead code just accidentally wasn't cleaned up.

@lucasoskorep lucasoskorep force-pushed the feat/allowCameraZoomToUseAllPhysicalCameras branch from a167025 to 90e04fc Compare August 5, 2022 04:03
Copy link
Contributor

@camsim99 camsim99 left a comment

Choose a reason for hiding this comment

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

Left a couple more comments, but looks good overall! @stuartmorgan or @bparrishMines pinging you for second review!

@@ -40,7 +40,7 @@ public void before() {
mockSensorArray = mock(Rect.class);

mockedStaticCameraZoom
.when(() -> ZoomUtils.computeZoom(anyFloat(), any(), anyFloat(), anyFloat()))
.when(() -> ZoomUtils.computeZoomRect(anyFloat(), any(), anyFloat(), anyFloat()))
.thenReturn(mockZoomArea);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh gotcha. Okay thanks!

@camsim99 camsim99 requested a review from stuartmorgan-g August 8, 2022 22:06
lucasoskorep and others added 3 commits August 8, 2022 18:15
…r/plugins/camera/features/zoomlevel/ZoomUtils.java

Co-authored-by: Camille Simon <[email protected]>
…icalCameras' into feat/allowCameraZoomToUseAllPhysicalCameras
@benkuper
Copy link

benkuper commented Aug 25, 2022

+1 ! This is a high priority necessity for me and it seems that the PR could go on ?
If not yet, how can I test this before the PR is merged ? Sorry, not a Flutter veteran so trying to understand how to use the modified package to test the functionality.
Thank you !

EDIT :
using the following seems to allow me to test the modified plugin, but not sure where to go from there, is there anything to add to the code or is the zoom detection / lens switch automatic ?

dependency_overrides:
  camera_android:
    git:
      dependency: transitive
      url: https://github.com/lucasoskorep/plugins.git
      ref: feat/allowCameraZoomToUseAllPhysicalCameras
      path: packages/camera/camera_android/

  camera:
    git:
      url: https://github.com/lucasoskorep/plugins.git
      ref: feat/allowCameraZoomToUseAllPhysicalCameras
      path: packages/camera/camera/ 

@lucasoskorep
Copy link
Contributor Author

+1 ! This is a high priority necessity for me and it seems that the PR could go on ? If not yet, how can I test this before the PR is merged ? Sorry, not a Flutter veteran so trying to understand how to use the modified package to test the functionality. Thank you !

EDIT : using the following seems to allow me to test the modified plugin, but not sure where to go from there, is there anything to add to the code or is the zoom detection / lens switch automatic ?

dependency_overrides:
  camera_android:
    git:
      dependency: transitive
      url: https://github.com/lucasoskorep/plugins.git
      ref: feat/allowCameraZoomToUseAllPhysicalCameras
      path: packages/camera/camera_android/

  camera:
    git:
      url: https://github.com/lucasoskorep/plugins.git
      ref: feat/allowCameraZoomToUseAllPhysicalCameras
      path: packages/camera/camera/ 

This is the fix I have for my application for now. Still pending @stuartmorgan's final approval though so sit tight for getting it from the main package itself!

As for functionality any android phone with a logical camera hiding multiple physical cameras will just return the correct min and max zoom values with the fix so no code changes outside of potentially how you are guarding min and max zoom values :)

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

Just a few minor nits, but otherwise looks good! (With the caveat that I'm not very familiar with the camera APIs, so I can't do a in-depth review on the substance of the change.)

@@ -1,52 +0,0 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it was replaced with ZoomLevelFeature during a refactor and the dead code just accidentally wasn't cleaned up.

@lucasoskorep
Copy link
Contributor Author

@stuartmorgan thanks for reviewing the PR! Got the requested changes added in and updated the version to +3 in pubspec and changelog.md so this PR would be up to date. Let me know if there is anything else you'd like added and I can get fixes set up for it :)

@GaryQian
Copy link
Contributor

GaryQian commented Jan 5, 2023

@stuartmorgan This seems to be ready for another look

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

Sorry for the really long delay getting back to this; I lost track of the review request :(

@stuartmorgan-g stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 17, 2023
@auto-submit auto-submit bot merged commit e85e0f2 into flutter:main Jan 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 18, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jan 18, 2023
* 6ef1bc1da Roll Flutter from 8c2fdb8 to cc7845e (2 revisions) (flutter/plugins#6983)

* 56ab33fd1 [shared_pref]: Bump mockito-inline (flutter/plugins#6976)

* e85e0f28f [camera] Allow logical cameras to use all physical cameras via zoom on android 11+ (flutter/plugins#6150)

* 11361d010 [camera] Use startVideoCapturing and expose concurrent stream/record (flutter/plugins#6815)
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
…#118682)

* 6ef1bc1da Roll Flutter from 8c2fdb8 to cc7845e (2 revisions) (flutter/plugins#6983)

* 56ab33fd1 [shared_pref]: Bump mockito-inline (flutter/plugins#6976)

* e85e0f28f [camera] Allow logical cameras to use all physical cameras via zoom on android 11+ (flutter/plugins#6150)

* 11361d010 [camera] Use startVideoCapturing and expose concurrent stream/record (flutter/plugins#6815)
mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023
…n android 11+ (flutter#6150)

* Adding support for the android 11+ Camera2 CONTROL_ZOOM_RATIO_RANGE camera preference. This preference is essential for supporting Logical cameras on the rear which after android 11 wrap multiple cameras (ultrawide, normal, superzoom, etc) into a single rear facing camera.

This updates change how zoom functions as well as how min and max zoom is set to be compliant with those updates as long as the user device is on android 11+.

* Adding updates to the changelog and pubspec to reflect the zoom and camera updates.

* comment cleanup for min and max zoom ratio functions

* Pull request fixes

* Update packages/camera/camera_android/CHANGELOG.md

Co-authored-by: Camille Simon <[email protected]>

* Update packages/camera/camera_android/android/src/main/java/io/flutter/plugins/camera/features/zoomlevel/ZoomUtils.java

Co-authored-by: Camille Simon <[email protected]>

* Updating comments from PR

* Fixing variable name formatting, and comment structures

* Fixing variable name formatting, and comment structures

* Autogormatter updates

Co-authored-by: Camille Simon <[email protected]>
Co-authored-by: stuartmorgan <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: camera platform-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants