Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1087)

Unified Diff: content/browser/renderer_host/media/audio_renderer_host.cc

Issue 2443573003: Factor out AudioOutputDelegate from AudioRendererHost. (Closed)
Patch Set: . Created 4 years, 1 month ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
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 98e931b295c9007e5e6bd6365320647bbfca16bc..fa89c4f677c9453cbee955e68d882fdde20f0788 100644
--- a/content/browser/renderer_host/media/audio_renderer_host.cc
+++ b/content/browser/renderer_host/media/audio_renderer_host.cc
@@ -10,8 +10,6 @@
#include "base/bind.h"
#include "base/bind_helpers.h"
#include "base/lazy_instance.h"
-#include "base/memory/ptr_util.h"
-#include "base/memory/shared_memory.h"
#include "base/metrics/histogram_macros.h"
#include "content/browser/bad_message.h"
#include "content/browser/browser_main_loop.h"
@@ -45,7 +43,13 @@ base::LazyInstance<media::AudioStreamsTracker> g_audio_streams_tracker =
LAZY_INSTANCE_INITIALIZER;
void NotifyRenderProcessHostThatAudioStateChanged(int render_process_id) {
- DCHECK_CURRENTLY_ON(BrowserThread::UI);
+ if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
miu 2016/11/29 21:36:28 ditto here: Burden the caller, not the callee with
+ BrowserThread::PostTask(
+ BrowserThread::UI, FROM_HERE,
+ base::Bind(&NotifyRenderProcessHostThatAudioStateChanged,
+ render_process_id));
+ return;
+ }
RenderProcessHost* render_process_host =
RenderProcessHost::FromID(render_process_id);
@@ -75,117 +79,19 @@ void ValidateRenderFrameId(int render_process_id,
} // namespace
-class AudioRendererHost::AudioEntry
- : public media::AudioOutputController::EventHandler {
- public:
- ~AudioEntry() override;
-
- // Returns nullptr on failure.
- static std::unique_ptr<AudioRendererHost::AudioEntry> Create(
- AudioRendererHost* host,
- int stream_id,
- int render_frame_id,
- const media::AudioParameters& params,
- const std::string& output_device_id);
-
- int stream_id() const {
- return stream_id_;
- }
-
- int render_frame_id() const { return render_frame_id_; }
-
- media::AudioOutputController* controller() const { return controller_.get(); }
-
- AudioSyncReader* reader() const { return reader_.get(); }
-
- // Used by ARH to track the number of active streams for UMA stats.
- bool playing() const { return playing_; }
- void set_playing(bool playing) { playing_ = playing; }
-
- private:
- AudioEntry(AudioRendererHost* host,
- int stream_id,
- int render_frame_id,
- const media::AudioParameters& params,
- const std::string& output_device_id,
- std::unique_ptr<AudioSyncReader> reader);
-
- // media::AudioOutputController::EventHandler implementation.
- void OnCreated() override;
- void OnPlaying() override;
- void OnPaused() override;
- void OnError() override;
-
- AudioRendererHost* const host_;
- const int stream_id_;
-
- // The routing ID of the source RenderFrame.
- const int render_frame_id_;
-
- // The synchronous reader to be used by |controller_|.
- const std::unique_ptr<AudioSyncReader> reader_;
-
- // The AudioOutputController that manages the audio stream.
- const scoped_refptr<media::AudioOutputController> controller_;
-
- bool playing_;
-
- DISALLOW_COPY_AND_ASSIGN(AudioEntry);
-};
-
-AudioRendererHost::AudioEntry::AudioEntry(
- AudioRendererHost* host,
- int stream_id,
- int render_frame_id,
- const media::AudioParameters& params,
- const std::string& output_device_id,
- std::unique_ptr<AudioSyncReader> reader)
- : host_(host),
- stream_id_(stream_id),
- render_frame_id_(render_frame_id),
- reader_(std::move(reader)),
- controller_(media::AudioOutputController::Create(host->audio_manager_,
- this,
- params,
- output_device_id,
- reader_.get())),
- playing_(false) {
- DCHECK(controller_);
-}
-
-AudioRendererHost::AudioEntry::~AudioEntry() {}
-
-// static
-std::unique_ptr<AudioRendererHost::AudioEntry>
-AudioRendererHost::AudioEntry::Create(AudioRendererHost* host,
- int stream_id,
- int render_frame_id,
- const media::AudioParameters& params,
- const std::string& output_device_id) {
- std::unique_ptr<AudioSyncReader> reader(AudioSyncReader::Create(params));
- if (!reader) {
- return nullptr;
- }
- return base::WrapUnique(new AudioEntry(host, stream_id, render_frame_id,
- params, output_device_id,
- std::move(reader)));
-}
-
///////////////////////////////////////////////////////////////////////////////
// AudioRendererHost implementations.
AudioRendererHost::AudioRendererHost(int render_process_id,
media::AudioManager* audio_manager,
AudioMirroringManager* mirroring_manager,
- MediaInternals* media_internals,
MediaStreamManager* media_stream_manager,
const std::string& salt)
: BrowserMessageFilter(AudioMsgStart),
render_process_id_(render_process_id),
audio_manager_(audio_manager),
mirroring_manager_(mirroring_manager),
- audio_log_(media_internals->CreateAudioLog(
- media::AudioLogFactory::AUDIO_OUTPUT_CONTROLLER)),
+ media_stream_manager_(media_stream_manager),
num_playing_streams_(0),
salt_(salt),
validate_render_frame_id_function_(&ValidateRenderFrameId),
@@ -199,7 +105,7 @@ AudioRendererHost::AudioRendererHost(int render_process_id,
AudioRendererHost::~AudioRendererHost() {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
- CHECK(audio_entries_.empty());
+ CHECK(delegates_.empty());
DaleCurtis 2016/11/29 20:31:24 Probably can go back to a DCHECK() now, was for de
// If we had any streams, report UMA stats for the maximum number of
// simultaneous streams for this render process and for the whole browser
@@ -226,10 +132,10 @@ void AudioRendererHost::GetOutputControllers(
void AudioRendererHost::OnChannelClosing() {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
// Since the IPC sender is gone, close all requested audio streams.
- while (!audio_entries_.empty()) {
- // Note: OnCloseStream() removes the entries from audio_entries_.
- OnCloseStream(audio_entries_.begin()->first);
- }
+ // The audio streams tracker isn't automatically decremented since the
+ // removal isn't done through OnCloseStream.
+ g_audio_streams_tracker.Get().DecreaseStreamCount(delegates_.size());
+ delegates_.clear();
miu 2016/11/29 21:36:27 Get ready for some pain here. In the past, wheneve
Max Morin 2016/12/01 16:08:37 Yes, this is tricky (and I and Olga spent a lot of
// Remove any authorizations for streams that were not yet created
authorizations_.clear();
@@ -239,62 +145,34 @@ void AudioRendererHost::OnDestruct() const {
BrowserThread::DeleteOnIOThread::Destruct(this);
}
-void AudioRendererHost::AudioEntry::OnCreated() {
- BrowserThread::PostTask(
- BrowserThread::IO,
- FROM_HERE,
- base::Bind(&AudioRendererHost::DoCompleteCreation, host_, stream_id_));
-}
-
-void AudioRendererHost::AudioEntry::OnPlaying() {
- BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
- base::Bind(&AudioRendererHost::StreamStateChanged,
- host_, stream_id_, true));
-}
-
-void AudioRendererHost::AudioEntry::OnPaused() {
- BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
- base::Bind(&AudioRendererHost::StreamStateChanged,
- host_, stream_id_, false));
-}
-
-void AudioRendererHost::AudioEntry::OnError() {
- BrowserThread::PostTask(
- BrowserThread::IO,
- FROM_HERE,
- base::Bind(&AudioRendererHost::ReportErrorAndClose, host_, stream_id_));
-}
-
-void AudioRendererHost::DoCompleteCreation(int stream_id) {
+void AudioRendererHost::OnStreamCreated(
+ int stream_id,
+ base::SharedMemory* shared_memory,
+ base::CancelableSyncSocket* foreign_socket) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
if (!PeerHandle()) {
DLOG(WARNING) << "Renderer process handle is invalid.";
- ReportErrorAndClose(stream_id);
+ OnStreamError(stream_id);
return;
}
- AudioEntry* const entry = LookupById(stream_id);
- if (!entry) {
- ReportErrorAndClose(stream_id);
+ if (!LookupById(stream_id)) {
+ SendErrorMessage(stream_id);
DaleCurtis 2016/11/29 20:31:24 Why a different error path than above?
return;
}
- // Now construction is done and we are ready to send the shared memory and the
- // sync socket to the renderer.
- base::SharedMemory* shared_memory = entry->reader()->shared_memory();
- base::CancelableSyncSocket* foreign_socket =
- entry->reader()->foreign_socket();
-
base::SharedMemoryHandle foreign_memory_handle;
base::SyncSocket::TransitDescriptor socket_descriptor;
size_t shared_memory_size = shared_memory->requested_size();
+ // TODO(maxmorin): Figure out how/if this is safe, since shared_memory is also
DaleCurtis 2016/11/29 20:31:24 Can you elaborate? IIRC this completes before any
Max Morin 2016/12/01 16:08:37 This function gets the shared memory from the audi
+ // accessed on the audio thread.
if (!(shared_memory->ShareToProcess(PeerHandle(), &foreign_memory_handle) &&
foreign_socket->PrepareTransitDescriptor(PeerHandle(),
&socket_descriptor))) {
// Something went wrong in preparing the IPC handles.
- ReportErrorAndClose(entry->stream_id());
+ OnStreamError(stream_id);
return;
}
@@ -303,35 +181,39 @@ void AudioRendererHost::DoCompleteCreation(int stream_id) {
base::checked_cast<uint32_t>(shared_memory_size)));
}
-void AudioRendererHost::DidValidateRenderFrame(int stream_id, bool is_valid) {
+void AudioRendererHost::OnStreamStateChanged(bool is_playing) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
+ if (is_playing) {
+ base::AtomicRefCountInc(&num_playing_streams_);
DaleCurtis 2016/11/29 20:31:24 We might be able to drop all this code now. altimi
Max Morin 2016/12/01 16:08:37 Looks good, but it should probably be another CL?
DaleCurtis 2016/12/01 19:33:06 Sure, I was just pointing it out. Filed http://crb
- if (!is_valid) {
- DLOG(WARNING) << "Render frame for stream (id=" << stream_id
- << ") no longer exists.";
- ReportErrorAndClose(stream_id);
+ // Inform the RenderProcessHost when audio starts playing for the first
+ // time. The nonatomic increment-and-read is ok since this is the only
+ // thread that |num_plaing_streams| may be updated on.
+ if (base::AtomicRefCountIsOne(&num_playing_streams_))
+ NotifyRenderProcessHostThatAudioStateChanged(render_process_id_);
+ } else {
+ // Inform the RenderProcessHost when there is no more audio playing.
+ if (!base::AtomicRefCountDec(&num_playing_streams_))
+ NotifyRenderProcessHostThatAudioStateChanged(render_process_id_);
}
}
-void AudioRendererHost::StreamStateChanged(int stream_id, bool is_playing) {
+void AudioRendererHost::OnStreamError(int stream_id) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
- AudioEntry* const entry = LookupById(stream_id);
- if (!entry)
- return;
+ SendErrorMessage(stream_id);
- if (is_playing) {
- AudioStreamMonitor::StartMonitoringStream(
- render_process_id_,
- entry->render_frame_id(),
- entry->stream_id(),
- base::Bind(&media::AudioOutputController::ReadCurrentPowerAndClip,
- entry->controller()));
- } else {
- AudioStreamMonitor::StopMonitoringStream(
- render_process_id_, entry->render_frame_id(), entry->stream_id());
+ OnCloseStream(stream_id);
+}
+
+void AudioRendererHost::DidValidateRenderFrame(int stream_id, bool is_valid) {
+ DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+ if (!is_valid) {
+ DLOG(WARNING) << "Render frame for stream (id=" << stream_id
+ << ") no longer exists.";
+ OnStreamError(stream_id);
}
- UpdateNumPlayingStreams(entry, is_playing);
}
RenderProcessHost::AudioOutputControllerList
@@ -339,11 +221,8 @@ AudioRendererHost::DoGetOutputControllers() const {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
RenderProcessHost::AudioOutputControllerList controllers;
- for (AudioEntryMap::const_iterator it = audio_entries_.begin();
- it != audio_entries_.end();
- ++it) {
- controllers.push_back(it->second->controller());
- }
+ for (const auto& delegate : delegates_)
+ controllers.push_back(delegate->controller());
return controllers;
}
@@ -379,6 +258,7 @@ void AudioRendererHost::OnRequestDeviceAuthorization(
<< ", render_frame_id=" << render_frame_id
<< ", session_id=" << session_id << ", device_id=" << device_id
<< ", security_origin=" << security_origin << ")";
+
if (LookupById(stream_id) || IsAuthorizationStarted(stream_id))
return;
@@ -453,8 +333,9 @@ void AudioRendererHost::OnCreateStream(int stream_id,
}
// Fail early if either of two sanity-checks fail:
- // 1. There should not yet exist an AudioEntry for the given |stream_id|
- // since the renderer may not create two streams with the same ID.
+ // 1. There should not yet exist an AudioOutputDelegate for the given
+ // |stream_id| since the renderer may not create two streams with the
+ // same ID.
// 2. An out-of-range render frame ID was provided. Renderers must *always*
// specify a valid render frame ID for each audio output they create, as
// several browser-level features depend on this (e.g., OOM manager, UI
@@ -481,64 +362,54 @@ void AudioRendererHost::OnCreateStream(int stream_id,
base::Bind(&AudioRendererHost::DidValidateRenderFrame, this,
stream_id)));
- std::unique_ptr<AudioEntry> entry = AudioEntry::Create(
- this, stream_id, render_frame_id, params, device_unique_id);
- if (!entry) {
- SendErrorMessage(stream_id);
- return;
- }
-
MediaObserver* const media_observer =
GetContentClient()->browser()->GetMediaObserver();
- if (media_observer)
- media_observer->OnCreatingAudioStream(render_process_id_, render_frame_id);
- if (mirroring_manager_) {
- mirroring_manager_->AddDiverter(
- render_process_id_, entry->render_frame_id(), entry->controller());
- }
- audio_entries_.insert(std::make_pair(stream_id, entry.release()));
- g_audio_streams_tracker.Get().IncreaseStreamCount();
+ MediaInternals* const media_internals = MediaInternals::GetInstance();
+ std::unique_ptr<media::AudioLog> audio_log = media_internals->CreateAudioLog(
+ media::AudioLogFactory::AUDIO_OUTPUT_CONTROLLER);
+ media_internals->SetWebContentsTitleForAudioLogEntry(
+ stream_id, render_process_id_, render_frame_id, audio_log.get());
+ delegates_.push_back(AudioOutputDelegate::Create(
+ this, audio_manager_, std::move(audio_log), mirroring_manager_,
+ media_observer, stream_id, render_frame_id, render_process_id_, params,
+ device_unique_id));
- audio_log_->OnCreated(stream_id, params, device_unique_id);
- MediaInternals::GetInstance()->SetWebContentsTitleForAudioLogEntry(
- stream_id, render_process_id_, render_frame_id, audio_log_.get());
+ g_audio_streams_tracker.Get().IncreaseStreamCount();
DaleCurtis 2016/11/29 20:31:24 Seems like code that should be moved to AudioStrea
Max Morin 2016/12/01 16:08:37 For another CL?
DaleCurtis 2016/12/01 19:33:06 Covered by http://crbug.com/670383
- if (audio_entries_.size() > max_simultaneous_streams_)
- max_simultaneous_streams_ = audio_entries_.size();
+ if (delegates_.size() > max_simultaneous_streams_)
+ max_simultaneous_streams_ = delegates_.size();
}
void AudioRendererHost::OnPlayStream(int stream_id) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
- AudioEntry* entry = LookupById(stream_id);
- if (!entry) {
+ AudioOutputDelegate* delegate = LookupById(stream_id);
+ if (!delegate) {
SendErrorMessage(stream_id);
return;
}
- entry->controller()->Play();
- audio_log_->OnStarted(stream_id);
+ delegate->OnPlayStream();
}
void AudioRendererHost::OnPauseStream(int stream_id) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
- AudioEntry* entry = LookupById(stream_id);
- if (!entry) {
+ AudioOutputDelegate* delegate = LookupById(stream_id);
+ if (!delegate) {
SendErrorMessage(stream_id);
return;
}
- entry->controller()->Pause();
- audio_log_->OnStopped(stream_id);
+ delegate->OnPauseStream();
}
void AudioRendererHost::OnSetVolume(int stream_id, double volume) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
- AudioEntry* entry = LookupById(stream_id);
- if (!entry) {
+ AudioOutputDelegate* delegate = LookupById(stream_id);
+ if (!delegate) {
SendErrorMessage(stream_id);
return;
}
@@ -546,8 +417,7 @@ void AudioRendererHost::OnSetVolume(int stream_id, double volume) {
// Make sure the volume is valid.
if (volume < 0 || volume > 1.0)
miu 2016/11/29 21:36:28 Should this volume check be in the delegate instea
Max Morin 2016/12/01 16:08:37 Right now I'm leaning towards "0 is minimum and 1
return;
- entry->controller()->SetVolume(volume);
- audio_log_->OnSetVolume(stream_id, volume);
+ delegate->OnSetVolume(volume);
}
void AudioRendererHost::SendErrorMessage(int stream_id) {
@@ -558,87 +428,34 @@ void AudioRendererHost::OnCloseStream(int stream_id) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
authorizations_.erase(stream_id);
- // Prevent oustanding callbacks from attempting to close/delete the same
- // AudioEntry twice.
- AudioEntryMap::iterator i = audio_entries_.find(stream_id);
- if (i == audio_entries_.end())
- return;
- std::unique_ptr<AudioEntry> entry(i->second);
- audio_entries_.erase(i);
- g_audio_streams_tracker.Get().DecreaseStreamCount();
-
- media::AudioOutputController* const controller = entry->controller();
- controller->Close(
- base::Bind(&AudioRendererHost::DeleteEntry, this, base::Passed(&entry)));
- audio_log_->OnClosed(stream_id);
-}
-
-void AudioRendererHost::DeleteEntry(std::unique_ptr<AudioEntry> entry) {
- DCHECK_CURRENTLY_ON(BrowserThread::IO);
-
- // De-register the controller from the AudioMirroringManager now that the
- // controller has closed the AudioOutputStream and shut itself down. This
- // ensures that calling RemoveDiverter() here won't trigger the controller to
- // re-start the default AudioOutputStream and cause a brief audio blip to come
- // out the user's speakers. http://crbug.com/474432
- if (mirroring_manager_)
- mirroring_manager_->RemoveDiverter(entry->controller());
-
- AudioStreamMonitor::StopMonitoringStream(
- render_process_id_, entry->render_frame_id(), entry->stream_id());
- UpdateNumPlayingStreams(entry.get(), false);
-}
-
-void AudioRendererHost::ReportErrorAndClose(int stream_id) {
- DCHECK_CURRENTLY_ON(BrowserThread::IO);
+ auto i = LookupIteratorById(stream_id);
DaleCurtis 2016/11/29 20:31:24 Could just use the delegates_.erase(std::remove_if
Max Morin 2016/12/01 16:08:37 This would require checking if the returned iterat
DaleCurtis 2016/12/01 19:33:06 Not true, remove_if() returns a range, so if nothi
Max Morin 2016/12/02 12:07:37 Right, but the code must still check if it should
DaleCurtis 2016/12/02 18:20:13 Whoops, sorry misread the question.
- // 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))
+ // Prevent oustanding callbacks from attempting to close/delete the same
+ // AudioOutputDelegate twice.
+ if (i == delegates_.end())
return;
- SendErrorMessage(stream_id);
+ std::swap(*i, delegates_.back());
miu 2016/11/29 21:36:28 I like this: Since ordering of this list doesn't m
+ delegates_.pop_back();
- audio_log_->OnError(stream_id);
- OnCloseStream(stream_id);
+ g_audio_streams_tracker.Get().DecreaseStreamCount();
}
-AudioRendererHost::AudioEntry* AudioRendererHost::LookupById(int stream_id) {
+AudioRendererHost::AudioOutputDelegateList::iterator
+AudioRendererHost::LookupIteratorById(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;
+ return std::find_if(delegates_.begin(), delegates_.end(),
+ [stream_id](const AudioOutputDelegate::UniquePtr& d) {
+ return d->stream_id() == stream_id;
+ });
}
-void AudioRendererHost::UpdateNumPlayingStreams(AudioEntry* entry,
- bool is_playing) {
+AudioOutputDelegate* AudioRendererHost::LookupById(int stream_id) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
- if (entry->playing() == is_playing)
- return;
-
- if (is_playing) {
- entry->set_playing(true);
- base::AtomicRefCountInc(&num_playing_streams_);
- // Inform the RenderProcessHost when audio starts playing for the first
- // time.
- if (base::AtomicRefCountIsOne(&num_playing_streams_)) {
- BrowserThread::PostTask(
- BrowserThread::UI, FROM_HERE,
- base::Bind(&NotifyRenderProcessHostThatAudioStateChanged,
- render_process_id_));
- }
- } else {
- entry->set_playing(false);
- // Inform the RenderProcessHost when there is no more audio playing.
- if (!base::AtomicRefCountDec(&num_playing_streams_)) {
- BrowserThread::PostTask(
- BrowserThread::UI, FROM_HERE,
- base::Bind(&NotifyRenderProcessHostThatAudioStateChanged,
- render_process_id_));
- }
- }
+ auto i = LookupIteratorById(stream_id);
+ return i != delegates_.end() ? i->get() : nullptr;
}
bool AudioRendererHost::IsAuthorizationStarted(int stream_id) {

Powered by Google App Engine
This is Rietveld 408576698