fix(android): issue #325

[klaw-PLS]

As stated in issue #325, it looks like PR #317 missed a spot in the corrections it was making. This commit adds that fix to the missed location. You can find more info in #325.

Platforms affected

Android

Motivation and Context

Fixes #325.

Description

When debugging through the plugin code, I was able to determine that when a simple file name is provided for the media.startRecord() API a file path is eventually generated in the createAudioFilePath() method using either getExternalFilesDir() or getCacheDir(). In my emulator, it creates a temporary file path like the following: /storage/emulated/0/Android/data/com../files/tmprecording-###.3gp. (The logic for creating this temporary file path was updated in PR #317.)

However, when a simple file name is provided to the media.play() API, a different file path is created instead. The logic is located in the loadAudioFile() method, but for my situation it eventually calls the Environment.getExternalStorageDirectory() to create the file path. I do not have the file path on hand at the moment, but it does not match the file path created via the createAudioFilePath() method. (I am just now realizing that PR #317 specifically was trying to remove getExternalStorageDirectory() and perhaps this use was overlooked?)

To work around this issue, I just updated line 709 of AudioPlayer.java to read the following instead:

//this.player.setDataSource(Environment.getExternalStorageDirectory().getPath() + "/" + file);
this.player.setDataSource(createAudioFilePath(file));

Using this workaround, I no longer have an error code thrown, and when I load my app onto an actual device, I am able to record and playback audio just fine. (For some reason, recording on my emulator does not pick up my microphone input.) My main concern with this approach is that, if I am understanding it correctly, the media.play() API is supposed to be able to accept a much larger range of audio sources compared to media.startRecord(). This leads me to be concerned that reusing code from media.startRecord() will somehow restrict media sources that should otherwise be acceptable.

Testing

Here is a snippet from my test program. The following functions are connected to the onclick event of buttons on my screen:

function recordAudioButtonPressed() {
var platform = device.platform;
var mediaType = platform === "Android" ? "aac" : "m4a";
var src = "testRecording." + mediaType;
var mediaRec = new Media(
src,
function () {},
function (err) {
navigator.notification.alert(
"An error occurred while recording audio: " + err.message,
function () { },
"Error",
"Ok");
});

mediaRec.startRecord();
setTimeout(function () {
  mediaRec.stopRecord();
  mediaRec.release();
}, 3000);

}

function playAudioButtonPressed() {
var platform = device.platform;
var mediaType = platform === "Android" ? "aac" : "m4a";
var src = "testRecording." + mediaType;
var mediaPlay = new Media(
src,
function () {
mediaPlay.release();
},
function (err) {
navigator.notification.alert(
"An error occurred while playing audio.",
function () { },
"Error",
"Ok");
});

mediaPlay.play();

}

I am encountering this issue on Android, using emulated devices running API 29 and 30, as well as a Samsung Galaxy S9+ running Android 10 (which should be API 29 iirc).

Checklist

• I've run the tests to see all new and existing tests pass
• I added automated test coverage as appropriate for this change
• Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
• If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
• I've updated the documentation if necessary

[timbru31]

LGTM, but a second pair of eyes are welcome

[breautek]

Looks good to me.

I believe getExternalStorageDirectory() returns the root directory, which is no longer a writable path as of API 30 with scoped storage, based on my own observations that I've done awhile back.

The change of createAudioFilePath makes it use either the external files directory or the internal cache directory, both of which should still be writable. So I'm pretty confident this is a good improvement.