[camera_windows] Support image streams on Windows platform #7067

jlundOverlay · 2024-07-08T04:57:01Z

Adds support to the windows camera implementation to allow streaming of frames.

Pre-launch Checklist

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

…client

flutter-dashboard · 2024-07-08T04:57:05Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

cbracken · 2024-07-09T17:32:42Z

Taking a look, but before I do, looks like some small Dart analysis errors:

   info - lib/camera_windows.dart:9:8 - Use relative imports for files in the 'lib' directory. Try converting the URI to a relative URI. - prefer_relative_imports
   info - lib/type_conversion.dart:19:15 - Missing type annotation. Try adding a type annotation. - always_specify_types
   info - lib/type_conversion.dart:22:24 - Unnecessary use of parentheses. Try removing the parentheses. - unnecessary_parenthesis

cbracken

Thanks for your PR! A few first-round comments.

packages/camera/camera_windows/windows/capture_controller.h

packages/camera/camera_windows/windows/camera_plugin.cpp

packages/camera/camera_windows/windows/capture_controller.cpp

jlundOverlay · 2024-07-09T23:58:48Z

Looks like the camera windows implementation was just switched to pigeon, unfortunate timing but I'll migrate the changes across

cbracken · 2024-07-10T00:09:24Z

Ugh yeah was just thinking about that as I reviewed the other one. On the plus side, the code should be significantly simpler, but frustrating to deal with the merge.

# Conflicts: # packages/camera/camera_windows/lib/camera_windows.dart # packages/camera/camera_windows/windows/camera_plugin.cpp # packages/camera/camera_windows/windows/camera_plugin.h

jlundOverlay · 2024-07-10T00:47:01Z

I think i have worked out how to replace the method channels, just unsure if i still need the frame event channel. flutter/flutter#66711 seems to indicate that pigeon doesn't use an event channel under the hood so I'm assuming its less performant for this use case?

bparrishMines

LGTM to the camera change. I will leave the windows review to @cbracken and will approve on version bumps.

sneurlax · 2024-07-20T17:19:58Z

I tested this as working :) using @jlundOverlay's fork until this is merged, thanks!

cbracken

lgtm for the change, modulo bot failures:

please add a changelog entry.
please fix formatting errors reported in the test failure.

Co-authored-by: Loïc Sharma <[email protected]>

jlundOverlay · 2024-07-25T23:10:23Z

I have made a new PR for the camera version bump #7220

loic-sharma · 2024-07-25T23:48:52Z

@jlundOverlay Ideally we should have tests for this change. What do you think of updating packages/camera/camera_windows/windows/test/camera_plugin_test.cpp to check that CameraPlugin::StartImageStream and CameraPlugin::StopImageStream interact correctly with the CaptureController?

jlundOverlay · 2024-07-26T01:38:51Z

Took me a bit to work out how to mock the EventSink but i think i got there in the end. let me know what you think.

loic-sharma

Looks great! Excellent work!

stuartmorgan · 2024-07-27T09:39:07Z

packages/camera/camera_windows/windows/camera_plugin.cpp

