Plumb audio focus support for spitzer clients.

Plumb audio focus support for spitzer clients.

Adds a new audio effects parameters "FOCUSABLE" which can be set by clients
which support platform specific audio focus concepts.  This tells the
AudioRendererHost that it should check with the focus controller before
allowing audio playback.

MediaWebContentsObserverAndroid is the focus controller in this case. Since
audio streams can be created far ahead of player creation, the controller
must support deferred responses for the focus request.

Focus grants are handled per render frame. I.e. the ARH requests focus
status using a render pid+render frame id and internally the MWCOA vends
status based on the frame id (once approved). While a render frame has an
active (playing) media session, all focus requests are granted.

BUG=529887
TEST=audio doesn't play until after focus request.

[DaleCurtis, 2016-01-12 02:00:21]

Patchset #1 (id:1) has been deleted

[DaleCurtis, 2016-01-12 02:11:26]

Description was changed from

==========
Plumb audio focus support for spitzer clients.

Adds a new audio effects parameters "FOCUSABLE" which can be set by clients
which support platform specific audio focus concepts.  This tells the
AudioRendererHost that it should check with the focus controller before
allowing audio playback.

MediaWebContentsObserverAndroid is the focus controller in this case. Since
audio streams can be created far ahead of player creation, the controller
must support deferred responses for the focus request.

Focus grants are handled per render frame. I.e. the ARH requests focus
status using a render pid+render frame id and internally the MWCOA vends
status based on the frame id (once approved).
// TODO(dalecurtis): Sanity check with //content// team.

BUG=
==========

to

==========
Plumb audio focus support for spitzer clients.

** not ready for review **

Adds a new audio effects parameters "FOCUSABLE" which can be set by clients
which support platform specific audio focus concepts.  This tells the
AudioRendererHost that it should check with the focus controller before
allowing audio playback.

MediaWebContentsObserverAndroid is the focus controller in this case. Since
audio streams can be created far ahead of player creation, the controller
must support deferred responses for the focus request.

Focus grants are handled per render frame. I.e. the ARH requests focus
status using a render pid+render frame id and internally the MWCOA vends
status based on the frame id (once approved).
// TODO(dalecurtis): Sanity check with //content// team.

BUG=
==========

[DaleCurtis, 2016-01-12 02:11:26]

dalecurtis@chromium.org changed reviewers:
+ grunell@chromium.org, mlamouri@chromium.org

[DaleCurtis, 2016-01-12 02:12:51]

This is not ready for review and may ultimately just be merged into
https://codereview.chromium.org/1570043002/ (certainly parts of it will be) but
+grunell for early high level feedback on using AudioParameter::effects for this
use case and how it might impact the shared mixer code.

[Henrik Grunell, 2016-01-12 08:16:05]

On 2016/01/12 02:12:51, DaleCurtis wrote:
> This is not ready for review and may ultimately just be merged into
> https://codereview.chromium.org/1570043002/ (certainly parts of it will be)
but
> +grunell for early high level feedback on using AudioParameter::effects for
this
> use case and how it might impact the shared mixer code.

I think using AudioParameter::effects is OK, although it's typically used for
enabling HW or OS capabilities. Afaict this is the first playout side effect,
and that's why we currently expect no effects in the mixer manager. Are you
using another mixer for the focusable streams because they should be
paused/resumed? The focusable streams will be paused/resumed independently,
right? So if my assumption is correct, should they perhaps not be mixed together
at all? (Please elaborate on the ideas in the mixing part.)

[DaleCurtis, 2016-01-12 18:31:51]

Focus control is handled via the creator the stream, so in my proposed model the
actual ARH::OnPlayStream() just needs to verify that focus has been approved for
the given tab.  Which would happen if:
1. focus has already been requested by the stream creator.
2. another stream already has requested focus.

The only problem we're trying to solve is playing audio when its not allowed by
the system (mlamouri, please correct me otherwise) so its okay to use an
existing focus request for approval.

[Henrik Grunell, 2016-01-13 08:12:03]

