Bug 132096 – [iOS] Manage AudioSession category according to media type

• Status: ASSIGNED
• Component: Media

[Eric Carlson]

Set the AudioSession category differently for WebAudio and <audio>/<video>.

[Eric Carlson]

<rdar://problem/16565372>

[Eric Carlson]

Created attachment 230033 [details]
Proposed patch.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/audio/ios/AudioSessionIOS.mm:114:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Total errors found: 1 in 13 files


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

[Eric Carlson]

Created attachment 230077 [details]
Updated patch - now with fewer bogus style errors!

[Jer Noble]

Comment on attachment 230077 [details]
Updated patch - now with fewer bogus style errors!

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

r=me, with nits.

> Source/WebCore/platform/audio/mac/MediaSessionManagerMac.cpp:62
> +    if (has(MediaSession::WebAudio)) {
> +        if (count(MediaSession::WebAudio) == 1)
> +            AudioSession::sharedSession().setActive(1);
> +    } else
> +        AudioSession::sharedSession().setActive(0);

the `0` and `1` should be `true` and `false`.  

setActive() should be safe to call multiple times with the same argument, which would greatly simplify this area of code.

[Darin Adler]

Comment on attachment 230077 [details]
Updated patch - now with fewer bogus style errors!

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

> Source/WebCore/platform/audio/ios/AudioDestinationIOS.h:44
> -class AudioDestinationIOS : public AudioDestination, public AudioSessionListener {
> +class AudioDestinationIOS : public AudioDestination, public MediaSessionClient {

Can we derive from MediaSessionClient privately instead of publicly? Can this class be marked final?

> Source/WebCore/platform/audio/ios/AudioDestinationIOS.h:54
> +    virtual void start() override;
> +    virtual void stop() override;
> +    virtual bool isPlaying() override { return m_isPlaying; }
> +
> +    virtual void pausePlayback() override { stop(); }
> +    virtual void resumePlayback() override { start(); }

Can any of these overrides be private instead of public?

> Source/WebCore/platform/audio/mac/MediaSessionManagerMac.cpp:60
> +        if (count(MediaSession::WebAudio) == 1)
> +            AudioSession::sharedSession().setActive(1);

Is leaving the shared session in the same state as before when the count is not 1 intentional? We’re not calling setActive at all, with either true or false, in that case, and maybe that’s right, but if so I don’t understand why.

[Eric Carlson]

(In reply to comment #6)
> (From update of attachment 230077 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230077&action=review
> 
> > Source/WebCore/platform/audio/ios/AudioDestinationIOS.h:44
> > -class AudioDestinationIOS : public AudioDestination, public AudioSessionListener {
> > +class AudioDestinationIOS : public AudioDestination, public MediaSessionClient {
> 
> Can we derive from MediaSessionClient privately instead of publicly? Can this class be marked final?
> 
  Yes, good point.

> > Source/WebCore/platform/audio/ios/AudioDestinationIOS.h:54
> > +    virtual void start() override;
> > +    virtual void stop() override;
> > +    virtual bool isPlaying() override { return m_isPlaying; }
> > +
> > +    virtual void pausePlayback() override { stop(); }
> > +    virtual void resumePlayback() override { start(); }
> 
> Can any of these overrides be private instead of public?
> 
  Yes, I will make them all private.

> > Source/WebCore/platform/audio/mac/MediaSessionManagerMac.cpp:60
> > +        if (count(MediaSession::WebAudio) == 1)
> > +            AudioSession::sharedSession().setActive(1);
> 
> Is leaving the shared session in the same state as before when the count is not 1 intentional? We’re not calling setActive at all, with either true or false, in that case, and maybe that’s right, but if so I don’t understand why.

  This code always calls the first time an AudioDestination is created so leaving the state as-is for subsequent allocations doesn't hurt, but as Jer noted in his review calling setActive(true) multiple times won't hurt and it will make the code simpler, so I will change this as well.

[Eric Carlson]

Created attachment 230094 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 230094 [details]
Patch for landing.

Clearing flags on attachment: 230094

Committed r167767: <http://trac.webkit.org/changeset/167767>