Skip to content

Commit

Permalink
Remove no_data_timer_ in AudioInputController.
Browse files Browse the repository at this point in the history 
Remove disabled test. Fix minor cpplint warnings.

The timer is currently only used for logging anyways. Note that the "no data callback" value for the AudioInputControllerCaptureStartupSuccess histogram was removed in the process. It's unclear (to me) what use it currently has.

BUG=357569

Review-Url: https://codereview.chromium.org/2124463002
Cr-Commit-Position: refs/heads/master@{#404663}
  • Loading branch information
maxmorin authored and Commit bot committed Jul 11, 2016
1 parent efcaee2 commit bd96797
Show file tree
Hide file tree
Showing 5 changed files with 54 additions and 217 deletions.
11 changes: 0 additions & 11 deletions content/browser/renderer_host/media/audio_input_renderer_host.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -290,17 +290,6 @@ void AudioInputRendererHost::DoHandleError(
return;
}

// This is a fix for crbug.com/357501. The error can be triggered when closing
// the lid on Macs, which causes more problems than it fixes.
// Also, in crbug.com/357569, the goal is to remove usage of the error since
// it was added to solve a crash on Windows that no longer can be reproduced.
if (error_code == media::AudioInputController::NO_DATA_ERROR) {
// TODO(henrika): it might be possible to do something other than just
// logging when we detect many NO_DATA_ERROR calls for a stream.
LogMessage(entry->stream_id, "AIC::DoCheckForNoData: NO_DATA_ERROR", false);
return;
}

std::ostringstream oss;
oss << "AIC reports error_code=" << error_code;
LogMessage(entry->stream_id, oss.str(), false);
Expand Down
143 changes: 23 additions & 120 deletions media/audio/audio_input_controller.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

#include "media/audio/audio_input_controller.h"

#include <algorithm>
#include <limits>
#include <utility>

#include "base/bind.h"
Expand All @@ -17,25 +19,10 @@
#include "media/audio/audio_input_writer.h"
#include "media/base/user_input_monitor.h"

using base::TimeDelta;

namespace {

const int kMaxInputChannels = 3;

// TODO(henrika): remove usage of timers and add support for proper
// notification of when the input device is removed. This was originally added
// to resolve http://crbug.com/79936 for Windows platforms. This then caused
// breakage (very hard to repro bugs!) on other platforms: See
// http://crbug.com/226327 and http://crbug.com/230972.
// See also that the timer has been disabled on Mac now due to
// crbug.com/357501.
const int kTimerResetIntervalSeconds = 1;
// We have received reports that the timer can be too trigger happy on some
// Mac devices and the initial timer interval has therefore been increased
// from 1 second to 5 seconds.
const int kTimerInitialIntervalSeconds = 5;

#if defined(AUDIO_POWER_MONITORING)
// Time in seconds between two successive measurements of audio power levels.
const int kPowerMonitorLogIntervalSeconds = 15;
Expand Down Expand Up @@ -91,28 +78,7 @@ float AveragePower(const media::AudioBus& buffer) {
}
#endif // AUDIO_POWER_MONITORING

}

// Used to log the result of capture startup.
// This was previously logged as a boolean with only the no callback and OK
// options. The enum order is kept to ensure backwards compatibility.
// Elements in this enum should not be deleted or rearranged; the only
// permitted operation is to add new elements before CAPTURE_STARTUP_RESULT_MAX
// and update CAPTURE_STARTUP_RESULT_MAX.
enum CaptureStartupResult {
CAPTURE_STARTUP_NO_DATA_CALLBACK = 0,
CAPTURE_STARTUP_OK = 1,
CAPTURE_STARTUP_CREATE_STREAM_FAILED = 2,
CAPTURE_STARTUP_OPEN_STREAM_FAILED = 3,
CAPTURE_STARTUP_RESULT_MAX = CAPTURE_STARTUP_OPEN_STREAM_FAILED
};

void LogCaptureStartupResult(CaptureStartupResult result) {
UMA_HISTOGRAM_ENUMERATION("Media.AudioInputControllerCaptureStartupSuccess",
result,
CAPTURE_STARTUP_RESULT_MAX + 1);

}
} // namespace

