Bug 171403 – Add audio device change notifications to AudioSession.

• Component: Media
• Status: REOPENED

[Jer Noble]

Add audio device change notifications to AudioSession.

[Jer Noble]

Created attachment 308532 [details]
Patch

[Jer Noble]

Created attachment 308533 [details]
Patch

[Jer Noble]

Created attachment 308534 [details]
Patch

[Jer Noble]

Created attachment 308535 [details]
Patch

[Eric Carlson]

Comment on attachment 308534 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308534&action=review

> Source/WebCore/platform/audio/AudioSession.h:52
> +        virtual void audioInputDeviceChanged() { };
> +        virtual void audioOutputDeviceChanged() { };

Nit: these don't tell the observer which device changed, just that the list of devices changed, so I think "Device" should be pluralized.

> Source/WebCore/platform/audio/mac/AudioSessionMac.mmSource/WebCore/platform/audio/mac/AudioSessionMac.cpp:87
> +    AudioObjectAddPropertyListenerBlock(defaultDevice(), &outputDeviceAddress, dispatch_get_main_queue(), m_private->outputDeviceChangedBlock);

Why do this in the destructor?

> Source/WebCore/platform/audio/mac/AudioSessionMac.mmSource/WebCore/platform/audio/mac/AudioSessionMac.cpp:279
> +        m_private->lastMutedState = isCurrentlyMuted;

Nit: this should probably be done before calling observers in case an observer does something that causes this to be called recursively.

[Jer Noble]

(In reply to Eric Carlson from comment #5)
> Comment on attachment 308534 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308534&action=review
> 
> > Source/WebCore/platform/audio/AudioSession.h:52
> > +        virtual void audioInputDeviceChanged() { };
> > +        virtual void audioOutputDeviceChanged() { };
> 
> Nit: these don't tell the observer which device changed, just that the list
> of devices changed, so I think "Device" should be pluralized.

Well, it's a notification that the "current" device changed. So maybe I'll change it to say that.

> > Source/WebCore/platform/audio/mac/AudioSessionMac.mmSource/WebCore/platform/audio/mac/AudioSessionMac.cpp:87
> > +    AudioObjectAddPropertyListenerBlock(defaultDevice(), &outputDeviceAddress, dispatch_get_main_queue(), m_private->outputDeviceChangedBlock);
> 
> Why do this in the destructor?

Because it's supposed to be AudioObject_Remove_PropertyListenerBlock. :-/

> > Source/WebCore/platform/audio/mac/AudioSessionMac.mmSource/WebCore/platform/audio/mac/AudioSessionMac.cpp:279
> > +        m_private->lastMutedState = isCurrentlyMuted;
> 
> Nit: this should probably be done before calling observers in case an
> observer does something that causes this to be called recursively.

Ok.

[Build Bot]

Comment on attachment 308535 [details]
Patch

Attachment 308535 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3627250

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 308547 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308535 [details]
Patch

Attachment 308535 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3627253

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 308548 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308535 [details]
Patch

Attachment 308535 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3627254

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 308550 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Jer Noble]

Created attachment 308565 [details]
Patch

[Build Bot]

Attachment 308565 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/audio/mac/AudioSessionMac.mm:74:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/audio/mac/AudioSessionMac.mm:269:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/audio/mac/AudioSessionMac.mm:282:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 308745 [details]
Patch

[Build Bot]

Attachment 308745 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/audio/mac/AudioSessionMac.mm:74:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/audio/mac/AudioSessionMac.mm:269:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/audio/mac/AudioSessionMac.mm:282:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Comment on attachment 308745 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308745&action=review

> Source/WebCore/platform/audio/mac/AudioSessionMac.mmSource/WebCore/platform/audio/mac/AudioSessionMac.cpp:266
> +    if (m_private->observers.size() > 1)
>          return;

Why "m_private->observers.size() > 1", should it be " > 0"?

[Jer Noble]

Comment on attachment 308745 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308745&action=review

>> Source/WebCore/platform/audio/mac/AudioSessionMac.mmSource/WebCore/platform/audio/mac/AudioSessionMac.cpp:266
>>          return;
> 
> Why "m_private->observers.size() > 1", should it be " > 0"?

Whoops! Good catch.

[Jer Noble]

(In reply to Jer Noble from comment #18)
> Comment on attachment 308745 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308745&action=review
> 
> >> Source/WebCore/platform/audio/mac/AudioSessionMac.mmSource/WebCore/platform/audio/mac/AudioSessionMac.cpp:266
> >>          return;
> > 
> > Why "m_private->observers.size() > 1", should it be " > 0"?
> 
> Whoops! Good catch.

Wait, no, this is correct.  We should bail out if we've already registered as an observer, so this should happen on the 0 -> 1 boundary. Hence early return if > 1

[WebKit Commit Bot]

Comment on attachment 308745 [details]
Patch

Clearing flags on attachment: 308745

Committed r216024: <http://trac.webkit.org/changeset/216024>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

This change caused LayoutTests to exit early on ios-simulator due to assertion failures:

ASSERTION FAILED: m_private->notificationCallbacks.isEmpty()
/Volumes/Data/slave/ios-simulator-10-debug/build/Source/WebCore/platform/audio/ios/AudioSessionIOS.mm(238) : void WebCore::AudioSession::addObserver(WebCore::AudioSession::Observer &)

https://build.webkit.org/builders/Apple%20iOS%2010%20Simulator%20Debug%20WK2%20%28Tests%29/builds/1007

[Ryan Haddad]

Reverted r216024 for reason:

This change caused ios-simulator LayoutTests to exit early with assertion failures.

Committed r216050: <http://trac.webkit.org/changeset/216050>