Handle AudioSession interruption and resume
<rdar://problem/32925263>
Created attachment 313639 [details] Proposed patch.
Comment on attachment 313639 [details] Proposed patch. LGTM. I tried to think about cases where we would run into issues with all these states but just got some questions. I wonder how much we would be able to test that all these state handling are fine with unit testing. View in context: https://bugs.webkit.org/attachment.cgi?id=313639&action=review > Source/WebCore/platform/mediastream/ios/CoreAudioCaptureSourceIOS.h:38 > +private: No need for space above private? > Source/WebCore/platform/mediastream/ios/CoreAudioCaptureSourceIOS.h:42 > + virtual ~CoreAudioCaptureSourceIOS(); Probably no need for virtual here. > Source/WebCore/platform/mediastream/ios/CoreAudioCaptureSourceIOS.mm:88 > + return; Can this case actually happen? If the listener callback is null, it is probably because it was invalidated and will not receive this notification? > Source/WebCore/platform/mediastream/ios/CoreAudioCaptureSourceIOS.mm:92 > + else { Should we assert that the interruptionType is AVAudioSessionInterruptionTypeEnded? Maybe overkill? > Source/WebCore/platform/mediastream/ios/CoreAudioCaptureSourceIOS.mm:97 > +#endif Probably no need for #if/#endif, except if wanting to do release logging, which might be useful. Do we want to log that an interruption started also? > Source/WebCore/platform/mediastream/ios/CoreAudioCaptureSourceIOS.mm:98 > + _callback->endInterruption(); If there is an error in setActive, should we still call endInterruption or do specific handling? > Source/WebCore/platform/mediastream/ios/CoreAudioCaptureSourceIOS.mm:107 > + return; Is it needed? > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:90 > + bool suspended() const { return m_suspended; } Since we have suspend which is close, maybe we should name it isSuspended()? > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:605 > +OSStatus CoreAudioSharedUnit::resume() This seems very close to startProducingData(). Maybe we can do some nice refactoring? > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:618 > + LOG(Media, "CoreAudioSharedUnit::start(%p) AudioOutputUnitStart failed with error %d (%.4s)", this, (int)err, (char*)&err); ::start -> ::resume. > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:668 > + These two lines are also good for iOS, in particular persistentID which is missing in iOS. Maybe, to help the readability, you could introduce some helper functions like deviceFromID and createCaptureSource that would have the #if/#endif and keep CoreAudioCaptureSource::create free from #if/#endif. The alternative would be to have a CoreAudioCaptureSourceIOS::create and have this handling in the factory. > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:685 > + return CaptureSourceOrError(*source); Use source.releaseNonNull() probably or you can stick with auto in both places. > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:858 > + m_suspendType = unit.isProducingData() ? SuspensionType::WhilePlaying : SuspensionType::WhilePaused; What happens in the case of the source being played before interruption, so m_suspendType is SuspensionType::WhilePlaying but the user paused the source during interruption? > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:885 > + return; Why is the handling of m_suspendType and m_reconfigurationState different? > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:888 > + scheduleDeferredTask([this, type, &unit] { No need to pass unit, sing you can get it through CoreAudioSharedUnit::singleton(). > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.h:73 > + void scheduleReconfiguration(); Maybe you can make WebCoreAudioCaptureSourceIOSListener a friend?
Created attachment 313667 [details] Patch for landing.
Comment on attachment 313639 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=313639&action=review >> Source/WebCore/platform/mediastream/ios/CoreAudioCaptureSourceIOS.mm:88 >> + return; > > Can this case actually happen? > If the listener callback is null, it is probably because it was invalidated and will not receive this notification? True in theory, I will leave it and add an ASSERT. >> Source/WebCore/platform/mediastream/ios/CoreAudioCaptureSourceIOS.mm:92 >> + else { > > Should we assert that the interruptionType is AVAudioSessionInterruptionTypeEnded? Maybe overkill? Might be overkill, but fixed. >> Source/WebCore/platform/mediastream/ios/CoreAudioCaptureSourceIOS.mm:98 >> + _callback->endInterruption(); > > If there is an error in setActive, should we still call endInterruption or do specific handling? We definitely want to end the interruption because we won't get another notification, but we will attempt to call setActive later when a media element begins playing so I think this is OK. >> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:90 >> + bool suspended() const { return m_suspended; } > > Since we have suspend which is close, maybe we should name it isSuspended()? Fixed >> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:605 >> +OSStatus CoreAudioSharedUnit::resume() > > This seems very close to startProducingData(). > Maybe we can do some nice refactoring? Good idea, done. >> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:668 >> + > > These two lines are also good for iOS, in particular persistentID which is missing in iOS. > Maybe, to help the readability, you could introduce some helper functions like deviceFromID and createCaptureSource that would have the #if/#endif and keep CoreAudioCaptureSource::create free from #if/#endif. > The alternative would be to have a CoreAudioCaptureSourceIOS::create and have this handling in the factory. Restructured. >> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:685 >> + return CaptureSourceOrError(*source); > > Use source.releaseNonNull() probably or you can stick with auto in both places. Reverted because of the restructuring of the platform-specific code. >> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:858 >> + m_suspendType = unit.isProducingData() ? SuspensionType::WhilePlaying : SuspensionType::WhilePaused; > > What happens in the case of the source being played before interruption, so m_suspendType is SuspensionType::WhilePlaying but the user paused the source during interruption? Good point! >> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:885 >> + return; > > Why is the handling of m_suspendType and m_reconfigurationState different? We only stop the unit for a Suspension, but we tear it down completely and rebuild it for a reconfiguration. >> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:888 >> + scheduleDeferredTask([this, type, &unit] { > > No need to pass unit, sing you can get it through CoreAudioSharedUnit::singleton(). Yes, but we may call it twice so that will require two calls to the getter.
Attachment 313667 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/ios/CoreAudioCaptureSourceIOS.mm:103: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 313713 [details] Patch for landing.
Comment on attachment 313713 [details] Patch for landing. Clearing flags on attachment: 313713 Committed r218746: <http://trac.webkit.org/changeset/218746>
This patched landed but this bug was not closed: https://github.com/WebKit/WebKit/commit/222633f9e8ce0aedf71f94653126404dc1de8a3c Marking this as "RESOLVED FIXED". Thanks!