[camerax] Clarify how CameraX uses preset resolution #6022
Conversation
|
Not sure about the version changes here and I assume if I do keep the version changes, then I will need to split this to land the changes in the interface and in the plugin separately. |
| /// If a preset is not available on the camera being used, a preset of lower | ||
| /// quality will be selected automatically. | ||
| /// | ||
| /// For the `camera_android_camerax` platform implementation of the plugin, |
There was a problem hiding this comment.
We generally try not to document specific implementations in the platform interface, and in fact try to remove existing comments like that (which usually predate federation) incrementally. I would prefer we combine this with the part just above in a more generic way. E.g.,
This is treated as a target resolution, and exact values are not guaranteed. Platform implementations may fall back to a higher or lower resolution if the specific preset is not available.
We could also remove the very specific details in the entries below, and just make them say ~240p, ~480p, ~720p, ~1080p, ~2160p, so it's clearer that we aren't promising exact values (and to make it more future-proof; it's already not saying what happens on Windows for instance).
There was a problem hiding this comment.
Modified this! I liked your phrasing for making it future-proof.
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
LGTM, with optional nit
| /// resolution if a specific preset is not available. | ||
| enum ResolutionPreset { | ||
| /// 352x288 on iOS, 240p (320x240) on Android and Web | ||
| /// 352x288 on iOS, ~240p (320x240) on Android and Web |
There was a problem hiding this comment.
Optional: I was thinking also removing all of these actual resolutions from all of these, so it was just the ~XXXp.
camsim99
added
the
autosubmit
flutter/packages@d37fb0a...ae3494d 2024-02-05 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.23.2 to 3.24.0 (flutter/packages#6050) 2024-02-03 [email protected] Skip the wasm test for now, it doesn't even actually test wasm currently (flutter/packages#6044) 2024-02-02 [email protected] [camerax] Clarify how CameraX uses preset resolution (flutter/packages#6022) 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
2 participants
Improves documentation to clarify that CameraX uses the preset resolution to target for [UseCase]s and are not guaranteed.
Fixes flutter/flutter#141766.
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).