[PoC] fix(android): API 30+ capture audio #277
Conversation
|
@ath0mas do you have and thoughts on this implementation pattern vs #215 which you commented on in your PR to prevent audio recording crashes.. #215 was another PR attempt to resolve the issue, but also currently out-dated with conflicts. I think in the long run, it would be better if people could migrate to MediaRecorder, the web api, but it depends on platform support requirements. MediaRecorder was supported in iOS 14.3+ and Chrome 49+. Also for reference the current major release of Cordova-Android min sdk support is API 24 which ships Chrome 51. But iOS is where the core minimum support falls. |
There was a problem hiding this comment.
Few comments, and would like to test it live quickly these days.
Still a doubt on addressing only the Audio case here while the Issues were often mentioning Video too ; but also true in my case too I am running current codebase without any FileProvider rework fine, up to targetSdk 32 for now and on "common" devices/ROMs.
| File tmpRootFile = new File(destFile); | ||
|
|
||
| try { | ||
| // If file exists |
There was a problem hiding this comment.
If file does not exist and successfully created
There was a problem hiding this comment.
I can split the try-catch like this.
// Create dest file
File tmpRootFile = new File(destFile);
try {
if (!tmpRootFile.createNewFile()) {
// When the file already exists
pendingRequests.resolveWithFailure(req, createErrorObject(CAPTURE_NO_MEDIA_FILES, "Error: failed to create file to application cache directory as it already exists."));
return;
}
} catch (IOException e) {
// When the path is invalid
pendingRequests.resolveWithFailure(req, createErrorObject(CAPTURE_NO_MEDIA_FILES, "Error: failed to create new file to application cache directory due to invalid file path."));
return;
}
try {
// Write src file content to dest (new file)
} catch (IOException e) {
// When it fails to copy content over
pendingRequests.resolveWithFailure(req, createErrorObject(CAPTURE_NO_MEDIA_FILES, "Error: failed to copy recording data to new file in application cache directory."));
return;
}This will provide better feedback on what fails.
- When the file already exists
- When the path is invalid and could not create a file
- When it fails to copy the src file contents to the dest file (new file).
Alternatively, we use createTempFile and do not reuse the filename from the recorder. There is always a chance that the UUID filename that the recorder app created is not unique to the app.
// Create dest file
String fileExtension = "." + srcContentUriString.substring(srcContentUriString.lastIndexOf('.') + 1);
File tempRootFile = new File(tempRoot);
File tmpNewFile = null;
try {
tmpNewFile = File.createTempFile("cdv-media-capture", fileExtension, tempRootFile);
} catch (IOException e) {
pendingRequests.resolveWithFailure(req, createErrorObject(CAPTURE_NO_MEDIA_FILES, "Error: failed to create new temp file in application cache directory."));
return;
}
try {
// Write src file content to dest (new file)
} catch (IOException e) {
// When it fails to copy content over
pendingRequests.resolveWithFailure(req, createErrorObject(CAPTURE_NO_MEDIA_FILES, "Error: failed to copy recording data to new file in application cache directory."));
return;
}There was a problem hiding this comment.
Why not splitting but balanced, as I see other capture types have runtime exception throwing in some cases, checks on null after it, or so. Again maybe keep some sort of common pattern and not go for a new structure only for Audio?
There was a problem hiding this comment.
as I see other capture types have runtime exception throwing in some cases
I think all RuntimeException usages have been removed now.
Again maybe keep some sort of common pattern and not go for a new structure only for Audio?
This PR is only touching the Audio aspect. I foresee further PRs being created to address video and image capture.
src/android/Capture.java
Outdated
| mediaFile.put("lastModifiedDate", tmpRootFile.lastModified()); | ||
| mediaFile.put("size", tmpRootFile.length()); | ||
| } catch (JSONException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
Keep the resolveWithFailure in this case, like for other mediaFile == null?
There was a problem hiding this comment.
I updated the PR to handle this.
There was a problem hiding this comment.
See // this will never happen for this same exception, and maybe prevent duplicate code extracting and calling a createMediaFile(File).
erisu
force-pushed
the
fix/capture-audio-android-11
branch
from
b9cbe4c to
0047839
Compare
@ath0mas I will need to test the video functionality to confirm and maybe create a separate PR for each feature (audio, video, etc.). Someone at work recently showed me that the video-capturing functionality was working, but the audio-capturing was not. That is why I focused only on the audio within this PR. |
erisu
changed the title
| try { | ||
| // If file exists | ||
| if (tmpRootFile.createNewFile()) { | ||
| FileOutputStream destFOS = new FileOutputStream(tmpRootFile); |
There was a problem hiding this comment.
destFOS and srcIS should be declared in a try-with-resources statement each
| JSONObject mediaFile = createMediaFile(data); | ||
| if (mediaFile == null) { | ||
| pendingRequests.resolveWithFailure(req, createErrorObject(CAPTURE_INTERNAL_ERR, "Error: no mediaFile created from " + data)); | ||
| // Get file name |
There was a problem hiding this comment.
What about not replacing but adding this new code inside the if (mediaFile == null) { as a "fallback"?
To not copy to and use the cacheDir if the captured audio file is already "correct".
(see my branch capture-audio-fallback-cachedir)
My first tests with your POC went really well 👍!
* feat(android)!: Android 13 support * refactor(android): simplify getPermissions logic * feat(android)!: bump cordova-android requirement to >=12.0.0 * feat(android): update saveAlbumPermission to include Android 9 and below use case --------- Co-authored-by: ochakov <evgeny@ochakov.com>
Platforms affected
android
Motivation and Context
Description
This PR will copy the recorded file into the app cache directory and return the cached file path. The file will be accessible and usable to the app.
Android might clean up the cache directory, so app developers should decide and implement what to do with the cached file. For example, move it into a persisted directory or upload it to some storage server. The developer should also handle cache cleanup.
Also, because the recording is a copy, the original recording will still exist in the recording app. App end-users will still have access to their original recording if it no longer exists on the app.
It also Declare package visibility & small refactor for MIME detection.
Testing
Checklist
(platform)if this change only applies to one platform (e.g.(android))