Commit 2bb318bb authored by Hidehiko Abe's avatar Hidehiko Abe Committed by Commit Bot

Migrate CrasAudioClient::GetNodes to DBusMethodCallback.

Along with the change, log_error_ in CrasAudioHandler is removed.
The flag is introduced to reduce the logging during reboot,
because cras service may not yet start. However, now we have
WaitForServiceToBeAvailable(), so that we can correctly
wait it is up.

BUG=739622
TEST=Ran trybots.
     Ran on DUT, and made sure no log spam in /var/log/chrome/chrome.

Change-Id: Ic36f3a3096ae0abf213fdf7329d6141d7d1475d9
Reviewed-on: https://chromium-review.googlesource.com/760437Reviewed-by: 's avatarDan Erat <derat@chromium.org>
Reviewed-by: 's avatarJenny Zhang <jennyz@chromium.org>
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516146}
parent ed6a1c0d
......@@ -30,7 +30,6 @@
#include "chrome/browser/ui/ash/ash_util.h"
#include "chrome/common/chrome_switches.h"
#include "chrome/common/pref_names.h"
#include "chromeos/audio/cras_audio_handler.h"
#include "chromeos/chromeos_switches.h"
#include "chromeos/cryptohome/cryptohome_parameters.h"
#include "chromeos/dbus/dbus_thread_manager.h"
......@@ -132,10 +131,6 @@ void StartUserSession(Profile* user_profile, const std::string& login_user_id) {
bool is_running_test = command_line->HasSwitch(::switches::kTestName) ||
command_line->HasSwitch(::switches::kTestType);
if (!is_running_test) {
// Enable CrasAudioHandler logging when chrome restarts after crashing.
if (chromeos::CrasAudioHandler::IsInitialized())
chromeos::CrasAudioHandler::Get()->LogErrors();
// We did not log in (we crashed or are debugging), so we need to
// restore Sync.
UserSessionManager::GetInstance()->RestoreAuthenticationSession(
......
......@@ -5,6 +5,8 @@
#ifndef CHROME_BROWSER_CHROMEOS_LOGIN_SESSION_CHROME_SESSION_MANAGER_H_
#define CHROME_BROWSER_CHROMEOS_LOGIN_SESSION_CHROME_SESSION_MANAGER_H_
#include <string>
#include "base/macros.h"
#include "components/session_manager/core/session_manager.h"
......
......@@ -647,10 +647,6 @@ void CrasAudioHandler::SetMuteForDevice(uint64_t device_id, bool mute_on) {
audio_pref_handler_->SetMuteValue(*device, mute_on);
}
void CrasAudioHandler::LogErrors() {
log_errors_ = true;
}
// If the HDMI device is the active output device, when the device enters/exits
// docking mode, or HDMI display changes resolution, or chromeos device
// suspends/resumes, cras will lose the HDMI output node for a short period of
......@@ -685,7 +681,6 @@ CrasAudioHandler::CrasAudioHandler(
output_mute_locked_(false),
output_channels_(2),
output_mono_on_(false),
log_errors_(false),
hdmi_rediscover_grace_period_duration_in_ms_(
kHDMIRediscoverGracePeriodDurationInMs),
hdmi_rediscovering_(false),
......@@ -699,8 +694,6 @@ CrasAudioHandler::CrasAudioHandler(
return;
GetCrasAudioClient()->AddObserver(this);
audio_pref_handler_->AddAudioPrefObserver(this);
if (DBusThreadManager::Get()->GetSessionManagerClient())
DBusThreadManager::Get()->GetSessionManagerClient()->AddObserver(this);
InitializeAudioState();
// Unittest may not have the task runner for the current thread.
if (base::ThreadTaskRunnerHandle::IsSet())
......@@ -712,17 +705,12 @@ CrasAudioHandler::~CrasAudioHandler() {
if (!HasCrasAudioClient())
return;
GetCrasAudioClient()->RemoveObserver(this);
if (DBusThreadManager::Get()->GetSessionManagerClient())
DBusThreadManager::Get()->GetSessionManagerClient()->RemoveObserver(this);
if (audio_pref_handler_.get())
audio_pref_handler_->RemoveAudioPrefObserver(this);
audio_pref_handler_ = nullptr;
}
void CrasAudioHandler::AudioClientRestarted() {
// Make sure the logging is enabled in case cras server
// restarts after crashing.
LogErrors();
InitializeAudioState();
}
......@@ -791,9 +779,8 @@ void CrasAudioHandler::ActiveOutputNodeChanged(uint64_t node_id) {
// During system boot, cras may change active input to unknown device 0x1,
// we don't need to log it, since it is not an valid device.
if (GetDeviceFromId(node_id)) {
LOG_IF(WARNING, log_errors_)
<< "Active output node changed unexpectedly by system node_id="
<< "0x" << std::hex << node_id;
LOG(WARNING) << "Active output node changed unexpectedly by system node_id="
<< "0x" << std::hex << node_id;
}
}
......@@ -805,9 +792,8 @@ void CrasAudioHandler::ActiveInputNodeChanged(uint64_t node_id) {
// During system boot, cras may change active input to unknown device 0x2,
// we don't need to log it, since it is not an valid device.
if (GetDeviceFromId(node_id)) {
LOG_IF(WARNING, log_errors_)
<< "Active input node changed unexpectedly by system node_id="
<< "0x" << std::hex << node_id;
LOG(WARNING) << "Active input node changed unexpectedly by system node_id="
<< "0x" << std::hex << node_id;
}
}
......@@ -820,12 +806,6 @@ void CrasAudioHandler::OnAudioPolicyPrefChanged() {
ApplyAudioPolicy();
}
void CrasAudioHandler::EmitLoginPromptVisibleCalled() {
// Enable logging after cras server is started, which will be after
// EmitLoginPromptVisible.
LogErrors();
}
const AudioDevice* CrasAudioHandler::GetDeviceFromId(uint64_t device_id) const {
AudioDeviceMap::const_iterator it = audio_devices_.find(device_id);
if (it == audio_devices_.end())
......@@ -856,9 +836,8 @@ void CrasAudioHandler::SetupAudioInputState() {
// Set the initial audio state to the ones read from audio prefs.
const AudioDevice* device = GetDeviceFromId(active_input_node_id_);
if (!device) {
LOG_IF(ERROR, log_errors_)
<< "Can't set up audio state for unknown input device id ="
<< "0x" << std::hex << active_input_node_id_;
LOG(ERROR) << "Can't set up audio state for unknown input device id ="
<< "0x" << std::hex << active_input_node_id_;
return;
}
input_gain_ = audio_pref_handler_->GetInputGainValue(device);
......@@ -872,9 +851,8 @@ void CrasAudioHandler::SetupAudioInputState() {
void CrasAudioHandler::SetupAudioOutputState() {
const AudioDevice* device = GetDeviceFromId(active_output_node_id_);
if (!device) {
LOG_IF(ERROR, log_errors_)
<< "Can't set up audio state for unknown output device id ="
<< "0x" << std::hex << active_output_node_id_;
LOG(ERROR) << "Can't set up audio state for unknown output device id ="
<< "0x" << std::hex << active_output_node_id_;
return;
}
DCHECK(!device->is_input);
......@@ -1028,11 +1006,8 @@ void CrasAudioHandler::SetInputMuteInternal(bool mute_on) {
}
void CrasAudioHandler::GetNodes() {
GetCrasAudioClient()->GetNodes(
base::Bind(&CrasAudioHandler::HandleGetNodes,
weak_ptr_factory_.GetWeakPtr()),
base::Bind(&CrasAudioHandler::HandleGetNodesError,
weak_ptr_factory_.GetWeakPtr()));
GetCrasAudioClient()->GetNodes(base::BindOnce(
&CrasAudioHandler::HandleGetNodes, weak_ptr_factory_.GetWeakPtr()));
}
bool CrasAudioHandler::ChangeActiveDevice(
......@@ -1494,24 +1469,21 @@ void CrasAudioHandler::HandleAudioDeviceChange(
}
}
void CrasAudioHandler::HandleGetNodes(const chromeos::AudioNodeList& node_list,
bool success) {
if (!success) {
LOG_IF(ERROR, log_errors_) << "Failed to retrieve audio nodes data";
void CrasAudioHandler::HandleGetNodes(
base::Optional<chromeos::AudioNodeList> node_list) {
if (!node_list.has_value()) {
LOG(ERROR) << "Failed to retrieve audio nodes data";
return;
}
UpdateDevicesAndSwitchActive(node_list);
if (node_list->empty())
return;
UpdateDevicesAndSwitchActive(node_list.value());
for (auto& observer : observers_)
observer.OnAudioNodesChanged();
}
void CrasAudioHandler::HandleGetNodesError(const std::string& error_name,
const std::string& error_msg) {
LOG_IF(ERROR, log_errors_) << "Failed to call GetNodes: "
<< error_name << ": " << error_msg;
}
void CrasAudioHandler::AddAdditionalActiveNode(uint64_t node_id, bool notify) {
const AudioDevice* device = GetDeviceFromId(node_id);
if (!device) {
......@@ -1686,7 +1658,7 @@ void CrasAudioHandler::GetDefaultOutputBufferSizeInternal() {
void CrasAudioHandler::HandleGetDefaultOutputBufferSize(int32_t buffer_size,
bool success) {
if (!success) {
LOG_IF(ERROR, log_errors_) << "Failed to retrieve output buffer size";
LOG(ERROR) << "Failed to retrieve output buffer size";
return;
}
......
......@@ -7,20 +7,22 @@
#include <stddef.h>
#include <stdint.h>
#include <queue>
#include <vector>
#include "base/containers/circular_deque.h"
#include "base/macros.h"
#include "base/memory/ref_counted.h"
#include "base/memory/weak_ptr.h"
#include "base/observer_list.h"
#include "base/optional.h"
#include "base/timer/timer.h"
#include "chromeos/audio/audio_device.h"
#include "chromeos/audio/audio_devices_pref_handler.h"
#include "chromeos/audio/audio_pref_observer.h"
#include "chromeos/dbus/audio_node.h"
#include "chromeos/dbus/cras_audio_client.h"
#include "chromeos/dbus/session_manager_client.h"
#include "chromeos/dbus/volume_state.h"
#include "media/base/video_facing.h"
......@@ -32,7 +34,6 @@ class AudioDevicesPrefHandler;
// browser main thread.
class CHROMEOS_EXPORT CrasAudioHandler : public CrasAudioClient::Observer,
public AudioPrefObserver,
public SessionManagerClient::Observer,
public media::VideoCaptureObserver {
public:
typedef std::
......@@ -259,9 +260,6 @@ class CHROMEOS_EXPORT CrasAudioHandler : public CrasAudioClient::Observer,
// Returns true if output mono is enabled.
bool IsOutputMonoEnabled() const;
// Enables error logging.
void LogErrors();
// If necessary, sets the starting point for re-discovering the active HDMI
// output device caused by device entering/exiting docking mode, HDMI display
// changing resolution, or chromeos device suspend/resume. If
......@@ -301,9 +299,6 @@ class CHROMEOS_EXPORT CrasAudioHandler : public CrasAudioClient::Observer,
// AudioPrefObserver overrides.
void OnAudioPolicyPrefChanged() override;
// SessionManagerClient::Observer overrides.
void EmitLoginPromptVisibleCalled() override;
// Sets the |active_device| to be active.
// If |notify|, notifies Active*NodeChange.
// Saves device active states in prefs. |activate_by| indicates how
......@@ -390,11 +385,7 @@ class CHROMEOS_EXPORT CrasAudioHandler : public CrasAudioClient::Observer,
bool* active_device_removed);
// Handles dbus callback for GetNodes.
void HandleGetNodes(const chromeos::AudioNodeList& node_list, bool success);
// Handles the dbus error callback.
void HandleGetNodesError(const std::string& error_name,
const std::string& error_msg);
void HandleGetNodes(base::Optional<chromeos::AudioNodeList> node_list);
// Adds an active node.
// If there is no active node, |node_id| will be switched to become the
......@@ -510,9 +501,6 @@ class CHROMEOS_EXPORT CrasAudioHandler : public CrasAudioClient::Observer,
int32_t output_channels_;
bool output_mono_on_;
// Failures are not logged at startup, since CRAS may not be running yet.
bool log_errors_;
// Timer for HDMI re-discovering grace period.
base::OneShotTimer hdmi_rediscover_timer_;
int hdmi_rediscover_grace_period_duration_in_ms_;
......
......@@ -20,10 +20,6 @@
namespace chromeos {
// Error name if cras dbus call fails with empty ErrorResponse.
const char kNoResponseError[] =
"org.chromium.cras.Error.NoResponse";
// The CrasAudioClient implementation used in production.
class CrasAudioClientImpl : public CrasAudioClient {
public:
......@@ -63,16 +59,13 @@ class CrasAudioClientImpl : public CrasAudioClient {
weak_ptr_factory_.GetWeakPtr(), callback));
}
void GetNodes(const GetNodesCallback& callback,
const ErrorCallback& error_callback) override {
void GetNodes(DBusMethodCallback<AudioNodeList> callback) override {
dbus::MethodCall method_call(cras::kCrasControlInterface,
cras::kGetNodes);
cras_proxy_->CallMethodWithErrorCallback(
cras_proxy_->CallMethod(
&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
base::BindOnce(&CrasAudioClientImpl::OnGetNodes,
weak_ptr_factory_.GetWeakPtr(), callback),
base::BindOnce(&CrasAudioClientImpl::OnError,
weak_ptr_factory_.GetWeakPtr(), error_callback));
weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
}
void SetOutputNodeVolume(uint64_t node_id, int32_t volume) override {
......@@ -430,54 +423,38 @@ class CrasAudioClientImpl : public CrasAudioClient {
callback.Run(buffer_size, success);
}
void OnGetNodes(const GetNodesCallback& callback,
void OnGetNodes(DBusMethodCallback<AudioNodeList> callback,
dbus::Response* response) {
bool success = true;
AudioNodeList node_list;
if (response) {
dbus::MessageReader response_reader(response);
dbus::MessageReader array_reader(response);
while (response_reader.HasMoreData()) {
if (!response_reader.PopArray(&array_reader)) {
success = false;
LOG(ERROR) << "Error reading response from cras: "
<< response->ToString();
break;
}
AudioNode node;
if (!GetAudioNode(response, &array_reader, &node)) {
success = false;
LOG(WARNING) << "Error reading audio node data from cras: "
<< response->ToString();
break;
}
// Filter out the "UNKNOWN" type of audio devices.
if (node.type != "UNKNOWN")
node_list.push_back(node);
}
if (!response) {
std::move(callback).Run(base::nullopt);
return;
}
if (node_list.empty())
return;
AudioNodeList node_list;
dbus::MessageReader response_reader(response);
dbus::MessageReader array_reader(response);
while (response_reader.HasMoreData()) {
if (!response_reader.PopArray(&array_reader)) {
LOG(ERROR) << "Error reading response from cras: "
<< response->ToString();
std::move(callback).Run(base::nullopt);
return;
}
callback.Run(node_list, success);
}
AudioNode node;
if (!GetAudioNode(response, &array_reader, &node)) {
LOG(WARNING) << "Error reading audio node data from cras: "
<< response->ToString();
std::move(callback).Run(base::nullopt);
return;
}
void OnError(const ErrorCallback& error_callback,
dbus::ErrorResponse* response) {
// Error response has optional error message argument.
std::string error_name;
std::string error_message;
if (response) {
dbus::MessageReader reader(response);
error_name = response->GetErrorName();
reader.PopString(&error_message);
} else {
error_name = kNoResponseError;
error_message = "";
// Filter out the "UNKNOWN" type of audio devices.
if (node.type != "UNKNOWN")
node_list.push_back(std::move(node));
}
error_callback.Run(error_name, error_message);
std::move(callback).Run(std::move(node_list));
}
bool GetAudioNode(dbus::Response* response,
......
......@@ -76,17 +76,6 @@ class CHROMEOS_EXPORT CrasAudioClient : public DBusClient {
// succeeded.
typedef base::Callback<void(int, bool)> GetDefaultOutputBufferSizeCallback;
// GetNodesCallback is used for GetNodes method. It receives 2 arguments,
// |audio_nodes| which containing a list of audio nodes data and
// |success| which indicates whether or not the request succeeded.
typedef base::Callback<void(const AudioNodeList&, bool)> GetNodesCallback;
// ErrorCallback is used for cras dbus method error response. It receives 2
// arguments, |error_name| indicates the dbus error name, and |error_message|
// contains the detailed dbus error message.
typedef base::Callback<void(const std::string&,
const std::string&)> ErrorCallback;
// Gets the volume state, asynchronously.
virtual void GetVolumeState(const GetVolumeStateCallback& callback) = 0;
......@@ -95,8 +84,7 @@ class CHROMEOS_EXPORT CrasAudioClient : public DBusClient {
const GetDefaultOutputBufferSizeCallback& callback) = 0;
// Gets an array of audio input and output nodes.
virtual void GetNodes(const GetNodesCallback& callback,
const ErrorCallback& error_callback) = 0;
virtual void GetNodes(DBusMethodCallback<AudioNodeList> callback) = 0;
// Sets output volume of the given |node_id| to |volume|, in the rage of
// [0, 100].
......
......@@ -81,18 +81,6 @@ const AudioNode kInternalMicV2(true,
false,
0);
// A mock ErrorCallback.
class MockErrorCallback {
public:
MockErrorCallback() {}
~MockErrorCallback() {}
MOCK_METHOD2(Run, void(const std::string& error_name,
const std::string& error_message));
CrasAudioClient::ErrorCallback GetCallback() {
return base::Bind(&MockErrorCallback::Run, base::Unretained(this));
}
};
// A mock CrasAudioClient Observer.
class MockObserver : public CrasAudioClient::Observer {
public:
......@@ -224,26 +212,27 @@ void WriteNodesToResponse(const AudioNodeList& node_list,
}
// Expect the AudioNodeList result.
void ExpectAudioNodeListResult(const AudioNodeList* expected_node_list,
const AudioNodeList& node_list,
bool call_status) {
EXPECT_EQ(true, call_status);
EXPECT_EQ(expected_node_list->size(), node_list.size());
void ExpectAudioNodeListResult(bool* called,
const AudioNodeList& expected_node_list,
base::Optional<AudioNodeList> result) {
*called = true;
ASSERT_TRUE(result.has_value());
const AudioNodeList& node_list = result.value();
ASSERT_EQ(expected_node_list.size(), node_list.size());
for (size_t i = 0; i < node_list.size(); ++i) {
EXPECT_EQ((*expected_node_list)[i].is_input, node_list[i].is_input);
EXPECT_EQ((*expected_node_list)[i].id, node_list[i].id);
EXPECT_EQ((*expected_node_list)[i].stable_device_id_v1,
EXPECT_EQ(expected_node_list[i].is_input, node_list[i].is_input);
EXPECT_EQ(expected_node_list[i].id, node_list[i].id);
EXPECT_EQ(expected_node_list[i].stable_device_id_v1,
node_list[i].stable_device_id_v1);
EXPECT_EQ((*expected_node_list)[i].stable_device_id_v2,
EXPECT_EQ(expected_node_list[i].stable_device_id_v2,
node_list[i].stable_device_id_v2);
EXPECT_EQ((*expected_node_list)[i].device_name, node_list[i].device_name);
EXPECT_EQ((*expected_node_list)[i].type, node_list[i].type);
EXPECT_EQ((*expected_node_list)[i].name, node_list[i].name);
EXPECT_EQ((*expected_node_list)[i].mic_positions,
node_list[i].mic_positions);
EXPECT_EQ((*expected_node_list)[i].active, node_list[i].active);
EXPECT_EQ((*expected_node_list)[i].plugged_time, node_list[i].plugged_time);
EXPECT_EQ((*expected_node_list)[i].StableDeviceIdVersion(),
EXPECT_EQ(expected_node_list[i].device_name, node_list[i].device_name);
EXPECT_EQ(expected_node_list[i].type, node_list[i].type);
EXPECT_EQ(expected_node_list[i].name, node_list[i].name);
EXPECT_EQ(expected_node_list[i].mic_positions, node_list[i].mic_positions);
EXPECT_EQ(expected_node_list[i].active, node_list[i].active);
EXPECT_EQ(expected_node_list[i].plugged_time, node_list[i].plugged_time);
EXPECT_EQ(expected_node_list[i].StableDeviceIdVersion(),
node_list[i].StableDeviceIdVersion());
}
}
......@@ -770,9 +759,7 @@ TEST_F(CrasAudioClientTest, OutputNodeVolumeChanged) {
TEST_F(CrasAudioClientTest, GetNodes) {
// Create the expected value.
AudioNodeList expected_node_list;
expected_node_list.push_back(kInternalSpeaker);
expected_node_list.push_back(kInternalMic);
AudioNodeList expected_node_list{kInternalSpeaker, kInternalMic};
// Create response.
std::unique_ptr<dbus::Response> response(dbus::Response::CreateEmpty());
......@@ -784,20 +771,17 @@ TEST_F(CrasAudioClientTest, GetNodes) {
base::Bind(&ExpectNoArgument),
response.get());
// Call method.
MockErrorCallback error_callback;
client_->GetNodes(base::Bind(&ExpectAudioNodeListResult,
&expected_node_list),
error_callback.GetCallback());
EXPECT_CALL(error_callback, Run(_, _)).Times(0);
bool called = false;
client_->GetNodes(
base::BindOnce(&ExpectAudioNodeListResult, &called, expected_node_list));
// Run the message loop.
base::RunLoop().RunUntilIdle();
EXPECT_TRUE(called);
}
TEST_F(CrasAudioClientTest, GetNodesV2) {
// Create the expected value.
AudioNodeList expected_node_list;
expected_node_list.push_back(kInternalSpeakerV2);
expected_node_list.push_back(kInternalMicV2);
AudioNodeList expected_node_list{kInternalSpeakerV2, kInternalMicV2};
// Create response.
std::unique_ptr<dbus::Response> response(dbus::Response::CreateEmpty());
......@@ -807,13 +791,14 @@ TEST_F(CrasAudioClientTest, GetNodesV2) {
// Set expectations.
PrepareForMethodCall(cras::kGetNodes, base::Bind(&ExpectNoArgument),
response.get());
// Call method.
MockErrorCallback error_callback;
client_->GetNodes(base::Bind(&ExpectAudioNodeListResult, &expected_node_list),
error_callback.GetCallback());
EXPECT_CALL(error_callback, Run(_, _)).Times(0);
bool called = false;
client_->GetNodes(
base::BindOnce(&ExpectAudioNodeListResult, &called, expected_node_list));
// Run the message loop.
base::RunLoop().RunUntilIdle();
EXPECT_TRUE(called);
}
TEST_F(CrasAudioClientTest, SetOutputNodeVolume) {
......
......@@ -107,9 +107,8 @@ void FakeCrasAudioClient::GetDefaultOutputBufferSize(
callback.Run(512, true);
}
void FakeCrasAudioClient::GetNodes(const GetNodesCallback& callback,
const ErrorCallback& error_callback) {
callback.Run(node_list_, true);
void FakeCrasAudioClient::GetNodes(DBusMethodCallback<AudioNodeList> callback) {
std::move(callback).Run(node_list_);
}
void FakeCrasAudioClient::SetOutputNodeVolume(uint64_t node_id,
......
......@@ -29,8 +29,7 @@ class CHROMEOS_EXPORT FakeCrasAudioClient : public CrasAudioClient {
void GetVolumeState(const GetVolumeStateCallback& callback) override;
void GetDefaultOutputBufferSize(
const GetDefaultOutputBufferSizeCallback& callback) override;
void GetNodes(const GetNodesCallback& callback,
const ErrorCallback& error_callback) override;
void GetNodes(DBusMethodCallback<AudioNodeList> callback) override;
void SetOutputNodeVolume(uint64_t node_id, int32_t volume) override;
void SetOutputUserMute(bool mute_on) override;
void SetInputNodeGain(uint64_t node_id, int32_t gain) override;
......
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