Add AudioStreamRegistry. Move stream counting logic

Add AudioStreamRegistry. Move stream counting logic

from AudioRendererHost to AudioStreamRegistry. AudioStreamRegistry lives in RendererProcessHost and is passed through AudioRendererHost to AudioDelegate.

BUG=670383
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

[Max Morin, 2016-12-15 13:09:09]

Description was changed from

==========
Add AudioStreamRegistry. Move stream counting logic
from AudioRendererHost to AudioStreamRegistry.

BUG=670383
==========

to

==========
Add AudioStreamRegistry. Move stream counting logic
from AudioRendererHost to AudioStreamRegistry.

BUG=670383
==========

[Max Morin, 2016-12-15 13:09:09]

maxmorin@chromium.org changed reviewers:
+ olka@chromium.org

[Max Morin, 2016-12-15 13:13:59]

Description was changed from

==========
Add AudioStreamRegistry. Move stream counting logic
from AudioRendererHost to AudioStreamRegistry.

BUG=670383
==========

to

==========
Add AudioStreamRegistry. Move stream counting logic

from AudioRendererHost to AudioStreamRegistry. AudioStreamRegistry lives in
RendererProcessHost and is passed through AudioRendererHost to AudioDelegate.

BUG=670383
==========

[Max Morin, 2016-12-15 13:22:25]

Olga: WDYT of this approach? If you think it makes sense, tests will have to be
touched up a bit and debug recording should be added to AudioOutputDelegate
before landing this, so I'll wait until Q1. This class will also make more sense
when we have AudioInputDelegate, but I don't think we have to wait for that (but
the commented-out lines for input are for illustrating the final API, and will
be removed before landing).

+CC Grunell as this somewhat pertains to debug recording.

[o1ka, 2016-12-16 09:48:16]

Yes - that's what we need. Looks good!

https://codereview.chromium.org/2578983003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/audio_stream_registry_impl.cc (right):

https://codereview.chromium.org/2578983003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/audio_stream_registry_impl.cc:57:
output_streams_.insert(stream);
DCheck that unique value is inserted?

