Fix AVAudioRecorder Android parity with AVFoundation - #29
Conversation
`currentTime` computed `startTime.timeIntervalSinceNow`, which measures from the start date *to now* and is therefore negative for a recording that has already begun. AVFoundation documents the property as "Get the current time of the recording", i.e. a positive elapsed duration. Also accumulate the duration of completed segments so that the elapsed time survives a `pause()`/`record()` cycle instead of restarting from zero, and reset the accumulator in `stop()`.
AVFoundation declares both power accessors as returning `float`:
- (float)peakPowerForChannel:(NSUInteger)channelNumber;
- (float)averagePowerForChannel:(NSUInteger)channelNumber;
Returning `Double` on Android meant that cross-platform code compiling
against the iOS signature failed to compile once transpiled to Kotlin.
AVFoundation's peakPowerForChannel:/averagePowerForChannel: return decibels relative to full scale, ranging from -160 (silence) to 0 (full scale). SkipAV returned MediaRecorder's raw amplitude scaled into 0...1, so a level meter written against the iOS API rendered incorrectly on Android. The conversion is extracted into `amplitudeToDecibels(_:)` so that it can be covered by a test without needing a live MediaRecorder. Note that SkipLib exposes log10(Double) and log10f(Float) but no log10(Float) overload, so the math is done in Double and narrowed to Float at the end.
AVFoundation gates metering behind two members that SkipAV was missing:
@Property(getter=isMeteringEnabled) BOOL meteringEnabled;
- (void)updateMeters;
SkipAV instead declared `meteringEnabled` — the Objective-C name rather than
the name Swift imports — and marked it `@available(*, unavailable)`, and had
no `updateMeters()` at all. The usual iOS metering sequence therefore did not
compile on Android:
recorder.isMeteringEnabled = true
recorder.updateMeters()
let level = recorder.averagePower(forChannel: 0)
Sampling the amplitude in `updateMeters()` also fixes a latent problem with
reading it from the accessors: `MediaRecorder.getMaxAmplitude()` resets on
each read, so calling peak and average in the same frame made the second call
report silence.
AVFoundation declares record as:
- (BOOL)record; // Start or resume recording to file.
SkipAV returned Void and, more importantly, called `prepareToRecord()` on
every invocation. That builds a fresh MediaRecorder over the same output
file, so `pause()` followed by `record()` discarded everything captured
before the pause instead of resuming. `MediaRecorder.resume()` is the
counterpart to the `pause()` already in use and is available at the same
API level.
`record()` now only prepares when there is no recorder to reuse — the one
created in `init` or by an explicit `prepareToRecord()` is kept, matching
AVFoundation's note that prepareToRecord "is called automatically on record".
`pause()` additionally ignores calls made while not recording, since
MediaRecorder throws IllegalStateException in that state.
The channel count was hardcoded to 2, so a mono recording could not be requested. The Showcase app's AudioPlayground already passes `AVNumberOfChannelsKey: 1` and gets stereo on Android. The remaining numeric settings were matched with `as? Int` only. AVFoundation documents these as NSNumber values and iOS code commonly writes `AVSampleRateKey: 44100.0`, which silently fell through to the default. `intSetting(_:)` now accepts Int, Double, and Float. `AVFormatIDKey` is still ignored: MediaRecorder has no linear PCM output format, so only the existing AAC/MPEG-4 combination is available. This is now called out in the README rather than left implicit.
The AVAudioRecorder API table documented `func record()` and a `Double` return from `averagePower(forChannel:)`, both of which no longer match the implementation, and did not mention metering at all. Adds the missing members and a short note covering the decibel scale, the ignored `AVFormatIDKey`, and the RECORD_AUDIO requirement. There were no AVAudioRecorder tests. The new test covers the amplitude to decibel conversion, which is the part that can be exercised without a live MediaRecorder or the RECORD_AUDIO permission.
|
Thank you for your pull request and welcome to the Skip community. We require contributors to sign our contributor license agreement (CLA), and we don't seem to have the user(s) @iyungui on file. In order for us to review and merge your code, for each noted user please add your GitHub username to Skip's .clabot file |
|
recheck |
|
Update on verification. I said in the description that I had not exercised this on Android; I've now done that, so here is what I actually observed. Transpiled Kotlin testsInstalled Gradle and ran the full suite locally, including the Device runRan the module's tests against an API 36 emulator ( What this confirms:
Two caveats I want to be straight about:
One thing worth flagging separately: |
|
Gentle ping on this one — no rush if it's not a priority right now. Since opening it I've verified the Android behavior on an API 36 emulator (details in the comment above), and CI is green with no conflicts against If the size of the change is what's holding it up, I'm happy to close this and open a smaller one with just the two non-behavioral commits — the Otherwise I'm content to leave it sitting; I mostly wanted to check it wasn't blocked on something from my end. |
1 participant
Fixes #28.
Brings the Android
AVAudioRecorderinSources/SkipAV/AVAudioRecorder.swiftin line with the AVFoundation API it stands in for. Only the#elseif SKIPbranch is touched — iOS re-exports AVKit and is unaffected.Each fix is a separate commit, so anything you'd rather not take can be dropped without unpicking the rest.
cff6cf5currentTimereturned a negative elapsed time, and restarted from zero afterpause()906417baveragePower(forChannel:)returnedDouble; the header declaresfloat50517a3-160.0...0.0)3c4cdbdisMeteringEnabledandupdateMeters()were missing, so metering was unusabled195af0record()restarted rather than resumed afterpause(), and returnedVoid2e1ae9bAVNumberOfChannelsKeywas ignored; numeric settings only matchedIntb91f9abBehavior changes
Two of these change what existing Android code observes. Flagging them for a
breaking-changelabel if you agree they warrant one.Metering now returns decibels. Code written against the old 0...1 range needs converting:
record()afterpause()now resumes. Code that relied on the old behavior to restart a take should callstop()and thenrecord().record()gaining aBoolreturn is source-compatible via@discardableResult.Notes on the implementation
updateMeters()samples the amplitude once and caches it, rather than each accessor readingMediaRecorder.getMaxAmplitude()directly. That call resets on every read, so reading it from both accessors made whichever ran second observe silence. The AVFoundation shape happens to be exactly the right fit for this.Doubleand narrowed toFloatat the end: SkipLib exposeslog10(Double)andlog10f(Float)but nolog10(Float)overload.amplitudeToDecibels(_:)ispublic staticand Android-only so the test can reach it from the test module — following the existing convention in this file forinit(platformValue:url:)andkotlin(nocopy:). Happy to make it internal and drop the test if you'd rather not add API surface.AVFormatIDKeyis still ignored, sinceMediaRecorderhas no linear PCM output format. Rather than partially mapping it, the README now states that recordings are always AAC/MPEG-4.Verification
swift testpasses locally: 5/5 Swift-side tests, including the new one. TheXCSkipTestsgradle harness did not run — no Gradle on this machine — so the transpiled Kotlin tests are down to CI..build/plugins/outputs/.../skip/av/AVAudioRecorder.ktand confirmed the conversions resolve againstskip.lib(Int(Number),Float(Number),log10(Double)).pause()/resume()change and the metering values need a device or emulator to confirm. I'd appreciate a check there, or point me at how you'd like it exercised and I'll do it.Marked as draft for that reason. Happy to reshape the scope — the first two commits are the uncontroversial ones if you'd prefer to start there.