-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[camera] add video stabilization #7108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
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. |
8242001
to
5339320
Compare
2a9c6d0
to
240e796
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only reviewed iOS part
...amera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/CameraPlugin.m
Outdated
Show resolved
Hide resolved
...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m
Outdated
Show resolved
Hide resolved
...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m
Outdated
Show resolved
Hide resolved
...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m
Outdated
Show resolved
Hide resolved
...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m
Outdated
Show resolved
Hide resolved
...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m
Outdated
Show resolved
Hide resolved
...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m
Outdated
Show resolved
Hide resolved
.../camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/messages.g.m
Show resolved
Hide resolved
Hi @hellohuanlin, I have improved camera_avfoundation's implementation based on your comments. Instead of adding a new commit, I forced pushed a squash commit of those changes and another commit on top of it that adds the dependency_overrides to the pubspecs. Assuming there will be more improvments I need to make, is it OK I keep adding commits until everything is fixed, and only then do a final squash merge, excluding the dependency_overrides commit, and force that squash commit? Thanks! |
The current version conflates |
Sure. It will be easier for me to find the time if the required changes are limited to fixing the confusion between exceptions and errors, which will increase the odds of being able to do so this year instead of later in 2025. |
Hi @stuartmorgan, I think next week I'll find enough time to finish this off. But, before doing so, I want to get a full agreement on the specification of Error HandlingFirst, concerning error handling:
Fallback behaviourConcerning the implementation with a fallback, here's a table of what
The implementation of this logic would be in the ExtraBecause of the time distance between the last time we discussed about this and now, I had to go over the entire discussion from scratch and while doing so it struck me that we could have it both ways. You wanted to make it easier for developers that want to just have the best available mode up to X and I wanted a more strict approach to cater to developers who, like me, prefer less nuanced behaviour. So, it occurred to me that the default behaviour could be the fallback behaviour, but we could add an optional bool named argument, for example,
I don't want to spend much more energy discussing this, so if you don't agree with this extra ( Let me know what you think. Thanks! Rui |
__weak typeof(self) weakSelf = self; | ||
|
||
dispatch_async(self.captureSessionQueue, ^{ | ||
bool isSupported = [weakSelf.camera isVideoStabilizationModeSupported:mode]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: BOOL
|
||
case FCPPlatformVideoStabilizationModeCinematic: | ||
return AVCaptureVideoStabilizationModeCinematic; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: inconsistent blank lines in this func
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iOS part LGTM
Thanks!! |
Sorry I was not able to respond before then; I was out of office for several weeks starting shortly before that comment.
I agree with empty array; I think not including
That sounds like a reasonable API, so adding it to better support your use case sounds good to me. I don't think it should be too hard to explain in the doc comment what it means; my suggestion would be to reverse its meaning and make it |
Hi guys, any progress on this one? I'm working on a sport's recording camera and video stabilization is a huge feature. |
Hi Renato, No, not at this point. Maybe during the next couple of weeks I'll have time to finish this off. Rui |
91fd9bf
to
75382f6
Compare
Hi @stuartmorgan, I have just pushed an update to this branch with the following changes:
I have some broken tests, but I don't think they're related to anything I did, except merging the main branch onto this one. On my machine, all Dart unit tests related to this package are OK. Native tests are indeed failing, but they're also failing when I run them on the latest version of the main branch (plus I'm getting failed Dart tests for other packages on both this branch and main). Except for the very tiny nits (bool -> BOOL and a couple of empty lines removed), all changes were made to Dart code, not native code. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Hello @ruicraveiro, Thank you for your work on this stabilization PR. I tested the update on my sports recording app and encountered a compilation error on iOS. Below is the error message from the console. Error log:Launching lib/main.dart on iPhone 11 in release mode...
Automatically signing iOS for device deployment using specified development team in Xcode project: H9UAZA7PV4
Xcode build done. 59.5s
Failed to build iOS app
Could not build the precompiled application for the device.
ARC Semantic Issue (Xcode): No visible @interface for 'NSObject<FLTCaptureDeviceFormat>' declares the selector 'isVideoStabilizationModeSupported:'
/Users/leo_ruiz/repositories/xports/flutter_packages/packages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m:1246:36
ARC Semantic Issue (Xcode): No visible @interface for 'NSObject<FLTCaptureDeviceFormat>' declares the selector 'isVideoStabilizationModeSupported:'
/Users/leo_ruiz/repositories/xports/flutter_packages/packages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m:1262:38
2
Error running application on iPhone 11.
Exited (1). Before compiling, I ran Environment details:
Flutter doctor output:❯ flutter doctor -v
[✓] Flutter (Channel stable, 3.27.1, on macOS 15.2 24C101 darwin-arm64, locale en-BR)
• Flutter version 3.27.1 on channel stable at /Users/leo_ruiz/.asdf/installs/flutter/3.27.1
• Upstream repository https://github.com/flutter/flutter.git
• Framework revision 17025dd882 (3 months ago), 2024-12-17 03:23:09 +0900
• Engine revision cb4b5fff73
• Dart version 3.6.0
• DevTools version 2.40.2
[✓] Android toolchain - develop for Android devices (Android SDK version 35.0.0)
• Android SDK at /Users/leo_ruiz/Library/Android/sdk
• Platform android-35, build-tools 35.0.0
• Java binary at: /Applications/Android Studio.app/Contents/jbr/Contents/Home/bin/java
• Java version OpenJDK Runtime Environment (build 17.0.6+0-17.0.6b802.4-9586694)
• All Android licenses accepted.
[✓] Xcode - develop for iOS and macOS (Xcode 16.2)
• Xcode at /Applications/Xcode.app/Contents/Developer
• Build 16C5032a
• CocoaPods version 1.16.2
[✓] Chrome - develop for the web
• Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome
[✓] Android Studio (version 2022.2)
• Android Studio at /Applications/Android Studio.app/Contents
• Flutter plugin can be installed from:
🔨 https://plugins.jetbrains.com/plugin/9212-flutter
• Dart plugin can be installed from:
🔨 https://plugins.jetbrains.com/plugin/6351-dart
• Java version OpenJDK Runtime Environment (build 17.0.6+0-17.0.6b802.4-9586694)
[✓] VS Code (version 1.98.2)
• VS Code at /Applications/Visual Studio Code.app/Contents
• Flutter extension version 3.106.0
[✓] Connected device (5 available)
• SM A055M (mobile) • R9XX60289MX • android-arm64 • Android 14 (API 34)
• iPhone 11 (mobile) • 00008030-0014059A1ED0402E • ios • iOS 18.3.2 22D82
• macOS (desktop) • macos • darwin-arm64 • macOS 15.2 24C101 darwin-arm64
• Mac Designed for iPad (desktop) • mac-designed-for-ipad • darwin • macOS 15.2 24C101 darwin-arm64
• Chrome (web) • chrome • web-javascript • Google Chrome 134.0.6998.166
! Error: Browsing on the local area network for iPad. Ensure the device is unlocked and attached with a cable or associated with the same local area network as this Mac.
The device must be opted into Developer Mode to connect wirelessly. (code -27)
[✓] Network resources
• All expected network resources are available.
• No issues found! If you need any further information or additional testing on my side, please let me know. Thanks again for your support and the excellent work on this PR! |
I'm not sure why you would be getting failures on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The app-level fallback logic looks good (modulo questions about how off
is managed in those APIs), mostly just minor comments before this can be split into sub-PRs.
@@ -687,6 +696,80 @@ class CameraController extends ValueNotifier<CameraValue> { | |||
} | |||
} | |||
|
|||
/// Set the video stabilization mode for the selected camera. | |||
/// | |||
/// On Android (when using camera_android_camerax) and on iOS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment should not be calling out specific platforms; the restriction should have the same behavior on all platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right and the comment is only creating confusion. Will remove it.
/// | ||
/// On Android (when using camera_android_camerax) and on iOS | ||
/// the supplied [mode] value should be a mode in the list returned | ||
/// by [getSupportedVideoStabilizationModes]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only true for allowFallback=false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will also remove it, especially since after that comment, I document both allowFallback cases.
/// be set to the best video stabilization mode up to, and including, [mode]. | ||
/// | ||
/// When either [allowFallback] is false or the only | ||
/// supported video stabilization mode is [VideoStabilizationMode.off], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes it sound like getSupportedVideoStabilizationModes
can return off
, which I believe we agreed should not happen. If it can't, then we can avoid confusion by saying "... or getSupportedVideoStabilizationModes()
returns an empty list".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three separate cases:
-
When the device has no camera stabilization modes. I floated the idea that we could return off, but I agreed with you not to.
-
When the camera does support some video stabilization mode other than off. Off was always part of the available options, so much so that is one of the options in both tables in my latest specification proposal, in the comment I made on the 20th December. Furthermore, it is one of the options returned by both Android and iOS.
-
When the only option is 'off'. This is the scenario represented by the third column in those specification tables. Basically what it means is that even if allowFallback is true, if the only available mode is off and something else is specified, then it is an error.
Here's the table again:
requested mode | no supported mode ([]) | only off supported | max level1 supported | max level2 supported | max level3 supported |
---|---|---|---|---|---|
off | throw ArgumentError() | off | off | off | off |
level1 | throw ArgumentError() | throw ArgumentError() | level1 | level1 | level1 |
level2 | throw ArgumentError() | throw ArgumentError() | level1 | level2 | level2 |
level3 | throw ArgumentError() | throw ArgumentError() | level1 | level2 | level 3 |
Let me know if you still have a different understanding. For me it is very clear that if level1, 2 or 3 are available, there is no reason to exclude off. It is also very clear, and I agreed with you, that we shouldn't introduce off artificially if the device doesn't return any available mode. What is really hard to define is the behaviour of when off is the only mode, even more so because I think that this is a theoretical discussion as I don't expect the case of where the platform returns only off to ever happen. None of the devices I tested showed that behaviour, as they would either report no supported stabilization mode or at least 2 supported modes, one of them always being off.
So, maybe, because it isn't really something expected to happen, I can simplify the comment and ignore that case. Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three separate cases:
- When the device has no camera stabilization modes. [...]
[...]- When the only option is 'off'. [...]
I'm not following. How are these separate cases? What is the conceptual difference here?
.instance | ||
.getSupportedVideoStabilizationModes(_cameraId); | ||
|
||
// if there are no supported modes or if the only supported mode is Off |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: capitalize as a sentence.
// and something else is requested, then we throw an ArgumentError. | ||
if (supportedModes.isEmpty || | ||
(mode != VideoStabilizationMode.off && | ||
supportedModes.every((VideoStabilizationMode sm) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are you referring to? The comment ... and something else ... (where it would be some other mode)? Is it the 'sm' variable, which stands for 'supportedMode'? Or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, the variable sm
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to run the repo tooling's format
command on the package to auto-format everything.
|
||
/// Sets the video stabilization mode. | ||
@async | ||
@ObjCSelector('isVideoStabilizationModeSupported:') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same naming nit here: supportsVideoStabilizationMode:
@@ -502,6 +502,44 @@ class MethodChannelCamera extends CameraPlatform { | |||
} | |||
} | |||
|
|||
@override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to this file should be reverted; if there are sill third-party implementations using this (hopefully not), they should get the default behavior of nothing being supported, rather than throwing unsupported platform channel method exceptions.
/// by [getVideoStabilizationSupportedModes]. | ||
/// | ||
/// Throws a [CameraException] when a not supported video stabilization | ||
/// mode is supplied. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be
Future<VideoStabilizationMode> setTargetVideoStabilizationMode(VideoStabilizationMode mode)
and it will return, not the requested mode, but the actual mode that was selected. The returned value will be the same as the one that will be returned by Controller.value.videoStabilizationMode.Sounds good; returning the actual value from a best-effort-request method is definitely useful, and something more of our APIs should do in general.
What happened to this design? It was a good suggestion, but it's not in the updated implementation.
/// Gets a list of video stabilization modes that are supported for the selected camera. | ||
Future<Iterable<VideoStabilizationMode>> getSupportedVideoStabilizationModes( | ||
int cameraId) async { | ||
throw UnimplementedError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to return an empty list, so that the default behavior for implementations that don't support stabilization is gracefully reporting that, rather than throwing.
Hi @ruicraveiro, I tested this PR on Android, and it worked fine; the crop indicates stabilization is being applied. I’m just unsure about the differences among VideoStabilizationMode levels 1, 2, and 3, as I couldn’t visually distinguish any variations... Enviroment: Device: Motorola Edge 20 - Android 13 Doctor summary (to see all details, run flutter doctor -v): |
Android only supports level 1. |
Hi, thank you and congratulations @ruicraveiro (and @stuartmorgan-g) for the amazing work on this! I am very interested in this feature, is there any way for me to help you progress on it? |
@ruicraveiro It looks like the |
Adds support for video stabilization to camera_platform_interface, camera_avfoundation, camera_android_camerax and camera packages.
The video stabilization modes are defined in the new VideoStabilizationMode enum defined in camera_platform_interface:
There is some subjectivity on the way with which I mapped the modes to both platforms, and here's a document that compares the several modes: https://docs.google.com/spreadsheets/d/1TLOLZHR5AcyPlr-y75aN-DbR0ssZLJjpV_OAJkRC1FI/edit?usp=sharing, which you can comment on.
List which issues are fixed by this PR. You must list at least one issue.
Partially implements flutter/flutter#89525
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.