https://codereview.chromium.org/2578983003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/audio_stream_registry_impl.cc:70: void
AudioStreamRegistryImpl::OutputStreamStateChanged(bool playing) {
Actually, to make it safer and more debuggable, we may want to store the current
state for each stream.

https://codereview.chromium.org/2578983003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/audio_stream_registry_impl.cc:86: if
(!base::AtomicRefCountDec(&num_playing_output_streams_)) {
Check that it's not below zero

https://codereview.chromium.org/2578983003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/audio_stream_registry_impl.h (right):

https://codereview.chromium.org/2578983003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/audio_stream_registry_impl.h:40: using
StreamSet = std::unordered_set<Stream*>;
Have you considered using <set>? We hardly ever have more than 250 streams and
pointer comparison is cheaper than hash calculation.

https://codereview.chromium.org/2578983003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2578983003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:681:
audio_stream_registry_(new AudioStreamRegistryImpl(id_)),
It would be cleaner to have a factory method in AudioStreamRegistry.

[Henrik Grunell, 2016-12-16 09:52:48]

On 2016/12/15 13:22:25, Max Morin OOO 22 Dec - 9 Jan wrote:
> Olga: WDYT of this approach? If you think it makes sense, tests will have to
be
> touched up a bit and debug recording should be added to AudioOutputDelegate
> before landing this, so I'll wait until Q1. This class will also make more
sense
> when we have AudioInputDelegate, but I don't think we have to wait for that
(but
> the commented-out lines for input are for illustrating the final API, and will
> be removed before landing).
> 
> +CC Grunell as this somewhat pertains to debug recording.

I plan to add debug recording close to the OS, in the platform/audio system
specific implementations. Whether it will be added close to the process boundary
is to decide later, when we start using close-to-OS recordings. Don't wait for
that.

[Max Morin, 2016-12-19 15:34:25]

Description was changed from

==========
Add AudioStreamRegistry. Move stream counting logic

from AudioRendererHost to AudioStreamRegistry. AudioStreamRegistry lives in
RendererProcessHost and is passed through AudioRendererHost to AudioDelegate.

BUG=670383
==========

to

==========
Add AudioStreamRegistry. Move stream counting logic

from AudioRendererHost to AudioStreamRegistry. AudioStreamRegistry lives in
RendererProcessHost and is passed through AudioRendererHost to AudioDelegate.

BUG=670383
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Max Morin, 2016-12-19 16:29:03]

I added debug recording stubs in AudioOutputDelegate. PTAL (no hurry, I don't
intend to land it this year).

https://codereview.chromium.org/2578983003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/audio_stream_registry_impl.cc (right):

https://codereview.chromium.org/2578983003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/audio_stream_registry_impl.cc:57:
output_streams_.insert(stream);
On 2016/12/16 09:48:16, o1ka OOO Dec 23 - Jan 3 wrote:
> DCheck that unique value is inserted?

Done.

https://codereview.chromium.org/2578983003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/audio_stream_registry_impl.cc:70: void
AudioStreamRegistryImpl::OutputStreamStateChanged(bool playing) {
On 2016/12/16 09:48:16, o1ka OOO Dec 23 - Jan 3 wrote:
> Actually, to make it safer and more debuggable, we may want to store the
current
> state for each stream.

Done. (Debug only).

https://codereview.chromium.org/2578983003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/audio_stream_registry_impl.cc:86: if
(!base::AtomicRefCountDec(&num_playing_output_streams_)) {
On 2016/12/16 09:48:16, o1ka OOO Dec 23 - Jan 3 wrote:
> Check that it's not below zero

Done.

https://codereview.chromium.org/2578983003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/audio_stream_registry_impl.h (right):

https://codereview.chromium.org/2578983003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/audio_stream_registry_impl.h:40: using
StreamSet = std::unordered_set<Stream*>;
On 2016/12/16 09:48:16, o1ka OOO Dec 23 - Jan 3 wrote:
> Have you considered using <set>? We hardly ever have more than 250 streams and
> pointer comparison is cheaper than hash calculation.

Done.

https://codereview.chromium.org/2578983003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2578983003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:681:
audio_stream_registry_(new AudioStreamRegistryImpl(id_)),
On 2016/12/16 09:48:16, o1ka OOO Dec 23 - Jan 3 wrote:
> It would be cleaner to have a factory method in AudioStreamRegistry.

Done. The interface in AudioStreamRegistry is for registering streams, so I
added the factory function to AudioStreamRegistryImpl instead (which also has
HasActiveAudio and will have debug recording control later).

[Henrik Grunell, 2016-12-21 10:39:12]

On 2016/12/19 16:29:03, Max Morin OOO 22 Dec - 9 Jan wrote:
> I added debug recording stubs in AudioOutputDelegate. PTAL (no hurry, I don't
> intend to land it this year).

The stubs are the same as I've added in my work (not uploaded quite yet), so I'm
fine with that. :) FYI and FTR, I want to inject the writer when creating the
stream, but it needs more discussion which ongoing in that CL.

[o1ka, 2016-12-22 10:46:52]

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
File content/browser/renderer_host/media/audio_output_delegate.h (right):

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_delegate.h:100: void
EnableDebugRecording(const base::FilePath& base_file_name) override;
Oh, I just realized. AusioStreamRegistry will live in the audio process, right?
Enable/Disable debug recording per stream should be a process-local operation.
And AuiodOutputDelegate is basically our representation of IPC. So, it looks
like AudioOutputController is one who should be AusioStreamRegistry::Stream.

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
File content/browser/renderer_host/media/audio_stream_registry.h (right):

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_stream_registry.h:16: // This class
tracks all the input and output streams and enables stream
Debug recording is enabled/disabled for all hosts at once. (We need to check if
there are are any plans/usecases to operate per host there). It should be a Mojo
interface to audio process.
We may need to have a single AudioStreamRegistry for all hosts; we won't have
ARH in Mojo audio process.

BTW, we missed AecDumpMsg_EnableAecDump from renderer/browser Mojo interfaces.

We need to design debug recording interactions before proceeding with this CL.
(But we may want to split stream counting/debug recording into separate CLs when
it comes to implementation)

- Sorry, I did not realize some aspects in the first review round.

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
File content/browser/renderer_host/media/audio_stream_registry_impl.cc (right):

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_stream_registry_impl.cc:70:
debug_recording_path_ = base_file_name;
Thread safety?

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_stream_registry_impl.cc:71: for
(Stream* stream : output_streams_)
Thread safety?

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_stream_registry_impl.cc:125:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
Thread check first, then access stream_currently_playing_?

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2273: if
(audio_stream_registry_) {
We should not have two approaches side by side, should we?

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2275: // alive now it
means that the task to delete |audio_stream_registry_|
Could you clarify in the comment how one follows from another?

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:533:
AudioStreamRegistryImpl::UniquePtr audio_stream_registry_;
Where will it live in audio process?

[Max Morin, 2017-01-09 15:34:23]

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
File content/browser/renderer_host/media/audio_output_delegate.h (right):

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_delegate.h:100: void
EnableDebugRecording(const base::FilePath& base_file_name) override;
I think AudioStreamRegistry will live in the browser process. The stream
counting is needed for RPH to know if the renderer should be backgrounded, and I
don't think we want the mojo jump every time we check that. Possibly,
AudioStreamRegistry just gets updates to a HasAudio bit from the audio process.
The stream counting (for stats) can be done completely in the browser process.
Finally, debug recording is a bit tricky (since we'll need to supply file
descriptors to the audio process). We might have the stream factories in the
audio process return two objects: an (Input/Output)Stream which the browser
sends to renderer and another interface to the stream which the browser keeps
with (Enable/Disable)DebugRecording (and maybe something else).

It doesn't matter much if AudioOutputDelegate or AudioOutputController is the
Stream, I picked AudioOutputDelegate because it lives on the IO thread, so we
have an easier time with thread management like this.

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
File content/browser/renderer_host/media/audio_stream_registry.h (right):

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_stream_registry.h:16: // This class
tracks all the input and output streams and enables stream
On 2016/12/22 10:46:52, o1ka wrote:
> Debug recording is enabled/disabled for all hosts at once. (We need to check
if
> there are are any plans/usecases to operate per host there). It should be a
Mojo
> interface to audio process.

Browser will need an interface to each individual stream (since it must supply
file descriptors), so the UI can just iterate over all the stream and enable
debug recording for each one.

> We may need to have a single AudioStreamRegistry for all hosts; we won't have
> ARH in Mojo audio process.

AudioStreamRegistry stays in browser process (see other comment).

> We need to design debug recording interactions before proceeding with this CL.
> (But we may want to split stream counting/debug recording into separate CLs
when
> it comes to implementation)

Right. If the new output debug recording will be controlled by the AudioManager,
we can just scrap the debug recording stuff in this CL.

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
File content/browser/renderer_host/media/audio_stream_registry_impl.cc (right):

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_stream_registry_impl.cc:70:
debug_recording_path_ = base_file_name;
On 2016/12/22 10:46:52, o1ka wrote:
> Thread safety?

Done.

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_stream_registry_impl.cc:71: for
(Stream* stream : output_streams_)
On 2016/12/22 10:46:52, o1ka wrote:
> Thread safety?

Done.

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_stream_registry_impl.cc:125:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
On 2016/12/22 10:46:52, o1ka wrote:
> Thread check first, then access stream_currently_playing_?

Done.

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2273: if
(audio_stream_registry_) {
On 2016/12/22 10:46:52, o1ka wrote:
> We should not have two approaches side by side, should we?

Right, null check is not actually necessary.

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2275: // alive now it
means that the task to delete |audio_stream_registry_|
On 2016/12/22 10:46:52, o1ka wrote:
> Could you clarify in the comment how one follows from another?

Done.

[o1ka, 2017-01-10 11:47:40]

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
File content/browser/renderer_host/media/audio_output_delegate.h (right):

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_output_delegate.h:100: void
EnableDebugRecording(const base::FilePath& base_file_name) override;
On 2017/01/09 15:34:23, Max Morin wrote:
> I think AudioStreamRegistry will live in the browser process. The stream
> counting is needed for RPH to know if the renderer should be backgrounded, and
I
> don't think we want the mojo jump every time we check that. Possibly,
> AudioStreamRegistry just gets updates to a HasAudio bit from the audio
process.
> The stream counting (for stats) can be done completely in the browser process.
> Finally, debug recording is a bit tricky (since we'll need to supply file
> descriptors to the audio process). We might have the stream factories in the
> audio process return two objects: an (Input/Output)Stream which the browser
> sends to renderer and another interface to the stream which the browser keeps
> with (Enable/Disable)DebugRecording (and maybe something else).
> 
> It doesn't matter much if AudioOutputDelegate or AudioOutputController is the
> Stream, I picked AudioOutputDelegate because it lives on the IO thread, so we
> have an easier time with thread management like this.

Ah ok, sounds good. Stream interfaces need to be figured, not sure how well it
will look in Mojo world.

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
File content/browser/renderer_host/media/audio_stream_registry.h (right):

https://codereview.chromium.org/2578983003/diff/100001/content/browser/render...
content/browser/renderer_host/media/audio_stream_registry.h:16: // This class
tracks all the input and output streams and enables stream
On 2017/01/09 15:34:23, Max Morin wrote:
> On 2016/12/22 10:46:52, o1ka wrote:
> > Debug recording is enabled/disabled for all hosts at once. (We need to check
> if
> > there are are any plans/usecases to operate per host there). It should be a
> Mojo
> > interface to audio process.
> 
> Browser will need an interface to each individual stream (since it must supply
> file descriptors), so the UI can just iterate over all the stream and enable
> debug recording for each one.
> 
> > We may need to have a single AudioStreamRegistry for all hosts; we won't
have
> > ARH in Mojo audio process.
> 
> AudioStreamRegistry stays in browser process (see other comment).

Ah ok, now I see. Fine then!
> 
> > We need to design debug recording interactions before proceeding with this
CL.
> > (But we may want to split stream counting/debug recording into separate CLs
> when
> > it comes to implementation)
> 
> Right. If the new output debug recording will be controlled by the
AudioManager,
> we can just scrap the debug recording stuff in this CL.

Let's keep debug recording in a separate CL, to pair it with input one.

https://codereview.chromium.org/2578983003/diff/120001/content/browser/render...
File content/browser/renderer_host/media/audio_stream_registry.h (right):

https://codereview.chromium.org/2578983003/diff/120001/content/browser/render...
content/browser/renderer_host/media/audio_stream_registry.h:33: virtual
~AudioStreamRegistry() {}
protected?

https://codereview.chromium.org/2578983003/diff/120001/content/browser/render...
File content/browser/renderer_host/media/audio_stream_registry_impl.h (right):

https://codereview.chromium.org/2578983003/diff/120001/content/browser/render...
content/browser/renderer_host/media/audio_stream_registry_impl.h:42: // void
RegisterInputStream(Stream* stream) override;
Add TODOs instead of commented out methods? (here, and there was some other
place I think)

https://codereview.chromium.org/2578983003/diff/120001/content/browser/render...
content/browser/renderer_host/media/audio_stream_registry_impl.h:68:
std::map<Stream*, bool> stream_currently_playing_;
How about to always have the current status (=map instead of set)? 
BTW vector will be more efficient in both cases, but up to you.

[Max Morin, 2017-01-12 12:42:21]

Much simpler now! Also, I added tests for the histograms since they are an
important part of this class.

[Max Morin, 2017-01-12 13:52:59]

PTAL.

[o1ka, 2017-01-12 18:09:46]

Nice and simple :)
lgtm

https://codereview.chromium.org/2578983003/diff/180001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/2578983003/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:550: const
AudioStreamRegistryImpl::UniquePtr audio_stream_registry_;
"Must outlive |audio_renderer_host_|" comment?

[DaleCurtis, 2017-01-12 19:22:11]

dalecurtis@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[DaleCurtis, 2017-01-12 19:22:12]

https://codereview.chromium.org/2578983003/diff/180001/content/browser/render...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/2578983003/diff/180001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:319:
stream_registry_->RegisterStream();
Typically we'd return a bound id or something here so you don't have to worry
about counting above when Deregistering.

https://codereview.chromium.org/2578983003/diff/180001/content/browser/render...
File content/browser/renderer_host/media/audio_stream_registry_impl.cc (right):

https://codereview.chromium.org/2578983003/diff/180001/content/browser/render...
content/browser/renderer_host/media/audio_stream_registry_impl.cc:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
I think instead all of this can be deleted. AudioStreamMonitor contains
everything needed to handle these cases now I think. It's notified when new
streams are added and fans out notifications when audio starts/stops.

https://codereview.chromium.org/2578983003/diff/180001/content/browser/render...
File content/browser/renderer_host/media/audio_stream_registry_impl.h (right):

https://codereview.chromium.org/2578983003/diff/180001/content/browser/render...
content/browser/renderer_host/media/audio_stream_registry_impl.h:24: explicit
AudioStreamRegistryImpl(int render_process_id);
Typically we don't expose both a normal constructor and a Create() method.

https://codereview.chromium.org/2578983003/diff/180001/content/browser/render...
content/browser/renderer_host/media/audio_stream_registry_impl.h:45: };
DISALLOW_COPY_AND_ASSIGN

[DaleCurtis, 2017-01-12 19:23:04]

In case my comment is a bit buried: this is a nice cleanup, but I think we can
go further and delete all of this code wholesale instead. AudioStreamMonitor
should be able to replace all of these behaviors.

[Max Morin, 2017-01-16 11:48:21]

The CQ bit was checked by maxmorin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 11:48:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-16 12:19:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 12:19:16]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Max Morin, 2017-01-16 14:43:16]

