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] Use AspectRatioStrategy and ResolutionFilter to help automatic selection of expected resolution #6357

Merged
merged 13 commits into from
Mar 27, 2024

Conversation

camsim99
Copy link
Contributor

@camsim99 camsim99 commented Mar 19, 2024

Defines AspectRatioStategys that will help CameraX select the resolution we expect.

Fixes flutter/flutter#144363.

Pre-launch Checklist


when(mockProcessCameraProvider.bindToLifecycle(mockBackCameraSelector, any))
when(mockProcessCameraProvider.bindToLifecycle(any, any))
Copy link
Contributor Author

@camsim99 camsim99 Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed similar testing of this particular call in a couple of tests in this PR and felt comfortable doing so because it is tested in other createCamera tests.

@camsim99 camsim99 marked this pull request as ready for review March 20, 2024 17:29
@camsim99 camsim99 requested a review from a team March 20, 2024 17:30
Copy link
Member

@gmackall gmackall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@camsim99 camsim99 added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 21, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 21, 2024
Copy link
Contributor

auto-submit bot commented Mar 21, 2024

auto label is removed for flutter/packages/6357, due to - The status or check suite Linux_android android_device_tests_shard_1 master has failed. Please fix the issues identified (or deflake) before re-applying this label.

@camsim99
Copy link
Contributor Author

camsim99 commented Mar 25, 2024

I was going to try to fix the linked issue in two PRs but tests are failing because I didn't implement a resolution filter for the one resolution preset that this PR does not address. Will work on implementing the filter and getting the tests to pass to avoid disabling/changing any tests.

@camsim99 camsim99 marked this pull request as draft March 25, 2024 19:24
@camsim99 camsim99 marked this pull request as ready for review March 26, 2024 19:34
@camsim99 camsim99 requested a review from gmackall March 26, 2024 19:35
@camsim99
Copy link
Contributor Author

camsim99 commented Mar 26, 2024

@gmackall if you are willing to take another look at this, I would greatly appreciate it!

Overview of the new changes:

Copy link
Member

@gmackall gmackall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments about documentation/a class name but LGTM outside that!

* <p>This class handles instantiating and adding native object instances that are attached to a
* Dart instance or handle method calls on the associated native class or an instance of the class.
*/
public class ResolutionFilterHostApiImpl implements ResolutionFilterHostApi {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I don't think this matters that much but just for my understanding) Isn't this more of a ResolutionFilterConstructor/FactoryHostApiImpl? Because we aren't wrapping the methods of the ResolutionFilter interface but rather of a class that defines and creates an instance of a ResolutionFilter?

Copy link
Contributor Author

@camsim99 camsim99 Mar 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooo yes I like this, thank you! I will rename the proxy class it uses to clarify that this calls into a factory of sorts.

@camsim99 camsim99 added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 27, 2024
@auto-submit auto-submit bot merged commit f61723b into flutter:main Mar 27, 2024
78 checks passed
@camsim99 camsim99 changed the title [camerax] Use AspectRatioStrategy to help automatic selection of expected resolution [camerax] Use AspectRatioStrategy and ResolutionFilter to help automatic selection of expected resolution Mar 27, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 28, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Mar 28, 2024
flutter/packages@e6b3e11...924c7e6

2024-03-28 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[ci] Temporarily allow-warnings in podspec_check_command.dart (#6416)" (flutter/packages#6419)
2024-03-28 [email protected] [ci] Temporarily allow-warnings in podspec_check_command.dart (flutter/packages#6416)
2024-03-28 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[interactive_media_ads] Creates and adds the `interactive_media_ads` plugin (#6060)" (flutter/packages#6417)
2024-03-27 [email protected] [interactive_media_ads] Creates and adds the `interactive_media_ads` plugin (flutter/packages#6060)
2024-03-27 [email protected] [camerax] Use `AspectRatioStrategy` to help automatic selection of expected resolution (flutter/packages#6357)

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
philipfranchi pushed a commit to philipfranchi/flutter-relabel-widgets that referenced this pull request Mar 28, 2024
flutter/packages@e6b3e11...924c7e6

2024-03-28 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[ci] Temporarily allow-warnings in podspec_check_command.dart (flutter#6416)" (flutter/packages#6419)
2024-03-28 [email protected] [ci] Temporarily allow-warnings in podspec_check_command.dart (flutter/packages#6416)
2024-03-28 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[interactive_media_ads] Creates and adds the `interactive_media_ads` plugin (flutter#6060)" (flutter/packages#6417)
2024-03-27 [email protected] [interactive_media_ads] Creates and adds the `interactive_media_ads` plugin (flutter/packages#6060)
2024-03-27 [email protected] [camerax] Use `AspectRatioStrategy` to help automatic selection of expected resolution (flutter/packages#6357)

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
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
…pected resolution (flutter#6357)

Defines `AspectRatioStategy`s that will help CameraX select the resolution we expect.

Fixes flutter/flutter#144363.
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 platform-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CameraX resolutions do not follow the ResolutionPreset values
2 participants