[Camera Plugin] Enhance camera functionality and support for transformations#11231
[Camera Plugin] Enhance camera functionality and support for transformations#11231Depdx wants to merge 21 commits intoflutter:mainfrom
Conversation
…thod signatures - Removed CameraLensDirection and ResolutionPreset enums from messages.g.h and messages.dart. - Eliminated CameraDescription and MediaSettings classes from messages.dart. - Updated CameraApi methods to reflect changes in camera creation and available cameras retrieval. - Added getTextureId method to CameraApi for retrieving texture ID. - Updated pubspec.yaml to include stream_transform dependency and asset for Pylon setup.
…ies for camera_avfoundation and camera_linux
…ersion 0.2 and remove optional MD5 checksum
…ency ref to camera_0.3 and format code for readability
…vfoundation dependencies to camera_0.5 and modify exposure and focus mode settings to use TrySetValue for better error handling.
…mat in initializeCamera method.
…endencies to camera_0.6
… and crop features
…mera sensor orientation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements to the camera plugin, including support for lens position control and geometric transformations on iOS, and adds a new Linux implementation. The changes are extensive and well-structured. However, there are several critical issues that need to be addressed, particularly in the new Linux implementation. These include a placeholder LICENSE file, dependencies on a forked repository, a broken widget test, and a couple of critical bugs in the C++ code related to iterator invalidation and incorrect use of a static variable. Addressing these issues is crucial for the stability, security, and maintainability of the plugin.
| testWidgets('Counter increments smoke test', (WidgetTester tester) async { | ||
| // Build our app and trigger a frame. | ||
| await tester.pumpWidget(const MyApp()); | ||
|
|
||
| // Verify that our counter starts at 0. | ||
| expect(find.text('0'), findsOneWidget); | ||
| expect(find.text('1'), findsNothing); | ||
|
|
||
| // Tap the '+' icon and trigger a frame. | ||
| await tester.tap(find.byIcon(Icons.add)); | ||
| await tester.pump(); | ||
|
|
||
| // Verify that our counter has incremented. | ||
| expect(find.text('0'), findsNothing); | ||
| expect(find.text('1'), findsOneWidget); | ||
| }); |
There was a problem hiding this comment.
| @@ -0,0 +1 @@ | |||
| TODO: Add your license here. | |||
| set(PYLON_DOWNLOAD_URL "https://github.com/LightX-Innovations/flutter_packages/releases/download/camera_linux_v0.1/pylon-${PYLON_VERSION}_linux-aarch64.tar.gz") | ||
| set(PYLON_ROOT ${CMAKE_BINARY_DIR}/pylon-sdk) | ||
|
|
||
| set(PYLON_MPEG_ARCHIVE_NAME "pylon-supplementary-package-for-mpeg-4-1.0.2.121_aarch64.tar.gz") | ||
| set(PYLON_MPEG_ARCHIVE_PATH ${CMAKE_BINARY_DIR}/downloads/${PYLON_MPEG_ARCHIVE_NAME}) | ||
| set(PYLON_MPEG_DOWNLOAD_URL "https://github.com/LightX-Innovations/flutter_packages/releases/download/camera_linux_v0.1/pylon-supplementary-package-for-mpeg-4-1.0.2.121_aarch64.tar.gz") |
There was a problem hiding this comment.
The CMake build script downloads dependencies (pylon-sdk and pylon-supplementary-package-for-mpeg-4) from a fork on GitHub (LightX-Innovations/flutter_packages). This introduces a dependency on a non-official, third-party source which is a security risk and can cause build failures if the fork becomes unavailable. For an official Flutter package, dependencies should be vendored or sourced from official, stable locations.
| for (auto&& camera_it = cameras.begin(); camera_it != cameras.end(); | ||
| ++camera_it) { | ||
| if (camera_it->camera_id == camera_id) { | ||
| cameras.erase(camera_it); | ||
| } | ||
| } |
There was a problem hiding this comment.
Erasing an element from a std::vector while iterating over it invalidates the iterator, which can lead to undefined behavior. Since camera IDs are expected to be unique, you should break the loop after erasing the element to prevent issues.
if (camera_it->camera_id == camera_id) {
cameras.erase(camera_it);
break;
}| return; | ||
| } | ||
|
|
||
| static bool isFirstFrame = true; |
There was a problem hiding this comment.
The isFirstFrame variable is declared as static. This means it will be shared across all instances of CameraVideoRecorderImageEventHandler. If multiple video recordings are started, only the very first frame of the very first recording will trigger the initialization block. This should be a member variable of the class to ensure each recording instance has its own state.
| camera_platform_interface: ^2.12.0 | ||
| camera_linux: | ||
| git: | ||
| url: git@github.com:LightX-Innovations/flutter_packages.git |
There was a problem hiding this comment.
The camera_linux dependency is pointing to a git@ URL on a personal fork (LightX-Innovations/flutter_packages). This will cause issues for CI and for other developers who do not have SSH access configured for this repository. It's also a dependency on a non-official repository. Please use an https:// URL if this is temporary, and for merging into the main repository, this should point to an official source or be a path dependency if it's part of the same monorepo.
url: https://github.com/LightX-Innovations/flutter_packages.git| { | ||
| "path": "./packages/camera/camera_avfoundation" | ||
| } |
| Row( | ||
| children: [ | ||
| ElevatedButton( | ||
| onPressed: () { | ||
| setState(() { | ||
| if (TargetPlatform.linux != defaultTargetPlatform) { | ||
| return; | ||
| } | ||
| final CameraLinux nativeCamera = | ||
| CameraPlatform.instance as CameraLinux; | ||
| nativeCamera.setImageFormatGroup( | ||
| controller!.cameraId, | ||
| PlatformImageFormatGroup.mono8, | ||
| ); | ||
| }); | ||
| }, | ||
| child: Text('mono8'), | ||
| ), | ||
| ElevatedButton( | ||
| onPressed: () { | ||
| setState(() { | ||
| if (TargetPlatform.linux != defaultTargetPlatform) { | ||
| return; | ||
| } | ||
| final CameraLinux nativeCamera = | ||
| CameraPlatform.instance as CameraLinux; | ||
| nativeCamera.setImageFormatGroup( | ||
| controller!.cameraId, | ||
| PlatformImageFormatGroup.rgb8, | ||
| ); | ||
| }); | ||
| }, | ||
| child: Text('rgb8'), | ||
| ), | ||
| ], | ||
| ), |
There was a problem hiding this comment.
The platform check for these Linux-specific buttons is inside an unconditional setState call. This is inefficient as it triggers unnecessary rebuilds on all platforms. Since the UI doesn't seem to depend on any state changed here, the setState call is not needed.
A better approach would be to conditionally render this Row only on Linux, or at least disable the buttons on other platforms and remove the setState call.
For example:
if (!kIsWeb && Platform.isLinux)
Row(
children: <Widget>[
ElevatedButton(
onPressed: () {
(CameraPlatform.instance as CameraLinux).setImageFormatGroup(
controller!.cameraId,
PlatformImageFormatGroup.mono8,
);
},
child: const Text('mono8'),
),
// ...
],
)| @@ -0,0 +1,3 @@ | |||
| ## 0.0.1 | |||
|
|
|||
| * TODO: Describe initial release. | |||
This update improves the camera plugin by adding support for lens position control, geometric transformations, and enhanced error handling. It also refactors the API for better clarity and performance, while updating dependencies to ensure compatibility with the latest Flutter versions. This addresses issue #1 regarding camera functionality enhancements.