On 2017/01/12 19:23:04, DaleCurtis wrote:
> In case my comment is a bit buried: this is a nice cleanup, but I think we can
> go further and delete all of this code wholesale instead. AudioStreamMonitor
> should be able to replace all of these behaviors.

This would be cool, but ASM is per-webcontents and this code is for the RPH. To
use the AudioStreamMonitor, it would need a way to notify the RPH when audio
state is changed, and the RPH would need a way to iterate over its webcontents
(there doesn't seem to be one, so I guess we're not supposed to iterate over
webcontents like that?). I'm not sure this is simpler than just having this data
in the RPH.

[DaleCurtis, 2017-01-17 19:15:01]

On 2017/01/16 at 14:43:16, maxmorin wrote:
> On 2017/01/12 19:23:04, DaleCurtis wrote:
> > In case my comment is a bit buried: this is a nice cleanup, but I think we
can
> > go further and delete all of this code wholesale instead. AudioStreamMonitor
> > should be able to replace all of these behaviors.
> 
> This would be cool, but ASM is per-webcontents and this code is for the RPH.
To use the AudioStreamMonitor, it would need a way to notify the RPH when audio
state is changed, and the RPH would need a way to iterate over its webcontents
(there doesn't seem to be one, so I guess we're not supposed to iterate over
webcontents like that?). I'm not sure this is simpler than just having this data
in the RPH.

