Skip to content
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

Merged
merged 12 commits into from
Feb 2, 2024

Conversation

camsim99
Copy link
Contributor

@camsim99 camsim99 commented Jan 31, 2024

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

@camsim99
Copy link
Contributor Author

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.

@camsim99 camsim99 marked this pull request as ready for review January 31, 2024 21:51
@camsim99 camsim99 requested a review from stuartmorgan January 31, 2024 21:51
/// 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,
Copy link
Contributor

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).

Copy link
Contributor Author

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.

@camsim99 camsim99 requested a review from stuartmorgan February 1, 2024 20:53
Copy link
Contributor

@stuartmorgan stuartmorgan left a 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
Copy link
Contributor

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.

@camsim99 camsim99 added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 2, 2024
@auto-submit auto-submit bot merged commit 19f88ae into flutter:main Feb 2, 2024
78 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 5, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 5, 2024
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
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
Improves documentation to clarify that CameraX uses the preset resolution to target for [UseCase]s and are not guaranteed.

Fixes flutter/flutter#141766.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: camera
Projects
None yet
2 participants