power: Make min_visible_backlight_level pref optional.
Make powerd's min_visible_backlight_level preference
optional instead of defaulting to 1. If it's set, it's used
unconditionally (after clamping to [1, max]). Otherwise, a
computed default (0.0065 * max) is used.
Previously, the max of the pref and the computed default
were used, meaning that the pref could be used to increase
the minimum level beyond the computed default but not to
decrease it. I think that powerd may not have supported
optional prefs initially.
BUG=chrome-os-partner:36196
TEST=added a test
Change-Id: Icd26fddc4ee2260b24df14f1e9d1ade76291f681
Reviewed-on: https://chromium-review.googlesource.com/245880
Commit-Queue: Dan Erat <derat@chromium.org>
Trybot-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Sameer Nanda <snanda@chromium.org>
diff --git a/power_manager/common/power_constants.h b/power_manager/common/power_constants.h
index 3b619ae..3a61084 100644
--- a/power_manager/common/power_constants.h
+++ b/power_manager/common/power_constants.h
@@ -46,7 +46,8 @@
extern const char kRetrySuspendAttemptsPref[];
// Minimum brightness level (in hardware-specific units) that the backlight
-// should be remain at before it's turned off entirely.
+// should be remain at before it's turned off entirely. If unset, a default
+// based on the maximum brightness level is used.
extern const char kMinVisibleBacklightLevelPref[];
// If true, powerd will jump directly from the min-visible-level to 0 rather
diff --git a/power_manager/default_prefs/min_visible_backlight_level b/power_manager/default_prefs/min_visible_backlight_level
deleted file mode 100644
index d00491f..0000000
--- a/power_manager/default_prefs/min_visible_backlight_level
+++ /dev/null
@@ -1 +0,0 @@
-1
diff --git a/power_manager/powerd/policy/internal_backlight_controller.cc b/power_manager/powerd/policy/internal_backlight_controller.cc
index 41005ab..a0390bc 100644
--- a/power_manager/powerd/policy/internal_backlight_controller.cc
+++ b/power_manager/powerd/policy/internal_backlight_controller.cc
@@ -38,15 +38,6 @@
// kDefaultLevelToPercentExponent is modified.
const double kDimmedBrightnessFraction = 0.1;
-// Minimum brightness, as a fraction of the maximum level in the range [0.0,
-// 1.0], that we'll remain at before turning the backlight off entirely. This
-// is arbitrarily chosen but seems to be a reasonable marginally-visible
-// brightness for a darkened room on current devices: http://crosbug.com/24569.
-// A higher level can be set via the kMinVisibleBacklightLevelPref setting.
-// This is a fraction rather than a percent so it won't change if
-// kDefaultLevelToPercentExponent is modified.
-const double kDefaultMinVisibleBrightnessFraction = 0.0065;
-
// Value for |level_to_percent_exponent_|, assuming that at least
// |kMinLevelsForNonLinearScale| brightness levels are available -- if not, we
// just use 1.0 to give us a linear scale.
@@ -137,6 +128,8 @@
const int64_t InternalBacklightController::kMaxBrightnessSteps = 16;
const double InternalBacklightController::kMinVisiblePercent =
kMaxPercent / kMaxBrightnessSteps;
+const double InternalBacklightController::kDefaultMinVisibleBrightnessFraction =
+ 0.0065;
const int InternalBacklightController::kAmbientLightSensorTimeoutSec = 10;
InternalBacklightController::InternalBacklightController()
@@ -186,14 +179,12 @@
max_level_ = backlight_->GetMaxBrightnessLevel();
current_level_ = backlight_->GetCurrentBrightnessLevel();
- if (!prefs_->GetInt64(kMinVisibleBacklightLevelPref, &min_visible_level_))
- min_visible_level_ = 1;
- min_visible_level_ = std::max(
- static_cast<int64_t>(
- lround(kDefaultMinVisibleBrightnessFraction * max_level_)),
- min_visible_level_);
- CHECK_GT(min_visible_level_, 0);
- min_visible_level_ = std::min(min_visible_level_, max_level_);
+ if (!prefs_->GetInt64(kMinVisibleBacklightLevelPref, &min_visible_level_)) {
+ min_visible_level_ = static_cast<int64_t>(
+ lround(kDefaultMinVisibleBrightnessFraction * max_level_));
+ }
+ min_visible_level_ = std::min(
+ std::max(min_visible_level_, static_cast<int64_t>(1)), max_level_);
const double initial_percent = LevelToPercent(current_level_);
ambient_light_brightness_percent_ = initial_percent;
diff --git a/power_manager/powerd/policy/internal_backlight_controller.h b/power_manager/powerd/policy/internal_backlight_controller.h
index e41446c..aff1e45 100644
--- a/power_manager/powerd/policy/internal_backlight_controller.h
+++ b/power_manager/powerd/policy/internal_backlight_controller.h
@@ -46,6 +46,16 @@
// lowest brightness step before the screen is turned off.
static const double kMinVisiblePercent;
+ // Minimum brightness, as a fraction of the maximum level in the range [0.0,
+ // 1.0], that is used as the bottom step before turning the backlight off
+ // entirely. This is arbitrarily chosen but seems to be a reasonable
+ // marginally-visible brightness for a darkened room on current devices:
+ // http://crosbug.com/24569. A custom level can be set via the
+ // kMinVisibleBacklightLevelPref setting. This is a fraction of the
+ // driver-supplied maximum level rather than a percent so it won't change if
+ // kDefaultLevelToPercentExponent is modified.
+ static const double kDefaultMinVisibleBrightnessFraction;
+
// If an ambient light reading hasn't been seen after this many seconds,
// give up on waiting for the sensor to be initialized and just set
// |use_ambient_light_| to false.
diff --git a/power_manager/powerd/policy/internal_backlight_controller_unittest.cc b/power_manager/powerd/policy/internal_backlight_controller_unittest.cc
index 894574d..3be558a 100644
--- a/power_manager/powerd/policy/internal_backlight_controller_unittest.cc
+++ b/power_manager/powerd/policy/internal_backlight_controller_unittest.cc
@@ -40,7 +40,7 @@
initial_als_lux_(0),
report_initial_als_reading_(true),
report_initial_power_source_(true),
- default_min_visible_level_(1),
+ default_min_visible_level_(-1),
default_als_steps_("50.0 -1 400\n90.0 100 -1"),
default_no_als_ac_brightness_("80.0"),
default_no_als_battery_brightness_("60.0"),
@@ -52,7 +52,10 @@
// event such that it should make its first adjustment to the backlight
// brightness.
virtual void Init(PowerSource power_source) {
- prefs_.SetInt64(kMinVisibleBacklightLevelPref, default_min_visible_level_);
+ if (default_min_visible_level_ > 0) {
+ prefs_.SetInt64(kMinVisibleBacklightLevelPref,
+ default_min_visible_level_);
+ }
prefs_.SetString(kInternalBacklightAlsStepsPref, default_als_steps_);
prefs_.SetString(kInternalBacklightNoAlsAcBrightnessPref,
default_no_als_ac_brightness_);
@@ -101,8 +104,8 @@
bool report_initial_als_reading_;
bool report_initial_power_source_;
- // Default values for prefs. Applied when Init() is called.
- int64_t default_min_visible_level_;
+ // Default values for prefs. Applied when Init() is called.
+ int64_t default_min_visible_level_; // Unset if non-positive.
std::string default_als_steps_;
std::string default_no_als_ac_brightness_;
std::string default_no_als_battery_brightness_;
@@ -118,7 +121,7 @@
default_min_visible_level_ = 100;
default_als_steps_ = "50.0 -1 -1";
Init(POWER_BATTERY);
- EXPECT_EQ(default_min_visible_level_,
+ ASSERT_EQ(default_min_visible_level_,
PercentToLevel(InternalBacklightController::kMinVisiblePercent));
const int64_t kAlsLevel = PercentToLevel(50.0);
EXPECT_EQ(kAlsLevel, backlight_.current_level());
@@ -866,5 +869,23 @@
display_power_setter_.state());
}
+TEST_F(InternalBacklightControllerTest, MinVisibleLevelPrefUndercutsDefault) {
+ // Set the min-visible-level pref below the computed default.
+ int64_t computed_min = static_cast<int64_t>(
+ lround(InternalBacklightController::kDefaultMinVisibleBrightnessFraction *
+ max_backlight_level_));
+ default_min_visible_level_ = static_cast<int64_t>(computed_min * 0.5);
+ ASSERT_GT(default_min_visible_level_, 0);
+ ASSERT_LT(default_min_visible_level_, computed_min);
+
+ // Set the brightness to 0%, increase it one step, and confirm that the level
+ // configured by the pref is used instead of the higher computed default.
+ Init(POWER_AC);
+ controller_->SetUserBrightnessPercent(
+ 0.0, BacklightController::TRANSITION_INSTANT);
+ controller_->IncreaseUserBrightness();
+ EXPECT_EQ(default_min_visible_level_, backlight_.current_level());
+}
+
} // namespace policy
} // namespace power_manager
