[camera] add video stabilization

[ruicraveiro]

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:

/// The possible video stabilization modes that can be capturing video.
enum VideoStabilizationMode {
  /// Video stabilization is disabled.
  off,

  /// Basic video stabilization is enabled.
  /// Maps to CONTROL_VIDEO_STABILIZATION_MODE_ON on Android
  /// and throws CameraException on iOS.
  on,

  /// Standard video stabilization is enabled.
  /// Maps to CONTROL_VIDEO_STABILIZATION_MODE_PREVIEW_STABILIZATION on Android
  /// (camera_android_camerax) and to AVCaptureVideoStabilizationModeStandard
  /// on iOS.
  standard,

  /// Cinematic video stabilization is enabled.
  /// Maps to CONTROL_VIDEO_STABILIZATION_MODE_PREVIEW_STABILIZATION on Android
  /// (camera_android_camerax) and to AVCaptureVideoStabilizationModeCinematic
  /// on iOS.
  cinematic,

  /// Extended cinematic video stabilization is enabled.
  /// Maps to AVCaptureVideoStabilizationModeCinematicExtended on iOS and
  /// throws CameraException on Android.
  cinematicExtended,
}

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

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene page, which explains my responsibilities.
• 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.)
• I signed the CLA.
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I linked to at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

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

[google-cla]

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.

[hellohuanlin]

only reviewed iOS part

[ruicraveiro]

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!

[bparrishMines]

Same for this one.

[bparrishMines]

This should throw an UnimplementedError by default like the other methods.

[bparrishMines]

Add a space after the first sentence: https://dart.dev/effective-dart/documentation#do-separate-the-first-sentence-of-a-doc-comment-into-its-own-paragraph.

Same for the other values.

[bparrishMines]

This helper should no longer be necessary because of enhanced enums. You can just use VideoStabilizationMode.off.name.

[bparrishMines]

This should be replaceable with return VideoStabilizationMode.values.firstWhere((A a) => a.name == value);.

This method can also be added to the VideoStabilizationMode enum.

[ruicraveiro]

Hi, I assumed a factory called .fromString would be a good way to add the method to the enum. Let me know if you prefer something else.

[bparrishMines]

nit: This should use mode.name instead. Although it looks like Android and iOS use their own implementations anyways.

[bparrishMines]

nit: The $ can be removed from the name of new tests because it has been known to mess up the indexing for some IDEs.

[ruicraveiro]

I think I didn't miss anything. Thanks for the review!

[bparrishMines]

LGTM besides some nits.

After handling those, this should be ready for the next step of https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changing-federated-plugins

[bparrishMines]

nit: To match the other methods

Suggested change

	throw UnimplementedError();
	throw UnimplementedError('getVideoStabilizationSupportedModes() is not implemented.');

[bparrishMines]

nit: same for this one

Suggested change

	throw UnimplementedError();
	throw UnimplementedError('setVideoStabilizationMode() has not been implemented.');

[bparrishMines]

nit: I think this is a bit clearer

Suggested change

	/// Throws a [CameraException] when a not supported video stabilization
	/// Throws a [CameraException] when an unsupported video stabilization

[bparrishMines]

Will return an empty list on all other platforms.

This is no longer true. It throws an UnimplementedError on other platforms.

[ruicraveiro]

Just fixed it. Thanks!

[ruicraveiro]

Just submitted #7548 for only the platform interface.

[ruicraveiro]

Hi reviewers, after feedback from @stuartmorgan, I am resubmitting this with a revised set of enum options. They were made more abstract. So, now VideoStabilizationMode no longer has off, on, standard, cinematic and cinematicExtended, it has off, level1, level2 and level3. Android only maps to off and level1. iOS maps to all levels.

[stuartmorgan]

I haven't finished reviewing the updated platform interface, but a few initial comments below:

[stuartmorgan]

Is this only for the method channel implementation? If so, it should be moved to that file as a stand-alone (or that file reverted entirely; it's a legacy implementation that we need for backward compatibility, but doesn't need new features).

[ruicraveiro]

I can safely remove the MethodChannel.setVideoStabilizationMode, but not CameraPlatform.setVideoStabilizationMode. Same thing for getVideoStabilizationSupportedModes.

The methods in MethodChannel are stub implementations as I implemented them only to follow the existing pattern. The methods in CameraPlatform are part of the interface I very intentionally added.

[stuartmorgan]

