Add audio signal to the ResourceScheduler.

content/browser/renderer_host/media/audio_renderer_host.cc

Index: content/browser/renderer_host/media/audio_renderer_host.cc
diff --git a/content/browser/renderer_host/media/audio_renderer_host.cc b/content/browser/renderer_host/media/audio_renderer_host.cc
index 8c06395eca68ecf94496c47946a26066060004d0..d8378908a32a8c172d186b7e4bde165ada0ac935 100644
--- a/content/browser/renderer_host/media/audio_renderer_host.cc
+++ b/content/browser/renderer_host/media/audio_renderer_host.cc
@@ -6,10 +6,12 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/logging.h"
 #include "base/memory/shared_memory.h"
 #include "base/metrics/histogram.h"
 #include "base/process/process.h"
 #include "content/browser/browser_main_loop.h"
+#include "content/browser/loader/resource_dispatcher_host_impl.h"
 #include "content/browser/media/audio_stream_monitor.h"
 #include "content/browser/media/capture/audio_mirroring_manager.h"
 #include "content/browser/media/media_internals.h"
@@ -257,7 +259,7 @@ void AudioRendererHost::DoNotifyStreamStateChanged(int stream_id,
                                                    bool is_playing) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  AudioEntry* const entry = LookupById(stream_id);
+  AudioEntry* const entry(LookupById(stream_id));
   if (!entry)
     return;
 
@@ -273,20 +275,11 @@ void AudioRendererHost::DoNotifyStreamStateChanged(int stream_id,
         entry->stream_id(),
         base::Bind(&media::AudioOutputController::ReadCurrentPowerAndClip,
                    entry->controller()));
-    // TODO(dalecurtis): See about using AudioStreamMonitor instead.
-    if (!entry->playing()) {
-      entry->set_playing(true);
-      base::AtomicRefCountInc(&num_playing_streams_);
-    }
   } else {
     AudioStreamMonitor::StopMonitoringStream(
         render_process_id_, entry->render_frame_id(), entry->stream_id());
-    // TODO(dalecurtis): See about using AudioStreamMonitor instead.
-    if (entry->playing()) {
-      entry->set_playing(false);
-      base::AtomicRefCountDec(&num_playing_streams_);
-    }
   }
+  UpdateNumPlayingStreams(entry, is_playing);
 }
 
 RenderViewHost::AudioOutputControllerList
@@ -294,8 +287,9 @@ AudioRendererHost::DoGetOutputControllers(int render_view_id) const {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   RenderViewHost::AudioOutputControllerList controllers;
-  AudioEntryMap::const_iterator it = audio_entries_.begin();
-  for (; it != audio_entries_.end(); ++it) {
+  for (AudioEntryMap::const_iterator it = audio_entries_.begin();
+       it != audio_entries_.end();
+       ++it) {
     AudioEntry* entry = it->second;
     if (entry->render_view_id() == render_view_id)
       controllers.push_back(entry->controller());
@@ -454,8 +448,7 @@ void AudioRendererHost::DeleteEntry(scoped_ptr<AudioEntry> entry) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   AudioStreamMonitor::StopMonitoringStream(
       render_process_id_, entry->render_frame_id(), entry->stream_id());
-  if (entry->playing())
-    base::AtomicRefCountDec(&num_playing_streams_);
+  UpdateNumPlayingStreams(entry.get(), false);

[aiolos (Not reviewing), 2014/09/19 17:33:56]

It looks like entry.get() is sometimes failing here on SEGV_MAPERR. Is there a
way to keep entry from going out of scope until after UpdateNumPlayingStreams?

[DaleCurtis, 2014/09/19 17:40:40]

That doesn't make sense. It shouldn't go out of scope until this function
returns.  The trybot failures suggest that |client| is NULL:

[26648:26664:0919/102833:171931532:FATAL:resource_scheduler.cc(844)] Check
failed: client.

[aiolos (Not reviewing), 2014/09/19 18:59:18]

It didn't make sense to me either, but this was the important part of the
stacktrace I was looking at that led me to ask that question:

Received signal 11 SEGV_MAPERR 000000000208
#0 0x7fc55babe8fe base::debug::StackTrace::StackTrace()
#1 0x7fc55babe433 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7fc54cad3cb0 <unknown>
#3 0x7fc5510355bc base::internal::scoped_ptr_impl<>::get()
#4 0x7fc551018da9 scoped_ptr<>::operator->()
#5 0x7fc551010c09
content::ResourceDispatcherHostImpl::OnAudioRenderHostStreamStateChanged()
#6 0x7fc55117e6bf content::AudioRendererHost::UpdateNumPlayingStreams()
#7 0x7fc55117de5a content::AudioRendererHost::DoNotifyStreamStateChanged()

[DaleCurtis, 2014/09/19 19:13:42]

Wait, this crash is in
content::ResourceDispatcherHostImpl::OnAudioRenderHostStreamStateChanged() not
AudioRendererHost.

 }
 
 void AudioRendererHost::ReportErrorAndClose(int stream_id) {
@@ -480,8 +473,35 @@ AudioRendererHost::AudioEntry* AudioRendererHost::LookupById(int stream_id) {
   return i != audio_entries_.end() ? i->second : NULL;
 }
 
+void AudioRendererHost::UpdateNumPlayingStreams(AudioEntry* entry,
+                                                bool is_playing) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  if (entry->playing() == is_playing)
+    return;
+  entry->set_playing(is_playing);
+  if (is_playing) {

[DaleCurtis, 2014/09/19 17:40:40]

Remove {} from single line if.

[aiolos (Not reviewing), 2014/09/19 18:59:18]

Even though it's part of the if/else? You and Matt seem to have differing
thoughts on that case.

[mmenke, 2014/09/19 20:32:40]

I'm certainly not going to fight over this, but I've seen comments myself over
not using them, and it's my experience that using them seems to be the slightly
preferred style.

The Google style guide technically allows both styles.

[DaleCurtis, 2014/09/19 20:39:41]

I didn't see Matt's previous disagreement. I don't care either and there's no
precedent elsewhere in this file for one style or the other, so choose whatever
style you like.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals

+    base::AtomicRefCountInc(&num_playing_streams_);
+  } else {
+    base::AtomicRefCountDec(&num_playing_streams_);
+  }
+  if (is_playing || !RenderViewHasActiveAudio(entry->render_view_id()))

[DaleCurtis, 2014/09/19 17:40:40]

Multiline if needs {}

[aiolos (Not reviewing), 2014/09/19 18:59:18]

Done.

+    ResourceDispatcherHostImpl::Get()->OnAudioRenderHostStreamStateChanged(

[DaleCurtis, 2014/09/19 17:40:40]

I take it your upstream code will just ignore repeated play events?

[aiolos (Not reviewing), 2014/09/19 18:59:18]

Close. It will quickly recompute whether the tab should be background or not,
but won't change the behavior. All of the signals are designed to handle that
upstream. I think I'll change this to avoid confusion since both you and Matt
had a question about it though.

+        render_process_id_, entry->render_view_id(), is_playing);
+}
+
 bool AudioRendererHost::HasActiveAudio() {
   return !base::AtomicRefCountIsZero(&num_playing_streams_);
 }
 
+bool AudioRendererHost::RenderViewHasActiveAudio(int render_view_id) {
+  for (AudioEntryMap::const_iterator it = audio_entries_.begin();
+       it != audio_entries_.end();
+       ++it) {
+    AudioEntry* entry = it->second;
+    if (entry->render_view_id() == render_view_id && entry->playing())
+      return true;
+  }
+  return false;
+}
+
 }  // namespace content