namespace media {

Expand All @@ -126,7 +92,7 @@ AudioInputController::AudioInputController(EventHandler* handler,
: creator_task_runner_(base::ThreadTaskRunnerHandle::Get()),
handler_(handler),
stream_(nullptr),
data_is_active_(false),
should_report_stats(0),
state_(CLOSED),
sync_writer_(sync_writer),
max_volume_(0.0),
Expand Down Expand Up @@ -234,11 +200,6 @@ scoped_refptr<AudioInputController> AudioInputController::CreateForStream(
event_handler, sync_writer, user_input_monitor, false));
controller->task_runner_ = task_runner;

// TODO(miu): See TODO at top of file. Until that's resolved, we need to
// disable the error auto-detection here (since the audio mirroring
// implementation will reliably report error and close events). Note, of
// course, that we're assuming CreateForStream() has been called for the audio
// mirroring use case only.
if (!controller->task_runner_->PostTask(
FROM_HERE,
base::Bind(&AudioInputController::DoCreateForStream,
Expand Down Expand Up @@ -284,10 +245,6 @@ void AudioInputController::DoCreate(AudioManager* audio_manager,
silence_state_ = SILENCE_STATE_NO_MEASUREMENT;
#endif

// TODO(miu): See TODO at top of file. Until that's resolved, assume all
// platform audio input requires the |no_data_timer_| be used to auto-detect
// errors. In reality, probably only Windows needs to be treated as
// unreliable here.
DoCreateForStream(audio_manager->MakeAudioInputStream(
params, device_id, base::Bind(&AudioInputController::LogMessage, this)));
}
Expand All @@ -314,6 +271,7 @@ void AudioInputController::DoCreateForStream(

DCHECK(!stream_);
stream_ = stream_to_control;
should_report_stats = 1;

if (!stream_) {
if (handler_)
Expand All @@ -331,8 +289,6 @@ void AudioInputController::DoCreateForStream(
return;
}

DCHECK(!no_data_timer_.get());

// Set AGC state using mode in |agc_is_enabled_| which can only be enabled in
// CreateLowLatency().
#if defined(AUDIO_POWER_MONITORING)
Expand All @@ -347,18 +303,6 @@ void AudioInputController::DoCreateForStream(
stream_->SetAutomaticGainControl(agc_is_enabled_);
#endif

// Create the data timer which will call FirstCheckForNoData(). The timer
// is started in DoRecord() and restarted in each DoCheckForNoData()
// callback.
// The timer is enabled for logging purposes. The NO_DATA_ERROR triggered
// from the timer must be ignored by the EventHandler.
// TODO(henrika): remove usage of timer when it has been verified on Canary
// that we are safe doing so. Goal is to get rid of |no_data_timer_| and
// everything that is tied to it. crbug.com/357569.
no_data_timer_.reset(new base::Timer(
FROM_HERE, base::TimeDelta::FromSeconds(kTimerInitialIntervalSeconds),
base::Bind(&AudioInputController::FirstCheckForNoData,
base::Unretained(this)), false));

state_ = CREATED;
if (handler_)
Expand All @@ -385,12 +329,6 @@ void AudioInputController::DoRecord() {
if (handler_)
handler_->OnLog(this, "AIC::DoRecord");

if (no_data_timer_) {
// Start the data timer. Once |kTimerResetIntervalSeconds| have passed,
// a callback to FirstCheckForNoData() is made.
no_data_timer_->Reset();
}

stream_->Start(this);
if (handler_)
handler_->OnRecording(this);
Expand All @@ -399,6 +337,9 @@ void AudioInputController::DoRecord() {
void AudioInputController::DoClose() {
DCHECK(task_runner_->BelongsToCurrentThread());
SCOPED_UMA_HISTOGRAM_TIMER("Media.AudioInputController.CloseTime");
// If we have already logged something, this does nothing.
// Otherwise, we haven't recieved data.
LogCaptureStartupResult(CAPTURE_STARTUP_NEVER_GOT_DATA);

if (state_ == CLOSED)
return;
Expand All @@ -418,11 +359,7 @@ void AudioInputController::DoClose() {
}
}

// Delete the timer on the same thread that created it.
no_data_timer_.reset();

DoStopCloseAndClearStream();
SetDataIsActive(false);

if (SharedMemoryAndSyncSocketMode())
sync_writer_->Close();
Expand Down Expand Up @@ -471,43 +408,6 @@ void AudioInputController::DoSetVolume(double volume) {
stream_->SetVolume(max_volume_ * volume);
}

void AudioInputController::FirstCheckForNoData() {
DCHECK(task_runner_->BelongsToCurrentThread());
LogCaptureStartupResult(GetDataIsActive() ?
CAPTURE_STARTUP_OK :
CAPTURE_STARTUP_NO_DATA_CALLBACK);
if (handler_) {
handler_->OnLog(this, GetDataIsActive() ?
"AIC::FirstCheckForNoData => data is active" :
"AIC::FirstCheckForNoData => data is NOT active");
}
DoCheckForNoData();
}

void AudioInputController::DoCheckForNoData() {
DCHECK(task_runner_->BelongsToCurrentThread());

if (!GetDataIsActive()) {
// The data-is-active marker will be false only if it has been more than
// one second since a data packet was recorded. This can happen if a
// capture device has been removed or disabled.
if (handler_)
handler_->OnError(this, NO_DATA_ERROR);
}

// Mark data as non-active. The flag will be re-enabled in OnData() each
// time a data packet is received. Hence, under normal conditions, the
// flag will only be disabled during a very short period.
SetDataIsActive(false);

// Restart the timer to ensure that we check the flag again in
// |kTimerResetIntervalSeconds|.
no_data_timer_->Start(
FROM_HERE, base::TimeDelta::FromSeconds(kTimerResetIntervalSeconds),
base::Bind(&AudioInputController::DoCheckForNoData,
base::Unretained(this)));
}

void AudioInputController::OnData(AudioInputStream* stream,
const AudioBus* source,
uint32_t hardware_delay_bytes,
Expand All @@ -526,10 +426,8 @@ void AudioInputController::OnData(AudioInputStream* stream,
this,
base::Passed(&source_copy)));
}

// Mark data as active to ensure that the periodic calls to
// DoCheckForNoData() does not report an error to the event handler.
SetDataIsActive(true);
// Now we have data, so we know for sure that startup was ok.
LogCaptureStartupResult(CAPTURE_STARTUP_OK);

{
base::AutoLock auto_lock(lock_);
Expand Down Expand Up @@ -679,14 +577,6 @@ void AudioInputController::DoStopCloseAndClearStream() {
handler_ = nullptr;
}

void AudioInputController::SetDataIsActive(bool enabled) {
base::subtle::Release_Store(&data_is_active_, enabled);
}

bool AudioInputController::GetDataIsActive() {
return (base::subtle::Acquire_Load(&data_is_active_) != false);
}

#if defined(AUDIO_POWER_MONITORING)
void AudioInputController::UpdateSilenceState(bool silence) {
if (silence) {
Expand Down Expand Up @@ -717,6 +607,19 @@ void AudioInputController::LogSilenceState(SilenceState value) {
}
#endif

void AudioInputController::LogCaptureStartupResult(
CaptureStartupResult result) {
// Decrement shall_report_stats and check if it was 1 before decrement,
// which would imply that this is the first time this method is called
// after initialization. To avoid underflow, we
// also check if should_report_stats is one before decrementing.
if (base::AtomicRefCountIsOne(&should_report_stats) &&
!base::AtomicRefCountDec(&should_report_stats)) {
UMA_HISTOGRAM_ENUMERATION("Media.AudioInputControllerCaptureStartupSuccess",
result, CAPTURE_STARTUP_RESULT_MAX + 1);
}
}

void AudioInputController::DoEnableDebugRecording(
AudioInputWriter* input_writer) {
DCHECK(task_runner_->BelongsToCurrentThread());
Expand Down
Loading

0 comments on commit bd96797

Please sign in to comment.