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.

Sign up for GitHub

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
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
273f583
fix(android): capture audio
erisu Aug 3, 2023
0047839
fix(android): handle some resolveWithFailure in AudioActivityResult
erisu Aug 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions plugin.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,18 @@ xmlns:android="http://schemas.android.com/apk/res/android"
</feature>
</config-file>

<config-file target="AndroidManifest.xml" parent="queries">
<intent>
<action android:name="android.provider.MediaStore.RECORD_SOUND" />
</intent>
<intent>
<action android:name="android.media.action.IMAGE_CAPTURE" />
</intent>
<intent>
<action android:name="android.media.action.VIDEO_CAPTURE" />
</intent>
</config-file>

<config-file target="AndroidManifest.xml" parent="/*">
<uses-permission android:name="android.permission.RECORD_AUDIO" />
<uses-permission android:name="android.permission.READ_MEDIA_AUDIO" />
Expand Down
65 changes: 55 additions & 10 deletions src/android/Capture.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,13 @@ Licensed to the Apache Software Foundation (ASF) under one
*/
package org.apache.cordova.mediacapture;

import static org.apache.cordova.mediacapture.FileHelper.AUDIO_3GPP;
import static org.apache.cordova.mediacapture.FileHelper.AUDIO_MP4;

import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
Expand Down Expand Up @@ -59,8 +64,7 @@ public class Capture extends CordovaPlugin {

private static final String VIDEO_3GPP = "video/3gpp";
private static final String VIDEO_MP4 = "video/mp4";
private static final String AUDIO_3GPP = "audio/3gpp";
private static final String[] AUDIO_TYPES = new String[] {"audio/3gpp", "audio/aac", "audio/amr", "audio/wav"};
private static final String[] AUDIO_TYPES = new String[] {AUDIO_3GPP, "audio/aac", "audio/amr", "audio/wav", AUDIO_MP4};
private static final String IMAGE_JPEG = "image/jpeg";

private static final int CAPTURE_AUDIO = 0; // Constant for capture audio
Expand Down Expand Up @@ -377,19 +381,60 @@ else if (resultCode == Activity.RESULT_CANCELED) {
}
}


public void onAudioActivityResult(Request req, Intent intent) {
// Get the uri of the audio clip
Uri data = intent.getData();
if (data == null) {
Uri srcContentUri = intent.getData();
breautek marked this conversation as resolved.
Show resolved Hide resolved

if (srcContentUri == null) {
pendingRequests.resolveWithFailure(req, createErrorObject(CAPTURE_NO_MEDIA_FILES, "Error: data is null"));
return;
}

// Create a file object from the uri
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 •
edited
Loading

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

String srcContentUriString = srcContentUri.toString();
String fileName = srcContentUriString.substring(srcContentUriString.lastIndexOf('/') + 1);

// Create dest file path
String tempRoot = cordova.getActivity().getCacheDir().getAbsolutePath();
String destFile = tempRoot + "/" + fileName;

// Create dest file
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 •
edited
Loading

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.

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


byte[] buf = new byte[8192];
int length;

ContentResolver srcContentResolver = cordova.getContext().getContentResolver();
InputStream srcIS = srcContentResolver.openInputStream(srcContentUri);

while ((length = srcIS.read(buf)) != -1) {
destFOS.write(buf, 0, length);
}
} else {
pendingRequests.resolveWithFailure(req, createErrorObject(CAPTURE_NO_MEDIA_FILES, "Error: failed to create new file to application cache directory."));
return;
}
} catch (IOException e) {
pendingRequests.resolveWithFailure(req, createErrorObject(CAPTURE_NO_MEDIA_FILES, "Error: failed to copy recording to application cache directory."));
return;
}

LOG.d(LOG_TAG, "Recording file path: " + destFile);

JSONObject mediaFile = new JSONObject();
try {
// File properties
mediaFile.put("name", tmpRootFile.getName());
mediaFile.put("fullPath", destFile);
mediaFile.put("type", FileHelper.getMimeType(Uri.fromFile(tmpRootFile), cordova));
mediaFile.put("lastModifiedDate", tmpRootFile.lastModified());
mediaFile.put("size", tmpRootFile.length());
} catch (JSONException e) {
pendingRequests.resolveWithFailure(req, createErrorObject(CAPTURE_INTERNAL_ERR, "Error: no mediaFile created from " + srcContentUri));
return;
}

Expand Down
7 changes: 6 additions & 1 deletion src/android/FileHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ Licensed to the Apache Software Foundation (ASF) under one

// TODO: Replace with CordovaResourceApi.getMimeType() post 3.1.
public class FileHelper {
static final String AUDIO_3GPP = "audio/3gpp";
static final String AUDIO_MP4 = "audio/mp4";

public static String getMimeTypeForExtension(String path) {
String extension = path;
int lastDot = extension.lastIndexOf('.');
Expand All @@ -36,7 +39,9 @@ public static String getMimeTypeForExtension(String path) {
// Convert the URI string to lower case to ensure compatibility with MimeTypeMap (see CB-2185).
extension = extension.toLowerCase(Locale.getDefault());
if (extension.equals("3ga")) {
return "audio/3gpp";
return AUDIO_3GPP;
} else if (extension.equals("m4a")) {
return AUDIO_MP4;
}
return MimeTypeMap.getSingleton().getMimeTypeFromExtension(extension);
}
Expand Down