On 2016/01/12 18:31:51, DaleCurtis wrote:
> Focus control is handled via the creator the stream, so in my proposed model
the
> actual ARH::OnPlayStream() just needs to verify that focus has been approved
for
> the given tab.  Which would happen if:
> 1. focus has already been requested by the stream creator.
> 2. another stream already has requested focus.
> 
> The only problem we're trying to solve is playing audio when its not allowed
by
> the system (mlamouri, please correct me otherwise) so its okay to use an
> existing focus request for approval.

OK, in that case this seems fine.  But we probably want to only use the
FOCUSABLE effect and not all effects in the mixer key, but those details can be
dealt with later.

[Henrik Grunell, 2016-01-13 09:40:35]

On 2016/01/13 08:12:03, Henrik Grunell wrote:
> On 2016/01/12 18:31:51, DaleCurtis wrote:
> > Focus control is handled via the creator the stream, so in my proposed model
> the
> > actual ARH::OnPlayStream() just needs to verify that focus has been approved
> for
> > the given tab.  Which would happen if:
> > 1. focus has already been requested by the stream creator.
> > 2. another stream already has requested focus.
> > 
> > The only problem we're trying to solve is playing audio when its not allowed
> by
> > the system (mlamouri, please correct me otherwise) so its okay to use an
> > existing focus request for approval.
> 
> OK, in that case this seems fine.  But we probably want to only use the
> FOCUSABLE effect and not all effects in the mixer key, but those details can
be
> dealt with later.

CC henrika & olka FYI

Dale, I'm however not exactly sure about what this about and how it should work.
My understanding is that the client(/individual stream) should consider the OS
audio focus (e.g.
http://developer.android.com/training/managing-audio/audio-focus.html on
Android) if it supports it. Could you give some use case or point to a design
doc? Based on your last comment it seems fine in terms of the mixing, but since
I'm confused about what would happen if the OS informs about lost focus.

[DaleCurtis, 2016-01-13 20:59:46]

The answer is the same as before, handling loss of focus is not the stream's
responsibility but the owner of the stream. The stream only needs to check that
audio focus has been approved. Any loss of focus or other control is the
responsibility of the stream owner.

[Henrik Grunell, 2016-01-14 11:19:29]

On 2016/01/13 20:59:46, DaleCurtis wrote:
> The answer is the same as before, handling loss of focus is not the stream's
> responsibility but the owner of the stream. The stream only needs to check
that
> audio focus has been approved. Any loss of focus or other control is the
> responsibility of the stream owner.

OK, seems fine with me.

[mlamouri (slow - plz ping), 2016-01-14 15:03:41]

couple of drive-by comments. Will have another look later.

https://codereview.chromium.org/1580493004/diff/20001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.cc (right):

https://codereview.chromium.org/1580493004/diff/20001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:84: if (rfh) {
nit:
```
if (!rfh)
  return;
```

Also, probably no need to null check `rfh`. `::FromRenderFrameHost()` will not
assume `rfh` is not null.

https://codereview.chromium.org/1580493004/diff/20001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:85: WebContents* wc =
WebContents::FromRenderFrameHost(rfh);
nit: we usually use `web_contents`, I can't remember seeing `wc` in the code
base.

https://codereview.chromium.org/1580493004/diff/20001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:86: if (wc) {
nit:
```
if (!wc)
  return;
```

[DaleCurtis, 2016-01-14 22:46:11]

Patchset #2 (id:40001) has been deleted

[DaleCurtis, 2016-01-14 23:15:23]

Patchset #2 (id:60001) has been deleted

[DaleCurtis, 2016-01-14 23:16:10]

Cleaned up and pulled things that belong in the other patch set out.

https://codereview.chromium.org/1580493004/diff/20001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.cc (right):

https://codereview.chromium.org/1580493004/diff/20001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:84: if (rfh) {
On 2016/01/14 15:03:41, Mounir Lamouri wrote:
> nit:
> ```
> if (!rfh)
>   return;
> ```
> 
> Also, probably no need to null check `rfh`. `::FromRenderFrameHost()` will not
> assume `rfh` is not null.

There was some reason I wrote it this way, but I don't see why now, so done.

