Skip to content
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

[PoC] fix(android): API 30+ capture audio #277

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

erisu
Copy link
Member

@erisu erisu commented Aug 7, 2023

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

  • Created temp cordova project
  • Added this PR
  • Added media plugin (for play test)
  • Built app
  • Test run on an Android physical device

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

@erisu erisu requested a review from breautek August 7, 2023 04:12
@erisu
Copy link
Member Author

erisu commented Aug 7, 2023

@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.

Copy link
Contributor

@breautek breautek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

codewise lgtm

Copy link
Contributor

@ath0mas ath0mas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/android/Capture.java Show resolved Hide resolved
File tmpRootFile = new File(destFile);

try {
// If file exists
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If file does not exist and successfully created

Copy link
Member Author

@erisu erisu Aug 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

mediaFile.put("lastModifiedDate", tmpRootFile.lastModified());
mediaFile.put("size", tmpRootFile.length());
} catch (JSONException e) {
throw new RuntimeException(e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep the resolveWithFailure in this case, like for other mediaFile == null?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the PR to handle this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See // this will never happen for this same exception, and maybe prevent duplicate code extracting and calling a createMediaFile(File).

@erisu erisu force-pushed the fix/capture-audio-android-11 branch from b9cbe4c to 0047839 Compare August 7, 2023 23:29
@erisu
Copy link
Member Author

erisu commented Aug 8, 2023

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.

@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 erisu marked this pull request as draft August 9, 2023 00:00
@erisu erisu changed the title fix(android): API 30+ capture audio [PoC] fix(android): API 30+ capture audio Aug 9, 2023
@breautek breautek added this to the 6.0.0 milestone Aug 9, 2023
try {
// If file exists
if (tmpRootFile.createNewFile()) {
FileOutputStream destFOS = new FileOutputStream(tmpRootFile);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

@ath0mas ath0mas Aug 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍!

breautek referenced this pull request in apache/cordova-plugin-camera Aug 27, 2023
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot use in Android 10+ Can't access captured media on Android 11
3 participants