-
Notifications
You must be signed in to change notification settings - Fork 217
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
Conversation
"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 |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this 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:)
- 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. - 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:)
- Error Handling: For unsupported
AudioFormatID
, the current implementation returns "audio" as a default value. It would be better to provide clear error messages. - 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"
}
}
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.
The calling function
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
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 |
There was a problem hiding this 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!
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.