+                               "Unable to make event channel from windows"));
+  }
+
+  if (camera->AddPendingVoidResult(PendingResultType::kStartStream,


Where are the pending results added in this PR resolved? I don't see anything in the diff that retrieves them.

good question, when i wrote it figured id deal with that later and forgot. What exactly are these checks used for and are they necessary if the function is synchronous?

There will be no result object if the API is made synchronous.

stuartmorgan · 2024-07-27T09:41:11Z

packages/camera/camera_windows/pigeons/messages.dart

@@ -70,6 +70,14 @@ abstract class CameraApi {
  @async
  String stopVideoRecording(int cameraId);

+  /// Starts the image stream for the given camera.
+  @async


Why are these @async? It looks like the logic is synchronous.

…lutter/packages#7067)

flutter/packages@3d358d9...247fb5f 2024-07-29 49699333+dependabot[bot]@users.noreply.github.com [image_picker]: Bump androidx.activity:activity from 1.9.0 to 1.9.1 in /packages/image_picker/image_picker_android/android (flutter/packages#7245) 2024-07-29 49699333+dependabot[bot]@users.noreply.github.com [in_app_pur]: Bump androidx.annotation:annotation from 1.8.0 to 1.8.1 in /packages/in_app_purchase/in_app_purchase_android/android (flutter/packages#7239) 2024-07-29 49699333+dependabot[bot]@users.noreply.github.com [image_picker]: Bump androidx.annotation:annotation from 1.7.1 to 1.8.0 in /packages/image_picker/image_picker_android/android (flutter/packages#6763) 2024-07-29 49699333+dependabot[bot]@users.noreply.github.com [file_selector]: Bump androidx.annotation:annotation from 1.8.0 to 1.8.1 in /packages/file_selector/file_selector_android/android (flutter/packages#7240) 2024-07-29 49699333+dependabot[bot]@users.noreply.github.com [url_launcher]: Bump androidx.annotation:annotation from 1.8.0 to 1.8.1 in /packages/url_launcher/url_launcher_android/android (flutter/packages#7243) 2024-07-29 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.12 to 3.25.15 (flutter/packages#7235) 2024-07-29 [email protected] Roll Flutter from 7d5c1c0 to 031dc3d (97 revisions) (flutter/packages#7225) 2024-07-26 [email protected] [camera_windows] Support image streams on Windows platform (flutter/packages#7067) 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

stuartmorgan · 2024-10-29T11:52:42Z

packages/camera/camera_windows/windows/camera_plugin.cpp

+            return nullptr;
+          },
+          [](auto arguments) {
+            event_sink.reset();


I was going to do a quick fix to the issues I raised in my previous comments, but then I noticed the code related to event_sink. This is a global, which is already very dangerous, but it's also std::moved between classes while still being referenced persistently in both. This code would likely fail the second time an image stream was started, and would absolutely fail with multiple engines.

Given the scope of issues here, I'm going to revert this. @cbracken FYI.

…7951) This reverts commit b31a279 (#7067) The implementation had significant bugs: - It had asynchronous results, but never responded to them, violating the engine API contract and preventing futures from completing. - It `std::move`d pointers between classes, while still keeping them as ivars and global references and referring to them from long-lived handlers.

The logic is synchronous, so there is no need to have the `@async` annotation here. Addresses: flutter#7067 (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)

The logic is synchronous, so there is no need to have the `@async` annotation here. Addresses: flutter#7067 (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)

jlundOverlay added 2 commits July 8, 2024 14:06

Adds actual windows support for streaming bgra frames to the flutter …

7a2a0fa

…client

add windows as an option for streaming frames

99ae0e4

jlundOverlay requested review from cbracken and bparrishMines as code owners July 8, 2024 04:57

github-actions bot added p: camera platform-windows labels Jul 8, 2024

Merge branch 'main' into main

d553119

cbracken requested changes Jul 9, 2024

View reviewed changes

jlundOverlay added 2 commits July 10, 2024 08:19

apply suggested format changes

a89cf54

change to explict types

20c6192

Merge branch 'local_main'

753e28f

# Conflicts: # packages/camera/camera_windows/lib/camera_windows.dart # packages/camera/camera_windows/windows/camera_plugin.cpp # packages/camera/camera_windows/windows/camera_plugin.h

Migrated streaming to pigeon

d48c932

jlundOverlay requested a review from cbracken July 10, 2024 01:40

jlundOverlay added 4 commits July 11, 2024 10:05

Merge branch 'main' into main

f694dbe

accepted the wrong changes

b725973

Merge branch 'main' into main

fa007d7

Merge branch 'main' into main

9bfbfd6

bparrishMines added federated: all_changes PR that contains changes for all packages for a federated plugin change and removed federated: all_changes PR that contains changes for all packages for a federated plugin change labels Jul 17, 2024

bparrishMines reviewed Jul 17, 2024

View reviewed changes

cbracken approved these changes Jul 23, 2024

View reviewed changes

jlundOverlay added 3 commits July 24, 2024 07:51

Merge branch 'main' into main

404488d

hopefully this is right

dfa486c

bump camera version

76f0360

jlundOverlay and others added 4 commits July 26, 2024 07:53

Update packages/camera/camera_windows/windows/camera_plugin.cpp

82fa269

Co-authored-by: Loïc Sharma <[email protected]>

remove redundant code and fix stopStreaming

0385677

rollback camera versions

7c8fc81

missed one

8c91e68

jlundOverlay changed the title ~~[camera] Support image streams on Windows platform~~ [camera_windows] Support image streams on Windows platform Jul 25, 2024

jlundOverlay mentioned this pull request Jul 25, 2024

[camera] Support image streams on Windows platform #7220

Closed

11 tasks

jlundOverlay added 3 commits July 26, 2024 11:01

adds unit tests and a helper function

0b59bfd

missed one

b20f69c

wrong function

e088f16

loic-sharma approved these changes Jul 26, 2024

View reviewed changes

loic-sharma added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 26, 2024

auto-submit bot merged commit b31a279 into flutter:main Jul 26, 2024
76 checks passed

stuartmorgan reviewed Jul 27, 2024

View reviewed changes

engine-flutter-autoroll mentioned this pull request Jul 29, 2024

Roll Packages from 3d358d921a84 to 247fb5f0b7e7 (8 revisions) flutter/flutter#152488

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 29, 2024

b31a279 [camera_windows] Support image streams on Windows platform (f…

155c60d

…lutter/packages#7067)

This comment was marked as off-topic.

Sign in to view

stuartmorgan reviewed Oct 29, 2024

View reviewed changes

This was referenced Oct 29, 2024

[camera_windows] Revert: Support image streams on Windows platform #7951

Merged

[camera] Support image streams on Windows platform flutter/flutter#97542

Open

liff added a commit to CodemateLtd/packages that referenced this pull request Dec 5, 2024

Make {Start,Stop}ImageStream synchronous

6481b07

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

liff added a commit to CodemateLtd/packages that referenced this pull request Dec 5, 2024

Make {Start,Stop}ImageStream synchronous

411353c

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[camera_windows] Support image streams on Windows platform #7067

[camera_windows] Support image streams on Windows platform #7067

jlundOverlay commented Jul 8, 2024 •

edited by cbracken

Loading

flutter-dashboard bot commented Jul 8, 2024

cbracken commented Jul 9, 2024

cbracken left a comment

jlundOverlay commented Jul 9, 2024

cbracken commented Jul 10, 2024

jlundOverlay commented Jul 10, 2024

bparrishMines left a comment

sneurlax commented Jul 20, 2024

cbracken left a comment

jlundOverlay commented Jul 25, 2024 •

edited

Loading

loic-sharma commented Jul 25, 2024

jlundOverlay commented Jul 26, 2024

loic-sharma left a comment

stuartmorgan Jul 27, 2024

jlundOverlay Aug 27, 2024

stuartmorgan Aug 28, 2024

stuartmorgan Jul 27, 2024

This comment was marked as off-topic.

stuartmorgan Oct 29, 2024

[camera_windows] Support image streams on Windows platform #7067

[camera_windows] Support image streams on Windows platform #7067

Conversation

jlundOverlay commented Jul 8, 2024 • edited by cbracken Loading

Pre-launch Checklist

flutter-dashboard bot commented Jul 8, 2024

cbracken commented Jul 9, 2024

cbracken left a comment

Choose a reason for hiding this comment

jlundOverlay commented Jul 9, 2024

cbracken commented Jul 10, 2024

jlundOverlay commented Jul 10, 2024

bparrishMines left a comment

Choose a reason for hiding this comment

sneurlax commented Jul 20, 2024

cbracken left a comment

Choose a reason for hiding this comment

jlundOverlay commented Jul 25, 2024 • edited Loading

loic-sharma commented Jul 25, 2024

jlundOverlay commented Jul 26, 2024

loic-sharma left a comment

Choose a reason for hiding this comment

stuartmorgan Jul 27, 2024

Choose a reason for hiding this comment

jlundOverlay Aug 27, 2024

Choose a reason for hiding this comment

stuartmorgan Aug 28, 2024

Choose a reason for hiding this comment

stuartmorgan Jul 27, 2024

Choose a reason for hiding this comment

This comment was marked as off-topic.

stuartmorgan Oct 29, 2024

Choose a reason for hiding this comment

jlundOverlay commented Jul 8, 2024 •

edited by cbracken

Loading

jlundOverlay commented Jul 25, 2024 •

edited

Loading