-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[camera] Clean up maxDuration
code
#7039
[camera] Clean up maxDuration
code
#7039
Conversation
`maxVideoDuration` was added to the platform interface a long time ago in preparation for adding that feature, but the other parts were never landed. The previous state was: - It has never been implemented for iOS or Android - It has never been settable from the app-facing package, so is always null unless someone uses the platform interface directly, which we don't consider a supported use case. - It cannot be implemented in the CameraX Android implementation. - It was implemented for Windows and web because when those platforms were added much later, nobody realized that the parameter was unused. There is no compelling need for this feature, as clients of the plugin can simply set their own timer to stop recording. Given that, rather than leave the confusing partial state, this marks the option as deprecated at the platform interface layer and warns implementers that it can be ignored. It also removes the implementations from Windows and web in order to reduce implementation complexity, since that code was not reachable from the app-facing API. This does not consider the Windows and web changes to be breaking, even though they arguably could be, because we do not expect clients to be calling platform interface methods directly.
(CI will make me split out the platform interface change, but I wanted to start with a combo PR to provide the whole picture.) |
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.
Android LGTM!
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.
LGTM
I'm not recently familiar with c++, but the code here looks fine to me.
I'm in support of removing it because it is in a partial state, it's not in the app-facing, and we don't have any current plans to add full support for it.
But I do think this is a feature that should be supported in the future. Although you can set a Timer
in Dart, its not really possible to get a video of an exact length by using it. Mostly because starting and stopping a camera are typically done on their own threads. It also looks like camerax and iOS do support this feature (but seemingly are not compatible with our current implementations):
https://developer.android.com/reference/androidx/camera/video/OutputOptions#getDurationLimitMillis()
https://developer.apple.com/documentation/avfoundation/avcapturefileoutput/1387390-maxrecordedduration
I'd want to get user feedback about cases where that's actually a concern; I would not expect that sub-second variation in max times would matter in real world usage. |
Platform interface portion of #7039 Part of flutter/flutter#150959
flutter/packages@14341d1...ea35fc6 2024-07-10 [email protected] [camera_avfoundation] Adds Swift Package Manager compatibility (flutter/packages#7080) 2024-07-10 [email protected] [webview_flutter_wkwebview] Adds Swift Package Manager compatibility (flutter/packages#7091) 2024-07-10 [email protected] [webview_flutter_web] Migrate to package:web. (flutter/packages#6792) 2024-07-10 [email protected] [camera] Clean up `maxDuration` code (flutter/packages#7039) 2024-07-10 [email protected] Update espresso dependencies (flutter/packages#7048) 2024-07-09 [email protected] [camera] Fix iOS torch mode regression (flutter/packages#7085) 2024-07-09 [email protected] [google_maps_flutter] Convert Obj-C->Dart calls to Pigeon (flutter/packages#7086) 2024-07-09 [email protected] Roll Flutter from fafd67d to 5103d75 (27 revisions) (flutter/packages#7084) 2024-07-09 [email protected] [camera_avfoundation] fix sample times not being numeric after pause/resume. (flutter/packages#6897) 2024-07-09 [email protected] [camera] Convert Windows to Pigeon (flutter/packages#6925) 2024-07-09 [email protected] [camera] Deprecate `maxDuration` in platform interface (flutter/packages#7078) 2024-07-09 [email protected] [google_maps_flutter] Semi-convert remaining iOS host API calls to Pigeon (flutter/packages#7079) 2024-07-09 [email protected] [path_provider] Remove `win32` (flutter/packages#7073) 2024-07-08 [email protected] [google_maps_flutter] Move iOS inspector to Pigeon (flutter/packages#6937) 2024-07-08 49699333+dependabot[bot]@users.noreply.github.com [camera]: Bump com.android.tools.build:gradle from 7.3.0 to 8.5.0 in /packages/camera/camera_android_camerax/android (flutter/packages#7072) 2024-07-08 49699333+dependabot[bot]@users.noreply.github.com [local_auth]: Bump com.android.tools.build:gradle from 7.3.1 to 8.5.0 in /packages/local_auth/local_auth_android/android (flutter/packages#7069) 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
maxVideoDuration
was added to the platform interface a long time ago in preparation for adding that feature, but the other parts were never landed. The previous state was:There is no compelling need for this feature, as clients of the plugin can simply set their own timer to stop recording. Given that, rather than leave the confusing partial state, this marks the option as deprecated at the platform interface layer and warns implementers that it can be ignored. It also removes the implementations from Windows and web in order to reduce implementation complexity, since that code was not reachable from the app-facing API.
This does not consider the Windows and web changes to be breaking, even though they arguably could be, because we do not expect clients to be calling platform interface methods directly.
Fixes flutter/flutter#150959
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.///
).