https://codereview.chromium.org/1580493004/diff/20001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:85: WebContents* wc =
WebContents::FromRenderFrameHost(rfh);
On 2016/01/14 15:03:41, Mounir Lamouri wrote:
> nit: we usually use `web_contents`, I can't remember seeing `wc` in the code
> base.

Done.

https://codereview.chromium.org/1580493004/diff/20001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:86: if (wc) {
On 2016/01/14 15:03:41, Mounir Lamouri wrote:
> nit:
> ```
> if (!wc)
>   return;
> ```

Done.

[mlamouri (slow - plz ping), 2016-01-19 17:18:18]

It looks generally good. Will have another look when it's "ready" for review ;)

https://codereview.chromium.org/1580493004/diff/80001/content/browser/media/a...
File content/browser/media/android/media_web_contents_observer_android.cc
(right):

https://codereview.chromium.org/1580493004/diff/80001/content/browser/media/a...
content/browser/media/android/media_web_contents_observer_android.cc:53:
OnSuspend(player_id_);
I guess that would no longer be needed?

https://codereview.chromium.org/1580493004/diff/80001/media/blink/webaudiosou...
File media/blink/webaudiosourceprovider_impl.h (right):

https://codereview.chromium.org/1580493004/diff/80001/media/blink/webaudiosou...
media/blink/webaudiosourceprovider_impl.h:43: void enable_audio_focus() {
enable_audio_focus_ = true; }
style: set_enable_audio_focus(bool), otherwise it looks like a getter.

[DaleCurtis, 2016-02-03 01:53:30]

Patchset #4 (id:120001) has been deleted

[DaleCurtis, 2016-02-03 02:10:08]

Description was changed from

==========
Plumb audio focus support for spitzer clients.

** not ready for review **

Adds a new audio effects parameters "FOCUSABLE" which can be set by clients
which support platform specific audio focus concepts.  This tells the
AudioRendererHost that it should check with the focus controller before
allowing audio playback.

MediaWebContentsObserverAndroid is the focus controller in this case. Since
audio streams can be created far ahead of player creation, the controller
must support deferred responses for the focus request.

Focus grants are handled per render frame. I.e. the ARH requests focus
status using a render pid+render frame id and internally the MWCOA vends
status based on the frame id (once approved).
// TODO(dalecurtis): Sanity check with //content// team.

BUG=
==========

to

==========
Plumb audio focus support for spitzer clients.

Adds a new audio effects parameters "FOCUSABLE" which can be set by clients
which support platform specific audio focus concepts.  This tells the
AudioRendererHost that it should check with the focus controller before
allowing audio playback.

MediaWebContentsObserverAndroid is the focus controller in this case. Since
audio streams can be created far ahead of player creation, the controller
must support deferred responses for the focus request.

Focus grants are handled per render frame. I.e. the ARH requests focus
status using a render pid+render frame id and internally the MWCOA vends
status based on the frame id (once approved). While a render frame has an
active (playing) media session, all focus requests are granted.

BUG=529887
TEST=audio doesn't play until after focus request.
==========

[DaleCurtis, 2016-02-03 02:15:31]

This is ready for review (though still needs unit tests) if we still want to
require this of Spitzer clients. The latest UMA stat show that 99.5% of the time
the request is approved. WDYT Mounir?

https://codereview.chromium.org/1580493004/diff/80001/media/blink/webaudiosou...
File media/blink/webaudiosourceprovider_impl.h (right):

https://codereview.chromium.org/1580493004/diff/80001/media/blink/webaudiosou...
media/blink/webaudiosourceprovider_impl.h:43: void enable_audio_focus() {
enable_audio_focus_ = true; }
On 2016/01/19 at 17:18:17, Mounir Lamouri wrote:
> style: set_enable_audio_focus(bool), otherwise it looks like a getter.

There's no real preference over all of chrome (20 vs 21 hits), but favors
"enable_xxx()" in media/ (2 vs 0 :)
https://code.google.com/p/chromium/codesearch#search/&q=%22void%20enable_%22&...
https://code.google.com/p/chromium/codesearch#search/&q=%22void%20set_enable_...

I think enable_xxx() works better since a getter for such a variable would be
something like is_audio_focus_enabled().

