Add audio signal to the ResourceScheduler.

content/browser/renderer_host/media/audio_renderer_host.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/renderer_host/media/audio_renderer_host.h"
 
 #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"
 #include "content/browser/renderer_host/media/audio_input_device_manager.h"
 #include "content/browser/renderer_host/media/audio_sync_reader.h"
 #include "content/browser/renderer_host/media/media_stream_manager.h"
 #include "content/common/media/audio_messages.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/media_observer.h"
 #include "content/public/common/content_switches.h"
@@ skipping 227 matching lines @@
 
   Send(new AudioMsg_NotifyStreamCreated(
       entry->stream_id(), foreign_memory_handle, socket_descriptor,
       entry->shared_memory()->requested_size()));
 }
 
 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;
 
   Send(new AudioMsg_NotifyStreamStateChanged(
       stream_id,
       is_playing ? media::AudioOutputIPCDelegate::kPlaying
                  : media::AudioOutputIPCDelegate::kPaused));
 
   if (is_playing) {
     AudioStreamMonitor::StartMonitoringStream(
         render_process_id_,
         entry->render_frame_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
 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());
   }
 
   return controllers;
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // IPC Messages handler
@@ skipping 138 matching lines @@
     mirroring_manager_->RemoveDiverter(controller);
   controller->Close(
       base::Bind(&AudioRendererHost::DeleteEntry, this, base::Passed(&entry)));
   audio_log_->OnClosed(stream_id);
 }
 
 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) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   // Make sure this isn't a stray callback executing after the stream has been
   // closed, so error notifications aren't sent after clients believe the stream
   // is closed.
   if (!LookupById(stream_id))
     return;
 
   SendErrorMessage(stream_id);
 
   audio_log_->OnError(stream_id);
   OnCloseStream(stream_id);
 }
 
 AudioRendererHost::AudioEntry* AudioRendererHost::LookupById(int stream_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   AudioEntryMap::const_iterator i = audio_entries_.find(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