You shouldn't need to do that, AudioStreamMonitor knows which RPH corresponds to
which stream, so it can just call RPH::AudioStateChanged() directly. You'll need
to add a audible bit there though now. Another way would be to tell the
WebContents which RPH was audible and it can take care of the forwarding. I
think that's more logical, it just requires changing the
WC::OnAudioStateChanged() method to take a |render_process_id|. Either way some
method will need a new parameter, up to you, but either should work.

[Max Morin, 2017-01-18 16:27:39]

On 2017/01/17 19:15:01, DaleCurtis wrote:
> On 2017/01/16 at 14:43:16, maxmorin wrote:
> > On 2017/01/12 19:23:04, DaleCurtis wrote:
> > > In case my comment is a bit buried: this is a nice cleanup, but I think we
> can
> > > go further and delete all of this code wholesale instead.
AudioStreamMonitor
> > > should be able to replace all of these behaviors.
> > 
> > This would be cool, but ASM is per-webcontents and this code is for the RPH.
> To use the AudioStreamMonitor, it would need a way to notify the RPH when
audio
> state is changed, and the RPH would need a way to iterate over its webcontents
> (there doesn't seem to be one, so I guess we're not supposed to iterate over
> webcontents like that?). I'm not sure this is simpler than just having this
data
> in the RPH.
> 
> You shouldn't need to do that, AudioStreamMonitor knows which RPH corresponds
to
> which stream, so it can just call RPH::AudioStateChanged() directly. You'll
need
> to add a audible bit there though now. Another way would be to tell the
> WebContents which RPH was audible and it can take care of the forwarding. I
> think that's more logical, it just requires changing the
> WC::OnAudioStateChanged() method to take a |render_process_id|. Either way
some
> method will need a new parameter, up to you, but either should work.

