Skip to content

Commit

Permalink
Revert of Add lock to fix race in AudioRendererMixerInput. (patchset c…
Browse files Browse the repository at this point in the history
…hromium#5 id:80001 of https://codereview.chromium.org/1748183006/ )

Reason for revert:
This seems to cause failures in LayoutTests/http/tests/security/media. Not sure why this wasn't caught by CQ...

Tracking CQ weirdness here:
https://bugs.chromium.org/p/chromium/issues/detail?id=592079

Original issue's description:
> Add lock to fix race in AudioRendererMixerInput.
>
> Clusterfuzz identified a race between the media thread calling SetVolume
> and the audio device thread calling ProvideInput. Add a lock to
> synchronize access to |volume_| between threads.
>
> Also adds some thread_checkers to just firm up the contract that the
> majority of these methods are called only by the media thread.
>
> BUG=588992
>
> Committed: https://crrev.com/57c6958a99f2c62a4408d2a9d262a22129309106
> Cr-Commit-Position: refs/heads/master@{#379349}

TBR=dalecurtis@chromium.org,olka@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=588992

Review URL: https://codereview.chromium.org/1771463002

Cr-Commit-Position: refs/heads/master@{#379388}
  • Loading branch information
chcunningham authored and Commit bot committed Mar 4, 2016
1 parent b5421b5 commit 6b206d7
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 43 deletions.
35 changes: 2 additions & 33 deletions media/base/audio_renderer_mixer_input.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,22 +29,15 @@ AudioRendererMixerInput::AudioRendererMixerInput(
mixer_(nullptr),
callback_(nullptr),
error_cb_(base::Bind(&AudioRendererMixerInput::OnRenderError,
base::Unretained(this))) {
// Can be constructed on any thread, but sink operations should all occur
// on same thread.
thread_checker_.DetachFromThread();
}
base::Unretained(this))) {}

AudioRendererMixerInput::~AudioRendererMixerInput() {
DCHECK(!started_);
DCHECK(!mixer_);
}

void AudioRendererMixerInput::Initialize(
const AudioParameters& params,
AudioRendererSink::RenderCallback* callback) {
DCHECK(thread_checker_.CalledOnValidThread());
DCHECK(!started_);
DCHECK(!mixer_);
DCHECK(callback);

Expand All @@ -53,7 +46,6 @@ void AudioRendererMixerInput::Initialize(
}

void AudioRendererMixerInput::Start() {
DCHECK(thread_checker_.CalledOnValidThread());
DCHECK(!started_);
DCHECK(!mixer_);
DCHECK(callback_); // Initialized.
Expand All @@ -76,8 +68,6 @@ void AudioRendererMixerInput::Start() {
}

void AudioRendererMixerInput::Stop() {
DCHECK(thread_checker_.CalledOnValidThread());

// Stop() may be called at any time, if Pause() hasn't been called we need to
// remove our mixer input before shutdown.
Pause();
Expand All @@ -100,8 +90,6 @@ void AudioRendererMixerInput::Stop() {
}

void AudioRendererMixerInput::Play() {
DCHECK(thread_checker_.CalledOnValidThread());

if (playing_ || !mixer_)
return;

Expand All @@ -110,8 +98,6 @@ void AudioRendererMixerInput::Play() {
}

void AudioRendererMixerInput::Pause() {
DCHECK(thread_checker_.CalledOnValidThread());

if (!playing_ || !mixer_)
return;

Expand All @@ -120,8 +106,6 @@ void AudioRendererMixerInput::Pause() {
}

bool AudioRendererMixerInput::SetVolume(double volume) {
DCHECK(thread_checker_.CalledOnValidThread());
base::AutoLock auto_lock(volume_lock_);
volume_ = volume;
return true;
}
Expand All @@ -134,8 +118,6 @@ void AudioRendererMixerInput::SwitchOutputDevice(
const std::string& device_id,
const url::Origin& security_origin,
const SwitchOutputDeviceCB& callback) {
DCHECK(thread_checker_.CalledOnValidThread());

if (!mixer_) {
if (pending_switch_callback_.is_null()) {
pending_switch_callback_ = callback;
Expand Down Expand Up @@ -177,16 +159,12 @@ void AudioRendererMixerInput::SwitchOutputDevice(
}

AudioParameters AudioRendererMixerInput::GetOutputParameters() {
DCHECK(thread_checker_.CalledOnValidThread());

if (mixer_)
return mixer_->GetOutputDevice()->GetOutputParameters();
return get_hardware_params_cb_.Run(device_id_, security_origin_);
}

OutputDeviceStatus AudioRendererMixerInput::GetDeviceStatus() {
DCHECK(thread_checker_.CalledOnValidThread());

if (mixer_)
return mixer_->GetOutputDevice()->GetDeviceStatus();

Expand All @@ -198,11 +176,6 @@ OutputDeviceStatus AudioRendererMixerInput::GetDeviceStatus() {

double AudioRendererMixerInput::ProvideInput(AudioBus* audio_bus,
base::TimeDelta buffer_delay) {
// No thread checker here. This method is called on a different thread as part
// of audio rendering. AudioRendererMixer has locks that protect us from
// things like attempting to ProvideInput while simultaneously removing
// ourselves from mixer inputs (see Pause()).

// TODO(chcunningham): Delete this conversion and change ProvideInput to more
// precisely describe delay as a count of frames delayed instead of TimeDelta.
// See http://crbug.com/587522.
Expand All @@ -217,11 +190,7 @@ double AudioRendererMixerInput::ProvideInput(AudioBus* audio_bus,
frames_filled, audio_bus->frames() - frames_filled);
}

// Synchronize access to |volume_| with SetVolume().
{
base::AutoLock auto_lock(volume_lock_);
return frames_filled > 0 ? volume_ : 0;
}
return frames_filled > 0 ? volume_ : 0;
}

void AudioRendererMixerInput::OnRenderError() {
Expand Down
10 changes: 0 additions & 10 deletions media/base/audio_renderer_mixer_input.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@

#include "base/callback.h"
#include "base/macros.h"
#include "base/synchronization/lock.h"
#include "base/threading/thread_checker.h"
#include "media/base/audio_converter.h"
#include "media/base/audio_renderer_sink.h"
#include "media/base/output_device.h"
Expand Down Expand Up @@ -71,14 +69,6 @@ class MEDIA_EXPORT AudioRendererMixerInput
private:
friend class AudioRendererMixerInputTest;

// Used to DCHECK that control methods (Start/Stop/Switch...) are called from
// the same thread.
base::ThreadChecker thread_checker_;

// Protect |volume_|, accessed by separate threads in ProvideInput() and
// SetVolume().
base::Lock volume_lock_;

bool started_;
bool playing_;
double volume_;
Expand Down

0 comments on commit 6b206d7

Please sign in to comment.