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

Assign default file extension based on audio format #615

Merged
merged 2 commits into from
Jun 17, 2024

Conversation

jbaudanza
Copy link
Contributor

I am using this library to record ogg/opus files, however, the default file name was always "sound.mp4" or "sound.m4a", regardless of the audio format.

To fix this, I tried passing in a custom path to startRecord, with the correct file extension, however a native extension is required to get the proper path to the cache directory on Android.

Since a native modification is required anyway, I decided to modify this module to just pick a reasonable default file extension for the giving audio format.

I also added the proper OGG / OPUS types for Android to index.ts. These types are supported as of apiVersion 29.

@hyochan hyochan added 🍗 enhancement New feature or request 🤖 android Related to android labels Jun 17, 2024
Comment on lines 391 to 402
"mp4", // DEFAULT = 0,
"3gp", // THREE_GPP,
"mp4", // MPEG_4,
"amr", // AMR_NB,
"amr", // AMR_WB,
"aac", // AAC_ADIF,
"aac", // AAC_ADTS,
"rtp", // OUTPUT_FORMAT_RTP_AVP,
"ts", // MPEG_2_TSMPEG_2_TS,
"webm",// WEBM
"xxx", // UNUSED
"ogg", // OGG
Copy link
Owner

Choose a reason for hiding this comment

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

commas in comments could be identical here!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the commas for consistency. Let me know if you'd like to make any other adjustments.

Copy link
Owner

@hyochan hyochan left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. Could you kindly review the code review suggested by GPT?

Improvement Suggestions

avFormat(fromString:)

  1. Default Value Return: The current implementation returns kAudioFormatAppleLossless when the encoding value is nil. It might be better to determine the default value externally or pass it as a parameter to this function.
  2. Error Handling: The function currently returns nil for unmatched strings. Adding exception handling to provide clear error messages would be beneficial.
private func avFormat(fromString encoding: String?) -> AudioFormatID? {
    guard let encoding = encoding else {
        return kAudioFormatAppleLossless
    }

    switch encoding {
    case "lpcm":
        return kAudioFormatAppleIMA4
    case "ima4":
        return kAudioFormatAppleIMA4
    case "aac":
        return kAudioFormatMPEG4AAC
    case "MAC3":
        return kAudioFormatMACE3
    case "MAC6":
        return kAudioFormatMACE6
    case "ulaw":
        return kAudioFormatULaw
    case "alaw":
        return kAudioFormatALaw
    case "mp1":
        return kAudioFormatMPEGLayer1
    case "mp2":
        return kAudioFormatMPEGLayer2
    case "mp4":
        return kAudioFormatMPEG4AAC
    case "alac":
        return kAudioFormatAppleLossless
    case "amr":
        return kAudioFormatAMR
    case "flac":
        if #available(iOS 11.0, *) {
            return kAudioFormatFLAC
        }
    case "opus":
        return kAudioFormatOpus
    case "wav":
        return kAudioFormatLinearPCM
    default:
        return nil
    }
}

fileExtension(forAudioFormat:)

  1. Error Handling: For unsupported AudioFormatID, the current implementation returns "audio" as a default value. It would be better to provide clear error messages.
  2. Documentation: Adding comments explaining each AudioFormatID in the code would make it easier to understand.
private func fileExtension(forAudioFormat format: AudioFormatID) -> String {
    switch format {
    case kAudioFormatOpus:
        return "ogg"
    case kAudioFormatLinearPCM:
        return "wav"
    case kAudioFormatAC3, kAudioFormat60958AC3:
        return "ac3"
    case kAudioFormatAppleIMA4:
        return "caf"
    case kAudioFormatMPEG4AAC, kAudioFormatMPEG4CELP, kAudioFormatMPEG4HVXC, kAudioFormatMPEG4TwinVQ, kAudioFormatMPEG4AAC_HE, kAudioFormatMPEG4AAC_LD, kAudioFormatMPEG4AAC_ELD, kAudioFormatMPEG4AAC_ELD_SBR, kAudioFormatMPEG4AAC_ELD_V2, kAudioFormatMPEG4AAC_HE_V2, kAudioFormatMPEG4AAC_Spatial:
        return "m4a"
    case kAudioFormatMACE3, kAudioFormatMACE6:
        return "caf"
    case kAudioFormatULaw, kAudioFormatALaw:
        return "wav"
    case kAudioFormatQDesign, kAudioFormatQDesign2:
        return "mov"
    case kAudioFormatQUALCOMM:
        return "qcp"
    case kAudioFormatMPEGLayer1:
        return "mp1"
    case kAudioFormatMPEGLayer2:
        return "mp2"
    case kAudioFormatMPEGLayer3:
        return "mp3"
    case kAudioFormatMIDIStream:
        return "mid"
    case kAudioFormatAppleLossless:
        return "m4a"
    case kAudioFormatAMR:
        return "amr"
    case kAudioFormatAMR_WB:
        return "awb"
    case kAudioFormatAudible:
        return "aa"
    case kAudioFormatiLBC:
        return "ilbc"
    case kAudioFormatDVIIntelIMA, kAudioFormatMicrosoftGSM:
        return "wav"
    default:
        return "audio"
    }
}

@jbaudanza
Copy link
Contributor Author

  1. Default Value Return: The current implementation returns kAudioFormatAppleLossless when the encoding value is nil. It might be better to determine the default value externally or pass it as a parameter to this function.

If the encoding value is unrecognized, I think the library should raise an error, rather than falling back to a default. This behavior would be more expected and less surprising to a consumer of this module.

btw, I think kAudioFormatMPEG4AAC would be a better choice of default format, rather than kAudioFormatAppleLossless, but I didn't want to address that in this PR.

  1. Error Handling: The function currently returns nil for unmatched strings. Adding exception handling to provide clear error messages would be beneficial.

The calling function startRecorder will indeed return a rejected promise to the caller if avFormat returns nil.

  1. Error Handling: For unsupported AudioFormatID, the current implementation returns "audio" as a default value. It would be better to provide clear error messages.

These formats are not necessarily "unsupported", but are traditionally not written to a file. If, for some reason, the caller wants to write these a file, I think .audio is a reasonable choice for a file extension.

  1. Documentation: Adding comments explaining each AudioFormatID in the code would make it easier to understand.

I think the constant names are descriptive enough without adding more comments. More details are available in the Apple documentation: https://developer.apple.com/documentation/coreaudiotypes/1572096-audio_format_identifiers

@jbaudanza jbaudanza requested a review from hyochan June 17, 2024 16:30
Copy link
Owner

@hyochan hyochan left a comment

Choose a reason for hiding this comment

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

Thank you for the prompt and kind review response. I'll go ahead and merge it!

@hyochan hyochan merged commit 9ae1795 into hyochan:main Jun 17, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖 android Related to android 🍗 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants