Add a method to tab_utils.h to find out whether a tab is playing audio.

Add a method to tab_utils.h to find out whether a tab is playing audio.


BUG=3541
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182364

[Bernhard Bauer, 2012-12-18 21:09:56]

Please review.

[Bernhard Bauer, 2012-12-18 22:23:00]

Adding OWNERS:

Scott (chrome/, specifically chrome/browser/tabs/ui/)
John (content/)

[sky, 2012-12-18 23:38:02]

LGTM

[miu, 2012-12-19 00:49:08]

lgtm

lgtm, assuming comments are addressed.

BTW--This is very similar to an earlier attempt (which you may want to take a
look at): https://codereview.chromium.org/10823097/

I'll be out-of-office until Dec 27th, so advanced apologies for any late
replies.

-Yuri

https://codereview.chromium.org/11573066/diff/10001/chrome/browser/media/audi...
File chrome/browser/media/audio_stream_indicator.h (right):

https://codereview.chromium.org/11573066/diff/10001/chrome/browser/media/audi...
chrome/browser/media/audio_stream_indicator.h:23: int render_view_id,
Please fix indentation here.

https://codereview.chromium.org/11573066/diff/10001/chrome/browser/media/audi...
chrome/browser/media/audio_stream_indicator.h:34: int render_view_id,
Ditto (fix indentation).

https://codereview.chromium.org/11573066/diff/10001/chrome/browser/media/audi...
chrome/browser/media/audio_stream_indicator.h:37: std::multiset<std::pair<int,
int> > audio_streams_;
Please document this member.  I assume it's the set of playing audio streams?

https://codereview.chromium.org/11573066/diff/10001/chrome/browser/media/audi...
chrome/browser/media/audio_stream_indicator.h:37: std::multiset<std::pair<int,
int> > audio_streams_;
Why is this a multiset<>?  Won't a set<> do the job?

https://codereview.chromium.org/11573066/diff/10001/chrome/browser/media/medi...
File chrome/browser/media/media_internals.cc (right):

https://codereview.chromium.org/11573066/diff/10001/chrome/browser/media/medi...
chrome/browser/media/media_internals.cc:104: render_view_id,
fix indentation.

https://codereview.chromium.org/11573066/diff/10001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/11573066/diff/10001/content/browser/renderer_...
content/browser/renderer_host/media/audio_renderer_host.cc:49: render_view_id(0)
{
nit: This should probably default to MSG_ROUTING_NONE instead of 0.

https://codereview.chromium.org/11573066/diff/10001/content/browser/renderer_...
content/browser/renderer_host/media/audio_renderer_host.cc:302:
entry->render_view_id = render_view_id;
While you're here, could you add a comment:

TODO(miu): Merge "AssociateWithProducer" message into "CreateStream" message so
AudioRendererHost can assume a simpler "render_view_id is set once" scheme. 
http://crbug.com/166779

[jam, 2012-12-19 01:37:16]

lgtm

[tommi (sloooow) - chröme, 2012-12-19 11:47:29]

https://codereview.chromium.org/11573066/diff/10001/chrome/browser/media/audi...
File chrome/browser/media/audio_stream_indicator.cc (right):

https://codereview.chromium.org/11573066/diff/10001/chrome/browser/media/audi...
chrome/browser/media/audio_stream_indicator.cc:22: int render_view_id,
fix indentation throughout.

https://codereview.chromium.org/11573066/diff/10001/chrome/browser/media/audi...
File chrome/browser/media/audio_stream_indicator.h (right):

https://codereview.chromium.org/11573066/diff/10001/chrome/browser/media/audi...
chrome/browser/media/audio_stream_indicator.h:37: std::multiset<std::pair<int,
int> > audio_streams_;
Instead of using std::pair, let's just define a proper struct for this with
render_process_id and render_view_id members.  
std::pair just makes the code harder to read if you have to remember what
'first' and 'second' is and it's easy to mix them up.

[James Cook, 2013-01-30 17:13:12]

This patch would be helpful for me on Chrome OS to deal with audio streams in
out-of-memory situations:
https://code.google.com/p/chromium/issues/detail?id=164081

Any chance this could land soon?

[miu, 2013-01-30 23:42:20]

BTW--Should the efforts in chrome/browser/media/audio_stream_indicator.* be
somehow merged in with the UI code in
chrome/browser/media/media_stream_capture_indicator.*?  Could the two conflict
somehow?  Was this tested (e.g., a video conferencing session where audio input
AND output are happening at the same time)?

[Bernhard Bauer, 2013-02-04 21:23:45]

On 2013/01/30 17:13:12, James Cook (Chromium) wrote:
> This patch would be helpful for me on Chrome OS to deal with audio streams in
> out-of-memory situations:
> https://code.google.com/p/chromium/issues/detail?id=164081
> 
> Any chance this could land soon?

Hm, the code changed a lot upstream with John's recent refactoring. I'll have to
rewrite the CL quite a bit.

On 2013/01/30 23:42:20, Yuri wrote:
> BTW--Should the efforts in chrome/browser/media/audio_stream_indicator.* be
> somehow merged in with the UI code in
> chrome/browser/media/media_stream_capture_indicator.*?  Could the two conflict
> somehow?  Was this tested (e.g., a video conferencing session where audio
input
> AND output are happening at the same time)?

Conflicts are explicitly dealt with in tab_utils.cc (where we already have that
for mirroring vs. capturing). But yeah, merging AudioStreamIndicator into
MediaStreamCaptureIndicator (maybe removing the "Capture"?) might make sense.

[Bernhard Bauer, 2013-02-05 18:45:18]

Okay, I rewrote the CL. John, could you take another look?

https://codereview.chromium.org/11573066/diff/10001/chrome/browser/media/audi...
File chrome/browser/media/audio_stream_indicator.cc (right):

https://codereview.chromium.org/11573066/diff/10001/chrome/browser/media/audi...
chrome/browser/media/audio_stream_indicator.cc:22: int render_view_id,
On 2012/12/19 11:47:29, tommi wrote:
> fix indentation throughout.

Done.

https://codereview.chromium.org/11573066/diff/10001/chrome/browser/media/audi...
File chrome/browser/media/audio_stream_indicator.h (right):

https://codereview.chromium.org/11573066/diff/10001/chrome/browser/media/audi...
chrome/browser/media/audio_stream_indicator.h:23: int render_view_id,
On 2012/12/19 00:49:08, Yuri wrote:
> Please fix indentation here.

Done.

https://codereview.chromium.org/11573066/diff/10001/chrome/browser/media/audi...
chrome/browser/media/audio_stream_indicator.h:34: int render_view_id,
On 2012/12/19 00:49:08, Yuri wrote:
> Ditto (fix indentation).

Done.

https://codereview.chromium.org/11573066/diff/10001/chrome/browser/media/audi...
chrome/browser/media/audio_stream_indicator.h:37: std::multiset<std::pair<int,
int> > audio_streams_;
On 2012/12/19 11:47:29, tommi wrote:
> Instead of using std::pair, let's just define a proper struct for this with
> render_process_id and render_view_id members.  
> std::pair just makes the code harder to read if you have to remember what
> 'first' and 'second' is and it's easy to mix them up.

Done.

https://codereview.chromium.org/11573066/diff/10001/chrome/browser/media/audi...
chrome/browser/media/audio_stream_indicator.h:37: std::multiset<std::pair<int,
int> > audio_streams_;
On 2012/12/19 00:49:08, Yuri wrote:
> Please document this member.  I assume it's the set of playing audio streams?

Done.

https://codereview.chromium.org/11573066/diff/10001/chrome/browser/media/audi...
chrome/browser/media/audio_stream_indicator.h:37: std::multiset<std::pair<int,
int> > audio_streams_;
On 2012/12/19 00:49:08, Yuri wrote:
> Why is this a multiset<>?  Won't a set<> do the job?

It's a multiset because it has one entry per audio stream in a render view.

https://codereview.chromium.org/11573066/diff/10001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/11573066/diff/10001/content/browser/renderer_...
content/browser/renderer_host/media/audio_renderer_host.cc:302:
entry->render_view_id = render_view_id;
On 2012/12/19 00:49:08, Yuri wrote:
> While you're here, could you add a comment:
> 
> TODO(miu): Merge "AssociateWithProducer" message into "CreateStream" message
so
> AudioRendererHost can assume a simpler "render_view_id is set once" scheme. 
> http://crbug.com/166779

Done.

[tommi (sloooow) - chröme, 2013-02-05 21:24:51]

lgtm

[miu, 2013-02-05 23:18:25]

A few things to fix first...

https://codereview.chromium.org/11573066/diff/19001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/11573066/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/audio_renderer_host.cc:388:
render_process_id_, entry->render_view_id, true);
I think you meant s/true/false/ here.

https://codereview.chromium.org/11573066/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/audio_renderer_host.cc:388:
render_process_id_, entry->render_view_id, true);
Also, note that streams are not necessarily paused before they are closed (or
error out).  You'll somehow need to call
MediaObserver::OnAudioStreamPlayingChanged(..., false) in those cases.  Probably
in AudioRendererHost::CloseAndDeleteStream().

...and the unfortunate news here is: How will you balance the number of
OnAudioStreamPlayingChanged(true) with OnAudioStreamPlayingChanged(false) calls?

https://codereview.chromium.org/11573066/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/audio_renderer_host.cc:433: }
If volume is set to zero (i.e., mute), or set back from non-zero, you will want
to call MediaObserver::OnAudioStreamPlayingChanged() here.

[jam, 2013-02-06 04:42:35]

lgtm

[cpu_(ooo_6.6-7.5), 2013-02-06 22:56:58]

imho the change looks good, my biggest concern is testing. 

Maybe there are already tests that we can piggyback this stuff? 

Note that we don't want a UI level test, but we just want to make sure if in the
guts of the media stack we call play, pause, stop, we get the expected number of
notifications.

Also (take it just as an opinion) I would like this change to land soon so I am
ok to have tests in a followup CL.

[miu, 2013-02-06 23:01:16]

On 2013/02/06 22:56:58, cpu wrote:
> Note that we don't want a UI level test, but we just want to make sure if in
the
> guts of the media stack we call play, pause, stop, we get the expected number
of
> notifications.

Per my last e-mail on this issue, I think there are some things that need to be
fixed before this change lands.

But, I agree: Manual testing will be enough to confirm correctness for.  Just be
ready to run dover to back-port to M26 later.  ;-)

[Bernhard Bauer, 2013-02-08 16:44:14]

https://codereview.chromium.org/11573066/diff/19001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/11573066/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/audio_renderer_host.cc:388:
render_process_id_, entry->render_view_id, true);
On 2013/02/05 23:18:25, Yuri wrote:
> I think you meant s/true/false/ here.

Urr, yes. Done.

https://codereview.chromium.org/11573066/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/audio_renderer_host.cc:388:
render_process_id_, entry->render_view_id, true);
On 2013/02/05 23:18:25, Yuri wrote:
> Also, note that streams are not necessarily paused before they are closed (or
> error out).  You'll somehow need to call
> MediaObserver::OnAudioStreamPlayingChanged(..., false) in those cases. 
Probably
> in AudioRendererHost::CloseAndDeleteStream().
> 
> ...and the unfortunate news here is: How will you balance the number of
> OnAudioStreamPlayingChanged(true) with OnAudioStreamPlayingChanged(false)
calls?

Hm. I changed it to a map from RenderView ID to a set of streams playing.

https://codereview.chromium.org/11573066/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/audio_renderer_host.cc:433: }
On 2013/02/05 23:18:25, Yuri wrote:
> If volume is set to zero (i.e., mute), or set back from non-zero, you will
want
> to call MediaObserver::OnAudioStreamPlayingChanged() here.

Done.

[Bernhard Bauer, 2013-02-11 20:12:40]

Yuri, friendly ping? :)

[miu, 2013-02-11 20:51:47]

lgtm

One nit...

https://codereview.chromium.org/11573066/diff/25002/chrome/browser/media/audi...
File chrome/browser/media/audio_stream_indicator.cc (right):

https://codereview.chromium.org/11573066/diff/25002/chrome/browser/media/audi...
chrome/browser/media/audio_stream_indicator.cc:60: RenderViewId
id(render_process_id, render_view_id);
nit: Can you add the following to the top of this method?

  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

[Bernhard Bauer, 2013-02-12 18:06:26]

https://codereview.chromium.org/11573066/diff/25002/chrome/browser/media/audi...
File chrome/browser/media/audio_stream_indicator.cc (right):

https://codereview.chromium.org/11573066/diff/25002/chrome/browser/media/audi...
chrome/browser/media/audio_stream_indicator.cc:60: RenderViewId
id(render_process_id, render_view_id);
On 2013/02/11 20:51:47, Yuri wrote:
> nit: Can you add the following to the top of this method?
> 
>   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

Done.

[commit-bot: I haz the power, 2013-02-12 18:07:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/11573066/31002

[commit-bot: I haz the power, 2013-02-13 19:27:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/11573066/31002

[commit-bot: I haz the power, 2013-02-14 01:11:27]

Change committed as 182364