I still don't get what you're saying :). It sounds like the RPH needs to keep a
collection of (or at least the number of) webcontents with active audio so that
is can figure out if it has active audio (which would need code very similar to
this code). In addition the (max) # of streams for a render process is also
needed somewhere, and it's unclear where that should be in this case.

[DaleCurtis, 2017-01-18 20:19:46]

Sorry I didn't explain correctly / clearly. Stream counts can move into
AudioStreamMonitor as a lazy instance like you have it now. All calls there
should happen on the UI thread.

From here there are two paths, either preserve the existing has_playing_stream
== has_active_audio or convert to using the audible state.

If we keep the original behavior the only change is that during
ASM::(Start|Stop)MonitoringStream() we'll notify RPHI::AudioStateChanged() with
the currently playing status which it can save internally and use instead of
ARH::HasActiveAudio(). This drops the atomics since they are no longer needed.

To switch to an audibility based system we can pursue the approach I mentioned
before. Where the ASM will track audibility state more carefully by
render_process_id. This information is then either fanned out from the ASM or by
the WC when notified that a given process id is audible/inaudible.

Currently the ASM only queries until it finds the first stream w/ non-zero power
levels for audibility, so we'd need to change that to query all streams and fan
notifications for audible ones as well as remembering which ones were audible
and are now no longer audible.

