-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[camera] fix RECORD_AUDIO permission on Android when there is no permission #10424
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] fix RECORD_AUDIO permission on Android when there is no permission #10424
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. 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. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
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.
Code Review
This pull request aims to fix how audio recording is handled when permissions are missing or audio is disabled. While the intent is correct, the implementation contains a critical logical error that would mute audio when it should be enabled. Additionally, the corresponding unit tests have not been updated to reflect the new logic, which would cause them to fail. I've provided a detailed review comment on the withAudioEnabled method with a suggested fix and a reference to the testing guidelines.
...droid_camerax/android/src/main/java/io/flutter/plugins/camerax/PendingRecordingProxyApi.java
Show resolved
Hide resolved
5e57600 to
e443aed
Compare
...droid_camerax/android/src/main/java/io/flutter/plugins/camerax/PendingRecordingProxyApi.java
Show resolved
Hide resolved
camsim99
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.
Wow thanks so much for catching + fixing this subtle bug!
Before we land this, we'll need to test this. You'll just need to add/modify tests in PendingRecordingTest.java.
...droid_camerax/android/src/main/java/io/flutter/plugins/camerax/PendingRecordingProxyApi.java
Show resolved
Hide resolved
|
In the future, please do not delete the checklist that is in the PR template; it is there for a reason. This PR is missing required elements described in the checklist (I’ve restored it to the PR description), which need to be addressed before it moves forward with review. I am marking the PR as a Draft. Please review the checklist, updating the PR as appropriate, and when that’s complete please feel free to mark the PR as ready for review. |
…o/flutter/plugins/camerax/PendingRecordingProxyApi.java Co-authored-by: Camille Simon <43054281+camsim99@users.noreply.github.com>
4c27904 to
61a6b51
Compare
camsim99
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.
Looking good thanks! Just two some small changes, then we'll just need a second review.
| .thenAnswer((Answer<Integer>) invocation -> PackageManager.PERMISSION_DENIED); | ||
|
|
||
| when(instance.withAudioEnabled(true)).thenReturn(newInstance); | ||
| when(instance.withAudioEnabled(true)).thenReturn(instance); |
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 make a separate test with these changes? This (withAudioEnabled_doesNotEnableAudioWhenRequestedAndPermissionNotGranted) is supposed to test when audio IS requested but permission is not granted.
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 might be missing something. i'm falling to see the issue. could you help?
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 believe the original test was testing an invalid assumption. withAudioEnabled_doesNotEnableAudioWhenRequestedAndPermissionNotGranted is testing when audio IS requested with permission denied.
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 don't believe so. withAudioEnabled takes a boolean initialMuted, which should be false to request audio. I'm asking if you can separate this out into two tests--one withAudioEnabled_doesNotEnableAudioWhenRequestedAndPermissionNotGranted (this one without changes) and another (for example) withAudioEnabled_doesNotEnableAudioWhenNotRequestedAndPermissionNotGranted (one that tests the case that this PR addresses).
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.
done.
Co-authored-by: Camille Simon <43054281+camsim99@users.noreply.github.com>
…r#10510) flutter/flutter@c8cfb2b...3f553f6 2025-11-24 engine-flutter-autoroll@skia.org Roll Skia from 1cbc0e6a7d21 to 43d2020be565 (7 revisions) (flutter/flutter#179010) 2025-11-24 matt.kosarek@canonical.com Potentially fixing the flakiness in win32 windowing tests, but it needs some running (flutter/flutter#178499) 2025-11-24 engine-flutter-autoroll@skia.org Roll Skia from ee2cd2c06cf7 to 1cbc0e6a7d21 (1 revision) (flutter/flutter#178995) 2025-11-24 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from lJTlAwQY4MnuRuORC... to pOO9Jl9HTLsEmks6y... (flutter/flutter#178994) 2025-11-23 engine-flutter-autoroll@skia.org Roll Dart SDK from a1422c6f8aa1 to 24cc9a740bd3 (1 revision) (flutter/flutter#178992) 2025-11-23 engine-flutter-autoroll@skia.org Roll Skia from 6592e75d6f7e to ee2cd2c06cf7 (1 revision) (flutter/flutter#178990) 2025-11-23 engine-flutter-autoroll@skia.org Roll Skia from fb6c00107a51 to 6592e75d6f7e (5 revisions) (flutter/flutter#178985) 2025-11-22 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 4ul9jvZ7jnDGIjtCD... to lJTlAwQY4MnuRuORC... (flutter/flutter#178971) 2025-11-22 engine-flutter-autoroll@skia.org Roll Dart SDK from c44cf2015e3d to a1422c6f8aa1 (2 revisions) (flutter/flutter#178968) 2025-11-22 engine-flutter-autoroll@skia.org Roll Skia from 3018c3053490 to fb6c00107a51 (4 revisions) (flutter/flutter#178952) 2025-11-22 engine-flutter-autoroll@skia.org Roll Dart SDK from 5af71c736b0a to c44cf2015e3d (1 revision) (flutter/flutter#178953) 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 Please CC bmparr@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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
Last bump to robolectric. The rest were previously done by dependabot. Partially Addresses flutter/flutter#177674 Partially Addresses flutter/flutter#163071 ## Pre-Review Checklist **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
Makes the analysis options changes described in flutter/flutter#178827: - Adding [omit_obvious_local_variable_types](https://dart.dev/tools/linter-rules/omit_obvious_local_variable_types) - Adding [specify_nonobvious_local_variable_types](https://dart.dev/tools/linter-rules/specify_nonobvious_local_variable_types) - Adding [specify_nonobvious_property_types](https://dart.dev/tools/linter-rules/specify_nonobvious_property_types) - Adding [type_annotate_public_apis](https://dart.dev/tools/linter-rules/type_annotate_public_apis) - Removing [always_specify_types](https://dart.dev/tools/linter-rules/always_specify_types) After those changes, makes the following repo-wide changes: - `dart fix --apply` in all packages and in script/tool/ - `dart format` in all packages and in script/tool/ - `update-excerpts` repo tooling command to update excerpts based on the changes to their sources Also updates the min Flutter/Dart SDK version to 3.35/3.9 for the following packages, to avoid `analyze` failures in the `N-2` legacy analysis run due to what appears to be a 3.9 change in what the Dart analyzer continues to be an obvious local type in loop iterations: - go_router - google_fonts - google_identity_services_web - google_maps_flutter_web - local_auth_platform_interface - metrics_center - multicast_dns - pigeon - rfw - shared_preferences - two_dimensional_scrollables - vector_graphics_compiler - mustache_template - path_parsing Because this is causing a significant amount of format churn already, I took this opportunity to update the repository tooling to a min Dart SDK of 3.8 (the N-2 stable version, so the earliest version we need the tooling to support) to pick up the new format style, so the amount of automated formatter change is higher in script/tool/ than in the packages. This does contain two manual changes (other than the repo tooling min version): - flutter@d700b45 changes `dynamic` to `Object?` in a few places where `dynamic` caused analyzer warnings under the new rule set. - Updates the repo tooling to ignore `.dart_tool/` when looking for unexpected local `analysis_options.yaml` files, to fix issues running the repo tool's `analyze` command locally based on recent changes in `dart` behavior. This does not include any CHANGELOG or version updates; even though we normally version any changes to production code, mass automated changes like this aren't worth the churn of releasing. This includes changes to lib/example/main.dart and to README.md excerpts; while the style changes will be user-visible on pub.dev, it's fine for those changes to wait for the next release of each package. Part of flutter/flutter#178827
Replaces the full build-and-analyze version of `pod lib lint` with the `--quick` version. This means that this step no longer does native code analysis (which `analyze` covers), so it no longer needs an exclusion list for packages with warnings. As a result of this change, flutter/packages no longer has a dependency on the version of Flutter that was published to CocoaPods to facilitate podspec testing. Fixes flutter/flutter#178806
camsim99
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.
Updated test LGTM! @jesswrd can you give this a second pass?
flutter/packages@5d8d954...b505d41 2025-11-26 dasyad00@gmail.com [camera_android_camerax] use MediaSettings::fps for image preview, streaming, and video recording (flutter/packages#10301) 2025-11-26 stuartmorgan@google.com [various] Update READMEs to reflect current OS support (flutter/packages#10470) 2025-11-26 engine-flutter-autoroll@skia.org Roll Flutter from 3f553f6 to 7b98d50 (30 revisions) (flutter/packages#10523) 2025-11-26 alex@swipelab.co [camera] fix RECORD_AUDIO permission on Android when there is no permission (flutter/packages#10424) 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 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
…r#179188) flutter/packages@5d8d954...b505d41 2025-11-26 dasyad00@gmail.com [camera_android_camerax] use MediaSettings::fps for image preview, streaming, and video recording (flutter/packages#10301) 2025-11-26 stuartmorgan@google.com [various] Update READMEs to reflect current OS support (flutter/packages#10470) 2025-11-26 engine-flutter-autoroll@skia.org Roll Flutter from 3f553f6 to 7b98d50 (30 revisions) (flutter/packages#10523) 2025-11-26 alex@swipelab.co [camera] fix RECORD_AUDIO permission on Android when there is no permission (flutter/packages#10424) 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 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
Allows to record video without audio when we init the CameraController with
enableAudio=falseRelated issue: flutter/flutter#175020
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3