Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[camera] Take picture with max resolution #3854

Closed

Conversation

comlhj1114
Copy link

@comlhj1114 comlhj1114 commented May 6, 2021

For using camera imageStream with low resolution, and take picture with high resolution, additional argument enableTakePictureWithMaxResolution is added.

Originally, what I want to modify was separate resolution control for preview and capture, however, in iOS multiple resolution in a single AVCaptureSession seems not available, except max resolution in capture using setHighResolutionPhotoEnabled option.

So, I propose enableTakePictureWithMaxResolution argument in CameraController to take a picture with max resolution.

Because this PR added an argument, camera_platform_interface should be modified accordingly.
I will make another PR soon. #3856

With this modification, I removed preview resolution limit in android to match behavior of iOS.

List which issues are fixed by this PR. You must list at least one issue.
flutter/flutter#46082
#1952 (comment)
#3843

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format. See plugin_tool format)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@stuartmorgan-g
Copy link
Contributor

Thanks for the submission! We’re currently working through a large backlog of PRs, and this will require non-trivial review, so it will take some time before we’re able to review it. As explained in CONTRIBUTING.md, votes for the corresponding issue are the primary way we’re prioritizing non-trivial reviews, so we encourage anyone interested in this PR to vote for the corresponding issue.

@stuartmorgan-g
Copy link
Contributor

Note that per Flutter policy, this will need tests before it could go forward, so I would encourage you to write tests now, otherwise it'll be the first thing that you'll get feedback on when it's reviewed.

@stuartmorgan-g
Copy link
Contributor

Please include the platform interface changes in this PR, as described at https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-federated-plugins so we can review the change holistically. Please read about adding parameters however, as your other interface PR was making a breaking change, which we don't want.

@@ -85,6 +85,7 @@
private final String cameraName;
private final Size captureSize;
private final Size previewSize;
private final boolean enableTakePictureWithMaxResolution;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the argument for this being a boolean, rather than two independent resolution options? If at some point someone wants a different but non-max option for photos, adding that would make the API extremely messy.

@godofredoc godofredoc changed the base branch from master to main January 6, 2022 23:33
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@stuartmorgan-g
Copy link
Contributor

Thank you for your contribution. I'm going to close this PR for now since there are outstanding comments, just to get this off our PR review queue. Please don't hesitate to submit a new PR if you have the time to address the review comments. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants