Part 1: Plumb render view ID to render host

Part 1: Plumb render view ID to render host

This is part 1 of the work to add audio indicator to tabs.

This CL plumbs the render view ID from the renderer to the browser.

BUG=64215

[Chris Rogers, 2012-07-31 06:59:11]

I'm a bit concerned about the massive propagation of render view ID.  It seems
really unfortunate.  Can we think carefully to see if there's a simpler and more
elegant way of getting this information to the browser.

[sail, 2012-07-31 07:55:31]

On 2012/07/31 06:59:11, Chris Rogers wrote:
> I'm a bit concerned about the massive propagation of render view ID.  It seems
> really unfortunate.  Can we think carefully to see if there's a simpler and
more
> elegant way of getting this information to the browser.

Hey Chris, thanks for the quick review!

It seems like we could skip passing the render view ID once CreateStream() is
called. AudioRendererHost::OnCreateStream caches the render view ID in an
AudioEntry object so we can just reuse that.

As for the content/renderer/media/* code, how about having RenderViewImpl own a
AudioOutputDevice object and passing that around?

[Chris Rogers, 2012-07-31 08:17:06]

+scherkus

Aside from the terrifying propagation of render view ID, this seems very
interesting.  Could you please provide somewhat more background on the overall
project?

Others who work more in the chromium code base may have some other suggestions
for simplifying this.  Maybe this is already the best we can do, but I would
hope we could somehow derive the render view ID closer to the IPC layer.  But
this kind of thing is far from the areas I know well, so maybe I'm way off base
here and that isn't possible.

On 2012/07/31 07:55:31, sail wrote:
> On 2012/07/31 06:59:11, Chris Rogers wrote:
> > I'm a bit concerned about the massive propagation of render view ID.  It
seems
> > really unfortunate.  Can we think carefully to see if there's a simpler and
> more
> > elegant way of getting this information to the browser.
> 
> Hey Chris, thanks for the quick review!
> 
> It seems like we could skip passing the render view ID once CreateStream() is
> called. AudioRendererHost::OnCreateStream caches the render view ID in an
> AudioEntry object so we can just reuse that.
> 
> As for the content/renderer/media/* code, how about having RenderViewImpl own
a
> AudioOutputDevice object and passing that around?

[tommi (sloooow) - chröme, 2012-07-31 10:52:41]

https://chromiumcodereview.appspot.com/10836025/diff/1/content/browser/render...
File content/browser/renderer_host/media/audio_renderer_host.h (right):

https://chromiumcodereview.appspot.com/10836025/diff/1/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.h:130: void
OnPlayStream(int render_view_id, int stream_id);
just including the render view id in OnCreateStream should be enough and just
store it with the AudioEntry.

https://chromiumcodereview.appspot.com/10836025/diff/1/content/renderer/media...
File content/renderer/media/audio_device_factory.h (right):

https://chromiumcodereview.appspot.com/10836025/diff/1/content/renderer/media...
content/renderer/media/audio_device_factory.h:32: static
media::AudioInputDevice* NewInputDevice();
don't we need the same for input devices?

https://chromiumcodereview.appspot.com/10836025/diff/1/content/renderer/media...
content/renderer/media/audio_device_factory.h:42: virtual
media::AudioRendererSink* CreateOutputDevice() = 0;
Also add the parameter to these two factory functions.  They should be identical
to the static methods above.

https://chromiumcodereview.appspot.com/10836025/diff/1/content/renderer/media...
File content/renderer/media/audio_output_ipc.h (right):

https://chromiumcodereview.appspot.com/10836025/diff/1/content/renderer/media...
content/renderer/media/audio_output_ipc.h:24: class CONTENT_EXPORT
AudioOutputIPC
I don't think it's a good idea to call this class exactly the same name (aside
from the namespace) as the interface it implements.  Let's at least append
'Impl' to the name.

https://chromiumcodereview.appspot.com/10836025/diff/1/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10836025/diff/1/content/renderer/rende...
content/renderer/render_view_impl.cc:2443: // channel count.
file a bug?

https://chromiumcodereview.appspot.com/10836025/diff/1/content/renderer/rende...
content/renderer/render_view_impl.cc:2470: layout = CHANNEL_LAYOUT_STEREO;
add break;

https://chromiumcodereview.appspot.com/10836025/diff/1/content/renderer/rende...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://chromiumcodereview.appspot.com/10836025/diff/1/content/renderer/rende...
content/renderer/renderer_webkitplatformsupport_impl.cc:599: CHECK(false);
add a comment that explains why this CHECK is here.

https://chromiumcodereview.appspot.com/10836025/diff/1/media/audio/audio_outp...
File media/audio/audio_output_device.h (right):

https://chromiumcodereview.appspot.com/10836025/diff/1/media/audio/audio_outp...
media/audio/audio_output_device.h:98: // before using. Takes owner ship of
|ipc|.
great.
also, s/owner ship/ownership.

https://chromiumcodereview.appspot.com/10836025/diff/1/media/audio/audio_outp...
File media/audio/audio_output_device_unittest.cc (right):

https://chromiumcodereview.appspot.com/10836025/diff/1/media/audio/audio_outp...
media/audio/audio_output_device_unittest.cc:131: MockAudioOutputIPC*
audio_output_ipc = new MockAudioOutputIPC;
new MockAudioOutputIPC();
(same throughout)

https://chromiumcodereview.appspot.com/10836025/diff/1/media/audio/audio_outp...
File media/audio/audio_output_ipc.h (right):

https://chromiumcodereview.appspot.com/10836025/diff/1/media/audio/audio_outp...
media/audio/audio_output_ipc.h:97: virtual ~AudioOutputIPC();
This changes the concept of the interface a bit.  Now, it's ann implementation
requirement of the interface that the object be deleted via this interface.  For
example reference counted objects must not inherit from this interface.

Can you add documentation?  Also, move the dtor up to the top.

[sail, 2012-07-31 22:02:08]

Thanks for the quick review. I addressed all comments except for updating input
device. Currently I'm not using the input device in my feature so I didn't need
to associate it with a render view. If you'd like me to change the input device
to match the output device let me know. Thanks!

https://chromiumcodereview.appspot.com/10836025/diff/1/content/browser/render...
File content/browser/renderer_host/media/audio_renderer_host.h (right):

https://chromiumcodereview.appspot.com/10836025/diff/1/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.h:130: void
OnPlayStream(int render_view_id, int stream_id);
On 2012/07/31 10:52:41, tommi wrote:
> just including the render view id in OnCreateStream should be enough and just
> store it with the AudioEntry.

Done.

https://chromiumcodereview.appspot.com/10836025/diff/1/content/renderer/media...
File content/renderer/media/audio_device_factory.h (right):

https://chromiumcodereview.appspot.com/10836025/diff/1/content/renderer/media...
content/renderer/media/audio_device_factory.h:32: static
media::AudioInputDevice* NewInputDevice();
On 2012/07/31 10:52:41, tommi wrote:
> don't we need the same for input devices?

I don't need input devices for this feature. I can add it if you like.

https://chromiumcodereview.appspot.com/10836025/diff/1/content/renderer/media...
content/renderer/media/audio_device_factory.h:42: virtual
media::AudioRendererSink* CreateOutputDevice() = 0;
On 2012/07/31 10:52:41, tommi wrote:
> Also add the parameter to these two factory functions.  They should be
identical
> to the static methods above.

Done.

https://chromiumcodereview.appspot.com/10836025/diff/1/content/renderer/media...
File content/renderer/media/audio_output_ipc.h (right):

https://chromiumcodereview.appspot.com/10836025/diff/1/content/renderer/media...
content/renderer/media/audio_output_ipc.h:24: class CONTENT_EXPORT
AudioOutputIPC
On 2012/07/31 10:52:41, tommi wrote:
> I don't think it's a good idea to call this class exactly the same name (aside
> from the namespace) as the interface it implements.  Let's at least append
> 'Impl' to the name.

Done.

https://chromiumcodereview.appspot.com/10836025/diff/1/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10836025/diff/1/content/renderer/rende...
content/renderer/render_view_impl.cc:2443: // channel count.
On 2012/07/31 10:52:41, tommi wrote:
> file a bug?

Done.
Filed bug 139882

https://chromiumcodereview.appspot.com/10836025/diff/1/content/renderer/rende...
content/renderer/render_view_impl.cc:2470: layout = CHANNEL_LAYOUT_STEREO;
On 2012/07/31 10:52:41, tommi wrote:
> add break;

Done.

https://chromiumcodereview.appspot.com/10836025/diff/1/content/renderer/rende...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://chromiumcodereview.appspot.com/10836025/diff/1/content/renderer/rende...
content/renderer/renderer_webkitplatformsupport_impl.cc:599: CHECK(false);
On 2012/07/31 10:52:41, tommi wrote:
> add a comment that explains why this CHECK is here.

Done.

https://chromiumcodereview.appspot.com/10836025/diff/1/media/audio/audio_outp...
File media/audio/audio_output_device.h (right):

https://chromiumcodereview.appspot.com/10836025/diff/1/media/audio/audio_outp...
media/audio/audio_output_device.h:98: // before using. Takes owner ship of
|ipc|.
On 2012/07/31 10:52:41, tommi wrote:
> great.
> also, s/owner ship/ownership.

Done.

https://chromiumcodereview.appspot.com/10836025/diff/1/media/audio/audio_outp...
File media/audio/audio_output_device_unittest.cc (right):

https://chromiumcodereview.appspot.com/10836025/diff/1/media/audio/audio_outp...
media/audio/audio_output_device_unittest.cc:131: MockAudioOutputIPC*
audio_output_ipc = new MockAudioOutputIPC;
On 2012/07/31 10:52:41, tommi wrote:
> new MockAudioOutputIPC();
> (same throughout)

Done.

https://chromiumcodereview.appspot.com/10836025/diff/1/media/audio/audio_outp...
File media/audio/audio_output_ipc.h (right):

https://chromiumcodereview.appspot.com/10836025/diff/1/media/audio/audio_outp...
media/audio/audio_output_ipc.h:97: virtual ~AudioOutputIPC();
On 2012/07/31 10:52:41, tommi wrote:
> This changes the concept of the interface a bit.  Now, it's ann implementation
> requirement of the interface that the object be deleted via this interface. 
For
> example reference counted objects must not inherit from this interface.
> 
> Can you add documentation?  Also, move the dtor up to the top.

Done.

Moved destructor. Not sure what documentation to add, any suggestions?

[sail, 2012-07-31 22:04:58]

> Aside from the terrifying propagation of render view ID, this seems very
> interesting.  Could you please provide somewhat more background on the overall
> project?

Sure thing! So I'd like to show an indicator in the tab strip about which tabs
are playing audio (see bug 64215).

I thought that an easy way to do this would be to associate audio streams with
the tab they're in. Once I have this association plumbed to the browser code I
can add the necessary UI to the tab strip.

[scherkus (not reviewing), 2012-07-31 22:30:19]

nice! I've been envisioning how this feature would be implemented for years :)

to answer croger's question -- we are in a bit of an unfortunate position of
plumbing render view IDs all the way down but I'm wracking my brain trying to
think of a way we can avoid it (or at least limit the scope of it)

In short, there is roughly a 1-to-1 mapping of render view id to a tab.

Therefore in order for the UI in the browser process to know which tab is
playing audio, it needs to know the render view id. It follows in the renderer
process that in order to associate an audio output stream with a render view id,
we need to plumb the render view id into any renderer-side object that might
create an audio stream (Web Audio, Pepper, media elements, Web RTC).

[Ami GONE FROM CHROMIUM, 2012-07-31 23:55:03]

drive-by, possibly-stupid suggestion:

- AudioRendererSink takes a callback to notify interested parties about
pause/play events.  AudioDeviceFactory binds a RenderThreadImpl method into this
CB so RTI always knows whether the renderer is playing or not.  (obvs. needs to
be more than a bool setter to track multiple concurrent audiodevices in a single
renderer).
- Add a browser->renderer IPC that asks "is audio playing in you?" (which
answers based on the RTI state above)
- Have the browser poll each of its renderers when a new stream is
started/stopped in the browser for audio status.

[sail, 2012-08-01 00:02:28]

On 2012/07/31 23:55:03, Ami Fischman wrote:
> drive-by, possibly-stupid suggestion:
> 
> - AudioRendererSink takes a callback to notify interested parties about
> pause/play events.  AudioDeviceFactory binds a RenderThreadImpl method into
this
> CB so RTI always knows whether the renderer is playing or not.  (obvs. needs
to
> be more than a bool setter to track multiple concurrent audiodevices in a
single
> renderer).
> - Add a browser->renderer IPC that asks "is audio playing in you?" (which
> answers based on the RTI state above)
> - Have the browser poll each of its renderers when a new stream is
> started/stopped in the browser for audio status.

Hi Ami, thanks for taking a look! From what I understand there are many
RenderView objects and one RenderThread object. We would know which RenderView
was associated with a particular audio stream.

[scherkus (not reviewing), 2012-08-01 00:10:55]

On 2012/07/31 23:55:03, Ami Fischman wrote:
> drive-by, possibly-stupid suggestion:
> 
> - AudioRendererSink takes a callback to notify interested parties about
> pause/play events.  AudioDeviceFactory binds a RenderThreadImpl method into
this
> CB so RTI always knows whether the renderer is playing or not.  (obvs. needs
to
> be more than a bool setter to track multiple concurrent audiodevices in a
single
> renderer).
> - Add a browser->renderer IPC that asks "is audio playing in you?" (which
> answers based on the RTI state above)
> - Have the browser poll each of its renderers when a new stream is
> started/stopped in the browser for audio status.

I don't think it's _that_ stupid :)

I've been thinking about this on and off and my question for sail@ is what
information is needed in the browser to implement the feature? Is it simply
playing/not playing?

fischman: while it's easy for RTI to answer "yes I'm playing" process-wide, the
tricky bit is to figure out which RVI is playing sound so that we find the tab
in the browser process. I think your solution is feasible if the CB is bound
with the RVI.

The idea I had bouncing in my head was that (ironically enough!) had we actually
plumbed a AudioRendererSink factory through everything like henrika@ proposed in
http://codereview.chromium.org/10537121/#ps8005 I *think* we could have used
said factory to return AudioRendererSink implementations that were bound to a
render view id. In the end the amount of code isn't very different as we'd be
trading plumbing render view IDs for RVI-specific factories :\

[Ami GONE FROM CHROMIUM, 2012-08-01 00:14:30]

What I don't get is that the render_view_id is actually a routing_id, which
presumably the browser<->renderer IPC must be tracking anyway.  IOW the
connection over which the browser gets a create/play/pause message must
already somehow be tagged w/ the relevant RVI, no?

[sail, 2012-08-01 00:22:48]

> I've been thinking about this on and off and my question for sail@ is what
> information is needed in the browser to implement the feature? Is it simply
> playing/not playing?

Yep, a start playing audio / end playing audio event per tab is all I need.

[scherkus (not reviewing), 2012-08-01 00:25:00]

On 2012/08/01 00:14:30, Ami Fischman wrote:
> What I don't get is that the render_view_id is actually a routing_id, which
> presumably the browser<->renderer IPC must be tracking anyway.  IOW the
> connection over which the browser gets a create/play/pause message must
> already somehow be tagged w/ the relevant RVI, no?

Unfortunately not in today's audio landscape as audio messaging is done as a
filter on the IO thread and has no association with any RVI.

The real question is (*gulp* huggggge potentially controversial tangent ahead!)
why _is_ audio messaging on the IO thread in today's audio landscape? Assuming
sync socket stays around for signalling and we remove all the other cruft inside
audio_messages.h, that leaves stream creation + shutdown + error reporting as
messages (maybe volume too). If we decide those aren't latency sensitive and can
be sent via RVI, then, yes, the RVH instance would receive the audio messages
and most of the CL would evaporate :)

[Chris Rogers, 2012-08-01 00:47:44]

On 2012/08/01 00:25:00, scherkus wrote:
> On 2012/08/01 00:14:30, Ami Fischman wrote:
> > What I don't get is that the render_view_id is actually a routing_id, which
> > presumably the browser<->renderer IPC must be tracking anyway.  IOW the
> > connection over which the browser gets a create/play/pause message must
> > already somehow be tagged w/ the relevant RVI, no?
> 
> Unfortunately not in today's audio landscape as audio messaging is done as a
> filter on the IO thread and has no association with any RVI.
> 
> The real question is (*gulp* huggggge potentially controversial tangent
ahead!)
> why _is_ audio messaging on the IO thread in today's audio landscape? Assuming
> sync socket stays around for signalling and we remove all the other cruft
inside
> audio_messages.h, that leaves stream creation + shutdown + error reporting as
> messages (maybe volume too). If we decide those aren't latency sensitive and
can
> be sent via RVI, then, yes, the RVH instance would receive the audio messages
> and most of the CL would evaporate :)

Sounds interesting - I'd like to talk more with you guys to understand how
viable this would be.  I understand the *gulp* :)  I'd be reluctant to pursue it
if it's quite complex, difficult to accomplish, or makes the code much harder to
understand.  Maybe the work would be difficult, but would result in
simpler/smaller code?  If we did it, would we do it now or later??  Are there
any other gotchas???

On the other topic of knowing if audio is in the "playing" or "not playing"
state, it's more complicated than just intercepting start/stop or play/pause
messages because the stream can often be in the "playing" state but, in fact,
silence is actually being rendered by the web application.  sail@ has observed
that, for example, the drum machine demo is using an AudioDevice and starts it,
yet there is silence until the sequence is started.  Here's my comment from that
email thread:

"
Hi Sailesh, Tommi is right that the play/stop buttons are only controlling the
sequencer.  The way the Web Audio API works (and most pro audio apps and game
engines) is that the audio stream is being continuously rendered even if there's
nothing playing at the moment.  Sometimes it just happens to be rendering
silence, but you never known when in the next microsecond somebody will press a
key (or play a MIDI keyboard) so you have to be prepared to render it as quickly
as possible.  There are so many ways that silence can be rendered from the API
(changing gain values at different nodes, not scheduling any events, oscillator
or high-pass filter frequencies turned up to high frequencies past Nyquist, etc.
etc.) that the only way to know if you're rendering silence or not is to
actually check the final rendered output stream and check for non-zero PCM data!
"

So, I think that to know for sure if there is actually non-silent audio being
sent to the hardware that the only way is to inspect the rendered buffers and
see if there are any non-zero samples being output.  Luckily, the code to do
this is quite simple and can be efficient.

[sail, 2012-08-01 17:26:36]

> > The real question is (*gulp* huggggge potentially controversial tangent
> ahead!)
> > why _is_ audio messaging on the IO thread in today's audio landscape?
Assuming
> > sync socket stays around for signalling and we remove all the other cruft
> inside
> > audio_messages.h, that leaves stream creation + shutdown + error reporting
as
> > messages (maybe volume too). If we decide those aren't latency sensitive and
> can
> > be sent via RVI, then, yes, the RVH instance would receive the audio
messages
> > and most of the CL would evaporate :)
> 
> Sounds interesting - I'd like to talk more with you guys to understand how
> viable this would be.  I understand the *gulp* :)

I'm not sure if I can be of much help here since I'm new to the audio code.
Would it be ok to decouple these projects? This CL doesn't add any new APIs so
it should be easy to change if the audio code is refactored.

> So, I think that to know for sure if there is actually non-silent audio being
> sent to the hardware that the only way is to inspect the rendered buffers and
> see if there are any non-zero samples being output.  Luckily, the code to do
> this is quite simple and can be efficient.

Ahh that's very encouraging.

[Chris Rogers, 2012-08-01 17:41:42]

On 2012/08/01 17:26:36, sail wrote:
> > > The real question is (*gulp* huggggge potentially controversial tangent
> > ahead!)
> > > why _is_ audio messaging on the IO thread in today's audio landscape?
> Assuming
> > > sync socket stays around for signalling and we remove all the other cruft
> > inside
> > > audio_messages.h, that leaves stream creation + shutdown + error reporting
> as
> > > messages (maybe volume too). If we decide those aren't latency sensitive
and
> > can
> > > be sent via RVI, then, yes, the RVH instance would receive the audio
> messages
> > > and most of the CL would evaporate :)
> > 
> > Sounds interesting - I'd like to talk more with you guys to understand how
> > viable this would be.  I understand the *gulp* :)
> 
> I'm not sure if I can be of much help here since I'm new to the audio code.
> Would it be ok to decouple these projects? This CL doesn't add any new APIs so
> it should be easy to change if the audio code is refactored.
> 
> > So, I think that to know for sure if there is actually non-silent audio
being
> > sent to the hardware that the only way is to inspect the rendered buffers
and
> > see if there are any non-zero samples being output.  Luckily, the code to do
> > this is quite simple and can be efficient.
> 
> Ahh that's very encouraging.

Thinking about this a bit more, how will you intend to indicate sound which is
playing sporadically with lots of silence in between.  For example, in a web
application that plays sound only when the mouse hovers over a UI element, but
is silent most of the time.  It would seem excessive to constantly flash the tab
for each one-shot sound.  Similar for a game, or a hidden tab which is playing
sequenced (but sparse) music.

The more I think about this it seems like it will hard to do this without either
falsely showing playing sound (because AudioDevice happens to be open but
playing silence) or annoying flashing of tabs.  Keep in mind that DAW (digital
audio workstations) visually show audio activity through "metering" (usually
horizontal or vertical green bars with red at the end).  But there's often a lot
of activity and movement in the UI there.  This is fine for this type of app,
but I'm concerned that it will be too "in your face" when displayed on tabs.

[sail, 2012-08-01 17:46:18]

On 2012/08/01 17:41:42, Chris Rogers wrote:
> On 2012/08/01 17:26:36, sail wrote:
> > > > The real question is (*gulp* huggggge potentially controversial tangent
> > > ahead!)
> > > > why _is_ audio messaging on the IO thread in today's audio landscape?
> > Assuming
> > > > sync socket stays around for signalling and we remove all the other
cruft
> > > inside
> > > > audio_messages.h, that leaves stream creation + shutdown + error
reporting
> > as
> > > > messages (maybe volume too). If we decide those aren't latency sensitive
> and
> > > can
> > > > be sent via RVI, then, yes, the RVH instance would receive the audio
> > messages
> > > > and most of the CL would evaporate :)
> > > 
> > > Sounds interesting - I'd like to talk more with you guys to understand how
> > > viable this would be.  I understand the *gulp* :)
> > 
> > I'm not sure if I can be of much help here since I'm new to the audio code.
> > Would it be ok to decouple these projects? This CL doesn't add any new APIs
so
> > it should be easy to change if the audio code is refactored.
> > 
> > > So, I think that to know for sure if there is actually non-silent audio
> being
> > > sent to the hardware that the only way is to inspect the rendered buffers
> and
> > > see if there are any non-zero samples being output.  Luckily, the code to
do
> > > this is quite simple and can be efficient.
> > 
> > Ahh that's very encouraging.
> 
> Thinking about this a bit more, how will you intend to indicate sound which is
> playing sporadically with lots of silence in between.  For example, in a web
> application that plays sound only when the mouse hovers over a UI element, but
> is silent most of the time.  It would seem excessive to constantly flash the
tab
> for each one-shot sound.  Similar for a game, or a hidden tab which is playing
> sequenced (but sparse) music.
> 
> The more I think about this it seems like it will hard to do this without
either
> falsely showing playing sound (because AudioDevice happens to be open but
> playing silence) or annoying flashing of tabs.  Keep in mind that DAW (digital
> audio workstations) visually show audio activity through "metering" (usually
> horizontal or vertical green bars with red at the end).  But there's often a
lot
> of activity and movement in the UI there.  This is fine for this type of app,
> but I'm concerned that it will be too "in your face" when displayed on tabs.

Hey  Chris. That's a great question. Thank fully we have great UX people to
solve these types of problems. My goal is to simply provide the functionality
they need ;-)

[scherkus (not reviewing), 2012-08-02 16:57:16]

On 2012/08/01 17:46:18, sail wrote:
> On 2012/08/01 17:41:42, Chris Rogers wrote:
> > On 2012/08/01 17:26:36, sail wrote:
> > > > > The real question is (*gulp* huggggge potentially controversial
tangent
> > > > ahead!)
> > > > > why _is_ audio messaging on the IO thread in today's audio landscape?
> > > Assuming
> > > > > sync socket stays around for signalling and we remove all the other
> cruft
> > > > inside
> > > > > audio_messages.h, that leaves stream creation + shutdown + error
> reporting
> > > as
> > > > > messages (maybe volume too). If we decide those aren't latency
sensitive
> > and
> > > > can
> > > > > be sent via RVI, then, yes, the RVH instance would receive the audio
> > > messages
> > > > > and most of the CL would evaporate :)
> > > > 
> > > > Sounds interesting - I'd like to talk more with you guys to understand
how
> > > > viable this would be.  I understand the *gulp* :)
> > > 
> > > I'm not sure if I can be of much help here since I'm new to the audio
code.
> > > Would it be ok to decouple these projects? This CL doesn't add any new
APIs
> so
> > > it should be easy to change if the audio code is refactored.
> > > 
> > > > So, I think that to know for sure if there is actually non-silent audio
> > being
> > > > sent to the hardware that the only way is to inspect the rendered
buffers
> > and
> > > > see if there are any non-zero samples being output.  Luckily, the code
to
> do
> > > > this is quite simple and can be efficient.
> > > 
> > > Ahh that's very encouraging.
> > 
> > Thinking about this a bit more, how will you intend to indicate sound which
is
> > playing sporadically with lots of silence in between.  For example, in a web
> > application that plays sound only when the mouse hovers over a UI element,
but
> > is silent most of the time.  It would seem excessive to constantly flash the
> tab
> > for each one-shot sound.  Similar for a game, or a hidden tab which is
playing
> > sequenced (but sparse) music.
> > 
> > The more I think about this it seems like it will hard to do this without
> either
> > falsely showing playing sound (because AudioDevice happens to be open but
> > playing silence) or annoying flashing of tabs.  Keep in mind that DAW
(digital
> > audio workstations) visually show audio activity through "metering" (usually
> > horizontal or vertical green bars with red at the end).  But there's often a
> lot
> > of activity and movement in the UI there.  This is fine for this type of
app,
> > but I'm concerned that it will be too "in your face" when displayed on tabs.
> 
> Hey  Chris. That's a great question. Thank fully we have great UX people to
> solve these types of problems. My goal is to simply provide the functionality
> they need ;-)

So what do we want to do here? I don't think it makes sense to have sail@
refactor audio IPC messaging (we can file a bug and get someone to pick that up
who knows the audio stack), so at this point in time we're kind of stuck with
plumbing render view ID.

Any other ideas?

[Chris Rogers, 2012-08-02 19:30:23]

On 2012/08/02 16:57:16, scherkus wrote:
> On 2012/08/01 17:46:18, sail wrote:
> > On 2012/08/01 17:41:42, Chris Rogers wrote:
> > > On 2012/08/01 17:26:36, sail wrote:
> > > > > > The real question is (*gulp* huggggge potentially controversial
> tangent
> > > > > ahead!)
> > > > > > why _is_ audio messaging on the IO thread in today's audio
landscape?
> > > > Assuming
> > > > > > sync socket stays around for signalling and we remove all the other
> > cruft
> > > > > inside
> > > > > > audio_messages.h, that leaves stream creation + shutdown + error
> > reporting
> > > > as
> > > > > > messages (maybe volume too). If we decide those aren't latency
> sensitive
> > > and
> > > > > can
> > > > > > be sent via RVI, then, yes, the RVH instance would receive the audio
> > > > messages
> > > > > > and most of the CL would evaporate :)
> > > > > 
> > > > > Sounds interesting - I'd like to talk more with you guys to understand
> how
> > > > > viable this would be.  I understand the *gulp* :)
> > > > 
> > > > I'm not sure if I can be of much help here since I'm new to the audio
> code.
> > > > Would it be ok to decouple these projects? This CL doesn't add any new
> APIs
> > so
> > > > it should be easy to change if the audio code is refactored.
> > > > 
> > > > > So, I think that to know for sure if there is actually non-silent
audio
> > > being
> > > > > sent to the hardware that the only way is to inspect the rendered
> buffers
> > > and
> > > > > see if there are any non-zero samples being output.  Luckily, the code
> to
> > do
> > > > > this is quite simple and can be efficient.
> > > > 
> > > > Ahh that's very encouraging.
> > > 
> > > Thinking about this a bit more, how will you intend to indicate sound
which
> is
> > > playing sporadically with lots of silence in between.  For example, in a
web
> > > application that plays sound only when the mouse hovers over a UI element,
> but
> > > is silent most of the time.  It would seem excessive to constantly flash
the
> > tab
> > > for each one-shot sound.  Similar for a game, or a hidden tab which is
> playing
> > > sequenced (but sparse) music.
> > > 
> > > The more I think about this it seems like it will hard to do this without
> > either
> > > falsely showing playing sound (because AudioDevice happens to be open but
> > > playing silence) or annoying flashing of tabs.  Keep in mind that DAW
> (digital
> > > audio workstations) visually show audio activity through "metering"
(usually
> > > horizontal or vertical green bars with red at the end).  But there's often
a
> > lot
> > > of activity and movement in the UI there.  This is fine for this type of
> app,
> > > but I'm concerned that it will be too "in your face" when displayed on
tabs.
> > 
> > Hey  Chris. That's a great question. Thank fully we have great UX people to
> > solve these types of problems. My goal is to simply provide the
functionality
> > they need ;-)
> 
> So what do we want to do here? I don't think it makes sense to have sail@
> refactor audio IPC messaging (we can file a bug and get someone to pick that
up
> who knows the audio stack), so at this point in time we're kind of stuck with
> plumbing render view ID.
> 
> Any other ideas?

Yes, I think that's probably the best and most sane approach at this time. 
After thinking about this some more, I'm not sure that switching over to routed
IPC would actually gain us much in code simplicity since I think we would end up
with the same plumbing issues, passing RenderView pointers around throughout
most of the same places we pass render-view-ID in this CL...

So, I'm fine with this approach.  We still have to resolve a layering issue on
the WebKit-side (to get the render-view-ID), but that's solvable...

[tommi (sloooow) - chröme, 2012-08-03 08:42:38]

I agree as well.  I'm away from work at the moment, so rubber stamp lgtm.


On Thu, Aug 2, 2012 at 9:30 PM, <crogers@google.com> wrote:

> On 2012/08/02 16:57:16, scherkus wrote:
>
>> On 2012/08/01 17:46:18, sail wrote:
>> > On 2012/08/01 17:41:42, Chris Rogers wrote:
>> > > On 2012/08/01 17:26:36, sail wrote:
>> > > > > > The real question is (*gulp* huggggge potentially controversial
>> tangent
>> > > > > ahead!)
>> > > > > > why _is_ audio messaging on the IO thread in today's audio
>>
> landscape?
>
>> > > > Assuming
>> > > > > > sync socket stays around for signalling and we remove all the
>> other
>> > cruft
>> > > > > inside
>> > > > > > audio_messages.h, that leaves stream creation + shutdown + error
>> > reporting
>> > > > as
>> > > > > > messages (maybe volume too). If we decide those aren't latency
>> sensitive
>> > > and
>> > > > > can
>> > > > > > be sent via RVI, then, yes, the RVH instance would receive the
>> audio
>> > > > messages
>> > > > > > and most of the CL would evaporate :)
>> > > > >
>> > > > > Sounds interesting - I'd like to talk more with you guys to
>> understand
>> how
>> > > > > viable this would be.  I understand the *gulp* :)
>> > > >
>> > > > I'm not sure if I can be of much help here since I'm new to the
>> audio
>> code.
>> > > > Would it be ok to decouple these projects? This CL doesn't add any
>> new
>> APIs
>> > so
>> > > > it should be easy to change if the audio code is refactored.
>> > > >
>> > > > > So, I think that to know for sure if there is actually non-silent
>>
> audio
>
>> > > being
>> > > > > sent to the hardware that the only way is to inspect the rendered
>> buffers
>> > > and
>> > > > > see if there are any non-zero samples being output.  Luckily, the
>> code
>> to
>> > do
>> > > > > this is quite simple and can be efficient.
>> > > >
>> > > > Ahh that's very encouraging.
>> > >
>> > > Thinking about this a bit more, how will you intend to indicate sound
>>
> which
>
>> is
>> > > playing sporadically with lots of silence in between.  For example,
>> in a
>>
> web
>
>> > > application that plays sound only when the mouse hovers over a UI
>> element,
>> but
>> > > is silent most of the time.  It would seem excessive to constantly
>> flash
>>
> the
>
>> > tab
>> > > for each one-shot sound.  Similar for a game, or a hidden tab which is
>> playing
>> > > sequenced (but sparse) music.
>> > >
>> > > The more I think about this it seems like it will hard to do this
>> without
>> > either
>> > > falsely showing playing sound (because AudioDevice happens to be open
>> but
>> > > playing silence) or annoying flashing of tabs.  Keep in mind that DAW
>> (digital
>> > > audio workstations) visually show audio activity through "metering"
>>
> (usually
>
>> > > horizontal or vertical green bars with red at the end).  But there's
>> often
>>
> a
>
>> > lot
>> > > of activity and movement in the UI there.  This is fine for this type
>> of
>> app,
>> > > but I'm concerned that it will be too "in your face" when displayed on
>>
> tabs.
>
>> >
>> > Hey  Chris. That's a great question. Thank fully we have great UX
>> people to
>> > solve these types of problems. My goal is to simply provide the
>>
> functionality
>
>> > they need ;-)
>>
>
>  So what do we want to do here? I don't think it makes sense to have sail@
>> refactor audio IPC messaging (we can file a bug and get someone to pick
>> that
>>
> up
>
>> who knows the audio stack), so at this point in time we're kind of stuck
>> with
>> plumbing render view ID.
>>
>
>  Any other ideas?
>>
>
> Yes, I think that's probably the best and most sane approach at this time.
> After thinking about this some more, I'm not sure that switching over to
> routed
> IPC would actually gain us much in code simplicity since I think we would
> end up
> with the same plumbing issues, passing RenderView pointers around
> throughout
> most of the same places we pass render-view-ID in this CL...
>
> So, I'm fine with this approach.  We still have to resolve a layering
> issue on
> the WebKit-side (to get the render-view-ID), but that's solvable...
>
>
>
https://chromiumcodereview.**appspot.com/10836025/<https://chromiumcodereview...
>

[scherkus (not reviewing), 2012-08-03 21:12:52]

LGTM w/ nits

https://chromiumcodereview.appspot.com/10836025/diff/1040/content/renderer/me...
File content/renderer/media/audio_output_ipc_impl.cc (right):

https://chromiumcodereview.appspot.com/10836025/diff/1040/content/renderer/me...
content/renderer/media/audio_output_ipc_impl.cc:4: //
remove extra //

https://chromiumcodereview.appspot.com/10836025/diff/1040/content/renderer/me...
File content/renderer/media/audio_output_ipc_impl.h (right):

https://chromiumcodereview.appspot.com/10836025/diff/1040/content/renderer/me...
content/renderer/media/audio_output_ipc_impl.h:5: // MessageFilter that handles
audio messages and delegates them to audio
nit: we (media folks) typically have the class doc by the class as opposed to
the file

https://chromiumcodereview.appspot.com/10836025/diff/1040/content/renderer/me...
content/renderer/media/audio_output_ipc_impl.h:24: class CONTENT_EXPORT
AudioOutputIPCImpl
new classes in content should be under the content namespace

[Chris Rogers, 2012-08-03 22:30:06]

seems fine - this depends on the WebKit-side patch landing, doesn't it?  Just
wanted to make sure things are landed in the right order :)

[sail, 2012-08-13 19:38:58]

On 2012/08/03 22:30:06, Chris Rogers wrote:
> seems fine - this depends on the WebKit-side patch landing, doesn't it?  Just
> wanted to make sure things are landed in the right order :)

Just a quick stats update. I'm currently stuck in Mac UI land right now. My plan
is to get back to this feature within 2 weeks and hopefully land it in time for
M23. Thanks.