[mlamouri (slow - plz ping), 2016-02-03 15:20:52]

Hmm, I started to review this then I realized we probably don't need all of that
and saw this is what you suggested in your latest comment.

Do you know:
- how long it takes between Play and Pause if audio focus is rejected? (on high
end and low end phones)
- if you can actually hear something?

I'm afraid this is making the code quite complex and opens a lot of door for
mistakes so I would be willing to live without real audio focus grabbing if it
doesn't have real consequences on our users.

https://codereview.chromium.org/1580493004/diff/140001/content/browser/media/...
File content/browser/media/android/media_web_contents_observer_android.cc
(right):

https://codereview.chromium.org/1580493004/diff/140001/content/browser/media/...
content/browser/media/android/media_web_contents_observer_android.cc:152:
kv.second->is_playing()) {
Couldn't a controller be playing even though it doesn't have the right kind of
audio focus?

https://codereview.chromium.org/1580493004/diff/140001/content/browser/media/...
File content/browser/media/android/media_web_contents_observer_android.h
(right):

https://codereview.chromium.org/1580493004/diff/140001/content/browser/media/...
content/browser/media/android/media_web_contents_observer_android.h:64: void
WaitForAudioFocusAsyncInternal(int render_frame_id,
Add comment saying that `callback` may never be called if the frame associated
with `render_frame_id` is deleted.

https://codereview.chromium.org/1580493004/diff/140001/content/browser/media/...
content/browser/media/android/media_web_contents_observer_android.h:65: const
base::Closure& focus_cb) override;
nit: `callback`

https://codereview.chromium.org/1580493004/diff/140001/content/browser/media/...
content/browser/media/android/media_web_contents_observer_android.h:111:
MediaSessionMap media_session_map_;
I was confused by your change because `media_session_map_` looks like a map of
all the media sessions but it's actually a map of all the controllers. Maybe
worth fixing in a follow-up?

https://codereview.chromium.org/1580493004/diff/140001/content/browser/media/...
content/browser/media/android/media_web_contents_observer_android.h:115: using
FocusWaiter = std::pair<int, base::Closure>;
nit: `using FocusCallback`? or `FocusCallbackWrapper`?

https://codereview.chromium.org/1580493004/diff/140001/content/browser/media/...
content/browser/media/android/media_web_contents_observer_android.h:116:
std::list<FocusWaiter> focus_waiters_;
nit: focus_callbacks_

[DaleCurtis, 2016-02-04 00:03:51]

On 2016/02/03 at 15:20:52, mlamouri wrote:
> Hmm, I started to review this then I realized we probably don't need all of
that and saw this is what you suggested in your latest comment.
> 
> Do you know:
> - how long it takes between Play and Pause if audio focus is rejected? (on
high end and low end phones)

I don't have an exact number today since I'm away from my desk, but in testing
on an N4 I could only get playout if I rapidly hammered on the play button after
mediasession switched it to a pause. This causes small chunks, probably ~20ms of
audio, to be written out before the pause can come back.

> - if you can actually hear something?

You can if you are hammering on the play/pause. Now that we have
SetVolumeMultiplier though there's an alternate solution. We can essentially
force the volume multiplier to zero on the renderer side during
WMPD::AddObserver() and WMPD::DidPause(). The volume multiplier won't be set
back to 1 until the focus request from the browser is successful (which always
sends vm=1.0 AFAICT). I will try this tomorrow and see how it feels, but it
should only take a couple lines to implement and be resilient to the
playback-hammer.

> 
> I'm afraid this is making the code quite complex and opens a lot of door for
mistakes so I would be willing to live without real audio focus grabbing if it
doesn't have real consequences on our users.
>

[DaleCurtis, 2016-02-04 22:24:31]

https://codereview.chromium.org/1671603002 implements muting which is much
cleaner. I think we should ditch this CL. Let me know and I'll close this one.

[Henrik Grunell, 2016-03-10 21:11:43]

On 2016/02/04 22:24:31, DaleCurtis wrote:
> https://codereview.chromium.org/1671603002 implements muting which is much
> cleaner. I think we should ditch this CL. Let me know and I'll close this one.

From my point of view: go for the other approach.