Switching to audibility based is probably better done as a followup CL after
nuking the duplicate functionality.

[Max Morin, 2017-01-25 17:35:23]

I'm afraid I still don't get it at all. :)

On 2017/01/18 20:19:46, DaleCurtis wrote:
> Sorry I didn't explain correctly / clearly. Stream counts can move into
> AudioStreamMonitor as a lazy instance like you have it now. All calls there
> should happen on the UI thread.
The stream count per process isn't currently in a lazy instance, and having a
map from render process id to count as global state seems very unnatural,
compared to having this data in the actual render process host it concerns.


> From here there are two paths, either preserve the existing has_playing_stream
> == has_active_audio or convert to using the audible state.
> 
> If we keep the original behavior the only change is that during
> ASM::(Start|Stop)MonitoringStream() we'll notify RPHI::AudioStateChanged()
with
> the currently playing status which it can save internally and use instead of
> ARH::HasActiveAudio(). This drops the atomics since they are no longer needed.
What saves the playing status internally? The ASM? How will it know if another
ASM has an active stream?

> To switch to an audibility based system we can pursue the approach I mentioned
> before. Where the ASM will track audibility state more carefully by
> render_process_id. This information is then either fanned out from the ASM or
by
> the WC when notified that a given process id is audible/inaudible.
> 
> Currently the ASM only queries until it finds the first stream w/ non-zero
power
> levels for audibility, so we'd need to change that to query all streams and
fan
> notifications for audible ones as well as remembering which ones were audible
> and are now no longer audible.
> 
> Switching to audibility based is probably better done as a followup CL after
> nuking the duplicate functionality.

[DaleCurtis, 2017-01-25 22:49:12]

On 2017/01/25 at 17:35:23, maxmorin wrote:
> I'm afraid I still don't get it at all. :)
> 
> On 2017/01/18 20:19:46, DaleCurtis wrote:
> > Sorry I didn't explain correctly / clearly. Stream counts can move into
> > AudioStreamMonitor as a lazy instance like you have it now. All calls there
> > should happen on the UI thread.
> The stream count per process isn't currently in a lazy instance, and having a
map from render process id to count as global state seems very unnatural,
compared to having this data in the actual render process host it concerns.

Sorry, this doesn't make sense. 1. The CL you have uploaded presents
AudioStreamTracker as a lazy instance. 2. All we care about is a max stream
count for browser process.

> 
> 
> > From here there are two paths, either preserve the existing
has_playing_stream
> > == has_active_audio or convert to using the audible state.
> > 
> > If we keep the original behavior the only change is that during
> > ASM::(Start|Stop)MonitoringStream() we'll notify RPHI::AudioStateChanged()
with
> > the currently playing status which it can save internally and use instead of
> > ARH::HasActiveAudio(). This drops the atomics since they are no longer
needed.
> What saves the playing status internally? The ASM? How will it know if another
ASM has an active stream?

RPHI can keep a count of audible streams to make the determination. Might be
easier to follow if RPHI::AudioStateChanged() was split into
RPHI::OnAudioStarted() and RPHI::OnAudioStopped(); or something similar.

