Bug 132096 - [iOS] Manage AudioSession category according to media type
Summary: [iOS] Manage AudioSession category according to media type
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-04-23 18:58 PDT by Eric Carlson
Modified: 2014-05-20 10:58 PDT (History)
7 users (show)

See Also:


Attachments
Proposed patch. (19.94 KB, patch)
2014-04-23 19:14 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Updated patch - now with fewer bogus style errors! (19.97 KB, patch)
2014-04-24 06:56 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch for landing. (20.04 KB, patch)
2014-04-24 11:29 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2014-04-23 18:58:46 PDT
Set the AudioSession category differently for WebAudio and <audio>/<video>.
Comment 1 Eric Carlson 2014-04-23 19:02:23 PDT
<rdar://problem/16565372>
Comment 2 Eric Carlson 2014-04-23 19:14:30 PDT
Created attachment 230033 [details]
Proposed patch.
Comment 3 WebKit Commit Bot 2014-04-23 19:16:34 PDT
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.
Comment 4 Eric Carlson 2014-04-24 06:56:53 PDT
Created attachment 230077 [details]
Updated patch - now with fewer bogus style errors!
Comment 5 Jer Noble 2014-04-24 09:21:57 PDT
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.
Comment 6 Darin Adler 2014-04-24 09:24:58 PDT
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.
Comment 7 Eric Carlson 2014-04-24 11:25:36 PDT
(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.
Comment 8 Eric Carlson 2014-04-24 11:29:04 PDT
Created attachment 230094 [details]
Patch for landing.
Comment 9 WebKit Commit Bot 2014-04-24 12:04:29 PDT
Comment on attachment 230094 [details]
Patch for landing.

Clearing flags on attachment: 230094

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