[camera_windows] Restore image streaming support #8234

liff · 2024-12-05T09:45:28Z

Restores support for streaming images from the camera(s) in Windows.

I reverted #7951 as a starting point and attempted to resolve the issues that were raised:

The startImageStream and stopImageStream methods are no longer async.
There is no global EventSink for the images. Each camera gets a dedicated EventChannel for the image data.

In addition, the images are submitted to the EventChannels from a dedicated hidden window -based task runner, based on flutter/engine#24232.

Implemented with assistance from @jokerttu.

Fixes flutter/flutter#97542.

Pre-launch Checklist

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

stuartmorgan-g

Thanks for the contribution!

I haven't reviewed the entire PR yet, but one high-level note about the overall PR structure, carried over from my comment on the original PR.

packages/camera/camera/lib/src/camera_controller.dart

…#8307) Add API support query, supportsImageStreaming for checking if the camera platform supports image streaming. As requested on this comment: #8234 (comment) Step 3 from the [changing federated plugins guide](https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changing-federated-plugins), split off from #8250 with a new Dart test added.

Add API support query, `supportsImageStreaming` for checking if the camera platform supports image streaming. As requested on this comment: #8234 (comment) Attempting to follow the [contribution guide wrt. changing federated plugins](https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changing-federated-plugins). There is no issue to link to, but should I create one?

stuartmorgan-g · 2025-01-14T21:02:01Z

From triage: This was waiting on the streaming support query API, so was blocked on that. I believe it's now unblocked.

liff · 2025-01-15T17:17:05Z

Rebased on main and added the support query.

packages/camera/camera_windows/lib/type_conversion.dart

packages/camera/camera_windows/test/camera_windows_test.dart

stuartmorgan-g · 2025-02-10T19:11:04Z

packages/camera/camera_windows/windows/camera_plugin.h

@@ -93,6 +95,7 @@ class CameraPlugin : public flutter::Plugin,
  flutter::TextureRegistrar* texture_registrar_;
  flutter::BinaryMessenger* messenger_;
  std::vector<std::unique_ptr<Camera>> cameras_;
+  std::shared_ptr<TaskRunner> task_runner_;


Why is this a shared_ptr rather than a unique_ptr. This class controls Camera lifetimes; in what scenario would the task runner need to outlive it?

The instance is shared with the cameras and ultimately with the CaptureController.

In terms of lifetime, I guess there might be a small window where IMF is still running the callback when the plugin is being disposed, though I’m not really sure if that’s actually possible.

The instance is shared with the cameras

Whose lifetime is, as I mentioned in my comment defined by CameraPlugin.

and ultimately with the CaptureController.

Whose lifetime is controlled by Camera (and thus also CameraPlugin).

I'm still not seeing where any of the instances of this shared_ptr can outlive CameraPlugin.

In terms of lifetime, I guess there might be a small window where IMF is still running the callback when the plugin is being disposed, though I’m not really sure if that’s actually possible.

If the callback you linked to can be called on a deallocated capture controller, then this PR is introducing undefined behavior that is extremely likely to cause racy crashes. Please re-check the logic and make sure the threading is demonstrably correct; if you don't know whether that can happen, this this PR's threading isn't demonstrably correct yet.

stuartmorgan-g · 2025-02-10T19:16:30Z

packages/camera/camera_windows/windows/capture_controller.cpp

+        {EncodableValue("width"),
+         EncodableValue(static_cast<int64_t>(preview_frame_width_))},
+        {EncodableValue("length"),
+         EncodableValue(static_cast<int64_t>(data_length))}};


Rather than using raw method channels, can you use CameraApi::GetCodec as the codec, and use a Pigeon-defined class here, to get strong typing?

(We've done something very similar with plugins that use platform view arguments, where we define a class in Pigeon that's not used in the Pigeon-managed APIs, and then use that class in other places where things are serialized via channel codecs.)