> 
> > To switch to an audibility based system we can pursue the approach I
mentioned
> > before. Where the ASM will track audibility state more carefully by
> > render_process_id. This information is then either fanned out from the ASM
or by
> > the WC when notified that a given process id is audible/inaudible.
> > 
> > Currently the ASM only queries until it finds the first stream w/ non-zero
power
> > levels for audibility, so we'd need to change that to query all streams and
fan
> > notifications for audible ones as well as remembering which ones were
audible
> > and are now no longer audible.
> > 
> > Switching to audibility based is probably better done as a followup CL after
> > nuking the duplicate functionality.

[Max Morin, 2017-01-26 16:09:18]

On 2017/01/25 22:49:12, DaleCurtis wrote:
> On 2017/01/25 at 17:35:23, maxmorin wrote:
> > I'm afraid I still don't get it at all. :)
> > 
> > On 2017/01/18 20:19:46, DaleCurtis wrote:
> > > Sorry I didn't explain correctly / clearly. Stream counts can move into
> > > AudioStreamMonitor as a lazy instance like you have it now. All calls
there
> > > should happen on the UI thread.
> > The stream count per process isn't currently in a lazy instance, and having
a
> map from render process id to count as global state seems very unnatural,
> compared to having this data in the actual render process host it concerns.
> 
> Sorry, this doesn't make sense. 1. The CL you have uploaded presents
> AudioStreamTracker as a lazy instance. 2. All we care about is a max stream
> count for browser process.

I agree that the global max stream count is fine as a lazy instance. The last
time I asked Grunell, he wanted to keep the per-renderer max stream count (for
evaluation of renderer-side mixing). In the current code, it's an interger in
ARH, and in this CL, it's in AudioStreamRegistry.

> > > From here there are two paths, either preserve the existing
> has_playing_stream
> > > == has_active_audio or convert to using the audible state.
> > > 
> > > If we keep the original behavior the only change is that during
> > > ASM::(Start|Stop)MonitoringStream() we'll notify RPHI::AudioStateChanged()
> with
> > > the currently playing status which it can save internally and use instead
of
> > > ARH::HasActiveAudio(). This drops the atomics since they are no longer
> needed.
> > What saves the playing status internally? The ASM? How will it know if
another
> ASM has an active stream?
> 
> RPHI can keep a count of audible streams to make the determination. Might be
> easier to follow if RPHI::AudioStateChanged() was split into
> RPHI::OnAudioStarted() and RPHI::OnAudioStopped(); or something similar.

Sure, I could keep this data directly in the RPHI if you like. I thought you
meant to ASM was supposed to know somehow :)

> > > To switch to an audibility based system we can pursue the approach I
> mentioned
> > > before. Where the ASM will track audibility state more carefully by
> > > render_process_id. This information is then either fanned out from the ASM
> or by
> > > the WC when notified that a given process id is audible/inaudible.
> > > 
> > > Currently the ASM only queries until it finds the first stream w/ non-zero
> power
> > > levels for audibility, so we'd need to change that to query all streams
and
> fan
> > > notifications for audible ones as well as remembering which ones were
> audible
> > > and are now no longer audible.
> > > 
> > > Switching to audibility based is probably better done as a followup CL
after
> > > nuking the duplicate functionality.

[o1ka, 2017-01-27 12:43:44]

> I think instead all of this can be deleted. AudioStreamMonitor contains
> everything needed to handle these cases now I think. It's notified when new
> streams are added and fans out notifications when audio starts/stops.
> 

This is how I see it:

We have 2 entities interested in audio stream states:
1) RendererProcessHost needs to know if a renderer can go to background, which
is possible if there are no active audio streams. AudioStreamTracker aggregates
this information for RPH.
2) AudioStreamMonitor needs to know if it should be polling a stream for audio
volume levels; this information is aggregated per WebContents.

AudioStreamMonitor is basically "WebContentsAudioPowerMonitor".
AudioStreamTracker is "RendererAudioStreamTracker".

They can be considered as two subscribers for stream state change notifications,
and they use these notifications for different purposes and aggregate
information on different levels.

I don't think we should combine these two entities, it will complicate the code
and object relations.

In the future we may figure out a more unified way to subscribe for stream state
changes - we will actually have to, before switching to a separate process.