This part of the comment should be removed. The whole point of this query structure is that the implementation support can change at any time.

[ruicraveiro]

Done.

[stuartmorgan]

nit: getSupportedVideoStabilizationModes.

[stuartmorgan]

Same with this comment.

[ruicraveiro]

Done.

[stuartmorgan]

With the levels naming system, I would lean toward instead saying that unsupported levels will fall back to the highest supported level, since I think it would be easier to use that way. /cc @bparrishMines for input.

(We may still want platforms that have no support to thow UnimplentedError.)

[stuartmorgan]

If we don't do that, this should be an Unsupported Error across the board. Exceptions are for things that should be caught and handled at runtime, and the correct runtime approach is to use getSupported... first and then call this accordingly. Calling with an unsupported mode is a programming error.

[ruicraveiro]

I don't agree with falling back to the highest supported level, especially because I added a method to return the supported levels. It is a bad principle for a method to do something different from what it was asked to do instead of returning an exception. Methods should either do exactly as they're told to do or they should throw to notify the caller that they couldn't or wouldn't. Adding a fallback can create more confusion to the user than the convenience it brings.

I agree that the user should first call getSupported... and then call the setVideo..., which is precisely what I am doing in the app for which I needed this feature. It is a really nice pattern because I am also adjusting the options I am presenting to the end-user based on getSupported... I'm not sure I understood what you meant by "Unsupported Error" across the board. Do you want me to change the default behaviour to throw UnsupportedError() in CameraPlatform and throw that exception from the platform implementations? Just let me know which exceptions you prefer at which levels and I'll adjust.

[stuartmorgan]

I don't agree with falling back to the highest supported level, especially because I added a method to return the supported levels. It is a bad principle for a method to do something different from what it was asked to do

It could be called setTargetVideoStabilizationMode. There's nothing fundamentally wrong with a method that does the best it can do as long as that's what it's documented to do.

instead of returning an exception.

Returning an exception for a programming error is bad practice. Errors, not exceptions, are for programming errors.

Just let me know which exceptions you prefer at which levels and I'll adjust.

I haven't seen any argument for why it would be correct to return an exception at any level.

[ruicraveiro]

Now, that's the kind of feedback I'm sorry I didn't have in the beginning before I went through all the other feedback I've been receiving in the meanwhile and wasting my time uselessly. Both arguments are a non-starter for me. You are free to pick up my changes and integrate them if you want. Alternatively, you are free to abandon the PR. But, this is one change I will not be doing to get this PR approved.

[ruicraveiro]

