[camera] Migrate to the new embedding #2165

mklim · 2019-10-08T00:00:24Z

Description

Migrates the camera plugin to the new embedding and adds an example
activity exercising it.

Unlike some of the previous PRs on this, this is the minimal migration
that would land this without adding any tech debt. From previous offline
discussions we'd like to scope these migrations to the bare minimum
required to keep the patches small. See #2114 for a migration combined
with a refactoring.

That said there are still some minor changes to prevent this from adding
additional tech debt, going a little beyond #2118:

#onMethodCall has been split up into its own MethodCallHandlerImpl
class to try and keep CameraPlugin from mixing concerns. Before the
migration, all it was doing was responding to method calls. Now
there's a significant amount of logic just based around responding to
lifecycle events. It still seemed better to split that into a separate
file than to try and manage that logic all in one place.

Other than the above refactorings, the original logic is untouched.

Related Issues

Fixes flutter/flutter#41834.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

cyanglaz

LGTM with some non-blocking comments

cyanglaz · 2019-10-08T20:54:19Z

packages/camera/android/src/main/java/io/flutter/plugins/camera/CameraPlugin.java

+/**
+ * Plugin implementation that uses the {@code io.flutter.embedding} package.
+ *
+ * <p>Instantiate this in an add to app scenario to gracefully handle activity and context changes.


nits(non-blocking): Is there a link can be provided here to explain more on how to use this in add to app scenario?

Reworded this.

packages/camera/android/src/main/java/io/flutter/plugins/camera/CameraPlugin.java

cyanglaz · 2019-10-08T21:05:37Z

packages/camera/example/android/app/src/main/AndroidManifest.xml

+    </activity>
+    <activity
+      android:configChanges="orientation|keyboardHidden|keyboard|screenSize|locale|layoutDirection"
+      android:exported="true"


i think it is supposed to be in the EmbeddingV1Activity

mehmetf · 2019-10-09T01:52:03Z

packages/camera/android/src/main/java/io/flutter/plugins/camera/CameraPlugin.java

+   */
  public static void registerWith(Registrar registrar) {
-    if (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP) {
+    if (registrar.activity() == null || Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP) {


We should talk about this. We recommend against this pattern because background vs foreground views are not distinguishable in most cases (a background view can switch to foreground).

Got it, I'll remove this and update the below comment. I saw the comment and noticed that the code didn't match it and assumed that there was a mistake. The code must have been patched at some point and the comment wasn't updated.

I thought about this a bit more and we also want to keep the logic the same on both the old and new plugin registration path. So I moved this guard into startListening and renamed that to maybeStartListening.

/cc @cyanglaz this means that now sometimes it is expected that when we call onDetached methodCallHandler is null after all. So I removed the Log.wtf from the guard clause there and replaced it with a comment explaining the known case where that could happen.

bparrishMines

LGTM

matthew-carroll · 2019-10-11T20:48:35Z

.../camera/example/android/app/src/main/java/io/flutter/plugins/cameraexample/MainActivity.java

+  public void configureFlutterEngine(@NonNull FlutterEngine flutterEngine) {
+    flutterEngine.getPlugins().add(new CameraPlugin());
+
+    ShimPluginRegistry shimPluginRegistry = new ShimPluginRegistry(flutterEngine);


It might be a good idea to register these first. Given that they're required by CameraPlugin, it is conceivable that CameraPlugin could immediately attempt to utilize them, in which case this would fail. So it looks like an implementation detail that this happens to work. But up to you.

That makes sense to me, but GeneratedPluginRegistrant registers them in this order. I thought it was better to match that.

Migrates the camera plugin to the new embedding and adds an example activity exercising it. DO NOT MERGE until we have an e2e testing solution and are clear to land this without having any issues with stable. However this is still ready for an initial review. Unlike some of the previous PRs on this, this is the minimal migration that would land this without adding any tech debt. From previous offline discussions we'd like to scope these migrations to the bare minimum required to keep the patches small. See #2114 for a migration combined with a refactoring. That said there are still some minor changes to prevent this from adding additional tech debt, unlike #2118: - The new code has been moved from `io` to `dev`. This is for consistency with the other migrated plugins. - `#onMethodCall` has been split up into its own `MethodCallHandlerImpl` class to try and keep `CameraPlugin` from mixing concerns. Before the migration, _all_ it was doing was responding to method calls. Now there's a significant amount of logic just based around responding to lifecycle events. It still seemed better to split that into a seperate file than to try and manage that logic all in one place. Other than the above refactorings, the original logic is untouched.

- Update version in pubspec and changelog. - Migrate tests to use e2e. Make them runnable (in at least one case). - Add dynamic Gradle include script.

mklim

The latest patch should add everything necessary for this to merge.

e2e tests (Only works on Android because I need to programmatically grant camera permissions to runt hem)
New flutter min SDK requirements
Pubspec increment/changelog
Dynamic gradle inclusion script

mklim · 2019-10-15T16:03:13Z

.../camera/example/android/app/src/main/java/io/flutter/plugins/cameraexample/MainActivity.java

+  public void configureFlutterEngine(@NonNull FlutterEngine flutterEngine) {
+    flutterEngine.getPlugins().add(new CameraPlugin());
+
+    ShimPluginRegistry shimPluginRegistry = new ShimPluginRegistry(flutterEngine);


That makes sense to me, but GeneratedPluginRegistrant registers them in this order. I thought it was better to match that.

PerLycke · 2019-10-25T12:20:20Z

@mklim After this merge using the camera plugin on a Note 10+ (and other devices) to stream images is useless. Everything freezes after a couple of seconds moving the camera around. Log shows this:

E/CameraCaptureSession( 5749): Session 1: Exception while stopping repeating: 
E/CameraCaptureSession( 5749): android.hardware.camera2.CameraAccessException: CAMERA_ERROR (3): The camera device has encountered a serious error
E/CameraCaptureSession( 5749): 	at android.hardware.camera2.impl.CameraDeviceImpl.checkIfCameraClosedOrInError(CameraDeviceImpl.java:2435)
E/CameraCaptureSession( 5749): 	at android.hardware.camera2.impl.CameraDeviceImpl.stopRepeating(CameraDeviceImpl.java:1126)
E/CameraCaptureSession( 5749): 	at android.hardware.camera2.impl.CameraCaptureSessionImpl.close(CameraCaptureSessionImpl.java:526)
E/CameraCaptureSession( 5749): 	at io.flutter.plugins.camera.Camera.closeCaptureSession(Camera.java:539)
E/CameraCaptureSession( 5749): 	at io.flutter.plugins.camera.Camera.close(Camera.java:545)
E/CameraCaptureSession( 5749): 	at io.flutter.plugins.camera.Camera$2.onError(Camera.java:203)
E/CameraCaptureSession( 5749): 	at android.hardware.camera2.impl.CameraDeviceImpl$CameraDeviceCallbacks.notifyError(CameraDeviceImpl.java:1929)
E/CameraCaptureSession( 5749): 	at android.hardware.camera2.impl.CameraDeviceImpl$CameraDeviceCallbacks.lambda$Sm85frAzwGZVMAK-NE_gwckYXVQ(Unknown Source:0)
E/CameraCaptureSession( 5749): 	at android.hardware.camera2.impl.-$$Lambda$CameraDeviceImpl$CameraDeviceCallbacks$Sm85frAzwGZVMAK-NE_gwckYXVQ.accept(Unknown Source:8)
E/CameraCaptureSession( 5749): 	at com.android.internal.util.function.pooled.PooledLambdaImpl.doInvoke(PooledLambdaImpl.java:258)
E/CameraCaptureSession( 5749): 	at com.android.internal.util.function.pooled.PooledLambdaImpl.invoke(PooledLambdaImpl.java:182)
E/CameraCaptureSession( 5749): 	at com.android.internal.util.function.pooled.OmniFunction.run(OmniFunction.java:77)
E/CameraCaptureSession( 5749): 	at android.os.Handler.handleCallback(Handler.java:873)
E/CameraCaptureSession( 5749): 	at android.os.Handler.dispatchMessage(Handler.java:99)
E/CameraCaptureSession( 5749): 	at android.os.Looper.loop(Looper.java:216)
E/CameraCaptureSession( 5749): 	at android.app.ActivityThread.main(ActivityThread.java:7263)
E/CameraCaptureSession( 5749): 	at java.lang.reflect.Method.invoke(Native Method)
E/CameraCaptureSession( 5749): 	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:494)
E/CameraCaptureSession( 5749): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:975)
E/BufferQueueProducer( 5749): [ImageReader-1280x720f23m2-5749-1] dequeueBuffer: attempting to exceed the max dequeued buffer count (9)

mklim · 2019-10-25T23:58:57Z

Thanks for reporting the issue and the stack trace @PerLycke, sorry about the regression. Do you mind also posting steps on how to reproduce the crash? A small test case would help too if the example app isn't enough to cause it.

PerLycke · 2019-10-26T09:32:48Z

@mklim Never mind my report, after some additional debugging it seems like it's device specific to the Note 10+, and not connected to this commit in particular. Sorry for the confusion, and keep up the good work.

mklim · 2019-10-28T16:46:20Z

@PerLycke no problem, thanks for debugging this further too. Looks like the Note 10+ issue is now being tracked at flutter/flutter#43566.

Migrates the camera plugin to the new embedding and adds an example activity exercising it. Unlike some of the previous PRs on this, this is the minimal migration that would land this without adding any tech debt. From previous offline discussions we'd like to scope these migrations to the bare minimum required to keep the patches small. See flutter#2114 for a migration combined with a refactoring. That said there are still some minor changes to prevent this from adding additional tech debt, going a little beyond flutter#2118: - `#onMethodCall` has been split up into its own `MethodCallHandlerImpl` class to try and keep `CameraPlugin` from mixing concerns. Before the migration, _all_ it was doing was responding to method calls. Now there's a significant amount of logic just based around responding to lifecycle events. It still seemed better to split that into a separate file than to try and manage that logic all in one place. Other than the above refactor, the original logic is untouched.

mklim requested review from bparrishMines and matthew-carroll October 8, 2019 00:00

googlebot added the cla: yes label Oct 8, 2019

cyanglaz approved these changes Oct 8, 2019

View reviewed changes

mehmetf reviewed Oct 9, 2019

View reviewed changes

bparrishMines approved these changes Oct 9, 2019

View reviewed changes

matthew-carroll reviewed Oct 11, 2019

View reviewed changes

matthew-carroll approved these changes Oct 11, 2019

View reviewed changes

Michael Klimushyn added 4 commits October 14, 2019 17:03

Naming changes from offline discussion.

479e359

Review feedback

3687742

Last migration fixes

a0ba4ff

- Update version in pubspec and changelog. - Migrate tests to use e2e. Make them runnable (in at least one case). - Add dynamic Gradle include script.

mklim commented Oct 15, 2019

View reviewed changes

mklim merged commit ba5c774 into flutter:master Oct 15, 2019

mklim deleted the camera_v2_embedder_migration branch October 15, 2019 18:21

[camera] Migrate to the new embedding #2165

[camera] Migrate to the new embedding #2165

Uh oh!

Conversation

mklim commented Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Breaking Change

Uh oh!

cyanglaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bparrishMines left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mklim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PerLycke commented Oct 25, 2019

Uh oh!

mklim commented Oct 25, 2019

Uh oh!

PerLycke commented Oct 26, 2019

Uh oh!

mklim commented Oct 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mklim commented Oct 8, 2019 •

edited

Loading