Am I missing something?

[DaleCurtis, 2017-01-27 20:30:01]

On 2017/01/27 at 12:43:44, olka wrote:
> > I think instead all of this can be deleted. AudioStreamMonitor contains
> > everything needed to handle these cases now I think. It's notified when new
> > streams are added and fans out notifications when audio starts/stops.
> > 
> 
> This is how I see it:
> 
> We have 2 entities interested in audio stream states:
> 1) RendererProcessHost needs to know if a renderer can go to background, which
is possible if there are no active audio streams. AudioStreamTracker aggregates
this information for RPH.
> 2) AudioStreamMonitor needs to know if it should be polling a stream for audio
volume levels; this information is aggregated per WebContents.
> 
> AudioStreamMonitor is basically "WebContentsAudioPowerMonitor".
> AudioStreamTracker is "RendererAudioStreamTracker".
> 
> They can be considered as two subscribers for stream state change
notifications, and they use these notifications for different purposes and
aggregate information on different levels.
> 
> I don't think we should combine these two entities, it will complicate the
code and object relations.
> 
> In the future we may figure out a more unified way to subscribe for stream
state changes - we will actually have to, before switching to a separate
process.
> 
> Am I missing something?

I agree with your points 1+2, but I disagree with your conclusion. Read another
way, why should we have 3 classes which all count streams in subtly different
ways? As written we have monitor, tracker, and registry which feels fairly
absurd from a complexity standpoint. Their current functionality is not ideal
either; thus my proposal above which gets us closer to the ideal functionality
with fewer classes. I also don't think my proposal complicated class
functionality at all. Apologies for not being able to get the core of my point
across more clearly, so I just wrote one way this could be done:
https://codereview.chromium.org/2655413004

[o1ka, 2017-01-30 16:15:05]

On 2017/01/27 20:30:01, DaleCurtis wrote:
> On 2017/01/27 at 12:43:44, olka wrote:
> > > I think instead all of this can be deleted. AudioStreamMonitor contains
> > > everything needed to handle these cases now I think. It's notified when
new
> > > streams are added and fans out notifications when audio starts/stops.
> > > 
> > 
> > This is how I see it:
> > 
> > We have 2 entities interested in audio stream states:
> > 1) RendererProcessHost needs to know if a renderer can go to background,
which
> is possible if there are no active audio streams. AudioStreamTracker
aggregates
> this information for RPH.
> > 2) AudioStreamMonitor needs to know if it should be polling a stream for
audio
> volume levels; this information is aggregated per WebContents.
> > 
> > AudioStreamMonitor is basically "WebContentsAudioPowerMonitor".
> > AudioStreamTracker is "RendererAudioStreamTracker".
> > 
> > They can be considered as two subscribers for stream state change
> notifications, and they use these notifications for different purposes and
> aggregate information on different levels.
> > 
> > I don't think we should combine these two entities, it will complicate the
> code and object relations.
> > 
> > In the future we may figure out a more unified way to subscribe for stream
> state changes - we will actually have to, before switching to a separate
> process.
> > 
> > Am I missing something?
> 
> I agree with your points 1+2, but I disagree with your conclusion. Read
another
> way, why should we have 3 classes which all count streams in subtly different
> ways? As written we have monitor, tracker, and registry which feels fairly
> absurd from a complexity standpoint. Their current functionality is not ideal
> either; thus my proposal above which gets us closer to the ideal functionality
> with fewer classes. I also don't think my proposal complicated class
> functionality at all. Apologies for not being able to get the core of my point
> across more clearly, so I just wrote one way this could be done:
> https://codereview.chromium.org/2655413004

Thanks for making a CL! Now I see what you mean. Looks like we were missing
understanding how RPH and WebContents are related.

[Max Morin, 2017-01-31 17:15:42]

Description was changed from

==========
Add AudioStreamRegistry. Move stream counting logic

from AudioRendererHost to AudioStreamRegistry. AudioStreamRegistry lives in
RendererProcessHost and is passed through AudioRendererHost to AudioDelegate.

BUG=670383
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add AudioStreamRegistry. Move stream counting logic

from AudioRendererHost to AudioStreamRegistry. AudioStreamRegistry lives in
RendererProcessHost and is passed through AudioRendererHost to AudioDelegate.

BUG=670383
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========