Can you point to an example where you’ve done this? It seems like I’d have to switch to using a BasicMessageChannel to take advantage of CameraApi::GetCodec.

Can you point to an example where you’ve done this?

There aren't any C++ examples, so I don't think they would be very helpful.

It seems like I’d have to switch to using a BasicMessageChannel to take advantage of CameraApi::GetCodec.

Why? The codec is passed when creating the event channel.

packages/camera/camera_windows/windows/capture_controller.cpp

stuartmorgan-g · 2025-02-10T19:24:36Z

packages/camera/camera_windows/windows/task_runner_window.cpp

+
+TaskRunnerWindow::~TaskRunnerWindow() {
+  if (window_handle_) {
+    DestroyWindow(window_handle_);


Can this use a unique_ptr with a custom destructor, instead of managing the reference manually?

Added a UniqueHWND typedef for dealing with this.

stuartmorgan-g · 2025-02-10T19:26:14Z

packages/camera/camera_windows/windows/task_runner_window.cpp

+void TaskRunnerWindow::ProcessTasks() {
+  // Even though it would usually be sufficient to process only a single task
+  // whenever we receive the message, if the message queue happens to be full,
+  // we might not receive a message for each individual task.


Please don't use "we" in comments, as it's often ambiguous who "we" is (e.g., here, "we" is very vague... it seems to be a personification of the code, but which part?)

Updated the comment.

stuartmorgan-g · 2025-02-10T19:27:22Z

packages/camera/camera_windows/windows/task_runner_window.cpp

+    if (tasks_.empty()) break;
+    TaskClosure task = tasks_.front();
+    tasks_.pop();
+    task();


Why is the queue staying locked for the entire execution of the task? Isn't it only the draining of the queue that needs the lock?

The lock scope extended too far indeed, fixed, thanks.

packages/camera/camera_windows/windows/task_runner_window.cpp

stuartmorgan-g · 2025-02-10T19:30:33Z

packages/camera/camera_windows/windows/test/capture_controller_test.cpp

It doesn't look like any of the new capture controller logic has tests.

Added test cases for the start/stop logic.

liff · 2025-02-11T21:08:38Z

Attempted to address some of the review issues, will continue with the remaining stuff later.

…tform (flutter#7951)" This reverts commit b3a5e33.

The logic is synchronous, so there is no need to have the `@async` annotation here. Addresses: flutter#7067 (comment)

When image stream frames are being received from Media Foundation, it appears to use a separate thread when calling the `OnSample` callback, which results in this error from: The 'plugins.flutter.io/camera_windows/imageStream' channel sent a message from native to Flutter on a non-platform thread. Platform channel messages must be sent on the platform thread. Failure to do so may result in data loss or crashes, and must be fixed in the plugin or application code creating that channel. In order to ensure that we are only ever using the `EventChannel` from a platform thread, create a `TaskRunner` that is backed by a hidden window to submit the frames to the image stream. Based on the suggestion in this comment: flutter/flutter#134346 (comment)

Instead of a global `EventSink` that is re-used for each stream, create a dedicated `EventChannel` for each capture stream. The channel gets an unique name like `plugins.flutter.io/camera_android/imageStream/<cameraId>` for each camera instance. Addresses: flutter#7067 (comment)

We don’t really need to keep track of the subscription and controller in instance variables.

Given that subscribing to the `EventChannel` tells the capture controller to start the image stream and cancelling the subscription tells it to stop, we don’t actually need a separate method in `CameraPlatform` for stopping the stream.

stuartmorgan-g

(Updating my review state to correctly reflect the open questions from the last round of reviews.)

stuartmorgan-g · 2025-05-13T19:41:05Z

From triage: @liff Are you still planning on updating this PR based on the review feedback?

liff · 2025-05-14T05:36:40Z

From triage: @liff Are you still planning on updating this PR based on the review feedback?

Yes—though I don’t know when, I’ve been preoccupied with other stuff lately.

…flutter#8307) Add API support query, supportsImageStreaming for checking if the camera platform supports image streaming. As requested on this comment: flutter#8234 (comment) Step 3 from the [changing federated plugins guide](https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changing-federated-plugins), split off from flutter#8250 with a new Dart test added.

Add API support query, `supportsImageStreaming` for checking if the camera platform supports image streaming. As requested on this comment: flutter#8234 (comment) Attempting to follow the [contribution guide wrt. changing federated plugins](https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changing-federated-plugins). There is no issue to link to, but should I create one?

…flutter#8307) Add API support query, supportsImageStreaming for checking if the camera platform supports image streaming. As requested on this comment: flutter#8234 (comment) Step 3 from the [changing federated plugins guide](https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changing-federated-plugins), split off from flutter#8250 with a new Dart test added.

Add API support query, `supportsImageStreaming` for checking if the camera platform supports image streaming. As requested on this comment: flutter#8234 (comment) Attempting to follow the [contribution guide wrt. changing federated plugins](https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changing-federated-plugins). There is no issue to link to, but should I create one?

liff requested review from cbracken and bparrishMines as code owners December 5, 2024 09:45

github-actions bot added p: camera platform-windows labels Dec 5, 2024

stuartmorgan-g requested changes Dec 5, 2024

View reviewed changes

packages/camera/camera/lib/src/camera_controller.dart Outdated Show resolved Hide resolved

This was referenced Dec 9, 2024

[camera] Add API support query for image streaming #8250

Merged

[camera_platform_interface] Add API support query for image streaming #8307

Merged

liff force-pushed the image-streaming-take-2 branch from 1bdbe66 to 46f38a6 Compare January 15, 2025 17:16

liff requested a review from stuartmorgan-g January 19, 2025 17:16

stuartmorgan-g requested changes Feb 10, 2025

View reviewed changes

liff force-pushed the image-streaming-take-2 branch from ef2acaa to 653b574 Compare February 11, 2025 21:05

liff force-pushed the image-streaming-take-2 branch from 653b574 to 68ad6b0 Compare February 20, 2025 20:22

liff added 10 commits February 20, 2025 22:23

Revert "[camera_windows] Revert: Support image streams on Windows pla…

692036d

…tform (flutter#7951)" This reverts commit b3a5e33.

Make {Start,Stop}ImageStream synchronous

56bd250

The logic is synchronous, so there is no need to have the `@async` annotation here. Addresses: flutter#7067 (comment)

Make type_conversion private

5cad20d

Avoid a copy in UpdateBuffer

641e18b

Simplify onStreamedFrameAvailable

5f9627f

We don’t really need to keep track of the subscription and controller in instance variables.

Remove the stopImageStream method

8a062a0

Given that subscribing to the `EventChannel` tells the capture controller to start the image stream and cancelling the subscription tells it to stop, we don’t actually need a separate method in `CameraPlatform` for stopping the stream.

Allow image streaming on camera_windows

3510ae4

Bump version to 0.2.7

d94312f

liff force-pushed the image-streaming-take-2 branch from 68ad6b0 to ef16b4a Compare February 20, 2025 20:23

Add more test cases for image streaming

9326ab2

liff force-pushed the image-streaming-take-2 branch from ef16b4a to 9326ab2 Compare February 21, 2025 05:06

liff requested a review from stuartmorgan-g February 21, 2025 11:02

stuartmorgan-g requested changes Apr 1, 2025

View reviewed changes

[camera_windows] Restore image streaming support #8234

Are you sure you want to change the base?

[camera_windows] Restore image streaming support #8234

Uh oh!

Conversation

liff commented Dec 5, 2024

Pre-launch Checklist

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stuartmorgan-g commented Jan 14, 2025

Uh oh!

liff commented Jan 15, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liff commented Feb 11, 2025

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented May 13, 2025

Uh oh!

liff commented May 14, 2025

Uh oh!

Uh oh!

stuartmorgan-g Mar 7, 2025 •

edited

Loading