[camerax] Implement video capture for CameraX android camera re-write #3467

gmackall · 2023-03-15T19:01:11Z

This PR implements video capture for the CameraX re-write of the android camera plugin.

Pre-launch Checklist

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

packages/camera/camera_android_camerax/CHANGELOG.md

camsim99 · 2023-03-17T17:15:05Z

...id_camerax/android/src/main/java/io/flutter/plugins/camerax/PendingRecordingHostApiImpl.java

+    recordingFlutterApi = new RecordingFlutterApiImpl(binaryMessenger, instanceManager);
+  }
+
+  public void setContext(Context context) {


Can you add javadoc explaining why this is necessary?

For all public methods, I suggest we do this. Private is optional but could be useful for methods that are not a direct wrapping of whatever call to CameraX.

Is there guidance about if this is an application context or an activity context? My guess is activity.

camsim99 · 2023-03-17T17:17:48Z

...id_camerax/android/src/main/java/io/flutter/plugins/camerax/PendingRecordingHostApiImpl.java

+  @Override
+  public Long start(@NonNull Long identifier) {
+    PendingRecording pendingRecording = getPendingRecordingFromInstanceId(identifier);
+    Recording recording = pendingRecording.start(this.getExecutor(), this::handleVideoRecordEvent);


:: was introduced in Java 8 and we have to stick to things available in Java 7 to keep things consistent with the internal version of this plugin.

camsim99 · 2023-03-17T17:20:22Z

...camerax/android/src/main/java/io/flutter/plugins/camerax/PendingRecordingFlutterApiImpl.java

+    this.instanceManager = instanceManager;
+  }
+
+  void create(PendingRecording pendingRecording, Reply<Void> reply) {


Nit: add nullable/nonnull annotations here

camsim99 · 2023-03-17T17:21:01Z

...android_camerax/android/src/main/java/io/flutter/plugins/camerax/RecorderFlutterApiImpl.java

+  }
+
+  void create(
+      Recorder recorder, @Nullable Long aspectRatio, @Nullable Long bitRate, Reply<Void> reply) {


Nit: add @NonNull

I see some other missing ones so just reminder to add them throughout the Java files!

camsim99 · 2023-03-17T17:51:41Z

packages/camera/camera_android_camerax/lib/src/system_services.dart

@@ -30,6 +30,9 @@ class SystemServices {
  static final StreamController<String> cameraErrorStreamController =
      StreamController<String>.broadcast();

+  static final StreamController<CameraEvent> cameraEventStreamController =


Remove this?

packages/camera/camera_android_camerax/pigeons/camerax_library.dart

camsim99 · 2023-03-17T17:59:40Z

...ra/camera_android_camerax/android/src/test/java/io/flutter/plugins/camerax/RecorderTest.java

+    when(mockRecorderBuilder.setExecutor(any(Executor.class))).thenReturn(mockRecorderBuilder);
+    when(mockRecorderBuilder.build()).thenReturn(mockRecorder);
+
+    recorderHostApi.create(0L, 1L, 2L);


For constants that matter in testing, we opted for defining constants and just converting inline between int and Long.

...era_android_camerax/android/src/test/java/io/flutter/plugins/camerax/SystemServicesTest.java

reidbaker · 2023-03-28T19:48:10Z

packages/camera/camera_android_camerax/example/lib/main.dart

-              cameraController == null ? null :
-              (cameraController.value.isRecordingPaused
-                  ? onResumeButtonPressed : onPauseButtonPressed),
+          onPressed: cameraController == null


Nested ternary operators can be difficult to read. Can you break this apart?

reidbaker · 2023-03-29T14:32:49Z

packages/camera/camera_android_camerax/lib/src/system_services.dart

@@ -61,6 +61,14 @@ class SystemServices {

    api.stopListeningForDeviceOrientationChange();
  }
+
+  /// Gets the current cache directory path from context.


Can you expand a bit on this documentation? For example what would change the current cache directory, who else uses the current cache directory, are there constraints on its use or the files that can be put in it.

camsim99

Looking good! I left a comment on one main concern about using aspect ratio instead of resolution (I think we actually can use resolution), but otherwise, just requests for some tests and nits.

camsim99 · 2023-04-20T16:35:13Z

...oid_camerax/android/src/main/java/io/flutter/plugins/camerax/CameraAndroidCameraxPlugin.java

@@ -20,6 +20,9 @@ public final class CameraAndroidCameraxPlugin implements FlutterPlugin, Activity
  private ProcessCameraProviderHostApiImpl processCameraProviderHostApi;
  private ImageCaptureHostApiImpl imageCaptureHostApi;
  public SystemServicesHostApiImpl systemServicesHostApi;
+  private PendingRecordingHostApiImpl pendingRecordingHostApi;
+  private RecorderHostApiImpl recorderHostApi;
+  private VideoCaptureHostApiImpl videoCaptureHostApi;


Nit: I decided to rename these to ApiImpl in #3419. Can you change these to keep it consistent?

...ra/camera_android_camerax/android/src/main/java/io/flutter/plugins/camerax/CameraXProxy.java

camsim99 · 2023-04-20T16:41:11Z

...camerax/android/src/main/java/io/flutter/plugins/camerax/PendingRecordingFlutterApiImpl.java

+  }
+
+  void create(@NonNull PendingRecording pendingRecording, @Nullable Reply<Void> reply) {
+    create(instanceManager.addHostCreatedInstance(pendingRecording), reply);


Side note: I think I'll file an issue for general cleanup of this plugin, including making sure we are consistent about checking whether or not an instance has been added to the instance manager before creating it in the FlutterApiImpl or where it's used in in a HostApiImpl. I know with the generated wrapped classes, it checks in the former, but we both have done it in the latter.

Added to flutter/flutter#125926

camsim99 · 2023-04-20T16:43:57Z

...id_camerax/android/src/main/java/io/flutter/plugins/camerax/PendingRecordingHostApiImpl.java

+  public Long start(@NonNull Long identifier) {
+    PendingRecording pendingRecording = getPendingRecordingFromInstanceId(identifier);
+    Recording recording =
+        pendingRecording.start(this.getExecutor(), x -> handleVideoRecordEvent(x));


Nit: can you rename x to something like event for clarity?

camsim99 · 2023-04-20T16:44:32Z

...id_camerax/android/src/main/java/io/flutter/plugins/camerax/PendingRecordingHostApiImpl.java

+    return ContextCompat.getMainExecutor(context);
+  }
+
+  private void handleVideoRecordEvent(VideoRecordEvent event) {


Nit: add javadoc

camsim99 · 2023-04-20T20:14:46Z

packages/camera/camera_android_camerax/lib/src/video_capture.dart

+    return instanceManager.getInstanceWithWeakReference(videoCaptureId)!
+        as VideoCapture;


I have received lint errors by using this instead of:

Suggested change

return instanceManager.getInstanceWithWeakReference(videoCaptureId)!

as VideoCapture;

return instanceManager.getInstanceWithWeakReference<VideoCapture>(videoCaptureId)!;

packages/camera/camera_android_camerax/test/android_camera_camerax_test.dart

camsim99 · 2023-04-20T20:20:04Z

packages/camera/camera_android_camerax/test/pending_recording_test.dart

+    when(mockApi.start(pendingRecordingId)).thenReturn(mockRecordingId);
+    expect(await pendingRecording.start(), mockRecording);
+    verify(mockApi.start(pendingRecordingId));
+  });


Can you also add a test for the Flutter API create method?

@camsim99 can you resolve this if you are happy with the test added?

Nit: can you make the test title more specific? Otherwise, lgtm

packages/camera/camera_android_camerax/test/recording_test.dart

packages/camera/camera_android_camerax/test/video_capture_test.dart

camsim99

LGTM!

camsim99 · 2023-05-02T16:33:26Z

...ra_android_camerax/android/src/main/java/io/flutter/plugins/camerax/RecorderHostApiImpl.java

+  public void create(@NonNull Long instanceId, @Nullable Long aspectRatio, @Nullable Long bitRate) {
+    Recorder.Builder recorderBuilder = cameraXProxy.createRecorderBuilder();
+    if (aspectRatio != null) {
+      recorderBuilder.setAspectRatio(aspectRatio.intValue());


We talked offline and decided to implement this in a later PR. This will fall under flutter/flutter#120462

camsim99 · 2023-05-02T16:35:12Z

...a_android_camerax/android/src/test/java/io/flutter/plugins/camerax/PendingRecordingTest.java

+    final Long mockRecordingId = testInstanceManager.addHostCreatedInstance(mockRecording);
+    testInstanceManager.addDartCreatedInstance(mockPendingRecording, mockPendingRecordingId);
+
+    doReturn(mockRecording).when(mockPendingRecording).start(any(), any());


Looks good!

...era/camera_android_camerax/android/src/test/java/io/flutter/plugins/camerax/PreviewTest.java

reidbaker

partial review.

reidbaker · 2023-05-02T20:06:35Z

packages/camera/camera_android_camerax/pigeons/camerax_library.dart

@@ -161,6 +163,60 @@ abstract class PreviewHostApi {
  ResolutionInfo getResolutionInfo(int identifier);
 }

+@HostApi(dartHostTestHandler: 'TestVideoCaptureHostApi')
+abstract class VideoCaptureHostApi {
+  int withOutput(int videoOutputId);


non blocking: is there a reason there are no dart docs on any of these objects?

FWIW, he's following a pattern I started. We should consider copying the documentation from the wrapped classes to these objects in a future cleanup PR.

reidbaker · 2023-05-02T20:09:20Z

packages/camera/camera_android_camerax/lib/src/video_capture.dart

+      {BinaryMessenger? binaryMessenger, InstanceManager? instanceManager})
+      : super.detached(
+            binaryMessenger: binaryMessenger,
+            instanceManager: instanceManager) {


In many of @camsim99 prs I see null instance managers overridden by globals.instanceManger is that something that should happen here. If not why not?

I missed this -- we should do this here too!

The instance manager here in VideoCapture is only being used in the constructor of the VideoCaptureHostApiImpl, and in that constructor we override to the global instance manager. This is the same pattern that is being used elsewhere, for examples see the Preview constructor vs the PreviewHostApiImpl constructor.

@camsim99 or @gmackall can one of you add to your growing clean up bug to make this pattern consistent. If we cant keep it straight then neither can first time contributors.

I believe the pattern here is consistent.

In the Thing constructor, we pass in an InstanceManager? instanceManager, and then use that to create a ThingHostApiImpl, and then in the ThingHostApiImpl constructor we check if that instanceManager we passed is null and override to the global if so.

Actually, I see that some of the newer wrapped classes like zoom state and exposure state do not create these HostApiImpls in their detached constructors. I'll file an issue to ensure the detached versions of the constructors all align on whether to create or not (probably not, given the point is that they are not attached to a native object and therefore do not need to call host api methods).

flutter/flutter#126293

packages/camera/camera_android_camerax/lib/src/video_capture.dart

reidbaker · 2023-05-02T20:11:15Z

packages/camera/camera_android_camerax/lib/src/video_capture.dart

+    AndroidCameraXCameraFlutterApis.instance.ensureSetUp();
+  }
+
+  /// Creates a VideoCapture associated with the given [Recorder].


Suggested change

/// Creates a VideoCapture associated with the given [Recorder].

/// Creates a [VideoCapture] associated with the given [Recorder].

packages/camera/camera_android_camerax/lib/src/video_capture.dart

packages/camera/camera_android_camerax/lib/src/system_services.dart

reidbaker · 2023-05-02T20:24:39Z

packages/camera/camera_android_camerax/test/android_camera_camerax_test.dart

@@ -471,6 +484,83 @@ void main() {
    expect(previewTexture.textureId, equals(textureId));
  });

+  test(


Would it make sense make these tests share a group?

Made them share a group in the new PR

reidbaker · 2023-05-02T20:25:31Z

packages/camera/camera_android_camerax/test/android_camera_camerax_test.dart

+    verifyNoMoreInteractions(recording);
+  });
+
+  test('resumeVideoRecording resumes the recording', () async {


Does this test not need to pause the recording first? If not would that be a valuable test?

Because this dart unit test is using a MockRecording, and the dart Recording class (and therefore the mock recording object) doesn't on its own have any tracking of an internal state without a real corresponding native Recording, it didn't seem to me like it mattered here.

Does that make sense?

reidbaker · 2023-05-02T20:26:19Z

packages/camera/camera_android_camerax/test/android_camera_camerax_test.dart

Are there error cases around recording that should be tested. For example calling stop twice in a row?

This sort of testing is good, and likely the sort we should use for several other methods tested here. Not sure this should block this PR, though, so if you don't get to it, please add it here: flutter/flutter#125928

blocking: I do think that some error testing should be required before merging even if the test is that we we swallow the error and don't crash (or that we crash and the crash is part of the documented behavior)

For example calling stop twice in a row?

This action currently throws a CameraException. I'll add a test for it.

Added a test for

if the recording is null (we haven't started a recording). In this case we throw a CameraException

if the videoOutputPath is null (we shouldn't generally reach this case). In this case we throw a CameraException

We call stop on a valid recording, and then we call it again (in this case we test everything performs normally for the first call to stop, and then we throw a CameraException on the second call).

Also added a logic and a test for calling recording twice (we silently do nothing in this case, based on a check of if the class variable recording is null or not).

reidbaker · 2023-05-02T20:29:47Z

packages/camera/camera_android_camerax/lib/src/recording.dart

+
+  /// Closes the specified recording instance.
+  Future<void> closeFromInstance(Recording recording) async {
+    close(instanceManager.getIdentifier(recording)!);


I dont think we should use a bang here or in the methods below. If a recording call fails I wouldn't expect an app crash and I don't see anything in the documentation about catching null pointer exceptions if we cant find an identifier.

Is this not the same behavior as in other usecases? See takePictureFromInstance for example. My understanding is that the assert statements only have effect in a debugging context, but in release use this would behave in the same way with an app crash?

Is that understanding correct/should we introduce a common error type or pattern here to replace the bang usages?

I agree it may be a useful tech debt item to include a more helpful error message, but hitting this means most likely someone adding new wrapped classes made an error or maybe someone modifying the plugin implementation did. It won't crash the app, either, so leaving it like this should be ok for now.

reidbaker · 2023-05-03T15:28:57Z

before continuing my pass I need the cla to be figured out.

reidbaker

Final approval is up for @camsim99 to decide. I know there is a balance to quality and speed we are trying to hit.

reidbaker · 2023-05-04T19:39:42Z

...roid_camerax/android/src/main/java/io/flutter/plugins/camerax/SystemServicesHostApiImpl.java

+  /** Returns a path to be used to create a temp file in the current cache directory. */
+  @Override
+  @NonNull
+  public String getTempFilePath(@NonNull String prefix, @NonNull String suffix) {


nit: filePrefux and fileSuffix, as opposed to path or some other definition. Also acceptable would be updating the documentation.
you also might consider making these named parameters for call site readability.

reidbaker · 2023-05-04T19:40:48Z

...roid_camerax/android/src/main/java/io/flutter/plugins/camerax/SystemServicesHostApiImpl.java

+    try {
+      File path = File.createTempFile(prefix, suffix, context.getCacheDir());
+      return path.toString();
+    } catch (IOException | SecurityException e) {


If you know when these might be expected consider adding a comment.

reidbaker · 2023-05-04T19:41:05Z

...roid_camerax/android/src/main/java/io/flutter/plugins/camerax/SystemServicesHostApiImpl.java

+      File path = File.createTempFile(prefix, suffix, context.getCacheDir());
+      return path.toString();
+    } catch (IOException | SecurityException e) {
+      throw new RuntimeException(e);


Should this be a typed exception?

At the time it was written, it was only possible to use RuntimeException here (the general pattern was that this method is wrapped in a try catch anyways in GeneratedCameraXLibrary, and then GeneratedCameraXLibrary would throw a PlatformException).

Since updating to pigeon 9.10, we can throw FlutterError instead (there is some discussion about the old pattern vs new FlutterError on this old PR). I will change here to throw a FlutterError.

reidbaker · 2023-05-04T19:43:18Z

...a_android_camerax/android/src/main/java/io/flutter/plugins/camerax/RecordingHostApiImpl.java

+    this.instanceManager = instanceManager;
+  }
+
+  @Override


Nit: consider ordering these methods in the order they will likely be called or into related pairs.

reidbaker · 2023-05-04T19:51:42Z

packages/camera/camera_android_camerax/lib/src/system_services.dart

+  /// File.createTempFile(prefix, suffix, cacheDir), on the android side, where
+  /// where cacheDir is the cache directory identified by the current application
+  /// context using context.getCacheDir().
+  static Future<String> getTempFilePath(String prefix, String suffix,


I think from the tests I read prefix is actually the file name and suffix is the extension. That came as a bit of a suprise from reading the documentation I would have expected prefix<something the caller does not control>suffix for the file name.

For example it means as a caller I think this means I need to ensure that I pass something different each time or risk overwriting the previous file.

I'll clarify the documentation before merging, the way it actually works is that the file ends up being
prefix<something the caller does not control>.suffix, i.e. 'suffix' is the file extension, and prefix is part of the file name, but more gets added by createTempFile (a random sequence of numbers).

So getTempFile('MOV', '.temp'), is something like <cachedir>/MOV1231242244.temp (notably this means you do not have to pass different things to avoid overwriting).

FWIW I chose prefix and suffix because those are the variable names used by the native methods implementation/documentation.

OK in that case it means the documentation is correct but that is not what the tests are testing for. I think using the same names as the native code is a reasonable choice.

Specifically in packages/camera/camera_android_camerax/test/system_services_test.dart and in packages/camera/camera_android_camerax/android/src/test/java/io/flutter/plugins/camerax/SystemServicesTest.java you are testing for the result to be equal totestPath + testPrefix + testSuffix (or the file for prefix and suffix). Instead of testing that the output had a path of testPath a file prefix of testPrefix and an extension of testSuffix. Which is fine it is just confusing when trying to understand what to expect from the tests.

reidbaker · 2023-05-04T19:54:27Z

packages/camera/camera_android_camerax/test/pending_recording_test.dart

+    when(mockApi.start(pendingRecordingId)).thenReturn(mockRecordingId);
+    expect(await pendingRecording.start(), mockRecording);
+    verify(mockApi.start(pendingRecordingId));
+  });


@camsim99 can you resolve this if you are happy with the test added?

camsim99

Approving to unblock this. I left comments on things I think can be done later, but otherwise agree with the rest of the review comments left!

camsim99 · 2023-05-04T20:15:35Z

...camerax/android/src/main/java/io/flutter/plugins/camerax/PendingRecordingFlutterApiImpl.java

+  }
+
+  void create(@NonNull PendingRecording pendingRecording, @Nullable Reply<Void> reply) {
+    create(instanceManager.addHostCreatedInstance(pendingRecording), reply);


Added to flutter/flutter#125926

camsim99 · 2023-05-04T20:20:48Z

packages/camera/camera_android_camerax/lib/src/recording.dart

+
+  /// Closes the specified recording instance.
+  Future<void> closeFromInstance(Recording recording) async {
+    close(instanceManager.getIdentifier(recording)!);


I agree it may be a useful tech debt item to include a more helpful error message, but hitting this means most likely someone adding new wrapped classes made an error or maybe someone modifying the plugin implementation did. It won't crash the app, either, so leaving it like this should be ok for now.

packages/camera/camera_android_camerax/lib/src/video_capture.dart

camsim99 · 2023-05-04T20:24:00Z

packages/camera/camera_android_camerax/pigeons/camerax_library.dart

@@ -161,6 +163,60 @@ abstract class PreviewHostApi {
  ResolutionInfo getResolutionInfo(int identifier);
 }

+@HostApi(dartHostTestHandler: 'TestVideoCaptureHostApi')
+abstract class VideoCaptureHostApi {
+  int withOutput(int videoOutputId);


FWIW, he's following a pattern I started. We should consider copying the documentation from the wrapped classes to these objects in a future cleanup PR.

camsim99 · 2023-05-04T20:26:16Z

packages/camera/camera_android_camerax/test/android_camera_camerax_test.dart

This sort of testing is good, and likely the sort we should use for several other methods tested here. Not sure this should block this PR, though, so if you don't get to it, please add it here: flutter/flutter#125928

camsim99 · 2023-05-04T20:26:57Z

packages/camera/camera_android_camerax/test/pending_recording_test.dart

+    when(mockApi.start(pendingRecordingId)).thenReturn(mockRecordingId);
+    expect(await pendingRecording.start(), mockRecording);
+    verify(mockApi.start(pendingRecordingId));
+  });


Nit: can you make the test title more specific? Otherwise, lgtm

…ng test to cover case. Also change to null videoOutputPath error case and test

gmackall · 2023-05-08T21:50:10Z

To make reviewing easier (because this has been updated a lot), since your last review @reidbaker the summary of changes is:

Added logic in startVideoRecording to explicitly do nothing if recording is not null (there is an active recording).
Added a test for this case where we call startVideoRecording twice, and verify the methods we expect to be called only once are indeed called only once (covering the above case).
Made a change to the stopVideoRecording error case where the videoOutputPath is null so that we clean up the recording objects (stop the recording, set recording and pendingRecording to null) because we can't recover from this case without starting a new recording as videoOutputPath is set in the startVideoRecording. This way change (1) doesn't get us stuck forever in the null videoOutputPath case.
Added a test to cover the above.

camsim99 · 2023-05-08T22:21:30Z

packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart

@@ -421,6 +421,11 @@ class AndroidCameraCameraX extends CameraPlatform {
    assert(cameraSelector != null);
    assert(processCameraProvider != null);

+    if (recording != null) {
+      // There is currently an active recording, so do not start a new one.


Can you add a statement to the doc for this method that this is the expected behavior for trying to start a new recording?

Added a note that we silently return without doing anything if there is an active recording.

camsim99 · 2023-05-08T22:22:06Z

packages/camera/camera_android_camerax/lib/src/android_camera_camerax.dart

+      // in this error case.
+      recording!.close();
+      recording = null;
+      pendingRecording = null;


Same here about adding it to the doc

Added a note on the cleanup of the recording objects in the doc

camsim99 · 2023-05-08T22:23:31Z

packages/camera/camera_android_camerax/test/android_camera_camerax_test.dart

+      camera.videoOutputPath = videoOutputPath;
+
+      final XFile file = await camera.stopVideoRecording(0);
+      assert(file.path == videoOutputPath);


Should this be an expect statement instead of assert?

Good catch! Changed

It was actually in two places because I copied this test to make the test of "call start video recording twice", changed in both places now

…test

camsim99 · 2023-05-09T17:47:32Z

Merging manually to unblock #3878

…flutter#3467) This PR implements video capture for the CameraX re-write of the android camera plugin. flutter/flutter#111138 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [relevant style guides] and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use `dart format`.) - [x] I signed the [CLA]. - [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g. `[shared_preferences]` - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated `pubspec.yaml` with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes]. - [x] I updated `CHANGELOG.md` to add a description of the change, [following repository CHANGELOG style]. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/packages/blob/main/CONTRIBUTING.md [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [relevant style guides]: https://github.com/flutter/packages/blob/main/CONTRIBUTING.md#style [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat [pub versioning philosophy]: https://dart.dev/tools/pub/versioning [exempt from version changes]: https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#version-and-changelog-updates [following repository CHANGELOG style]: https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changelog-style [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests --------- Co-authored-by: Gray Mackall <[email protected]>

gmackall requested a review from camsim99 as a code owner March 15, 2023 19:01

This comment was marked as resolved.

Sign in to view

github-actions bot added p: camera platform-android labels Mar 15, 2023

gmackall changed the title ~~Video capture~~ Implement video capture for CameraX android camera re-write Mar 15, 2023

gmackall changed the title ~~Implement video capture for CameraX android camera re-write~~ [camera] Implement video capture for CameraX android camera re-write Mar 15, 2023

camsim99 requested changes Mar 17, 2023

View reviewed changes

gmackall requested a review from reidbaker March 21, 2023 16:05

reidbaker requested changes Mar 28, 2023

View reviewed changes

reidbaker reviewed Mar 29, 2023

View reviewed changes

camsim99 reviewed Apr 20, 2023

View reviewed changes

gmackall changed the title ~~[camera] Implement video capture for CameraX android camera re-write~~ [camerax] Implement video capture for CameraX android camera re-write May 1, 2023

gmackall requested a review from camsim99 May 1, 2023 23:42

camsim99 approved these changes May 2, 2023

View reviewed changes

gmackall requested a review from reidbaker May 2, 2023 16:39

reidbaker reviewed May 3, 2023

View reviewed changes

Gray Mackall added 2 commits May 3, 2023 11:41

remaking video capture pr to fix CLA issue

f876e73

accepting documentation change to createTempFile

ca2a353

gmackall mentioned this pull request May 3, 2023

[camerax] Implement video capture for CameraX android camera re-write (remake to fix cla issue) #3899

Closed

11 tasks

This comment was marked as outdated.

Sign in to view

making video recording tests into a group, and random typo

22eea7a

gmackall force-pushed the video_capture branch from 3c6e30a to 22eea7a Compare May 4, 2023 19:32

Merge branch 'main' into video_capture

77dfba1

reidbaker reviewed May 4, 2023

View reviewed changes

reidbaker requested a review from camsim99 May 4, 2023 19:55

camsim99 approved these changes May 4, 2023

View reviewed changes

gmackall and others added 3 commits May 8, 2023 10:58

Merge branch 'flutter:main' into video_capture

daf385b

documentation changes from review comments

167f9c2

changing test name

23ec03e

format

9797067

gmackall mentioned this pull request May 8, 2023

[camerax] Standardize the detached versions of wrapped class constructors flutter/flutter#126293

Open

Gray Mackall added 2 commits May 8, 2023 14:13

adding tests for error cases in stopVideoRecording

c76d1a4

formatting

3141b02

reidbaker approved these changes May 8, 2023

View reviewed changes

Gray Mackall added 2 commits May 8, 2023 14:46

Making explicit that we do nothing on pressing record twice, and addi…

92a51b8

…ng test to cover case. Also change to null videoOutputPath error case and test

formatting new tests

5e776a5

camsim99 reviewed May 8, 2023

View reviewed changes

Gray Mackall added 3 commits May 8, 2023 15:31

review comments around documentation and an assert

0ba0dfc

removing use of the word 'we' per the style guide

89d7936

removing second 'assert' that was copied over from first instance of …

9eecbed

…test

gmackall added the autosubmit Merge PR when tree becomes green via auto submit App label May 8, 2023

Merge branch 'main' into video_capture

f8eee31

camsim99 mentioned this pull request May 9, 2023

[camerax] Publish plugin #3896

Merged

11 tasks

Merge branch 'main' into video_capture

fb4770b

camsim99 merged commit d6bb772 into flutter:main May 9, 2023

gmackall mentioned this pull request May 9, 2023

[camera] Add Recorder class to CameraX plugin flutter/flutter#111191

Closed

3 tasks

reidbaker mentioned this pull request May 12, 2023

[url_launcher] Set broadcast reciever visability as required by target api 34 #3973

Merged

11 tasks

This was referenced Jul 7, 2023

[Tool] New tool to download android dependencies #4408

Merged

[camera] tree fix format 2023 05 12 #4450

Closed

[tooling] mark infra steps as infra steps in yaml files #4473

Merged

reidbaker mentioned this pull request Jul 26, 2023

[Tooling] Add google owned cache for dependencies as an option in ci #4567

Merged

11 tasks

reidbaker mentioned this pull request Aug 4, 2023

[All] Expand artifact hub to all plugins #4645

Merged

11 tasks

		return instanceManager.getInstanceWithWeakReference(videoCaptureId)!
		as VideoCapture;

	/// Creates a VideoCapture associated with the given [Recorder].
	/// Creates a [VideoCapture] associated with the given [Recorder].

[camerax] Implement video capture for CameraX android camera re-write #3467

[camerax] Implement video capture for CameraX android camera re-write #3467

Conversation

gmackall commented Mar 15, 2023 • edited Loading

Pre-launch Checklist

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camsim99 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camsim99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reidbaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmackall May 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reidbaker May 8, 2023 • edited Loading

Choose a reason for hiding this comment

gmackall May 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reidbaker commented May 3, 2023

This comment was marked as outdated.

reidbaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmackall May 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camsim99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmackall commented Mar 15, 2023 •

edited

Loading

camsim99 left a comment •

edited

Loading

gmackall May 8, 2023 •

edited

Loading

reidbaker May 8, 2023 •

edited

Loading

gmackall May 8, 2023 •

edited

Loading

gmackall May 8, 2023 •

edited

Loading

gmackall commented May 8, 2023 •

edited

Loading