-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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.
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
[camerax] Implements torch mode #4903
Conversation
@@ -81,7 +82,8 @@ public void setUp( | |||
GeneratedCameraXLibrary.LiveDataHostApi.setup(binaryMessenger, liveDataHostApiImpl); | |||
GeneratedCameraXLibrary.ObserverHostApi.setup( | |||
binaryMessenger, new ObserverHostApiImpl(binaryMessenger, instanceManager)); | |||
imageAnalysisHostApiImpl = new ImageAnalysisHostApiImpl(binaryMessenger, instanceManager); | |||
imageAnalysisHostApiImpl = | |||
new ImageAnalysisHostApiImpl(binaryMessenger, instanceManager, context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR also makes a fix to this implementation by adding a Context
to the ImageAnalysisHostApiImpl
constructor to remove an unnecessary call to update the context for the Host API implementations after running this method also setting the Context
for those implementations (see line 131).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://github.com/flutter/packages/pull/4903/files#r1323473452 for my reasoning behind these changes.
After revisiting https://developer.android.com/training/camerax/configuration#torch, I realize I could get the torch state as well as whether or not the device has a flash unit and use that info in this PR versus tracking the torch state myself and not throwing an error/warning when torch state is not possible. If folks have strong opinions, I can fix in this PR; otherwise, I'll file an issue and fix this after I finish feature parity. |
@HostApi(dartHostTestHandler: 'TestCameraControlHostApi') | ||
abstract class CameraControlHostApi { | ||
@async | ||
void enableTorch(int identifier, bool torch); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bparrishMines If the Result
this returns ends up with an error, would I be able to catch this from the Dart side and throw an error there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, an exception on the Java side should cause a PlatformException
error on the Dart side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to modify this PR to catch PlatformException
s that may be cause by CameraX failing to turn on torch mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM, just have one question about how flash modes and torch modes interact with each other.
packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart
Show resolved
Hide resolved
@gmackall Made the fix on your comment and added a test for it and also added error handling for failure to change torch state in the latest commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, minus some requests for explanation in the documentation of setFlashMode
!
@@ -491,6 +500,14 @@ class AndroidCameraCameraX extends CameraPlatform { | |||
/// Sets the flash mode for the selected camera. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Might be worth adding to the documentation here that this method combines the notion of flash and torch from
https://developer.android.com/reference/androidx/camera/core/ImageCapture and
https://developer.android.com/reference/androidx/camera/core/CameraControl#enableTorch(boolean).
// Turn off torch mode if it is enabled and not being redundantly set. | ||
if (mode != FlashMode.torch && torchEnabled) { | ||
cameraControl = await camera!.getCameraControl(); | ||
await cameraControl.enableTorch(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it still might be worth adding a note to the documentation here that points out the difference between this method and the underlying CameraX apis (specifically just that torch and flash can't be set independently in this plugin, like they can with the CameraX apis).
So something like
setFlashMode(id, FlashMode.torch)
takePicture(id)
setFlashMode(id, FlashMode.auto)
takePicture(id)
would behave differently in the plugin (torch disabled, flash mode on auto) from what someone familiar with the native apis would expect (torch enabled, flash mode on auto).
flutter/packages@4b483f2...93c3f69 2023-10-12 goderbauer@google.com [shared_preferences] update file version constraints (flutter/packages#5121) 2023-10-12 supercionci@hotmail.it [cross_file] Improved documentation about ignored parameters in IO module. (flutter/packages#4416) 2023-10-11 41873024+droidbg@users.noreply.github.com [in_app_purchase] [#135759] Fix. doc reference finishPurchase to completePurchase. (flutter/packages#5081) 2023-10-10 zeucxb@gmail.com Update repo README issues link (flutter/packages#5106) 2023-10-10 43643339+nohli@users.noreply.github.com [flutter_markdown] Fix typo in readme (flutter/packages#5096) 2023-10-10 43054281+camsim99@users.noreply.github.com [camerax] Implements torch mode (flutter/packages#4903) 2023-10-10 25266387+Leptopoda@users.noreply.github.com [shared_preferences] fix documentation wording (flutter/packages#4986) 2023-10-10 katelovett@google.com [two_dimensional_scrollables] Fix paint bug when rows/columns are pinned and axes are reversed (flutter/packages#5038) 2023-10-10 engine-flutter-autoroll@skia.org Roll Flutter from f52fe4f to 83134ac (25 revisions) (flutter/packages#5104) 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 flutter-ecosystem@google.com,rmistry@google.com 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 the torch flash mode. Also wraps classes necessary for the implementation (a method in `Camera`, `CameraControl`). Fixes flutter/flutter#120715. Fixes flutter/flutter#115846. Part of flutter/flutter#115847.
Implements the torch flash mode. Also wraps classes necessary for the implementation (a method in
Camera
,CameraControl
).Fixes flutter/flutter#120715.
Fixes flutter/flutter#115846.
Part of flutter/flutter#115847.
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).