Skip to content

Commit

Permalink
Makes AudioOutputDispatcher non-ref-counted.
Browse files Browse the repository at this point in the history
AudioManagerBase owns AudioOutputDispatcher instances. AudioOutputProxy
no longer holds a strong reference to AudioOutputDispatcher. This means
dispatcher instances will be deleted when AudioManager shuts down and
fire CHECKs for any open proxy streams.

BUG=608049
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2570923002
Cr-Commit-Position: refs/heads/master@{#438285}
  • Loading branch information
alokp-chromium authored and Commit bot committed Dec 13, 2016
1 parent 3da536f commit 1873f14
Show file tree
Hide file tree
Showing 8 changed files with 32 additions and 75 deletions.
25 changes: 11 additions & 14 deletions media/audio/audio_manager_base.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "base/bind_helpers.h"
#include "base/command_line.h"
#include "base/macros.h"
#include "base/memory/ptr_util.h"
#include "base/single_thread_task_runner.h"
#include "base/strings/string_number_conversions.h"
#include "base/threading/thread_task_runner_handle.h"
Expand Down Expand Up @@ -49,7 +50,7 @@ struct AudioManagerBase::DispatcherParams {
const AudioParameters input_params;
const AudioParameters output_params;
const std::string output_device_id;
scoped_refptr<AudioOutputDispatcher> dispatcher;
std::unique_ptr<AudioOutputDispatcher> dispatcher;

private:
DISALLOW_COPY_AND_ASSIGN(DispatcherParams);
Expand Down Expand Up @@ -258,20 +259,18 @@ AudioOutputStream* AudioManagerBase::MakeAudioOutputStreamProxy(

const base::TimeDelta kCloseDelay =
base::TimeDelta::FromSeconds(kStreamCloseDelaySeconds);
scoped_refptr<AudioOutputDispatcher> dispatcher;
std::unique_ptr<AudioOutputDispatcher> dispatcher;
if (output_params.format() != AudioParameters::AUDIO_FAKE) {
dispatcher = new AudioOutputResampler(this, params, output_params,
output_device_id,
kCloseDelay);
dispatcher = base::MakeUnique<AudioOutputResampler>(
this, params, output_params, output_device_id, kCloseDelay);
} else {
dispatcher = new AudioOutputDispatcherImpl(this, output_params,
output_device_id,
kCloseDelay);
dispatcher = base::MakeUnique<AudioOutputDispatcherImpl>(
this, output_params, output_device_id, kCloseDelay);
}

dispatcher_params->dispatcher = dispatcher;
dispatcher_params->dispatcher = std::move(dispatcher);
output_dispatchers_.push_back(dispatcher_params);
return new AudioOutputProxy(dispatcher.get());
return new AudioOutputProxy(dispatcher_params->dispatcher.get());
}

void AudioManagerBase::ShowAudioInputSettings() {
Expand Down Expand Up @@ -306,11 +305,9 @@ void AudioManagerBase::ReleaseInputStream(AudioInputStream* stream) {

void AudioManagerBase::Shutdown() {
DCHECK(GetTaskRunner()->BelongsToCurrentThread());

// Close all output streams.
while (!output_dispatchers_.empty()) {
output_dispatchers_.back()->dispatcher->Shutdown();
output_dispatchers_.pop_back();
}
output_dispatchers_.clear();

#if defined(OS_MACOSX)
// On mac, AudioManager runs on the main thread, loop for which stops
Expand Down
11 changes: 2 additions & 9 deletions media/audio/audio_output_dispatcher.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
#define MEDIA_AUDIO_AUDIO_OUTPUT_DISPATCHER_H_

#include "base/macros.h"
#include "base/memory/ref_counted.h"
#include "media/audio/audio_io.h"
#include "media/audio/audio_manager.h"
#include "media/base/audio_parameters.h"
Expand All @@ -32,12 +31,12 @@ namespace media {

class AudioOutputProxy;

class MEDIA_EXPORT AudioOutputDispatcher
: public base::RefCountedThreadSafe<AudioOutputDispatcher> {
class MEDIA_EXPORT AudioOutputDispatcher {
public:
AudioOutputDispatcher(AudioManager* audio_manager,
const AudioParameters& params,
const std::string& device_id);
virtual ~AudioOutputDispatcher();

// Called by AudioOutputProxy to open the stream.
// Returns false, if it fails to open it.
Expand All @@ -61,15 +60,9 @@ class MEDIA_EXPORT AudioOutputDispatcher
// Called by AudioOutputProxy when the stream is closed.
virtual void CloseStream(AudioOutputProxy* stream_proxy) = 0;

// Called on the audio thread when the AudioManager is shutting down.
virtual void Shutdown() = 0;

const std::string& device_id() const { return device_id_; }

protected:
friend class base::RefCountedThreadSafe<AudioOutputDispatcher>;
virtual ~AudioOutputDispatcher();

// A no-reference-held pointer (we don't want circular references) back to the
// AudioManager that owns this object.
AudioManager* audio_manager_;
Expand Down
24 changes: 6 additions & 18 deletions media/audio/audio_output_dispatcher_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ AudioOutputDispatcherImpl::AudioOutputDispatcherImpl(
audio_stream_id_(0) {}

AudioOutputDispatcherImpl::~AudioOutputDispatcherImpl() {
CHECK(task_runner_->BelongsToCurrentThread());

// Close all idle streams immediately. The |close_timer_| will handle
// invalidating any outstanding tasks upon its destruction.
CloseAllIdleStreams();

// There must be no idle proxy streams.
CHECK_EQ(idle_proxies_, 0u);

Expand Down Expand Up @@ -122,24 +128,6 @@ void AudioOutputDispatcherImpl::CloseStream(AudioOutputProxy* stream_proxy) {
close_timer_.Reset();
}

void AudioOutputDispatcherImpl::Shutdown() {
DCHECK(task_runner_->BelongsToCurrentThread());

// Close all idle streams immediately. The |close_timer_| will handle
// invalidating any outstanding tasks upon its destruction.
CloseAllIdleStreams();

// No AudioOutputProxy objects should hold a reference to us when we get
// to this stage.
DCHECK(HasOneRef()) << "Only the AudioManager should hold a reference";

LOG_IF(WARNING, idle_proxies_ > 0u) << "Idle proxy streams during shutdown: "
<< idle_proxies_;
LOG_IF(WARNING, !proxy_to_physical_map_.empty())
<< "Active proxy streams during shutdown: "
<< proxy_to_physical_map_.size();
}

bool AudioOutputDispatcherImpl::HasOutputProxies() const {
return idle_proxies_ || !proxy_to_physical_map_.empty();
}
Expand Down
6 changes: 1 addition & 5 deletions media/audio/audio_output_dispatcher_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ class MEDIA_EXPORT AudioOutputDispatcherImpl : public AudioOutputDispatcher {
const AudioParameters& params,
const std::string& output_device_id,
const base::TimeDelta& close_delay);
~AudioOutputDispatcherImpl() override;

// Opens a new physical stream if there are no pending streams in
// |idle_streams_|. Do not call Close() or Stop() if this method fails.
Expand All @@ -61,18 +62,13 @@ class MEDIA_EXPORT AudioOutputDispatcherImpl : public AudioOutputDispatcher {
// kept alive until |close_timer_| fires.
void CloseStream(AudioOutputProxy* stream_proxy) override;

void Shutdown() override;

// Returns true if there are any open AudioOutputProxy objects.
bool HasOutputProxies() const;

// Closes all |idle_streams_|.
void CloseAllIdleStreams();

private:
friend class base::RefCountedThreadSafe<AudioOutputDispatcherImpl>;
~AudioOutputDispatcherImpl() override;

// Creates a new physical output stream, opens it and pushes to
// |idle_streams_|. Returns false if the stream couldn't be created or
// opened.
Expand Down
5 changes: 2 additions & 3 deletions media/audio/audio_output_proxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

#include "base/compiler_specific.h"
#include "base/macros.h"
#include "base/memory/ref_counted.h"
#include "base/threading/non_thread_safe.h"
#include "media/audio/audio_io.h"
#include "media/base/audio_parameters.h"
Expand Down Expand Up @@ -40,7 +39,7 @@ class MEDIA_EXPORT AudioOutputProxy
void Close() override;

AudioOutputDispatcher* get_dispatcher_for_testing() const {
return dispatcher_.get();
return dispatcher_;
}

private:
Expand All @@ -55,7 +54,7 @@ class MEDIA_EXPORT AudioOutputProxy

~AudioOutputProxy() override;

scoped_refptr<AudioOutputDispatcher> dispatcher_;
AudioOutputDispatcher* dispatcher_;
State state_;

// Need to save volume here, so that we can restore it in case the stream
Expand Down
13 changes: 6 additions & 7 deletions media/audio/audio_output_proxy_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <memory>
#include <string>

#include "base/memory/ptr_util.h"
#include "base/message_loop/message_loop.h"
#include "base/run_loop.h"
#include "base/single_thread_task_runner.h"
Expand Down Expand Up @@ -174,10 +175,8 @@ class AudioOutputProxyTest : public testing::Test {
}

virtual void InitDispatcher(base::TimeDelta close_delay) {
dispatcher_impl_ = new AudioOutputDispatcherImpl(&manager(),
params_,
std::string(),
close_delay);
dispatcher_impl_ = base::MakeUnique<AudioOutputDispatcherImpl>(
&manager(), params_, std::string(), close_delay);
}

virtual void OnStart() {}
Expand Down Expand Up @@ -422,7 +421,7 @@ class AudioOutputProxyTest : public testing::Test {
}

base::MessageLoop message_loop_;
scoped_refptr<AudioOutputDispatcherImpl> dispatcher_impl_;
std::unique_ptr<AudioOutputDispatcherImpl> dispatcher_impl_;
MockAudioManager manager_;
MockAudioSourceCallback callback_;
AudioParameters params_;
Expand All @@ -439,7 +438,7 @@ class AudioOutputResamplerTest : public AudioOutputProxyTest {
resampler_params_ = AudioParameters(
AudioParameters::AUDIO_PCM_LOW_LATENCY, CHANNEL_LAYOUT_STEREO,
16000, 16, 1024);
resampler_ = new AudioOutputResampler(
resampler_ = base::MakeUnique<AudioOutputResampler>(
&manager(), params_, resampler_params_, std::string(), close_delay);
}

Expand All @@ -454,7 +453,7 @@ class AudioOutputResamplerTest : public AudioOutputProxyTest {

protected:
AudioParameters resampler_params_;
scoped_refptr<AudioOutputResampler> resampler_;
std::unique_ptr<AudioOutputResampler> resampler_;
};

TEST_F(AudioOutputProxyTest, CreateAndClose) {
Expand Down
15 changes: 2 additions & 13 deletions media/audio/audio_output_resampler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "base/bind_helpers.h"
#include "base/compiler_specific.h"
#include "base/macros.h"
#include "base/memory/ptr_util.h"
#include "base/metrics/histogram_macros.h"
#include "base/metrics/sparse_histogram.h"
#include "base/numerics/safe_conversions.h"
Expand Down Expand Up @@ -255,7 +256,6 @@ void AudioOutputResampler::Reinitialize() {
// Log a trace event so we can get feedback in the field when this happens.
TRACE_EVENT0("audio", "AudioOutputResampler::Reinitialize");

dispatcher_->Shutdown();
output_params_ = original_output_params_;
streams_opened_ = false;
Initialize();
Expand All @@ -264,7 +264,7 @@ void AudioOutputResampler::Reinitialize() {
void AudioOutputResampler::Initialize() {
DCHECK(!streams_opened_);
DCHECK(callbacks_.empty());
dispatcher_ = new AudioOutputDispatcherImpl(
dispatcher_ = base::MakeUnique<AudioOutputDispatcherImpl>(
audio_manager_, output_params_, device_id_, close_delay_);
}

Expand Down Expand Up @@ -391,17 +391,6 @@ void AudioOutputResampler::CloseStream(AudioOutputProxy* stream_proxy) {
}
}

void AudioOutputResampler::Shutdown() {
DCHECK(task_runner_->BelongsToCurrentThread());

// No AudioOutputProxy objects should hold a reference to us when we get
// to this stage.
DCHECK(HasOneRef()) << "Only the AudioManager should hold a reference";

dispatcher_->Shutdown();
DCHECK(callbacks_.empty());
}

OnMoreDataConverter::OnMoreDataConverter(const AudioParameters& input_params,
const AudioParameters& output_params)
: io_ratio_(static_cast<double>(input_params.GetBytesPerSecond()) /
Expand Down
8 changes: 2 additions & 6 deletions media/audio/audio_output_resampler.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
#include <map>

#include "base/macros.h"
#include "base/memory/ref_counted.h"
#include "base/time/time.h"
#include "base/timer/timer.h"
#include "media/audio/audio_io.h"
Expand Down Expand Up @@ -37,6 +36,7 @@ class MEDIA_EXPORT AudioOutputResampler : public AudioOutputDispatcher {
const AudioParameters& output_params,
const std::string& output_device_id,
const base::TimeDelta& close_delay);
~AudioOutputResampler() override;

// AudioOutputDispatcher interface.
bool OpenStream() override;
Expand All @@ -45,12 +45,8 @@ class MEDIA_EXPORT AudioOutputResampler : public AudioOutputDispatcher {
void StopStream(AudioOutputProxy* stream_proxy) override;
void StreamVolumeSet(AudioOutputProxy* stream_proxy, double volume) override;
void CloseStream(AudioOutputProxy* stream_proxy) override;
void Shutdown() override;

private:
friend class base::RefCountedThreadSafe<AudioOutputResampler>;
~AudioOutputResampler() override;

// Converts low latency based output parameters into high latency
// appropriate output parameters in error situations.
void SetupFallbackParams();
Expand All @@ -62,7 +58,7 @@ class MEDIA_EXPORT AudioOutputResampler : public AudioOutputDispatcher {
void Initialize();

// Dispatcher to proxy all AudioOutputDispatcher calls too.
scoped_refptr<AudioOutputDispatcherImpl> dispatcher_;
std::unique_ptr<AudioOutputDispatcherImpl> dispatcher_;

// Map of outstanding OnMoreDataConverter objects. A new object is created
// on every StartStream() call and destroyed on CloseStream().
Expand Down

0 comments on commit 1873f14

Please sign in to comment.