feat: add convertAudio API with iOS m4a support#10
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a new public API Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Plugin as CapacitorFFmpegPlugin
participant Impl as CapacitorFFmpeg
participant Export as AVAssetExportSession
participant FS as FileSystem
Client->>Plugin: convertAudio({inputPath, outputPath, format})
Plugin->>Plugin: validate parameters
Plugin->>Impl: convertAudio(inputPath, outputPath, format)
Impl->>Impl: validate format == 'm4a' and input has audio track
Impl->>FS: ensure output directory exists
Impl->>FS: create temporary output file
Impl->>Export: create/configure export session (factory)
Export->>Export: export asynchronously
alt Export succeeds
Export-->>Impl: .completed
Impl->>FS: move temp -> destination (atomic)
Impl-->>Plugin: return { outputPath, format }
Plugin-->>Client: resolve promise with result
else Export fails
Export-->>Impl: error
Impl->>FS: remove partial temp (and don't overwrite existing destination)
Impl-->>Plugin: throw FFmpegError.transcodeFailed(...)
Plugin-->>Client: reject promise with error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66069bbbd9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/definitions.ts (1)
21-27:⚠️ Potential issue | 🟠 MajorAvoid making the new v8 surface required.
convertAudiois being added as a required member on two exported interfaces. That is a source-level breaking change for consumers that construct typed capability fixtures or implement plugin test doubles against the current v8 API. Please keep these additions optional in this major, or defer the stricter contract to the next Capacitor-major release.♻️ Backward-compatible shape for this major
export interface FFmpegCapabilitiesFeatures { getPluginVersion: FFmpegCapability; getCapabilities: FFmpegCapability; reencodeVideo: FFmpegCapability; convertImage: FFmpegCapability; - convertAudio: FFmpegCapability; + convertAudio?: FFmpegCapability; progressEvents: FFmpegCapability; probeMedia: FFmpegCapability; generateThumbnail: FFmpegCapability; extractAudio: FFmpegCapability; remux: FFmpegCapability; trim: FFmpegCapability; } @@ - convertAudio(options: ConvertAudioOptions): Promise<ConvertAudioResult>; + convertAudio?(options: ConvertAudioOptions): Promise<ConvertAudioResult>;As per coding guidelines, "Do not introduce breaking changes in
src/definitions.tsunless explicitly asked or the current definition is broken or unusable. Breaking changes belong to the matching Capacitor major migration, and all other changes must stay backward compatible".Also applies to: 125-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/definitions.ts` around lines 21 - 27, The review points out that adding convertAudio as a required member on exported interface FFmpegCapabilitiesFeatures (and the other exported interface at lines ~125-131) is a breaking change; make convertAudio optional (e.g., convertAudio?: FFmpegCapability) on both exported interfaces (including FFmpegCapabilitiesFeatures and the other exported interface that currently adds convertAudio) so the v8 surface remains backward-compatible for consumers who construct typed fixtures or implement plugin test doubles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/definitions.ts`:
- Around line 21-27: The review points out that adding convertAudio as a
required member on exported interface FFmpegCapabilitiesFeatures (and the other
exported interface at lines ~125-131) is a breaking change; make convertAudio
optional (e.g., convertAudio?: FFmpegCapability) on both exported interfaces
(including FFmpegCapabilitiesFeatures and the other exported interface that
currently adds convertAudio) so the v8 surface remains backward-compatible for
consumers who construct typed fixtures or implement plugin test doubles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa0df2e0-2b1a-45d9-bb28-3836f92dfcb3
📒 Files selected for processing (14)
FEATURE_MATRIX.mdREADME.mdROADMAP.mdandroid/src/main/java/ee/forgr/capacitor_ffmpeg/CapacitorFFmpegPlugin.javaandroid/src/test/java/ee/forgr/capacitor_ffmpeg/CapacitorFFmpegPluginTest.javaexample-app/src/js/app.jsexample-app/src/js/app.vitest.jsios/Sources/CapacitorFFmpegPlugin/CapacitorFFmpeg.swiftios/Sources/CapacitorFFmpegPlugin/CapacitorFFmpegPlugin.swiftios/Tests/CapacitorFFmpegPluginTests/CapacitorFFmpegPluginTests.swiftsrc/definitions.tssrc/web.tstest/TEST_INVENTORY.mdtest/web.test.ts
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ios/Sources/CapacitorFFmpegPlugin/CapacitorFFmpeg.swift`:
- Around line 528-552: The output URL extension can disagree with the requested
format (normalizedFormat), causing M4A data to be written to a mismatched
filename; update the validation in the conversion path to reject or normalize
output extensions: check outputURL.pathExtension (or
outputURL.lastPathComponent) against normalizedFormat and throw
FFmpegError.invalidArgument if they differ, or else force temporaryOutputURL and
final output filenames to use the requested extension (ensure temporaryOutputURL
is created with extension normalizedFormat and verify any final move/rename uses
that extension) so the file extension always matches the requested format (refer
to normalizedFormat, outputURL, outputDirectory, temporaryOutputURL).
- Around line 564-568: convertAudio currently blocks the bridge thread using
DispatchSemaphore.wait after calling exportSession.exportAsynchronously;
instead, make convertAudio follow the async job pattern used by reencodeVideo:
generate and return a job ID immediately from convertAudio, start
exportSession.exportAsynchronously without waiting, store the job metadata and
the resolver/rejector (or a completion handler map keyed by job ID) so you can
resolve or reject later, and move all resolve/reject calls into the
exportSession.exportAsynchronously completion block (including handling error
cases and updating job status). Remove the DispatchSemaphore usage and add
optional timeout/cancellation handling for the stored job entry so exports can
be cancelled or time out without blocking the main thread.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c30bce30-95b5-45f7-a23d-77bef0a531c0
📒 Files selected for processing (2)
ios/Sources/CapacitorFFmpegPlugin/CapacitorFFmpeg.swiftios/Tests/CapacitorFFmpegPluginTests/CapacitorFFmpegPluginTests.swift
✅ Files skipped from review due to trivial changes (1)
- ios/Tests/CapacitorFFmpegPluginTests/CapacitorFFmpegPluginTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ios/Sources/CapacitorFFmpegPlugin/CapacitorFFmpeg.swift (1)
544-548: Consider using!isEmptyinstead of extracting an unused track.The guard extracts
audioTrackonly to discard it. A cleaner approach checks track presence directly.♻️ Suggested simplification
- let asset = AVURLAsset(url: inputURL) - guard let audioTrack = asset.tracks(withMediaType: .audio).first else { + let asset = AVURLAsset(url: inputURL) + guard !asset.tracks(withMediaType: .audio).isEmpty else { throw FFmpegError.invalidArgument("The input media does not contain an audio track.") } - _ = audioTrack🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Sources/CapacitorFFmpegPlugin/CapacitorFFmpeg.swift` around lines 544 - 548, The current code creates an AVURLAsset and binds a discarded audioTrack via guard let audioTrack = asset.tracks(withMediaType: .audio).first then _ = audioTrack; replace this with a direct presence check to avoid extracting an unused variable: call asset.tracks(withMediaType: .audio) and guard that the resulting array is not empty (using !isEmpty) and otherwise throw the same FFmpegError.invalidArgument; update the guard to reference AVURLAsset and the tracks(withMediaType:) call and remove the unused audioTrack binding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ios/Sources/CapacitorFFmpegPlugin/CapacitorFFmpeg.swift`:
- Around line 559-591: The temporary file removal currently in the top-level
defer runs before AVAssetExportSession finishes—remove the defer that calls
fileManager.removeItem(at: temporaryOutputURL) from convertAudio and instead
perform cleanup inside the exportSession.exportAsynchronously completion handler
(after successful move/replace or in the catch/failure branch) so
temporaryOutputURL is only deleted once the async export and any file operations
complete; locate the exportSession.exportAsynchronously block (created via
audioExportSessionFactory) and add cleanup logic there, using
fileManager.removeItem(at:) safely (try? or handling errors) to ensure the temp
file is removed in all terminal paths without racing the export.
---
Nitpick comments:
In `@ios/Sources/CapacitorFFmpegPlugin/CapacitorFFmpeg.swift`:
- Around line 544-548: The current code creates an AVURLAsset and binds a
discarded audioTrack via guard let audioTrack = asset.tracks(withMediaType:
.audio).first then _ = audioTrack; replace this with a direct presence check to
avoid extracting an unused variable: call asset.tracks(withMediaType: .audio)
and guard that the resulting array is not empty (using !isEmpty) and otherwise
throw the same FFmpegError.invalidArgument; update the guard to reference
AVURLAsset and the tracks(withMediaType:) call and remove the unused audioTrack
binding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cfcc9ffd-d813-48c6-a05b-3bb67c185e8f
📒 Files selected for processing (3)
ios/Sources/CapacitorFFmpegPlugin/CapacitorFFmpeg.swiftios/Sources/CapacitorFFmpegPlugin/CapacitorFFmpegPlugin.swiftios/Tests/CapacitorFFmpegPluginTests/CapacitorFFmpegPluginTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- ios/Sources/CapacitorFFmpegPlugin/CapacitorFFmpegPlugin.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd73e3d060
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ios/Sources/CapacitorFFmpegPlugin/CapacitorFFmpeg.swift (2)
555-557: Simplify unreachable ternary branch.Since lines 540-542 enforce that
outputURL.pathExtension.lowercased() == normalizedFormat, the extension can never be empty at this point. The ternary's first branch is unreachable.♻️ Optional simplification
let temporaryOutputURL = outputDirectory .appendingPathComponent(".ffmpeg-audio-\(UUID().uuidString)") - .appendingPathExtension(outputURL.pathExtension.isEmpty ? "m4a" : outputURL.pathExtension) + .appendingPathExtension(normalizedFormat)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Sources/CapacitorFFmpegPlugin/CapacitorFFmpeg.swift` around lines 555 - 557, The ternary in the temporaryOutputURL creation is unnecessary because outputURL.pathExtension is guaranteed non-empty (enforced earlier), so simplify the call by removing the conditional branch and always use outputURL.pathExtension; update the expression that builds temporaryOutputURL (referencing temporaryOutputURL and outputURL.pathExtension) to append the path extension directly instead of using the unreachable ternary.
544-547: Consider async track loading for better compatibility with iOS 15+ guidelines.
asset.tracks(withMediaType:)performs synchronous I/O and is discouraged in iOS 15+ in favor of asynchronous loading vialoadTracks(withMediaType:completionHandler:)or newer async APIs. While this works reliably for local files, Apple recommends the async approach for future compatibility.However, a full conversion would require restructuring the entire method to use async/await or completion-based async loading, as the current completion-handler pattern cannot easily wrap the track check in a Task. If your minimum deployment target is iOS 15+, a proper refactor would involve converting
convertAudioto an async function:func convertAudio( inputPath: String, outputPath: String, format: String ) async throws -> FFmpegConvertedAudio { let asset = AVURLAsset(url: inputURL) let tracks = try await asset.loadTracks(withMediaType: .audio) guard !tracks.isEmpty else { throw FFmpegError.invalidArgument("The input media does not contain an audio track.") } // Continue with async export... }This is a larger refactoring that would affect the entire method signature and call sites. The current synchronous check is acceptable for local files and does not block noticeably in practice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Sources/CapacitorFFmpegPlugin/CapacitorFFmpeg.swift` around lines 544 - 547, The synchronous call asset.tracks(withMediaType:) in convertAudio can perform blocking I/O and should be replaced with the async track-loading API; refactor convertAudio to be async (convertAudio(...) async throws) and replace the synchronous check with the async loader (use try await asset.loadTracks(withMediaType: .audio) and then guard !tracks.isEmpty else throw FFmpegError.invalidArgument("The input media does not contain an audio track.")), updating all call sites to await the new signature or provide an async wrapper if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ios/Sources/CapacitorFFmpegPlugin/CapacitorFFmpeg.swift`:
- Around line 555-557: The ternary in the temporaryOutputURL creation is
unnecessary because outputURL.pathExtension is guaranteed non-empty (enforced
earlier), so simplify the call by removing the conditional branch and always use
outputURL.pathExtension; update the expression that builds temporaryOutputURL
(referencing temporaryOutputURL and outputURL.pathExtension) to append the path
extension directly instead of using the unreachable ternary.
- Around line 544-547: The synchronous call asset.tracks(withMediaType:) in
convertAudio can perform blocking I/O and should be replaced with the async
track-loading API; refactor convertAudio to be async (convertAudio(...) async
throws) and replace the synchronous check with the async loader (use try await
asset.loadTracks(withMediaType: .audio) and then guard !tracks.isEmpty else
throw FFmpegError.invalidArgument("The input media does not contain an audio
track.")), updating all call sites to await the new signature or provide an
async wrapper if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a56a5fc-9da0-43ca-8da8-6b5ac8687451
📒 Files selected for processing (1)
ios/Sources/CapacitorFFmpegPlugin/CapacitorFFmpeg.swift
1 participant
What
convertAudioplugin APIm4aconvertAudioas unsupported on Android and web for nowWhy
How
src/definitions.tswithconvertAudio,ConvertAudioOptions, andConvertAudioResultAVAssetExportSessionusing the Apple M4A presetUNIMPLEMENTEDwith capability metadataTesting
bun run buildbun run verifybun run lintbun run test:webbun run test:androidbun run test:iosNot Tested
AVAssetExportSessionoutput in CI; the iOS success path is covered with a deterministic mocked export session after validating the input asset contains audioSummary by CodeRabbit
New Features
Documentation
Tests
Example app