-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[camera_avfoundation] Set highest available resolution for ResolutionPreset.Max #5245
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
[camera_avfoundation] Set highest available resolution for ResolutionPreset.Max #5245
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
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 or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). 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. |
hellohuanlin
left a comment
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.
Can you add some unit test?
| if ( [_captureDevice lockForConfiguration:NULL] == YES ) { | ||
|
|
||
| // set best device format and finish device configuration | ||
| _captureDevice.activeFormat = bestFormat; |
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.
is this only required in best format but not other resolutions?
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.
In all other cases device is configured with sessionPreset through capture session, like this:
_videoCaptureSession.sessionPreset = AVCaptureSessionPreset3840x2160;
The max resolution case is different because there is no predetermined option
| bestFormat = format; | ||
| } | ||
| } | ||
| return bestFormat; |
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.
what if [_captureDevice formats] is empty? do we want to handle nil case?
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 do handle it actually, this method returns nil if there is no bestFormat and switch case from setCaptureSessionPreset doesn't break and falls through to other cases
@hellohuanlin |
Are you saying we didn't have tests this function? Now it's a good time to add those tests |
|
@sergeidesenko For testing, at a high level, we will want verify if each of different configs correctly updates the capture session. If you take a look at the FLTCam constructor, it takes both audio and video sessions, so we can inject them in the test. Let me know if it makes sense. |
|
@sergeidesenko Are you planning on updating this PR with tests as discussed above? |
|
@stuartmorgan Hi! Didn't have enough bandwidth at the end of the year to look into this. Yes, I will add tests before the end of January, does this work? |
|
I'm converting this to draft state. Feel free to add tests and re-request for review. |
|
@hellohuanlin Hi! Added tests that check whether setting the resolutionPreset actually updates the captureSession. Would be happy to hear what you think! |
|
@hellohuanlin can you take another look? |
| } | ||
|
|
||
| /// Finds the highest available resolution in terms of pixel count for the given device | ||
| - (AVCaptureDeviceFormat *)getHighestResolutionFormatFor:(AVCaptureDevice *)captureDevice { |
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.
nit: getHighestResolutionFormatForCaptureDevice
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.
fixed
| } | ||
|
|
||
| - (void) | ||
| testResolutionPresetWithCanSetSessionPresetUltraHigh_mustUpdateCaptureSessionPreset { // TODO |
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.
is this still TODO?
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.
fixed
| OCMExpect([videoSessionMock setSessionPreset:[OCMArg checkWithBlock:^BOOL(id value) { | ||
| // Return YES if the value is one of the expected presets | ||
| return [value isEqualToString:expectedUltraHighPreset] || | ||
| [value isEqualToString:expectedMaxPreset]; |
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.
can you explicitly mock the values and verify each of them?
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.
Thanks for the review!
I'm not sure I understood your message here. If you are suggesting two different tests (one for expectedUltraHighPreset and one for expectedMaxPreset), then that's not really the expected behaviour.
The test is for the case where camera receives FLTResolutionPresetMax, and in that case sessionPreset can be one of these two values and both are acceptable.
If I missed your point - could you please clarify what you meant?
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.
in that case sessionPreset can be one of these two values and both are acceptable.
Can you explain when will it be one value and when the other value? It's unclear which one will be true when running the test.
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.
If you take a look in the file FLTCam.m at line 372, there are two options on how to handle FLTResolutionPresetMax. Either we can find bestFormat, in which case sessionPreset is set to AVCaptureSessionPresetInputPriority, or the bestFormat is nil, in which case we try to set sessionPreset to AVCaptureSessionPreset3840x2160.
Since method canSetSessionPreset of our mock object always returns true, we can expect that if we try to set AVCaptureSessionPreset3840x2160, we will always succeed. That's how we get only to possible outcomes.
It's unclear which one will be true when running the test.
About that - we could mock further and provide our videoSessionMock with availableFormats, split the test in two different tests - one that covers case with the best format and one that covers case with no formats and AVCaptureSessionPreset3840x2160. But since both of those cases are an acceptable response to setting FLTResolutionPresetMax (as I explained above), I didn't see the need to go this far
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.
Since method
canSetSessionPresetof our mock object always returns true, we can expect that if we try to setAVCaptureSessionPreset3840x2160, we will always succeed. That's how we get only to possible outcomes.
If we have two codepaths, we should be testing both of them.
But since both of those cases are an acceptable response to setting FLTResolutionPresetMax (as I explained above), I didn't see the need to go this far
Why would we want no test coverage that one of the possible outcomes actually works?
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.
If we have two codepaths, we should be testing both of them.
Thank you! I'll add another test
Why would we want no test coverage that one of the possible outcomes actually works?
I'm not sure, since this whole behavior of resolution presets wasn't covered by any test at all :)
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.
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.
packages/camera/camera/CHANGELOG.md
Outdated
| @@ -1,3 +1,7 @@ | |||
| ## 0.10.5+6 | |||
|
|
|||
| * Fixes bug where max resolution preset does not produce highest available resolution on iOS. | |||
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.
this doesn't need a bump
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.
fixed
|
@stuartmorgan Ready for re-review |
| /// Videos are written to disk by `videoAdaptor` on an internal queue managed by AVFoundation. | ||
| @property(strong, nonatomic) dispatch_queue_t photoIOQueue; | ||
| @property(assign, nonatomic) UIDeviceOrientation deviceOrientation; | ||
| /// A block property to dynamically determine video dimensions based on a given |
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.
Nit: comments should not explain things that are inherently obvious to anyone who can read the language, so a property comment should not say that it's a property (same with class, method, etc.), and it should not say what the types of the block params/returns are.
In the context of an implementation file, the interesting thing is to explain what this is and why it exists:
/// A wrapper for CMVideoFormatDescriptionGetDimensions to allow for alternate implementations in tests.
| @property(nonatomic, copy) VideoDimensionsForFormatBlock videoDimensionsForFormatBlock; | ||
| /// A block property to retrieve an AVCaptureDevice instance without input parameters. | ||
| /// Designed to return a predefined or default capture device, facilitating simple access within the | ||
| /// application. |
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.
Same comments here. Also, this doesn't exist to facilitate access, it exists for dependency injection.
/// A wrapper for AVCaptureDevice creation to allow for dependency injection in tests.
| return [self initWithCameraName:cameraName | ||
| resolutionPreset:resolutionPreset | ||
| return [self initWithCameraName:(NSString *)cameraName | ||
| resolutionPreset:(NSString *)resolutionPreset |
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.
Why are you adding explicit casts to the types they already have?
| /// A block property to retrieve an AVCaptureDevice instance without input parameters. | ||
| /// Designed to return a predefined or default capture device, facilitating simple access within the | ||
| /// application. | ||
| @property(nonatomic, copy) CaptureDeviceBlock captureDeviceBlock; |
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.
captureDeviceFactory
| /// A block property to dynamically determine video dimensions based on a given | ||
| /// AVCaptureDeviceFormat. It accepts an AVCaptureDeviceFormat object and returns a | ||
| /// CMVideoDimensions struct, indicating the video's width and height. | ||
| @property(nonatomic, copy) VideoDimensionsForFormatBlock videoDimensionsForFormatBlock; |
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.
videoDimensionsForFormat
| This block is intended to be used for determining the video dimensions (width and height) for a | ||
| given capture device format. It accepts an AVCaptureDeviceFormat object as its input and returns a | ||
| CMVideoDimensions struct representing the video dimensions associated with that format. |
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.
See previous comments. The goal is to explain to someone who knows Obj-C but not this specific code what this is, not to explain to someone who doesn't know anything about the language what this is.
| Returns: | ||
| A CMVideoDimensions struct containing the width and height (in pixels) of the video associated with | ||
| the provided format. |
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.
If you are going to do formal parameter and return documentation (which is optional), it needs to use Doxygen format for consistency.
| An AVCaptureDevice instance. The specific instance returned by the block can be predefined within | ||
| the block's implementation, allowing for consistent access to a particular device, such as the | ||
| default camera or microphone, without the need for specifying its unique ID each time. | ||
| */ |
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.
All the same comments apply here.
| /** | ||
| A block type definition named VideoDimensionsForFormatBlock. | ||
| This block is intended to be used for determining the video dimensions (width and height) for a |
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.
"This block is intended to be used for" adds a lot of unnecessary verbosity; people understand that the purpose of a declaration comment is to explain what something is used for.
| This initializer allows the explicit configuration of media capture settings and direct | ||
| manipulation of internal components like capture sessions and device selection blocks, facilitating | ||
| thorough testing scenarios. |
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.
Readers can be expected to understand the purpose of declarations that are in a _Test.h file; you don't have to explain that part.
|
@stuartmorgan |
|
@hellohuanlin |
| /// Allows for alternate implementations in tests. | ||
| @property(nonatomic, copy) VideoDimensionsForFormatBlock videoDimensionsForFormat; | ||
| /// A wrapper for AVCaptureDevice creation to allow for dependency injection in tests. | ||
| @property(nonatomic, copy) CaptureDeviceBlock captureDeviceFactory; |
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.
can we call this type CaptureDeviceFactory?
| @property(assign, nonatomic) UIDeviceOrientation deviceOrientation; | ||
| /// A wrapper for CMVideoFormatDescriptionGetDimensions. | ||
| /// Allows for alternate implementations in tests. | ||
| @property(nonatomic, copy) VideoDimensionsForFormatBlock videoDimensionsForFormat; |
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.
Nit: can we make the type name and variable name consistent?
|
@stuartmorgan Same question - is there anything else you would like to see changed? |
stuartmorgan-g
left a comment
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.
A few more nits for things I didn't catch before, but otherwise LGTM. Thanks!
| break; | ||
| } | ||
| } | ||
| } |
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.
Nit: This should be after the code below, enclosing the entire case implementation, otherwise the indentation is very confusing.
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.
fixed everything
In this case I decided to just remove everything related to AVCaptureSessionPreset3840x2160, since it was just a repetition of the next case (FLTResolutionPresetUltraHigh). Now it's more in line with other cases
Out of curiosity - what's your definition of nit in this context? I thought these are not compulsory
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.
I generally use it to indicate that something is a very small change. I wasn't aware there was a doc that equated it with being optional; I'll stop using it given that.
| [self highestResolutionFormatForCaptureDevice:_captureDevice]; | ||
| if (bestFormat) { | ||
| _videoCaptureSession.sessionPreset = AVCaptureSessionPresetInputPriority; | ||
| if ([_captureDevice lockForConfiguration:NULL] == YES) { |
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.
Nit: Never compare to YES; see go/objc-style#bool-expressions-and-conversions
| (AVCaptureDevice *)captureDevice { | ||
| AVCaptureDeviceFormat *bestFormat = nil; | ||
| NSUInteger maxPixelCount = 0; | ||
| for (AVCaptureDeviceFormat *format in [_captureDevice formats]) { |
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.
Nit: in _captureDevice.formats
|
@stuartmorgan @hellohuanlin (sorry, not sure who would be responsible for this) |
stuartmorgan-g
left a comment
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.
The next step is to fix the issues I raised above. I had set my status bit to approval to make it easier to land once that was done, but it seems like that created confusion, so updating it to clarify.
…solutionPreset.Max (flutter/packages#5245)
…solutionPreset.Max (flutter/packages#5245)
flutter/packages@a9c68b8...0625827 2024-03-02 [email protected] Roll Flutter from ba719bc to 65cd84b (5 revisions) (flutter/packages#6239) 2024-03-01 [email protected] Roll Flutter from e92bca3 to ba719bc (37 revisions) (flutter/packages#6235) 2024-03-01 [email protected] [camera_avfoundation] Set highest available resolution for ResolutionPreset.Max (flutter/packages#5245) 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
…Preset.Max (flutter#5245) The current implementation of the camera plugin for iOS iterates though all other resolution presets when set to FLTResolutionPresetMax. This results in a resolution of 3840x2160 at most, while native camera is able to produce 4032x3024 This change should partially address these issues at least. - flutter/flutter#58163 - flutter/flutter#78247 - flutter/flutter#45906 P.S. I'm not really sure about tests - it seems that resolution presets are not covered by any. Any feedback is appreciated!
|
@sergeidesenko FYI This may have caused a regression based on discussion here: flutter/flutter#162376 (comment) (Sorry for pinging in the PR since I wasn't able to tag you in the issue) |
|
|
||
| _captureDevice = [AVCaptureDevice deviceWithUniqueID:cameraName]; | ||
| _captureDevice = self.captureDeviceFactory(); | ||
|
|
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.
@hellohuanlin you are probably right, this line is causing camera not updating.
Reverting this one line would fix it, I think, and wouldn't break the tests that depend on captureDeviceFactory to work. Unfortunately, I don't think I have enough bandwidth right now to go through with a new pr, which again will require writing new tests and going through a code review.
Would you consider just reverting this pr altogether?
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.
It shouldn't be reverted, the cameraName should just be a parameter to the factory method. I missed in review that it was no longer using the local name.
The current implementation of the camera plugin for iOS iterates though all other resolution presets when set to FLTResolutionPresetMax. This results in a resolution of 3840x2160 at most, while native camera is able to produce 4032x3024
This change should partially address these issues at least.
P.S. I'm not really sure about tests - it seems that resolution presets are not covered by any. Any feedback is appreciated!
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.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.