Repository navigation
fix(ios): recording resume after an interruption - #1257
SomePersonFromMars wants to merge 25 commits into
Conversation
…ting a closed writer
36d372e to
900e04b
Compare
…s actually delivered
900e04b to
6283fb7
Compare
Also JS handler is only executed on an engine state change.
534c3c0 to
3bd07bf
Compare
3bd07bf to
86a9ae7
Compare
maciejmakowski2003
left a comment
There was a problem hiding this comment.
could you perform the same research on android? in perfect world we would like to provide SotA for interruptions om both platforms.
There was a problem hiding this comment.
could you research and add a section about background resume for playback? I am quite sure that below audio session settings allow it.
AudioManager.setAudioSessionOptions({
iosCategory: 'playback',
iosMode: 'spokenAudio',
})There was a problem hiding this comment.
It is out of scope of this PR, so I will remember to do it in another one.
| - (void)emitInterruptionBeganIfAccepted:(bool)accepted | ||
| { | ||
| if (!self.audioInterruptionsObserved || !accepted) { | ||
| return; | ||
| } | ||
|
|
||
| [self.audioAPIModule invokeHandlerWithEventName:audioapi::AudioEvent::INTERRUPTION | ||
| payload:audioapi::InterruptionPayload{ | ||
| .type = "began", .shouldResume = false}]; | ||
| } |
There was a problem hiding this comment.
let's inline it
There was a problem hiding this comment.
Objective C doesn't really have inline methods, because sending messages always happens in runtime via dynamic dispatch. The closest thing we can get would be the following:
inline void privateHelper(MyClass *self, NSString *value) {
[self doSomething];
}
@implementation MyClass
- (void)publicMethod {
privateHelper(self, @"Hello");
}
@endI think, though, that it would introduce too much complexity.
| AudioEngine *audioEngine = self.audioAPIModule.audioEngine; | ||
|
|
||
| if (self.interruptionEndedDelivered && [audioEngine getState] == AudioEngineStateInterrupted) { | ||
| [self performInterruptionEndOnEngine:audioEngine shouldResume:true]; |
There was a problem hiding this comment.
verify hardcoded shouldResume: true
There was a problem hiding this comment.
Please verify the new caching policy.
| if (!shouldResume && self.inputRegistration == nil) { | ||
| [self stopEngine]; | ||
| [self rebuildAudioEngine]; | ||
| self.state = AudioEngineState::AudioEngineStatePaused; | ||
| [self notifyInput:AudioEngineInputNotificationHardwareChanged]; | ||
| return AudioEngineInterruptionEndOutcomePaused; | ||
| } | ||
|
|
||
| if (![self.sessionManager ensureActive:true error:&error]) { | ||
| NSLog(@"Error while activating audio session after interruption: %@", [error debugDescription]); | ||
| return AudioEngineInterruptionEndOutcomeStillInterrupted; |
There was a problem hiding this comment.
what if we have both player and recorder paused <==> engine paused and interruption happens? does it mean that engine will auto-start after interruption
There was a problem hiding this comment.
Please verify thew new more explicit playback/recording resume policy.
WPT non-regression comparisonPASS — no regressions · 0 improved section(s) · overall 3420 → 3420 (0) Unchanged sections (28)
Baseline: Workflow run · this comment is updated on every push. |
…n device that it still works.
Co-authored-by: Maciej Makowski <maciej.makowski2608@gmail.com>
@maciejmakowski2003 It seems out of scope of this PR, so I will remember to do it in another one. |
mdydek
left a comment
There was a problem hiding this comment.
I think we should in different PR implement also recoverable android from the interruption, because right now the only thing it does is sending event to the js about the interruption which would make both platforms behavior non uniform
| Defaults to `true`. | ||
|
|
||
| Allows the app to play audio in the background on iOS. | ||
| Allows the app to play audio in the background on iOS. Corresponds to adding `audio` to [`UIBackgroundModes`](https://developer.apple.com/documentation/bundleresources/information-property-list/uibackgroundmodes). |
There was a problem hiding this comment.
| Allows the app to play audio in the background on iOS. Corresponds to adding `audio` to [`UIBackgroundModes`](https://developer.apple.com/documentation/bundleresources/information-property-list/uibackgroundmodes). | |
| Allows the app to play audio in the background on iOS. |
there is no need to tell about UIBackgrounModes, user does not care about it
There was a problem hiding this comment.
It is useful for a programmer for the same reason we point to Apple's documentation in the case of AudioManager.setAudioSessionOptions(): In both cases the settings are merely thin wrappers on top of a native iOS specific functionality. We shouldn't try to explain it in our docs, since we would repeat Apple's docs.
|
|
||
| An interruption is the system taking the audio session away due to a phone call, an alarm, or another non-mixable audio session. It deactivates the session and stops the I/O unit. Audio playback and capture therefore stop. | ||
|
|
||
| A resume that completes while the app is still backgrounded is possible only when all of the conditions below hold. |
There was a problem hiding this comment.
| A resume that completes while the app is still backgrounded is possible only when all of the conditions below hold. | |
| A resume that completes while the app is still **backgrounded** is possible only when all of the conditions below hold. |
lets emphasize that it applies to backgrounded application, because normally there is no need for those condition to be true in order to resume after an interruption
| #### Execution | ||
|
|
||
| To complete a background resume, audio I/O must have been started in the foreground, and it must still have been running when the app went to the background. The background audio mode lets that already-running I/O *continue*; it does not allow *starting* audio from the background. iOS treats an `interruption` event of type `ended` as the wake on which a background resume may be attempted. A later background timer is not such a wake. Another app must have released the audio route and the microphone. For recording, the recorder must not have been stopped with [`stop()`](/docs/inputs/audio-recorder#stop), because stopping ends the take. | ||
|
|
||
| #### iOS policy | ||
|
|
||
| iOS policy determines the overall outcome of restarts. Activating the session and starting I/O are independent steps. An active session does not grant permission to run I/O. Even when every condition above is met, iOS may still refuse the restart: it applies internal checks on why the app is awake. The library therefore does not promise a background restart after a call, Siri, or a similar interruption. Because of this, the library retries with a best-effort strategy when the app returns to the foreground after a failed resume. |
There was a problem hiding this comment.
I'm not so sure what to do with this section, to me it looks like a wall of text, even though I am familiar with the concept, lets try to spend a little time to include only high-level informations without going into details, which would be unnecessary for normal dev
| - `IOSMode` maps to [`AVAudioSession.Mode`](https://developer.apple.com/documentation/avfaudio/avaudiosession/mode-swift.struct) | ||
| - `IOSOption` maps to [`AVAudioSession.CategoryOptions`](https://developer.apple.com/documentation/avfaudio/avaudiosession/categoryoptions-swift.struct) | ||
|
|
||
| Not all `iosOptions` are compatible with every `iosCategory`. Passing an invalid combination to the native API will cause the configuration to fail. Always verify valid category and option combinations in [Apple's AVAudioSession Documentation](https://developer.apple.com/documentation/avfaudio/avaudiosession?language=objc). |
There was a problem hiding this comment.
it is repeated info with the warning in the setAudioSessionOptions
| // TODO: Ideally, we would | ||
| // only resume recording and not playback, | ||
| // but it would be complex to implement and | ||
| // the usecase is not common, therefore | ||
| // not resuming the engine is safer. |
There was a problem hiding this comment.
why so little comments? they could easily fit in fewer lines
There was a problem hiding this comment.
That's right. I looked at other comments in places nearby. I see that they most often end at column 80, so that's what I'd stick to most often.
| reprepareForLiveInput(); | ||
| } | ||
|
|
||
| void IOSAudioRecorder::handleCaptureLost() |
There was a problem hiding this comment.
maybe handleRecordingLost, would be more meaningful
| if (!resumeEngine) { | ||
| [self stopEngine]; | ||
| [self rebuildAudioEngine]; | ||
| self.state = AudioEngineState::AudioEngineStatePaused; | ||
| [self notifyInput:AudioEngineInputNotificationHardwareChanged]; | ||
| return AudioEngineInterruptionEndOutcomePaused; | ||
| } | ||
|
|
||
| if (![self.sessionManager ensureActive:true error:&error]) { | ||
| NSLog(@"Error while activating audio session after interruption: %@", [error debugDescription]); | ||
| return AudioEngineInterruptionEndOutcomeStillInterrupted; | ||
| } | ||
|
|
||
| [self stopEngine]; | ||
| [self rebuildAudioEngine]; |
There was a problem hiding this comment.
if we resume or not, either way we stop and rebuild, so we can do it on top and not repeat it
There was a problem hiding this comment.
The problem is that the resumeEngine branch also tries to ensureActive on the sessionManager before stop and rebuild audio engine. An alternative shape is the following:
if (resumeEngine && ![self.sessionManager ensureActive:true error:&error]) {
NSLog(@"Error while activating audio session after interruption: %@", [error debugDescription]);
return AudioEngineInterruptionEndOutcomeStillInterrupted;
}
[self stopEngine];
[self rebuildAudioEngine];
if (!resumeEngine) {
self.state = AudioEngineState::AudioEngineStatePaused;
[self notifyInput:AudioEngineInputNotificationHardwareChanged];
return AudioEngineInterruptionEndOutcomePaused;
}But I think that the cognitive load of resumeEngine condition appearing twice and the if statement with an && in the condition is not worth it. I think that it is easier to read if the branches are expressed explicitely.
| selector:@selector(handleWillEnterForeground:) | ||
| name:UIApplicationWillEnterForegroundNotification | ||
| object:nil]; | ||
| [self.notificationCenter addObserver:self | ||
| selector:@selector(handleDidBecomeActive:) |
There was a problem hiding this comment.
| selector:@selector(handleWillEnterForeground:) | |
| name:UIApplicationWillEnterForegroundNotification | |
| object:nil]; | |
| [self.notificationCenter addObserver:self | |
| selector:@selector(handleDidBecomeActive:) | |
| selector:@selector(retryInterruptedRecordingIfNeeded:) | |
| name:UIApplicationWillEnterForegroundNotification | |
| object:nil]; | |
| [self.notificationCenter addObserver:self | |
| selector:@selector(retryInterruptedRecordingIfNeeded:) |
i think the name of event gives enough context about what it is being responded to, so we can call this function directly without serving as "proxy"
There was a problem hiding this comment.
I can imagine that in the future we might want to add more tasks that we would like to execute on such a notification. In such a case it might be tempting to add those under retryInterruptedRecordingIfNeeded for example, because it works. In any case, the point is to emphasize that even though for now there is only one task on that handle, it isn't necessary.
| - (void)emitInterruptionBeganIfAccepted:(bool)accepted | ||
| { | ||
| if (!self.audioInterruptionsObserved || !accepted) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
if we immediately return if accepted is false we can check this condition in the caller and not call this function at all
There was a problem hiding this comment.
this way we also omit awkward ifAccepted part
Closes #1220
observeAudioInterruptions(true)no longer replaces native recovery. NativeonInterruptionBegin/onInterruptionEndalways run. The flag only enables JSinterruptionevents, which matches the existingAudioManagerdocs. Apps that subscribed because observing used to skip native resume should not own engine recovery for a recorder (in particular, do not callRecorder.pause()onbegan- that moves the engine toPausedand disables the native retry). Playback can still pause onbegan. Native pauses onbegan, but it does not resume audio contexts.Introduced changes
Fixes iOS recording that never came back after an interruption. Updated library policy: the same take resumes when the app is, or becomes, foreground. Background restart after the interrupter leaves is best-effort; iOS can still refuse I/O while the app is backgrounded.
Responds to the issue
§1
onInterruptionEndnever activated the session; failure poisonedIdle; no input-node guardstartEngine).startAndReturnErrorfailure, stayInterruptedand return (do not setIdle). A later retry can run.Interruptedand treat it as capture lost (do not look like a successful start with no buffers).§2 Docs vs behavior for
observeAudioInterruptionsobserveAudioInterruptions, an appropriate handler runs only whenAudioEngineactually transitioned to a new state. For example, if the engine wasInterrupted, the handler runs on a successful transition toRunning.§3 No-op
setActive:falsestill ran deactivationsetAudioSessionActivity(false)only calls session-deactivation handling if the session was actually managed and active beforesetActive. It no longer transitionsInterruptedtoPaused, which used to disableonInterruptionEnd.§3 / footnote -
resume()/ storeRecordingon a dead engineresume()still does not have a return type, but it is now guaranteed thatisRecording()is true if and only if the audio engine is not in an interrupted state and the recorder is actually running.AudioEngine'sonInterruptionEndrecovery afterAVAudioSessionInterruptionTypeEndedcan still fail. That can happen, for example, due to a race with a higher-priority app or internal iOS privacy policy. In that case we retry once the app returns to the foreground, which is then guaranteed by iOS to succeed.Consumer note -
mixWithOthers/CannotInterruptOthersplayAndRecordis required to avoidCannotInterruptOtherswhen activating in the background, and is not sufficient for background restart. The Record demo enablesmixWithOtherswithdefaultmode.Additional useful tweaks and fixes
WillEnterForegroundNotificationandDidBecomeActiveNotification. iOS guarantees that a foreground audio start succeeds. The retry uses a cachedshouldResumeflag.handleInputConfigurationChangeto handle errors. A failed engine resume now finalizes the audio recorder.HardwareChanged) must open a new rotating segment instead of callingrotateFiles()on a writer that is already closed.IOSRotatingFileWriter::reprepareStreamFormatreopens when the inner file is closed;rotateFiles()no-ops if nothing is open.startIfNecessary, matching the recorder. A later recorder start must not play leftover source audio.observeAudioInterruptions(true); freeze the waveform onbegan, unfreeze only onended.Docs
AudioManagersubsection "Resume recording after an interruption".Manual tests and reproductions used while debugging
This issue is hard to reproduce with automated tests. During research and implementation I used the following manual tests, with the
apps/fabric-exampleapp and the included Recorder demo.Checklist