diff options
| author | chcunningham <chcunningham@chromium.org> | 2016-03-04 15:22:01 -0800 |
|---|---|---|
| committer | Commit bot <commit-bot@chromium.org> | 2016-03-04 23:23:54 +0000 |
| commit | 6b206d7b4963be6e8e7ff269dcf0015eb7d8bffa (patch) | |
| tree | ea7cfc69d2a9172ddd43cfbff9131061e3deeda5 | |
| parent | b5421b5647a0627226b8b2f48e6143b9d0cf3647 (diff) | |
| download | chromium_src-6b206d7b4963be6e8e7ff269dcf0015eb7d8bffa.zip chromium_src-6b206d7b4963be6e8e7ff269dcf0015eb7d8bffa.tar.gz chromium_src-6b206d7b4963be6e8e7ff269dcf0015eb7d8bffa.tar.bz2 | |
Revert of Add lock to fix race in AudioRendererMixerInput. (patchset #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}
| -rw-r--r-- | media/base/audio_renderer_mixer_input.cc | 35 | ||||
| -rw-r--r-- | media/base/audio_renderer_mixer_input.h | 10 |
2 files changed, 2 insertions, 43 deletions
diff --git a/media/base/audio_renderer_mixer_input.cc b/media/base/audio_renderer_mixer_input.cc index f990538..5743700 100644 --- a/media/base/audio_renderer_mixer_input.cc +++ b/media/base/audio_renderer_mixer_input.cc @@ -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); @@ -53,7 +46,6 @@ void AudioRendererMixerInput::Initialize( } void AudioRendererMixerInput::Start() { - DCHECK(thread_checker_.CalledOnValidThread()); DCHECK(!started_); DCHECK(!mixer_); DCHECK(callback_); // Initialized. @@ -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(); @@ -100,8 +90,6 @@ void AudioRendererMixerInput::Stop() { } void AudioRendererMixerInput::Play() { - DCHECK(thread_checker_.CalledOnValidThread()); - if (playing_ || !mixer_) return; @@ -110,8 +98,6 @@ void AudioRendererMixerInput::Play() { } void AudioRendererMixerInput::Pause() { - DCHECK(thread_checker_.CalledOnValidThread()); - if (!playing_ || !mixer_) return; @@ -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; } @@ -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; @@ -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(); @@ -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. @@ -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() { diff --git a/media/base/audio_renderer_mixer_input.h b/media/base/audio_renderer_mixer_input.h index 6ae5a4bc..34f37c5 100644 --- a/media/base/audio_renderer_mixer_input.h +++ b/media/base/audio_renderer_mixer_input.h @@ -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" @@ -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_; |
