Commit 28c1e190 authored by Jordan Bayles's avatar Jordan Bayles Committed by Commit Bot

Revert diagnostics for SnooperNode::Render() bug

This patch reverts a previous patch that added diagnostics for a tricky
SnooperNode::Render() bug. Most of the files had merge conflicts that
required manual resolution.

Bug: crbug.com/888478
Change-Id: I7ff4ff07df9cf845ebc500facbbc3487d20891c1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2128845Reviewed-by: default avatarmark a. foltz <mfoltz@chromium.org>
Reviewed-by: default avatarDale Curtis <dalecurtis@chromium.org>
Commit-Queue: Jordan Bayles <jophba@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756401}
parent 286b61cc
......@@ -67,7 +67,6 @@ source_set("audio") {
deps = [
":testing_api_support",
"//build:chromecast_buildflags",
"//components/crash/core/common:crash_key", # Temporary: crbug.com/888478
]
if (audio_processing_in_audio_service_supported) {
......
include_rules = [
"+components/crash/core/common/crash_key.h", # Temporary: crbug.com/888478
"+media/audio",
"+media/base",
"+media/mojo",
......
......@@ -5,17 +5,14 @@
#include "services/audio/loopback_stream.h"
#include <algorithm>
#include <cinttypes>
#include <string>
#include "base/bind.h"
#include "base/stl_util.h"
#include "base/strings/stringprintf.h"
#include "base/sync_socket.h"
#include "base/threading/sequenced_task_runner_handle.h"
#include "base/time/default_tick_clock.h"
#include "base/trace_event/trace_event.h"
#include "components/crash/core/common/crash_key.h"
#include "media/base/audio_bus.h"
#include "media/base/vector_math.h"
#include "mojo/public/cpp/system/buffer.h"
......@@ -240,9 +237,6 @@ void LoopbackStream::OnError() {
// take care of the rest.
}
// static
std::atomic<int> LoopbackStream::FlowNetwork::instance_count_;
LoopbackStream::FlowNetwork::FlowNetwork(
scoped_refptr<base::SequencedTaskRunner> flow_task_runner,
const media::AudioParameters& output_params,
......@@ -251,39 +245,26 @@ LoopbackStream::FlowNetwork::FlowNetwork(
flow_task_runner_(flow_task_runner),
output_params_(output_params),
writer_(std::move(writer)),
mix_bus_(media::AudioBus::Create(output_params_)) {
++instance_count_;
magic_bytes_ = 0x600DC0DEu;
HelpDiagnoseCauseOfLoopbackCrash("constructed");
}
mix_bus_(media::AudioBus::Create(output_params_)) {}
void LoopbackStream::FlowNetwork::AddInput(SnooperNode* node) {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK_CALLED_ON_VALID_SEQUENCE(control_sequence_);
base::AutoLock scoped_lock(lock_);
if (inputs_.empty()) {
HelpDiagnoseCauseOfLoopbackCrash("adding first input");
}
DCHECK(!base::Contains(inputs_, node));
inputs_.push_back(node);
}
void LoopbackStream::FlowNetwork::RemoveInput(SnooperNode* node) {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK_CALLED_ON_VALID_SEQUENCE(control_sequence_);
base::AutoLock scoped_lock(lock_);
const auto it = std::find(inputs_.begin(), inputs_.end(), node);
DCHECK(it != inputs_.end());
inputs_.erase(it);
if (inputs_.empty()) {
HelpDiagnoseCauseOfLoopbackCrash("removed last input");
}
}
void LoopbackStream::FlowNetwork::SetVolume(double volume) {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK_CALLED_ON_VALID_SEQUENCE(control_sequence_);
base::AutoLock scoped_lock(lock_);
......@@ -291,15 +272,12 @@ void LoopbackStream::FlowNetwork::SetVolume(double volume) {
}
void LoopbackStream::FlowNetwork::Start() {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK_CALLED_ON_VALID_SEQUENCE(control_sequence_);
DCHECK(!is_started());
timer_.emplace(clock_);
// Note: GenerateMoreAudio() will schedule the timer.
HelpDiagnoseCauseOfLoopbackCrash("starting");
first_generate_time_ = clock_->NowTicks();
frames_elapsed_ = 0;
next_generate_time_ = first_generate_time_;
......@@ -313,17 +291,11 @@ void LoopbackStream::FlowNetwork::Start() {
}
LoopbackStream::FlowNetwork::~FlowNetwork() {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK(flow_task_runner_->RunsTasksInCurrentSequence());
DCHECK(inputs_.empty());
HelpDiagnoseCauseOfLoopbackCrash("destructing");
magic_bytes_ = 0xDEADBEEFu;
--instance_count_;
}
void LoopbackStream::FlowNetwork::GenerateMoreAudio() {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK(flow_task_runner_->RunsTasksInCurrentSequence());
TRACE_EVENT_WITH_FLOW0("audio", "GenerateMoreAudio", this,
......@@ -338,8 +310,6 @@ void LoopbackStream::FlowNetwork::GenerateMoreAudio() {
base::AutoLock scoped_lock(lock_);
output_volume = volume_;
HelpDiagnoseCauseOfLoopbackCrash("generating");
// Compute the reference time to use for audio rendering. Query each input
// node and update |capture_delay_|, if necessary. This is used to always
// generate audio from a "safe point" in the recent past, to prevent buffer
......@@ -435,45 +405,4 @@ void LoopbackStream::FlowNetwork::GenerateMoreAudio() {
&FlowNetwork::GenerateMoreAudio);
}
void LoopbackStream::FlowNetwork::HelpDiagnoseCauseOfLoopbackCrash(
const char* event) {
static crash_reporter::CrashKeyString<512> crash_string(
"audio-service-loopback");
const auto ToAbbreviatedParamsString =
[](const media::AudioParameters& params) {
return base::StringPrintf(
"F%d|L%d|R%d|FPB%d", static_cast<int>(params.format()),
static_cast<int>(params.channel_layout()), params.sample_rate(),
params.frames_per_buffer());
};
std::vector<std::string> input_formats;
input_formats.reserve(inputs_.size());
for (const SnooperNode* input : inputs_) {
input_formats.push_back(ToAbbreviatedParamsString(input->input_params()));
}
crash_string.Set(base::StringPrintf(
"num_instances=%d, event=%s, elapsed=%" PRId64 ", first_gen_ts=%" PRId64
", next_gen_ts=%" PRId64
", has_transfer_bus=%c, format=%s, volume=%f, has_timer=%c, inputs={%s}",
instance_count_.load(), event, frames_elapsed_,
(first_generate_time_ - base::TimeTicks()).InMicroseconds(),
(next_generate_time_ - base::TimeTicks()).InMicroseconds(),
transfer_bus_ ? 'Y' : 'N',
ToAbbreviatedParamsString(output_params_).c_str(), volume_,
timer_ ? 'Y' : 'N', base::JoinString(input_formats, ", ").c_str()));
// If there are any crashes from this code, please record to crbug.com/888478.
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
CHECK(mix_bus_);
CHECK_GT(mix_bus_->channels(), 0);
CHECK_EQ(mix_bus_->channels(), output_params_.channels());
CHECK_GT(mix_bus_->frames(), 0);
CHECK_EQ(mix_bus_->frames(), output_params_.frames_per_buffer());
for (int i = 0; i < mix_bus_->channels(); ++i) {
float* const data = mix_bus_->channel(i);
CHECK(data);
memset(data, 0, mix_bus_->frames() * sizeof(data[0]));
}
}
} // namespace audio
......@@ -5,7 +5,6 @@
#ifndef SERVICES_AUDIO_LOOPBACK_STREAM_H_
#define SERVICES_AUDIO_LOOPBACK_STREAM_H_
#include <atomic>
#include <map>
#include <memory>
#include <utility>
......@@ -157,12 +156,6 @@ class LoopbackStream : public media::mojom::AudioInputStream,
// becomes stopped.
void GenerateMoreAudio();
// TODO(crbug.com/888478): Remove this and all call points after diagnosis.
// This generates crash key strings exposing the current state of the flow
// network, and also ensures |mix_bus_| is valid, hasn't been corrupted, and
// that writing to its data arrays will not cause a page fault.
void HelpDiagnoseCauseOfLoopbackCrash(const char* event);
const base::TickClock* clock_;
// Task runner that calls GenerateMoreAudio() to drive all the audio data
......@@ -213,10 +206,6 @@ class LoopbackStream : public media::mojom::AudioInputStream,
std::unique_ptr<media::AudioBus> transfer_bus_;
const std::unique_ptr<media::AudioBus> mix_bus_;
// TODO(crbug.com/888478): Remove these after diagnosis.
volatile uint32_t magic_bytes_;
static std::atomic<int> instance_count_;
SEQUENCE_CHECKER(control_sequence_);
DISALLOW_COPY_AND_ASSIGN(FlowNetwork);
......
......@@ -16,7 +16,6 @@
#include "base/system/system_monitor.h"
#include "base/time/default_tick_clock.h"
#include "base/trace_event/trace_event.h"
#include "components/crash/core/common/crash_key.h"
#include "media/audio/audio_manager.h"
#include "media/base/bind_to_current_loop.h"
#include "services/audio/debug_recording.h"
......@@ -31,22 +30,12 @@
namespace audio {
namespace {
// TODO(crbug.com/888478): Remove this after diagnosis.
crash_reporter::CrashKeyString<64> g_service_state_for_crashing(
"audio-service-state");
} // namespace
Service::Service(std::unique_ptr<AudioManagerAccessor> audio_manager_accessor,
bool enable_remote_client_support,
mojo::PendingReceiver<mojom::AudioService> receiver)
: receiver_(this, std::move(receiver)),
audio_manager_accessor_(std::move(audio_manager_accessor)),
enable_remote_client_support_(enable_remote_client_support) {
magic_bytes_ = 0x600DC0DEu;
g_service_state_for_crashing.Set("constructing");
DCHECK(audio_manager_accessor_);
if (enable_remote_client_support_) {
......@@ -67,30 +56,22 @@ Service::Service(std::unique_ptr<AudioManagerAccessor> audio_manager_accessor,
metrics_ =
std::make_unique<ServiceMetrics>(base::DefaultTickClock::GetInstance());
g_service_state_for_crashing.Set("constructed");
}
Service::~Service() {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
g_service_state_for_crashing.Set("destructing");
TRACE_EVENT0("audio", "audio::Service::~Service");
metrics_.reset();
g_service_state_for_crashing.Set("destructing - killed metrics");
// Stop all streams cleanly before shutting down the audio manager.
stream_factory_.reset();
g_service_state_for_crashing.Set("destructing - killed stream_factory");
// Reset |debug_recording_| to disable debug recording before AudioManager
// shutdown.
debug_recording_.reset();
g_service_state_for_crashing.Set("destructing - killed debug_recording");
audio_manager_accessor_->Shutdown();
g_service_state_for_crashing.Set("destructing - did shut down manager");
magic_bytes_ = 0xDEADBEEFu;
}
// static
......@@ -113,7 +94,6 @@ void Service::SetTestingApiBinderForTesting(TestingApiBinder binder) {
void Service::BindSystemInfo(
mojo::PendingReceiver<mojom::SystemInfo> receiver) {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
auto& binder_override = GetSystemInfoBinderForTesting();
......@@ -131,7 +111,6 @@ void Service::BindSystemInfo(
void Service::BindDebugRecording(
mojo::PendingReceiver<mojom::DebugRecording> receiver) {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
// Accept only one bind request at a time. Old receiver is overwritten.
......@@ -144,7 +123,6 @@ void Service::BindDebugRecording(
void Service::BindStreamFactory(
mojo::PendingReceiver<mojom::StreamFactory> receiver) {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
if (!stream_factory_)
......@@ -154,7 +132,6 @@ void Service::BindStreamFactory(
void Service::BindDeviceNotifier(
mojo::PendingReceiver<mojom::DeviceNotifier> receiver) {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
DCHECK(enable_remote_client_support_);
......@@ -166,7 +143,6 @@ void Service::BindDeviceNotifier(
void Service::BindLogFactoryManager(
mojo::PendingReceiver<mojom::LogFactoryManager> receiver) {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
DCHECK(log_factory_manager_);
DCHECK(enable_remote_client_support_);
......@@ -181,7 +157,6 @@ void Service::BindTestingApi(
}
void Service::InitializeDeviceMonitor() {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
#if defined(OS_MACOSX)
if (audio_device_listener_mac_)
......
......@@ -125,9 +125,6 @@ class Service : public mojom::AudioService {
std::unique_ptr<LogFactoryManager> log_factory_manager_;
std::unique_ptr<ServiceMetrics> metrics_;
// TODO(crbug.com/888478): Remove this after diagnosis.
volatile uint32_t magic_bytes_;
DISALLOW_COPY_AND_ASSIGN(Service);
};
......
......@@ -67,8 +67,6 @@ class SnooperNode : public LoopbackGroupMember::Snooper {
~SnooperNode() final;
const media::AudioParameters& input_params() const { return input_params_; }
// GroupMember::Snooper implementation. Inserts more data into the delay
// buffer.
void OnData(const media::AudioBus& input_bus,
......
......@@ -11,7 +11,6 @@
#include "base/trace_event/trace_event.h"
#include "base/unguessable_token.h"
#include "build/chromecast_buildflags.h"
#include "components/crash/core/common/crash_key.h"
#include "media/audio/audio_device_description.h"
#include "services/audio/input_stream.h"
#include "services/audio/local_muter.h"
......@@ -24,19 +23,13 @@ namespace audio {
StreamFactory::StreamFactory(media::AudioManager* audio_manager)
: audio_manager_(audio_manager),
loopback_worker_thread_("Loopback Worker") {
magic_bytes_ = 0x600DC0DEu;
SetStateForCrashing("constructed");
}
StreamFactory::~StreamFactory() {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK_CALLED_ON_VALID_SEQUENCE(owning_sequence_);
SetStateForCrashing("destructing");
magic_bytes_ = 0xDEADBEEFu;
}
void StreamFactory::Bind(mojo::PendingReceiver<mojom::StreamFactory> receiver) {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK_CALLED_ON_VALID_SEQUENCE(owning_sequence_);
receivers_.Add(this, std::move(receiver));
}
......@@ -53,9 +46,7 @@ void StreamFactory::CreateInputStream(
base::ReadOnlySharedMemoryRegion key_press_count_buffer,
mojom::AudioProcessingConfigPtr processing_config,
CreateInputStreamCallback created_callback) {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK_CALLED_ON_VALID_SEQUENCE(owning_sequence_);
SetStateForCrashing("creating input stream");
TRACE_EVENT_NESTABLE_ASYNC_INSTANT2("audio", "CreateInputStream", this,
"device id", device_id, "params",
params.AsHumanReadableString());
......@@ -73,7 +64,6 @@ void StreamFactory::CreateInputStream(
log->OnError();
// The callback must still be invoked or mojo complains.
std::move(created_callback).Run(nullptr, false, base::nullopt);
SetStateForCrashing("input stream create failed");
return;
}
......@@ -88,23 +78,18 @@ void StreamFactory::CreateInputStream(
UserInputMonitor::Create(std::move(key_press_count_buffer)), device_id,
params, shared_memory_count, enable_agc, &stream_monitor_coordinator_,
std::move(processing_config)));
SetStateForCrashing("created input stream");
}
void StreamFactory::AssociateInputAndOutputForAec(
const base::UnguessableToken& input_stream_id,
const std::string& output_device_id) {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK_CALLED_ON_VALID_SEQUENCE(owning_sequence_);
SetStateForCrashing("associating for AEC");
for (const auto& stream : input_streams_) {
if (stream->id() == input_stream_id) {
stream->SetOutputDeviceForAec(output_device_id);
SetStateForCrashing("associated for AEC");
return;
}
}
SetStateForCrashing("did not associate for AEC");
}
void StreamFactory::CreateOutputStream(
......@@ -117,9 +102,7 @@ void StreamFactory::CreateOutputStream(
const base::UnguessableToken& group_id,
const base::Optional<base::UnguessableToken>& processing_id,
CreateOutputStreamCallback created_callback) {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK_CALLED_ON_VALID_SEQUENCE(owning_sequence_);
SetStateForCrashing("creating output stream");
TRACE_EVENT_NESTABLE_ASYNC_INSTANT2("audio", "CreateOutputStream", this,
"device id", output_device_id, "params",
params.AsHumanReadableString());
......@@ -147,15 +130,12 @@ void StreamFactory::CreateOutputStream(
audio_manager_, device_id_or_group_id, params, &coordinator_, group_id,
&stream_monitor_coordinator_,
processing_id.value_or(base::UnguessableToken())));
SetStateForCrashing("created output stream");
}
void StreamFactory::BindMuter(
mojo::PendingAssociatedReceiver<mojom::LocalMuter> receiver,
const base::UnguessableToken& group_id) {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK_CALLED_ON_VALID_SEQUENCE(owning_sequence_);
SetStateForCrashing("binding muter");
TRACE_EVENT_NESTABLE_ASYNC_INSTANT1("audio", "BindMuter", this, "group id",
group_id.GetLowForSerialization());
......@@ -177,7 +157,6 @@ void StreamFactory::BindMuter(
// Add the receiver.
muter->AddReceiver(std::move(receiver));
SetStateForCrashing("bound muter");
}
void StreamFactory::CreateLoopbackStream(
......@@ -188,9 +167,7 @@ void StreamFactory::CreateLoopbackStream(
uint32_t shared_memory_count,
const base::UnguessableToken& group_id,
CreateLoopbackStreamCallback created_callback) {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK_CALLED_ON_VALID_SEQUENCE(owning_sequence_);
SetStateForCrashing("creating loopback stream");
TRACE_EVENT_NESTABLE_ASYNC_INSTANT2("audio", "CreateLoopbackStream", this,
"group id",
group_id.GetLowForSerialization(),
......@@ -231,29 +208,21 @@ void StreamFactory::CreateLoopbackStream(
std::move(observer), params, shared_memory_count, &coordinator_,
group_id);
loopback_streams_.emplace_back(std::move(stream));
SetStateForCrashing("created loopback stream");
}
void StreamFactory::DestroyInputStream(InputStream* stream) {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK_CALLED_ON_VALID_SEQUENCE(owning_sequence_);
SetStateForCrashing("destroying input stream");
size_t erased = input_streams_.erase(stream);
DCHECK_EQ(1u, erased);
SetStateForCrashing("destroyed input stream");
}
void StreamFactory::DestroyOutputStream(OutputStream* stream) {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK_CALLED_ON_VALID_SEQUENCE(owning_sequence_);
SetStateForCrashing("destroying output stream");
size_t erased = output_streams_.erase(stream);
DCHECK_EQ(1u, erased);
SetStateForCrashing("destroyed output stream");
}
void StreamFactory::DestroyMuter(LocalMuter* muter) {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK_CALLED_ON_VALID_SEQUENCE(owning_sequence_);
DCHECK(muter);
......@@ -266,14 +235,12 @@ void StreamFactory::DestroyMuter(LocalMuter* muter) {
auto do_destroy = [](base::WeakPtr<StreamFactory> weak_this,
LocalMuter* muter) {
if (weak_this) {
weak_this->SetStateForCrashing("destroying muter");
const auto it =
std::find_if(weak_this->muters_.begin(), weak_this->muters_.end(),
base::MatchesUniquePtr(muter));
DCHECK(it != weak_this->muters_.end());
weak_this->muters_.erase(it);
weak_this->SetStateForCrashing("destroyed muter");
}
};
......@@ -283,20 +250,15 @@ void StreamFactory::DestroyMuter(LocalMuter* muter) {
}
void StreamFactory::DestroyLoopbackStream(LoopbackStream* stream) {
CHECK_EQ(magic_bytes_, 0x600DC0DEu);
DCHECK_CALLED_ON_VALID_SEQUENCE(owning_sequence_);
DCHECK(stream);
SetStateForCrashing("destroying loopback stream");
const auto it =
std::find_if(loopback_streams_.begin(), loopback_streams_.end(),
base::MatchesUniquePtr(stream));
DCHECK(it != loopback_streams_.end());
loopback_streams_.erase(it);
SetStateForCrashing("destroyed loopback stream");
// If all LoopbackStreams have ended, stop and join the worker thread.
if (loopback_streams_.empty()) {
TRACE_EVENT0("audio", "Stop Loopback Worker");
......@@ -304,17 +266,4 @@ void StreamFactory::DestroyLoopbackStream(LoopbackStream* stream) {
}
}
void StreamFactory::SetStateForCrashing(const char* state) {
static crash_reporter::CrashKeyString<256> crash_string(
"audio-service-factory-state");
crash_string.Set(base::StringPrintf(
"%s: binding_count=%d, muters_count=%d, loopback_count=%d, "
"input_stream_count=%d, output_stream_count=%d",
state, static_cast<int>(receivers_.size()),
static_cast<int>(muters_.size()),
static_cast<int>(loopback_streams_.size()),
static_cast<int>(input_streams_.size()),
static_cast<int>(output_streams_.size())));
}
} // namespace audio
......@@ -100,9 +100,6 @@ class StreamFactory final : public mojom::StreamFactory {
void DestroyMuter(LocalMuter* muter);
void DestroyLoopbackStream(LoopbackStream* stream);
// TODO(crbug.com/888478): Remove this after diagnosis.
void SetStateForCrashing(const char* state);
SEQUENCE_CHECKER(owning_sequence_);
media::AudioManager* const audio_manager_;
......@@ -118,11 +115,7 @@ class StreamFactory final : public mojom::StreamFactory {
InputStreamSet input_streams_;
OutputStreamSet output_streams_;
// TODO(crbug.com/888478): Remove this after diagnosis.
volatile uint32_t magic_bytes_;
base::WeakPtrFactory<StreamFactory> weak_ptr_factory_{this};
DISALLOW_COPY_AND_ASSIGN(StreamFactory);
};
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment