[camerax] Implement lockCaptureOrientation & unlockCaptureOrientation
#5285
Conversation
| @@ -708,7 +779,7 @@ class AndroidCameraCameraX extends CameraPlatform { | |||
| width: imageProxy.width); | |||
|
|
|||
| weakThis.target!.cameraImageDataStreamController!.add(cameraImageData); | |||
| unawaited(imageProxy.close()); | |||
| await imageProxy.close(); | |||
There was a problem hiding this comment.
Part of flutter/flutter#132499
| Future<void> _onFrameStreamListen(int cameraId) async { | ||
| await _configureImageAnalysis(cameraId); | ||
| } | ||
|
|
There was a problem hiding this comment.
Just changed _onFrameStreamListen reference to _configureImageAnalysis.
| /// Should be specified in terms of one of the [Surface] | ||
| /// rotation constants that represents the counter-clockwise degrees of | ||
| /// rotation relative to [DeviceOrientation.portraitUp]. | ||
| final int? initialTargetRotation; |
There was a problem hiding this comment.
Reflecting on this now and wondering if we should even save the target rotation for UseCases and instead expose methods like https://developer.android.com/reference/androidx/camera/core/ImageCapture#getTargetRotation() that allow us to retrieve the target rotation for each of them. (this is a side effect of these wrapper classes needing to be immutable)
@gmackall lmk what you think. I would probably do this in a follow up PR because this has a lot going on already. Running into a similar thing in the last wrappings I need to do and want to take a similar path.
There was a problem hiding this comment.
I support replacing with the native method, but am also fine with it being a follow up!
There was a problem hiding this comment.
Ok great! Filed flutter/flutter#140664 to follow up.
packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart
Outdated
Show resolved
Hide resolved
| final int targetLockedRotation = | ||
| _getRotationConstantFromDeviceOrientation(orientation); | ||
|
|
||
| // Update UseCases to use target device orientation. |
There was a problem hiding this comment.
The Preview usecase also has a method to set the target rotation, should we be setting that as well?
https://developer.android.com/reference/kotlin/androidx/camera/core/Preview#setTargetRotation(int)
There was a problem hiding this comment.
I don't believe so since this is only supposed to impact the capture orientation. The preview rotation should be managed by the Flutter app using the plugin like, for example, our example app.
| } else { | ||
| return Configuration.ORIENTATION_PORTRAIT; | ||
| } | ||
| int getDefaultRotation() { |
There was a problem hiding this comment.
nit: I believe this method is named getDefaultRotation because it is the rotation that CameraX use cases (at least ImageCapture) default to right? I think we might want to name it something else, though, because without that context from the comment here, it mostly gets used in places where it looks like we are getting a default rotation of the camera proxy, (i.e. proxy.getDefaultRotation()) which at first glance doesn't seem like something that would be a function of the current rotation of the device, but rather a persistent default rotation.
Basically it just feels more like a getCurrentRotation than a getDefaultRotation, to me at least. Not a big deal, though.
There was a problem hiding this comment.
To address this, I renamed the wrapped method to getDefaultDisplayRotation() and left this since this really a part of the DeviceOrientationManager and would like to keep it consistent with camera_android for now.
| // Set target rotation to current photo orientation only if capture | ||
| // orientation not locked. | ||
| if (!captureOrientationLocked && shouldSetDefaultRotation) { | ||
| await imageCapture!.setTargetRotation(await proxy.getDefaultRotation()); |
There was a problem hiding this comment.
In testing, does this achieve the same behavior between "plugin-managed" unlocked case (i.e. lock->unluck), and the "camerax" managed unlocked case? I'm curious because after reading the documentation on ImageCapture's setTargetRotation, particularly:
If no target rotation is set by the application, it is set to the value of getRotation of the default display at the time the use case is bound.
To me this sounds like we would be doing "rotation of default display at time of photo", while camerax is doing "rotation of default display at time of use case creation", which sounds like it would be different if the phone was rotated after creation. Am I misinterpreting the docs, or do the docs not line up with what happens when we test the use cases?
(this comment applies to how we manage the rotation for all use cases, I just put it here as an example)
There was a problem hiding this comment.
My understanding is that the default display does not change anywhere in the plugin and the rotation of that display always will return the rotation needed to return to its "natural" orientation (https://developer.android.com/reference/android/view/Display.html#getRotation()). From my testing, this yields the desired result of matching camera_android behavior.
This makes sense to me because my interpretation of "To return to the default value, set the value to context.getSystemService(WindowManager.class).getDefaultDisplay().getRotation();" was that you should be able to do that at anytime to have the UseCase return to its default rotation by CameraX standards. I think there would be a concern here if the default display were changing between binding UseCases and setting the target rotation, but like I mentioned, I don't believe it should be a concern of ours.
| /// Should be specified in terms of one of the [Surface] | ||
| /// rotation constants that represents the counter-clockwise degrees of | ||
| /// rotation relative to [DeviceOrientation.portraitUp]. | ||
| final int? initialTargetRotation; |
There was a problem hiding this comment.
I support replacing with the native method, but am also fine with it being a follow up!
packages/camera/camera_android_camerax/lib/src/image_capture.dart
Outdated
Show resolved
Hide resolved
camsim99
added
autosubmit
camsim99
added
the
autosubmit
flutter/packages@fa74d3e...bbb4134 2024-01-02 [email protected] [camerax] Implement `lockCaptureOrientation` & `unlockCaptureOrientation` (flutter/packages#5285) 2024-01-02 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.22.11 to 3.22.12 (flutter/packages#5738) 2024-01-02 [email protected] Add Flutter CI Status badge to README (flutter/packages#5733) 2023-12-26 49699333+dependabot[bot]@users.noreply.github.com [path_provider]: Bump androidx.annotation:annotation from 1.7.0 to 1.7.1 in /packages/path_provider/path_provider_android/android (flutter/packages#5709) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Implements
lockCaptureOrientation&unlockCaptureOrientationfor all cameraUseCases.Also fixes small bug concerning not initially setting the target rotation of the
UseCases to the requested sensor orientation whencreateCamerais called.Fixes flutter/flutter#125915.
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).