apache / cordova-plugin-media

Apache Cordova Media Plugin
https://cordova.apache.org/
Apache License 2.0
388 stars 767 forks source link

Can't record audio on app targeting Android 13 due to plugin requesting WRITE_EXTERNAL_STORAGE permission (which always fails) #373

Closed suzukieng closed 1 year ago

suzukieng commented 1 year ago

Bug Report

Problem

What is expected to happen?

Recording audio should work on Android 13.

What does actually happen?

Recording audio fails, because the permission request for WRITE_EXTERNAL_STORAGE always fails on Android 13. The check is always for both RECORD_AUDIO and WRITE_EXTERNAL_STORAGE.

Information

The app does not request legacy external storage. I've seen #268 but from my understanding that change only removes the permission from the manifest, but it still tries to acquire it a runtime?

Command or Code

media.startRecord()

Environment, Platform, Device

Android 13 running on Samsung Galaxy S20

Version information

cordova-media-plugin 6.1

Checklist

mirko77 commented 1 year ago

WRITE_EXTERNAL_STORAGE was removed a while ago https://github.com/apache/cordova-plugin-media/commit/e8ca2244d04b5f9463b9c01ba1a47bce3eace0bb

suzukieng commented 1 year ago

WRITE_EXTERNAL_STORAGE was removed a while ago e8ca224

In the manifest, yes. But it’s still requested at run-time:

https://github.com/apache/cordova-plugin-media/blob/a8bbc02a06cbf01328738ee3e76cac7e32efa042/src/android/AudioHandler.java#L571

mirko77 commented 1 year ago

Good point. That might explain why we cannot get it to work on Android 13 when targeting API 33, as that permission WRITE_EXTERNAL_STORAGE will always be denied.

Basically, the audio recording never starts, and no errors are thrown. So we end up with a file of 0 bytes.

@breautek I guess checking the Build.VERSION.SDK_INT and when 33 (or higher) replace WRITE_EXTERNAL_STORAGE with READ_MEDIA_AUDIO ??

mirko77 commented 1 year ago

I tried and it works, at least on Android 13 and 11 with API 33. I do not have any Android 12 devices.

READ_MEDIA_AUDIO is not needed.

@suzukieng you can try this fork https://github.com/epicollect5/cordova-plugin-media @breautek I opened a pull request

mirko77 commented 1 year ago

I have seen the other commit by adrianhardy, I am not sure it works on old devices targeting a lower API though. READ_MEDIA_AUDIO exists only in API level 33+, if I am not mistaken.

suzukieng commented 1 year ago

@mirko77 correct, READ_MEDIA_AUDIO was introduced in API level 33+. An app that previously used READ_EXTERNAL_STORAGE should now request READ_MEDIA_*.

So something like this is needed:

String READ_IMAGES_PERMISSION =
            Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU ?
                    Manifest.permission.READ_MEDIA_IMAGES : Manifest.permission.READ_EXTERNAL_STORAGE;
mirko77 commented 1 year ago

@suzukieng have you tried my fork? Any issues?

suzukieng commented 1 year ago

@mirko77 No, I haven't tested it, but judging from your code it will work. That's exactly how I locally patched it at first. We decided to write a separate plugin in our app to handle audio recording. We have a custom native app where we embed Cordova as a "library", so this was easy for us to do.

adrianhardy commented 1 year ago

Forgive my ignorance, but when looking at your suggested fix:

String READ_IMAGES_PERMISSION =
            Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU ?
                    Manifest.permission.READ_MEDIA_IMAGES : Manifest.permission.READ_EXTERNAL_STORAGE;

Is that providing support for building using older "compileSDK"? Or is that a requirement to support multiple android versions at runtime?

Again, apologies if that's a silly question - I don't know a lot about Java, Android SDK, etc.

suzukieng commented 1 year ago

Hi @adrianhardy. It's just basically saying "if you're running on Android 13, request READ_MEDIA_IMAGES permission, otherwise request READ_EXTERNAL_STORAGE permission". So this makes an app targeting Android 13 comply with platform requirements, while falling back to legacy behavior on older Android versions (which have no READ_MEDIA_IMAGES permission). The ternary expression might make it a big hard to read.