Commit 5f08ba57 authored by Jia's avatar Jia Committed by Commit Bot

[On-device adaptive brightness] Consolidate sequence task runners.

All jobs will run on the main UI thread except the blocking ones that will run
on a blocking task runner (created by the relevant object).

The following tasks will run on a blocking task runner:

1. AlsReaderImpl
(i). Running cmd to
- Check if Als is enabled.
- Get Als config.
- Get path to Als file from which we'll read Als values.
(ii). Reading Als values.

2. ModellerImpl
(i). Load curve from disk and save curve to disk.
(ii). Call trainer to
- Set its initial curves.
- Train a model.
Hence trainer needs to be associated with the blocking
task runner via base::OnTaskRunnerDeleter.

Bug: 881215
Change-Id: Iecb5805304c5aa6e0a1b194e136b81ed97534d7b
Reviewed-on: https://chromium-review.googlesource.com/c/1321655Reviewed-by: default avatarAndrew Moylan <amoylan@chromium.org>
Commit-Queue: Jia Meng <jiameng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606743}
parent ac464a7c
......@@ -24,7 +24,7 @@ namespace auto_screen_brightness {
constexpr base::TimeDelta Adapter::kAmbientLightShortHorizon;
constexpr int Adapter::kNumberAmbientValuesToTrack;
Adapter::Adapter(Profile* profile,
Adapter::Adapter(const Profile* profile,
AlsReader* als_reader,
BrightnessMonitor* brightness_monitor,
Modeller* modeller,
......
......@@ -112,7 +112,7 @@ class Adapter : public AlsReader::Observer,
kMaxValue = kDisabled
};
Adapter(Profile* profile,
Adapter(const Profile* profile,
AlsReader* als_reader,
BrightnessMonitor* brightness_monitor,
Modeller* modeller,
......@@ -183,7 +183,7 @@ class Adapter : public AlsReader::Observer,
base::Optional<double> GetBrightnessBasedOnAmbientLogLux(
double ambient_lux) const;
Profile* const profile_;
const Profile* const profile_;
ScopedObserver<AlsReader, AlsReader::Observer> als_reader_observer_;
ScopedObserver<BrightnessMonitor, BrightnessMonitor::Observer>
......
......@@ -106,14 +106,17 @@ constexpr int AlsReaderImpl::kMaxInitialAttempts;
constexpr base::TimeDelta AlsReaderImpl::kAlsPollInterval;
AlsReaderImpl::AlsReaderImpl()
: als_task_runner_(base::CreateSequencedTaskRunnerWithTraits(
: blocking_task_runner_(base::CreateSequencedTaskRunnerWithTraits(
{base::TaskPriority::BEST_EFFORT, base::MayBlock(),
base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})),
base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN})),
weak_ptr_factory_(this) {}
AlsReaderImpl::~AlsReaderImpl() = default;
AlsReaderImpl::~AlsReaderImpl() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
}
void AlsReaderImpl::AddObserver(Observer* const observer) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(observer);
observers_.AddObserver(observer);
if (status_ != AlsInitStatus::kInProgress)
......@@ -121,24 +124,27 @@ void AlsReaderImpl::AddObserver(Observer* const observer) {
}
void AlsReaderImpl::RemoveObserver(Observer* const observer) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(observer);
observers_.RemoveObserver(observer);
}
void AlsReaderImpl::Init() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
base::PostTaskAndReplyWithResult(
als_task_runner_.get(), FROM_HERE, base::BindOnce(&IsAlsEnabled),
blocking_task_runner_.get(), FROM_HERE, base::BindOnce(&IsAlsEnabled),
base::BindOnce(&AlsReaderImpl::OnAlsEnableCheckDone,
weak_ptr_factory_.GetWeakPtr()));
}
void AlsReaderImpl::SetTaskRunnerForTesting(
const scoped_refptr<base::SequencedTaskRunner> task_runner) {
als_task_runner_ = task_runner;
als_timer_.SetTaskRunner(task_runner);
const scoped_refptr<base::SequencedTaskRunner> test_blocking_task_runner) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
blocking_task_runner_ = test_blocking_task_runner;
}
void AlsReaderImpl::InitForTesting(const base::FilePath& ambient_light_path) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(!ambient_light_path.empty());
ambient_light_path_ = ambient_light_path;
status_ = AlsInitStatus::kSuccess;
......@@ -147,6 +153,7 @@ void AlsReaderImpl::InitForTesting(const base::FilePath& ambient_light_path) {
}
void AlsReaderImpl::OnAlsEnableCheckDone(const bool is_enabled) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (!is_enabled) {
status_ = AlsInitStatus::kDisabled;
OnInitializationComplete();
......@@ -154,12 +161,13 @@ void AlsReaderImpl::OnAlsEnableCheckDone(const bool is_enabled) {
}
base::PostTaskAndReplyWithResult(
als_task_runner_.get(), FROM_HERE, base::BindOnce(&VerifyAlsConfig),
blocking_task_runner_.get(), FROM_HERE, base::BindOnce(&VerifyAlsConfig),
base::BindOnce(&AlsReaderImpl::OnAlsConfigCheckDone,
weak_ptr_factory_.GetWeakPtr()));
}
void AlsReaderImpl::OnAlsConfigCheckDone(const bool is_config_valid) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (!is_config_valid) {
status_ = AlsInitStatus::kIncorrectConfig;
OnInitializationComplete();
......@@ -170,6 +178,7 @@ void AlsReaderImpl::OnAlsConfigCheckDone(const bool is_config_valid) {
}
void AlsReaderImpl::OnAlsPathReadAttempted(const std::string& path) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (!path.empty()) {
ambient_light_path_ = base::FilePath(path);
status_ = AlsInitStatus::kSuccess;
......@@ -191,27 +200,31 @@ void AlsReaderImpl::OnAlsPathReadAttempted(const std::string& path) {
}
void AlsReaderImpl::RetryAlsPath() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
base::PostTaskAndReplyWithResult(
als_task_runner_.get(), FROM_HERE, base::BindOnce(&GetAlsPath),
blocking_task_runner_.get(), FROM_HERE, base::BindOnce(&GetAlsPath),
base::BindOnce(&AlsReaderImpl::OnAlsPathReadAttempted,
weak_ptr_factory_.GetWeakPtr()));
}
void AlsReaderImpl::OnInitializationComplete() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK_NE(status_, AlsInitStatus::kInProgress);
for (auto& observer : observers_)
observer.OnAlsReaderInitialized(status_);
}
void AlsReaderImpl::ReadAlsPeriodically() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
base::PostTaskAndReplyWithResult(
als_task_runner_.get(), FROM_HERE,
blocking_task_runner_.get(), FROM_HERE,
base::BindOnce(&ReadAlsFromFile, ambient_light_path_),
base::BindOnce(&AlsReaderImpl::OnAlsRead,
weak_ptr_factory_.GetWeakPtr()));
}
void AlsReaderImpl::OnAlsRead(const std::string& data) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
std::string trimmed_data;
base::TrimWhitespaceASCII(data, base::TRIM_ALL, &trimmed_data);
int value = 0;
......
......@@ -10,6 +10,7 @@
#include "base/memory/scoped_refptr.h"
#include "base/memory/weak_ptr.h"
#include "base/observer_list.h"
#include "base/sequence_checker.h"
#include "base/sequenced_task_runner.h"
#include "base/task_runner_util.h"
#include "base/timer/timer.h"
......@@ -22,6 +23,8 @@ namespace auto_screen_brightness {
// Real implementation of AlsReader.
// It periodically reads lux values from the ambient light sensor (ALS)
// if powerd has been configured to use it.
// An object of this class must be used on the same sequence that created this
// object.
class AlsReaderImpl : public AlsReader {
public:
// ALS file location may not be ready immediately, so we retry every
......@@ -45,9 +48,9 @@ class AlsReaderImpl : public AlsReader {
// reads ambient light file path.
void Init();
// Sets the task runner for testing purpose.
// Sets the |blocking_task_runner_| for testing purpose.
void SetTaskRunnerForTesting(
scoped_refptr<base::SequencedTaskRunner> task_runner);
scoped_refptr<base::SequencedTaskRunner> test_blocking_task_runner);
// Sets ambient light path for testing purpose and initialize. This will cause
// all the checks to be skipped, i.e. whether ALS is enabled and if config is
......@@ -89,8 +92,13 @@ class AlsReaderImpl : public AlsReader {
// Timer used to retry initialization and also for periodic ambient light
// sampling.
base::OneShotTimer als_timer_;
scoped_refptr<base::SequencedTaskRunner> als_task_runner_;
// Background task runner for checking ALS status and reading ALS values.
scoped_refptr<base::SequencedTaskRunner> blocking_task_runner_;
base::ObserverList<Observer> observers_;
SEQUENCE_CHECKER(sequence_checker_);
base::WeakPtrFactory<AlsReaderImpl> weak_ptr_factory_;
DISALLOW_COPY_AND_ASSIGN(AlsReaderImpl);
......
......@@ -20,11 +20,16 @@ constexpr base::TimeDelta BrightnessMonitorImpl::kBrightnessSampleDelay;
BrightnessMonitorImpl::BrightnessMonitorImpl(
chromeos::PowerManagerClient* const power_manager_client)
: BrightnessMonitorImpl(
power_manager_client,
base::CreateSequencedTaskRunnerWithTraits(
{base::TaskPriority::BEST_EFFORT, base::MayBlock(),
base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})) {}
: power_manager_client_observer_(this),
power_manager_client_(power_manager_client),
weak_ptr_factory_(this) {
DCHECK(power_manager_client);
power_manager_client_observer_.Add(power_manager_client);
power_manager_client_->WaitForServiceToBeAvailable(
base::BindOnce(&BrightnessMonitorImpl::OnPowerManagerServiceAvailable,
weak_ptr_factory_.GetWeakPtr()));
}
BrightnessMonitorImpl::~BrightnessMonitorImpl() = default;
......@@ -73,29 +78,6 @@ void BrightnessMonitorImpl::ScreenBrightnessChanged(
stable_brightness_percent_ = base::Optional<double>(change.percent());
}
std::unique_ptr<BrightnessMonitorImpl> BrightnessMonitorImpl::CreateForTesting(
chromeos::PowerManagerClient* const power_manager_client,
const scoped_refptr<base::SequencedTaskRunner> task_runner) {
return base::WrapUnique(
new BrightnessMonitorImpl(power_manager_client, task_runner));
}
BrightnessMonitorImpl::BrightnessMonitorImpl(
chromeos::PowerManagerClient* const power_manager_client,
const scoped_refptr<base::SequencedTaskRunner> task_runner)
: power_manager_client_observer_(this),
power_manager_client_(power_manager_client),
brightness_task_runner_(task_runner),
weak_ptr_factory_(this) {
DCHECK(power_manager_client);
power_manager_client_observer_.Add(power_manager_client);
brightness_sample_timer_.SetTaskRunner(brightness_task_runner_);
power_manager_client_->WaitForServiceToBeAvailable(
base::BindOnce(&BrightnessMonitorImpl::OnPowerManagerServiceAvailable,
weak_ptr_factory_.GetWeakPtr()));
}
void BrightnessMonitorImpl::OnPowerManagerServiceAvailable(
const bool service_is_ready) {
if (!service_is_ready) {
......
......@@ -47,14 +47,7 @@ class BrightnessMonitorImpl : public BrightnessMonitor,
void ScreenBrightnessChanged(
const power_manager::BacklightBrightnessChange& change) override;
static std::unique_ptr<BrightnessMonitorImpl> CreateForTesting(
chromeos::PowerManagerClient* power_manager_client,
scoped_refptr<base::SequencedTaskRunner> task_runner);
private:
BrightnessMonitorImpl(chromeos::PowerManagerClient* power_manager_client,
scoped_refptr<base::SequencedTaskRunner> task_runner);
void OnPowerManagerServiceAvailable(bool service_is_ready);
// Sets initial brightness obtained from powerd. If nullopt is received from
......@@ -83,8 +76,6 @@ class BrightnessMonitorImpl : public BrightnessMonitor,
power_manager_client_observer_;
chromeos::PowerManagerClient* const power_manager_client_;
scoped_refptr<base::SequencedTaskRunner> brightness_task_runner_;
// This timer is started when we receive the 1st user-requested brightness
// change and times out after kBrightnessSampleDelay if there are no more
// user-requested changes. The timer is reset if there is another
......
......@@ -89,11 +89,11 @@ class BrightnessMonitorImplTest : public testing::Test {
->SetScreenBrightness(request);
}
monitor_ = BrightnessMonitorImpl::CreateForTesting(
chromeos::DBusThreadManager::Get()->GetPowerManagerClient(),
base::SequencedTaskRunnerHandle::Get());
monitor_ = std::make_unique<BrightnessMonitorImpl>(
chromeos::DBusThreadManager::Get()->GetPowerManagerClient());
test_observer_ = std::make_unique<TestObserver>();
monitor_->AddObserver(test_observer_.get());
scoped_task_environment_.RunUntilIdle();
}
protected:
......@@ -106,6 +106,7 @@ class BrightnessMonitorImplTest : public testing::Test {
static_cast<chromeos::FakePowerManagerClient*>(
chromeos::DBusThreadManager::Get()->GetPowerManagerClient())
->SendScreenBrightnessChanged(change);
scoped_task_environment_.RunUntilIdle();
}
base::test::ScopedTaskEnvironment scoped_task_environment_;
......@@ -121,7 +122,6 @@ class BrightnessMonitorImplTest : public testing::Test {
TEST_F(BrightnessMonitorImplTest, PowerManagerClientBrightnessUnset) {
// Do not set initial brightess in FakePowerManagerClient.
SetUpBrightnessMonitor(-1);
scoped_task_environment_.FastForwardUntilNoTasksRemain();
EXPECT_EQ(BrightnessMonitor::Status::kDisabled, test_observer_->status());
// User request will be ignored.
......@@ -137,7 +137,6 @@ TEST_F(BrightnessMonitorImplTest, PowerManagerClientBrightnessUnset) {
// kBrightnessSampleDelay, hence final brightness is recorded.
TEST_F(BrightnessMonitorImplTest, TwoUserAdjustmentsShortGap) {
SetUpBrightnessMonitor(10);
scoped_task_environment_.FastForwardUntilNoTasksRemain();
EXPECT_EQ(BrightnessMonitor::Status::kSuccess, test_observer_->status());
EXPECT_EQ(0, test_observer_->num_brightness_changes());
EXPECT_EQ(0, test_observer_->num_user_brightness_change_requested());
......@@ -169,7 +168,6 @@ TEST_F(BrightnessMonitorImplTest, TwoUserAdjustmentsShortGap) {
// kBrightnessSampleDelay, hence two brightness changes are recorded.
TEST_F(BrightnessMonitorImplTest, TwoUserAdjustmentsLongGap) {
SetUpBrightnessMonitor(10);
scoped_task_environment_.FastForwardUntilNoTasksRemain();
EXPECT_EQ(BrightnessMonitor::Status::kSuccess, test_observer_->status());
EXPECT_EQ(0, test_observer_->num_brightness_changes());
EXPECT_EQ(0, test_observer_->num_user_brightness_change_requested());
......@@ -204,7 +202,6 @@ TEST_F(BrightnessMonitorImplTest, TwoUserAdjustmentsLongGap) {
// change. The gap between the two is shorter than |kBrightnessSampleDelay|.
TEST_F(BrightnessMonitorImplTest, NonUserFollowedByUserShortGap) {
SetUpBrightnessMonitor(10);
scoped_task_environment_.FastForwardUntilNoTasksRemain();
// Non-user.
ReportBrightnessChangeEvent(
......@@ -230,7 +227,6 @@ TEST_F(BrightnessMonitorImplTest, NonUserFollowedByUserShortGap) {
// change. The gap between the two is longer than |kBrightnessSampleDelay|.
TEST_F(BrightnessMonitorImplTest, NonUserFollowedByUserLongGap) {
SetUpBrightnessMonitor(10);
scoped_task_environment_.FastForwardUntilNoTasksRemain();
ReportBrightnessChangeEvent(
20, power_manager::BacklightBrightnessChange_Cause_USER_ACTIVITY);
......@@ -255,7 +251,6 @@ TEST_F(BrightnessMonitorImplTest, NonUserFollowedByUserLongGap) {
// change.
TEST_F(BrightnessMonitorImplTest, UserAdjustmentsSeparatedByNonUser) {
SetUpBrightnessMonitor(10);
scoped_task_environment_.FastForwardUntilNoTasksRemain();
// User request.
ReportBrightnessChangeEvent(
......
......@@ -13,7 +13,9 @@
#include "base/memory/weak_ptr.h"
#include "base/optional.h"
#include "base/scoped_observer.h"
#include "base/sequence_checker.h"
#include "base/sequenced_task_runner.h"
#include "base/threading/thread_checker.h"
#include "base/time/time.h"
#include "chrome/browser/chromeos/power/auto_screen_brightness/als_reader.h"
#include "chrome/browser/chromeos/power/auto_screen_brightness/brightness_monitor.h"
......@@ -32,6 +34,8 @@ namespace auto_screen_brightness {
// It monitors user-requested brightness changes, ambient light values and
// trains personal brightness curves when user remains idle for a period of
// time.
// An object of this class must be used on the same thread that created this
// object.
class ModellerImpl : public Modeller,
public AlsReader::Observer,
public BrightnessMonitor::Observer,
......@@ -64,7 +68,7 @@ class ModellerImpl : public Modeller,
static constexpr char kCurveFileName[] = "curve";
// ModellerImpl has weak dependencies on all parameters except |trainer|.
ModellerImpl(Profile* profile,
ModellerImpl(const Profile* profile,
AlsReader* als_reader,
BrightnessMonitor* brightness_monitor,
ui::UserActivityDetector* user_activity_detector,
......@@ -90,12 +94,12 @@ class ModellerImpl : public Modeller,
// ModellerImpl has weak dependencies on all parameters except |trainer|.
static std::unique_ptr<ModellerImpl> CreateForTesting(
Profile* profile,
const Profile* profile,
AlsReader* als_reader,
BrightnessMonitor* brightness_monitor,
ui::UserActivityDetector* user_activity_detector,
std::unique_ptr<Trainer> trainer,
scoped_refptr<base::SequencedTaskRunner> task_runner,
scoped_refptr<base::SequencedTaskRunner> blocking_task_runner,
const base::TickClock* tick_clock);
// Current average ambient light.
......@@ -105,14 +109,16 @@ class ModellerImpl : public Modeller,
// training.
size_t NumberTrainingDataPointsForTesting() const;
// Calls GetCurvePathFromProfile directly.
base::FilePath GetCurvePathForTesting(Profile* profile) const;
MonotoneCubicSpline GetGlobalCurveForTesting() const;
// Returns the path that will be used to store curves. It also creates
// intermediate directories if they do not exist. Returns an empty path on
// failures.
static base::FilePath GetCurvePathFromProfile(const Profile* profile);
private:
// ModellerImpl has weak dependencies on all parameters except |trainer|.
ModellerImpl(Profile* profile,
ModellerImpl(const Profile* profile,
AlsReader* als_reader,
BrightnessMonitor* brightness_monitor,
ui::UserActivityDetector* user_activity_detector,
......@@ -121,11 +127,6 @@ class ModellerImpl : public Modeller,
const base::TickClock* tick_clock,
bool is_testing = false);
// Returns the path that will be used to store curves. It also creates
// intermediate directories if they do not exist. Returns an empty path on
// failures.
base::FilePath GetCurvePathFromProfile(Profile* profile) const;
// Updates |model_status_| by checking |als_init_status_| and
// |brightness_monitor_status_| and optionally loads a curve.
// 1. |model_status_| is |kDisabled| if either |als_init_status_| is not
......@@ -156,6 +157,13 @@ class ModellerImpl : public Modeller,
// |global_curve_|.
void OnCurveLoadedFromDisk(const base::Optional<MonotoneCubicSpline>& curve);
void OnCurveSavedToDisk(bool is_successful);
// Called after we've set trainer's initial curves.
void OnSetInitialCurves(
const base::Optional<MonotoneCubicSpline>& loaded_curve,
bool is_personal_curve_valid);
// Starts |model_timer_| to start training after certain inactivity period.
void ScheduleTrainerStart();
......@@ -178,7 +186,9 @@ class ModellerImpl : public Modeller,
ScopedObserver<ui::UserActivityDetector, ui::UserActivityObserver>
user_activity_observer_;
scoped_refptr<base::SequencedTaskRunner> model_task_runner_;
// Background task runner for IO work (loading a curve from disk and writing a
// curve to disk) and training jobs.
scoped_refptr<base::SequencedTaskRunner> blocking_task_runner_;
std::unique_ptr<Trainer, base::OnTaskRunnerDeleter> trainer_;
// This will be replaced by a mock tick clock during tests.
......@@ -212,6 +222,8 @@ class ModellerImpl : public Modeller,
base::ObserverList<Modeller::Observer> observers_;
SEQUENCE_CHECKER(sequence_checker_);
base::WeakPtrFactory<ModellerImpl> weak_ptr_factory_;
DISALLOW_COPY_AND_ASSIGN(ModellerImpl);
......
......@@ -206,7 +206,7 @@ class ModellerImplTest : public testing::Test {
protected:
void WriteCurveToFile(const MonotoneCubicSpline& curve) {
const base::FilePath curve_path =
modeller_->GetCurvePathForTesting(profile_.get());
ModellerImpl::ModellerImpl::GetCurvePathFromProfile(profile_.get());
CHECK(!curve_path.empty());
const std::string data = curve.ToString();
......
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