[camera] Fix: use legacy profiles when list is empty #6867
Conversation
|
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 (don't just cc him here, he won't see it! He's on 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. |
MbIXjkee
changed the title
MbIXjkee
changed the title
MbIXjkee
changed the title
|
cc @camsim99 |
|
@stuartmorgan @bparrishMines This is a fix for an Android bug that is fixed in a recent Android update, see issue: flutter/flutter#109769. Happy to review this PR to revert to deprecated code that works, but the fix has been released, so I'm not sure it's worth the investment. What is the protocol here? |
It's hard to tell from a quick skim, but is this just adding a condition where we follow an existing legacy codepath that we have regardless? If so, that seems like minimal downside to avoid a crash; even if there's a fix on the Android side, developers can't control how quickly people update their devices. There's no clear protocol here though; it's a cost/benefit analysis of the cost of adding and supporting the workaround, and ultimately a judgement call. |
You've checked this box, but I don't see any tests. If we're going to change the codepaths here, we need a test that simulates the Android bug and demonstrates that this PR fixes that crash, as well as ensuring that it's not regressed later. This in particular is the kind of thing that would be highly prone to being accidentally removed/broken by someone later without a test because it appears to be unnecessary based on docs and up-to-date device testing. |
This makes sense. I'm going to go ahead and review it so folks can get the fix and we can revert it later when the build with the fix on Android's side is a little bit older. |
There was a problem hiding this comment.
@MbIXjkee Thank you so much for the fix! I think it will be great to land this.
Most importantly in my review, looks like there is some code missing in Camera.java and I left a comment with a suggestion with regards to simplifying the changes in MediaRecorderBuilder.java. This will also need tests -- let me know if I can help in that regard.
| boolean isAudioNeedSetup = enableAudio; | ||
| boolean isVideoNeedSetup = true; | ||
|
|
||
| if (Build.VERSION.SDK_INT >= 31 && encoderProfiles != null) { |
There was a problem hiding this comment.
The changes here seem to suggest that there may be a case where one of videoProfile or audioProfile may be null but not the other. This does not seem to be true according to the docs.
Thus, you could add a check here for if (Build.VERSION.SDK_INT >= 31 && encoderProfiles != null) to use the non-deprecated code versus just if (Build.VERSION.SDK_INT >= 31) and alleviate the need for the isXsetup type of checks.
In order for that to work (and I think the code you have here, as well, since one of encoderProfiles and camcorderProfile will always be null), we'll need to handle using the correct MediaRecorderBuilder constructor at the Camera.java level:
There was a problem hiding this comment.
I left a comment above in ResolutionFeature.java that may help determine which constructor to use here.
| } | ||
| } | ||
|
|
||
| if (captureSize == null) { |
There was a problem hiding this comment.
As a follow up to the comment I left below with regards to constructing MediaRecorderBuilder properly, you may want to ensure that either recordingProfile and recordingProfileLegacy is null. It may be easier to tell based on this whether CamcorderProfile or EncoderProfiles is being used as a result.
|
Hello~ When will this PR be merged and publish new camera library version |
|
It will be landed once it is approved by the reviewers. |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@MbIXjkee Wanted to follow up on this! Are you planning to address the review? |
|
@camsim99 Thank you for your participation. Not sure that I have now enough time to finish this solution completely, especially since I am not so familiar with writing tests for the java code and it needs additional effort on this from my side. Feel free to improve this solution, will be happy if it helps to community 👍 |
|
Closing as obsoleted by #7073 |
A temporary fix before the main problem with an empty list of camera profiles will be fixed. If profile list is empty use legacy implementation.
The issue affected - flutter/flutter#109769
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.