-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified this! I liked your phrasing for making it future-proof.
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, with optional nit
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: I was thinking also removing all of these actual resolutions from all of these, so it was just the ~XXXp
.
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 eyebrowsoffire@gmail.com Skip the wasm test for now, it doesn't even actually test wasm currently (flutter/packages#6044) 2024-02-02 43054281+camsim99@users.noreply.github.com [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 flutter-ecosystem@google.com,rmistry@google.com 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
Improves documentation to clarify that CameraX uses the preset resolution to target for [UseCase]s and are not guaranteed. Fixes flutter/flutter#141766.
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.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.///
).