I'll begin with my mistake and move on to my disagreement. Maybe wrongly I didn't assign the importance to the distinction between errors and exceptions that it deserves, so maybe I misread your intention. Basically I read it as you not agreeing with throwing "anything" at all. And, probably I also explained myself badly when I talked about throwing exceptions as a general term without making that distinction (and I have just discovered a weak spot in my own Dart competence - at least in my communication - as probably I am still talking in C# terms sometimes, where everything that is thrown is by definition an Exception). I apologize for this misunderstanding. If your issue is against throwing exceptions as opposed to throwing errors when the user asks for an unsupported video stabilization mode, but throwing something nonetheless, I see the point and I'm totally fine with it.

Now, what I am very, very, strongly against, is the fallback idea, an actual deal-breaker for me. As I mentioned previously, a method either does what it is meant to, or it throws (or returns a value indicating that it went wrong, which is the pattern we use in Go, for instance). I am completely against setTargetVideoStabilizationMode. Here are several reasons:

• What is the next best level to 'level1'? Is it 'off'. Why? Why would 'off' be considered a valid stabilization mode when the user asked for 'level1', 'level2' or 'level3'? Why are we making these decisions for the developers and not the developers themselves?

• What happens when the developer didn't expect this behaviour, asked for 'level1' and didn't handle the downgrading to 'off' properly? The counter argument is that it is the developer's fault for not having read the documentation. But, nonetheless, this is very likely going to trip more developers than it is going to help them (and I can say this because I have exceedingly far more experience as an API user rather than an API developer). This will create a ton of confusion when end users complain that the app isn't doing what it is supposed to do and the developers will be scratching their heads until they finally read the documentation [as they should] and find out that in order to report to the user which stabilization level the application is in, they need to check that and confirm after calling setTargetVideoStabilizationMode. In this scenario - it will happen! - our good intention to help the developer will do precisely the opposite. It will trip them and require them to do more work to understand all the confusion we led them into. Even worse, because we tripped the developers using this API, how much end user effort will be wasted until the developers figure out what was going on? This is the kind of APIs that make me curse the developers who came up with them. Simple APIs, with no magic, present no surprises.

• Compounding the previous argument, it is very easy to miss the complete description of the behaviour of a method in the documentation (many times even after reading everything), but it is very hard to miss when the API throws an error or an exception. I tend to think that this is the main advantage of throwing: Because it is hard to miss (and this is also the number one reason why I exceedingly rarely implement empty catch statements and why I tend to reject code that includes them while reviewing). Moreover, developers who log when catching as a normal practice and implement a log retrieval will benefit from having errors thrown. Once again the only way they'll understand, for logging purposes, that the app did something different than what they requested with setTargetVideoStabilizationMode will be by checking which current video stabilization level they ended in at the end.

• How do we know that if the user requests 'level3' and the device only supports 'level1', that it is acceptable to downgrade to 'level1' automatically? What if there is a business rule that clearly requires 'level3' and forbids video captures that are done using 'level1'? Again, we might be tripping the developers into a non-compliance situation, because we are guessing what is best for them. Again, because we're guessing what is good for them, now instead of simply handling an error or an exception, they need to check the current stabilization mode. The counterpoint to this argument is that they could have first checked which modes are supported. But if we're requiring that in the first place, why are we complicating everything so much?

• Future proofing. Let's say that tomorrow there's some sort of different video stabilization level that isn't clearly superior or inferior to the current levels. It's just different. How will we add that level and still keep up with setTargetVideoStabilizationMode? Naming such a level will be a huge challenge all to itself, but still nothing compared to the challenge that it will require to make it fit in a method where a level hierarchy needs to exist.

• We could have at most (but I still think it is a really bad idea), trySetVideoStabilizationMode, which would be a companion to setVideoStabilizationMode, following an existing pattern. For example, int.parse() will throw if you give it a string that isn't convertible to a number. int.tryParse() will return null. The distinction between setTargetVideoStabilizationMode and trySetVideoStabilizationMode is that the former is a replacement of setVideoStabilizationMode while the latter is a companion method. However I still don't like this idea because int.tryParse() doesn't have a fallback behaviour. Instead, it presents a different way of reporting failure (returns null instead of throwing), so it's not really the same pattern.

• With the methods I implemented, it is easy for the developers to implement their own method with a fallback if they chose to do so. Maybe the developer is OK with a fallback all the way down to 'off'. Maybe another developer is OK all the way to 'off', but not including 'off'. Those are semantic decisions that should be made by the developers using this API, not by us. Again, it's the kind of logic that makes every sense to exist in the app's code, not in the API's code.

So, from where I am standing, there is no good reason to make it more complicated than:

• A method to return the available video stabilization modes (getVideoStabilizationSupportedModes or after renaming it according to your feedback, getSupportedVideoStabilizationModes)
• A method, setVideoStabilizationMode, to set the required video stabilization mode and it either sets the mode if it can, or it informs the user that it can't. Because we are using Dart, the conventional way to inform the user when it can't is by throwing something. If we were using Go, the conventional way would be returning an error instead of nil.
• A property in Camera Controller to get the current mode: CameraValue.videoStabilizationMode.

[stuartmorgan]

• What is the next best level to 'level1'? Is it 'off'. Why? Why would 'off' be considered a valid stabilization mode when the user asked for 'level1', 'level2' or 'level3'?

The last line in my initial comment was to say that I didn't think we would probably want anything to fall back to off.

Why are we making these decisions for the developers and not the developers themselves?

I don't understand what decisions you think are being taken away from developers here. In both options, developers would have access to query for supported levels. In both options, developers would have the option of only ever passing a supported level as an argument to the setter.

The only thing that doing fallback to a lower level would change is that developers who didn't have a strict need for an exact level but wanted to aim for the highest level available could write code like (pseudocode):

if (getVideoStabilizationSupportedModes().isNotEmpty) {
  setVideoStabilizationMode(level3);
}

instead of having to write a helper method to do a series of fallback checks themselves. The tradeoff is that if they actually wanted, say, level2, and no other level is acceptable, and they didn't pre-check, they would get the wrong behavior.

I would expect that "level 2 or bust" or "level 3 or bust" is a much less likely use case than "level 2 if possible, but otherwise the best stabilization we can get", so making that use case more convenient at the expense of people in the case I strongly expect to be less common getting unexpected behavior if they don't read the docs seems like a tradeoff worth considering.

• What happens when the developer didn't expect this behaviour, asked for 'level1' and didn't handle the downgrading to 'off' properly?

I don't particularly think we should do that, so it's not a very interesting case to discuss in detail.

in order to report to the user which stabilization level the application is in, they need to check that and confirm after calling setTargetVideoStabilizationMode.

Yes, I think this is a good argument against my suggestion, since it could apply to the I-assume-common case as well. The primary use case I am thinking of though is one where the UI is just enable/disable stabilization, rather than exposing all options, in which case there's no need to read back the actual level. But maybe I'm wrong in assuming that's a UI many people would want to build.

• How do we know that if the user requests 'level3' and the device only supports 'level1', that it is acceptable to downgrade to 'level1' automatically? What if there is a business rule that clearly requires 'level3' and forbids video captures that are done using 'level1'? Again, we might be tripping the developers into a non-compliance situation, because we are guessing what is best for them.

This seems highly contrived as a use case. If we were talking about, say, a security API, I wouldn't suggest silent downgrade as an option. Not every API has the same cost/benefit analysis for automatic fallback.

The counterpoint to this argument is that they could have first checked which modes are supported. But if we're requiring that in the first place, why are we complicating everything so much?

"We" wouldn't be requiring that in your scenario, the person who made the bizarre business rule that video can only be recorded at stabilization level 3, but not 2 or 1 if 3 isn't available, is requiring that.

And again, the tradeoff here is that if we don't do fallback automatically, we are complicating usage for people who just want a simple option for the best available stabilization, by requiring all of them to implement the fallback themselves. But we could also use the strict API approach, ship that, and if we see a significant demand for the "I just want the best option" use case we could add a different API just for that use case later, e.g., a bestAvailable enum option that is documented to be a subjective, per-platform choice about the best available option (although such an API would have the future-proofing problem as well).

• Future proofing. Let's say that tomorrow there's some sort of different video stabilization level that isn't clearly superior or inferior to the current levels. It's just different. How will we add that level and still keep up with setTargetVideoStabilizationMode?

This is a good point. It's hard to reason about in the abstract; e.g., one possibility would be to say that we only do fallback within the levelX hierarchy. But that could certainly become more trouble than it's worth from an API complexity standpoint depending on what the other modes do exactly.

• We could have at most (but I still think it is a really bad idea), trySetVideoStabilizationMode

I don't see any value in that approach, given that we need the query method anyway (e.g., to allow building UI giving relevant options for the device in apps that want to expose all the options), and the handler for a try failure would be no less complex than using the query method first.

• With the methods I implemented, it is easy for the developers to implement their own method with a fallback if they chose to do so. [...] Those are semantic decisions that should be made by the developers using this API, not by us.

It is self-evidently less easy for developers to implement their own fallback than for us to implement the same fallback, so if we can predict a fallback that we believe most developers would want—as I was thinking is the case here (although I agree with the point you raised that if the modes become more divergent over time, that will be less true and we'll be stuck with it)—then it's worth at least considering adding that fallback internally. So long as we are still providing the APIs to allow people to building their own custom approach instead of using ours, which we will be doing regardless.

Not providing a default fallback is itself a semantic decision we would be making.

So, from where I am standing, there is no good reason to make it more complicated than:

• A method to return the available video stabilization modes (getVideoStabilizationSupportedModes or after renaming it according to your feedback, getSupportedVideoStabilizationModes)
• A method, setVideoStabilizationMode, to set the required video stabilization mode and it either sets the mode if it can, or it informs the user that it can't. Because we are using Dart, the conventional way to inform the user when it can't is by throwing something. If we were using Go, the conventional way would be returning an error instead of nil.
• A property in Camera Controller to get the current mode: CameraValue.videoStabilizationMode.

That's a perfectly reasonable set of APIs, with one caveat:

and it either sets the mode if it can, or it informs the user that it can't.

Unless you are using the phrase "the user" to mean "the developer" (i.e., the user of the API itself), I disagree. If the model is going to be that we only handle supported values, and there is a method to query supported values at runtime, then we should consider it a programming error to pass in an unsupported value, and throw an Error accordingly. We should not be throwing an Exception that the programmer then reports back to the user of the app in some way for a case that the programmer should never have caused to happen and had all the tools to avoid.

[ruicraveiro]

Indeed, when I wrote "... it informs the user", I was referring to the developer, but sloppily I used the term "user" as in "developer who is the user of the API", and I agree with the rest of your paragraph.

As to your desire to give the developer an option to simply use the best video stabilization mode, I see where you're coming from. However, I needed to implement this for a customer and it is already being used in production, which I would expect should be the very first app in the world built on Flutter to use video stabilization and after initial tests, the feedback my customer gave was that they wouldn't be using cinematicExtended on the iPhone, because the latency is just too big for their use case. So, for what is likely the very first app to use this feature (and at the moment the only app), the highest video stabilization mode turned out not to be the best video stabilization mode. Not an abstract theory, but a real life use-case.

Anyhow, there might be a simple solution to cater for that need, without trading off on anything else. Picking on your code snippet and tweaking it a bit, if getVideoStabilizationSupportedModes always returns the modes sorted by level (which I think it does, but I would need to check), all the user needs to do to get the best highest available mode is the following:

final modes = await controller.getVideoStabilizationSupportedModes();

if (modes.isNotEmpty) {
    controller.setVideoStabilizationMode(modes.last);
}

This is very convenient and we still get to throw if the developer calls setVideoStabilizationMode with an invalid value, all the while still leaving all decisions to the developer. Let's say that 'off' isn't an acceptable mode, they could write:

final modes = await controller.getVideoStabilizationSupportedModes();
if (modes.length < 2) {
    // report to the user that video stabilization is not available
}
else {
    controller.setVideoStabilizationMode(modes.last);
}

Or:

final modes = (await controller.getVideoStabilizationSupportedModes())
                          .where((p) => p != VideoStabilizationMode.off);

if (modes.isNotEmpty) {
    controller.setVideoStabilizationMode(modes.last);
} else {
   // report to the user ....
}

Finally, when I implemented my customer's app, I decided to give the users the option to select the video stabilization level from all the available. If I intended to replace this with an automatic selection of the best, according to my customer's exclusion of cinematicExtended, I could write:

final modes = (await controller.getVideoStabilizationSupportedModes())
                          .where((p) => ![
                                VideoStabilizationMode.off,
                                VideoStabilizationMode.level3,
                              ].contains(p));

if (modes.isNotEmpty) {
    controller.setVideoStabilizationMode(modes.last);
} else {
   // report to the user ....
}

Guaranteeing that getVideoStabilizationSupportedModes always returns the available levels sorted according to the existing definition 'off', 'level1' ... '...3', is an improvement I would be happy to make (if needed to make it at all), even though it doesn't solve the future-proof problem. This would all fall apart when we needed to introduce a new level that doesn't neatly fit a level sorting order.

[stuartmorgan]

As to your desire to give the developer an option to simply use the best video stabilization mode

The advantage of the API I originally proposed is that it allows setting a target in the middle of the range and still getting fallback, rather than the only option being the highest. (See also camera resolution, where we allow for fallback from an arbitrary target resolution level.)

Guaranteeing that getVideoStabilizationSupportedModes always returns the available levels sorted according to the existing definition 'off', 'level1' ... '...3', is an improvement I would be happy to make (if needed to make it at all), even though it doesn't solve the future-proof problem.

It makes the future-proofing problem worse than all other options previously discussed, because it makes it impossible to do anything for fallback that isn't a strictly linear ordering of every possible enum, and would thus make adding a new enum value that's not a new levelX a breaking change for anyone using the client code pattern you are suggesting.

[ruicraveiro]

OK, fair enough. I just learned that other methods are behaving similarly. I just checked that I can ask to setFlashMode(FlashMode.torch) on my Chromebook (Android app), which has no flash, and the method doesn't throw or return something to let me know that nothing happened. I just checked that I can set zoom level to a level above what getMaxZoomLevel returns. This behaviour pattern just caught me by surprise, especially setFlashMode(), because there is no getSupportedFlashModes() and even worse, because when I check Controller.value.flashMode it will return the requested flash mode and not what the device actually did. I really didn't expect this.

In that case, for the sake of consistency, and completely against very deeply ingrained principles (in which for me, methods either work, literally as asked for, or they throw - or return error in Golang), I may implement setTargetVideoStabilizationMode, but with one caveat, if you agree with it (and after I go through the remaining stages of the grief cycle 🧘). 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. How's that for a compromise?

How do we handle the case when there are no available stabilization methods except off?