power: Dejankify backlight brightness animations.
Make powerd not restart the backlight animation timer when
the current transition is interrupted by a new request, as
happens when holding down a brightness key.
Also simplify some old code in the InternalBacklight class
and reduce the brightness update frequency from 30 ms to 20
ms (it was effectively 50 ms before this change when holding
a brightness key, but 30 ms is still slightly flickery on
lumpy).
BUG=chromium:358938
TEST=manual: panel backlight animations are no longer
flickery on lumpy, and panel and keyboard animations
both look fine on pixel; also added tests
Change-Id: I4e18541dadbeb9fc8abf241f2833bf32d5bc6597
Reviewed-on: https://chromium-review.googlesource.com/196261
Tested-by: Daniel Erat <[email protected]>
Reviewed-by: Chris Masone <[email protected]>
Commit-Queue: Daniel Erat <[email protected]>
diff --git a/powerd/system/internal_backlight.cc b/powerd/system/internal_backlight.cc
index d458047..fdefb33 100644
--- a/powerd/system/internal_backlight.cc
+++ b/powerd/system/internal_backlight.cc
@@ -22,9 +22,40 @@
namespace {
+// Reads the value from |path| to |level|. Returns false on failure.
+bool ReadBrightnessLevelFromFile(const base::FilePath& path, int64* level) {
+ DCHECK(level);
+
+ std::string level_str;
+ if (!base::ReadFileToString(path, &level_str)) {
+ LOG(ERROR) << "Unable to read brightness from " << path.value();
+ return false;
+ }
+
+ TrimWhitespaceASCII(level_str, TRIM_TRAILING, &level_str);
+ if (!base::StringToInt64(level_str, level)) {
+ LOG(ERROR) << "Unable to parse brightness \"" << level_str << "\" from "
+ << path.value();
+ return false;
+ }
+
+ return true;
+}
+
+// Writes |level| to |path|. Returns false on failure.
+bool WriteBrightnessLevelToFile(const base::FilePath& path, int64 level) {
+ std::string buf = base::Int64ToString(level);
+ if (file_util::WriteFile(path, buf.data(), buf.size()) == -1) {
+ LOG(ERROR) << "Unable to write brightness \"" << buf << "\" to "
+ << path.value();
+ return false;
+ }
+ return true;
+}
+
// When animating a brightness level transition, amount of time in milliseconds
// to wait between each update.
-const int kTransitionIntervalMs = 30;
+const int kTransitionIntervalMs = 20;
} // namespace
@@ -45,34 +76,50 @@
bool InternalBacklight::Init(const base::FilePath& base_path,
const base::FilePath::StringType& pattern) {
- base::FilePath dir_path;
- base::FileEnumerator dir_enumerator(
+ base::FileEnumerator enumerator(
base_path, false, base::FileEnumerator::DIRECTORIES, pattern);
// Find the backlight interface with greatest granularity (highest max).
- for (base::FilePath check_path = dir_enumerator.Next(); !check_path.empty();
- check_path = dir_enumerator.Next()) {
- base::FilePath check_name = check_path.BaseName();
- std::string str_check_name = check_name.value();
- if (str_check_name[0] == '.')
+ for (base::FilePath device_path = enumerator.Next(); !device_path.empty();
+ device_path = enumerator.Next()) {
+ if (device_path.BaseName().value()[0] == '.')
continue;
- int64 max = CheckBacklightFiles(check_path);
- if (max <= max_brightness_level_)
+ const base::FilePath max_brightness_path =
+ device_path.Append(InternalBacklight::kMaxBrightnessFilename);
+ if (!base::PathExists(max_brightness_path)) {
+ LOG(WARNING) << "Can't find " << max_brightness_path.value();
+ continue;
+ }
+
+ const base::FilePath brightness_path =
+ device_path.Append(InternalBacklight::kBrightnessFilename);
+ if (access(brightness_path.value().c_str(), R_OK | W_OK) != 0) {
+ LOG(WARNING) << "Can't write to " << brightness_path.value();
+ continue;
+ }
+
+ int64 max_level = 0;
+ if (!ReadBrightnessLevelFromFile(max_brightness_path, &max_level))
continue;
- max_brightness_level_ = max;
- GetBacklightFilePaths(check_path,
- &actual_brightness_path_,
- &brightness_path_,
- &max_brightness_path_,
- &resume_brightness_path_);
+ if (max_level <= max_brightness_level_)
+ continue;
+
+ brightness_path_ = brightness_path;
+ max_brightness_path_ = max_brightness_path;
+ max_brightness_level_ = max_level;
// Technically all screen backlights should implement actual_brightness,
- // but we'll handle ones that don't. This allows us to work with keyboard
+ // but we'll handle ones that don't. This allows us to work with keyboard
// backlights too.
+ actual_brightness_path_ =
+ device_path.Append(InternalBacklight::kActualBrightnessFilename);
if (!base::PathExists(actual_brightness_path_))
actual_brightness_path_ = brightness_path_;
+
+ resume_brightness_path_ =
+ device_path.Append(InternalBacklight::kResumeBrightnessFilename);
}
if (max_brightness_level_ <= 0) {
@@ -99,15 +146,6 @@
return current_brightness_level_;
}
-bool InternalBacklight::SetResumeBrightnessLevel(int64 level) {
- if (resume_brightness_path_.empty()) {
- LOG(ERROR) << "Cannot find backlight resume brightness file.";
- return false;
- }
-
- return WriteBrightnessLevelToFile(resume_brightness_path_, level);
-}
-
bool InternalBacklight::SetBrightnessLevel(int64 level,
base::TimeDelta interval) {
if (brightness_path_.empty()) {
@@ -115,12 +153,13 @@
return false;
}
- CancelTransition();
-
- if (level == current_brightness_level_)
+ if (level == current_brightness_level_) {
+ CancelTransition();
return true;
+ }
if (interval.InMilliseconds() <= kTransitionIntervalMs) {
+ CancelTransition();
if (!WriteBrightnessLevelToFile(brightness_path_, level))
return false;
current_brightness_level_ = level;
@@ -131,84 +170,22 @@
transition_end_time_ = transition_start_time_ + interval;
transition_start_level_ = current_brightness_level_;
transition_end_level_ = level;
- transition_timer_.Start(FROM_HERE,
- base::TimeDelta::FromMilliseconds(kTransitionIntervalMs), this,
- &InternalBacklight::HandleTransitionTimeout);
+ if (!transition_timer_.IsRunning()) {
+ transition_timer_.Start(FROM_HERE,
+ base::TimeDelta::FromMilliseconds(kTransitionIntervalMs), this,
+ &InternalBacklight::HandleTransitionTimeout);
+ transition_timer_start_time_ = transition_start_time_;
+ }
return true;
}
-// static
-void InternalBacklight::GetBacklightFilePaths(
- const base::FilePath& dir_path,
- base::FilePath* actual_brightness_path,
- base::FilePath* brightness_path,
- base::FilePath* max_brightness_path,
- base::FilePath* resume_brightness_path) {
- if (actual_brightness_path)
- *actual_brightness_path = dir_path.Append(kActualBrightnessFilename);
- if (brightness_path)
- *brightness_path = dir_path.Append(kBrightnessFilename);
- if (max_brightness_path)
- *max_brightness_path = dir_path.Append(kMaxBrightnessFilename);
- if (resume_brightness_path)
- *resume_brightness_path = dir_path.Append(kResumeBrightnessFilename);
-}
-
-// static
-int64 InternalBacklight::CheckBacklightFiles(const base::FilePath& dir_path) {
- base::FilePath actual_brightness_path, brightness_path, max_brightness_path,
- resume_brightness_path;
- GetBacklightFilePaths(dir_path,
- &actual_brightness_path,
- &brightness_path,
- &max_brightness_path,
- &resume_brightness_path);
-
- if (!base::PathExists(max_brightness_path)) {
- LOG(WARNING) << "Can't find " << max_brightness_path.value();
- return 0;
- } else if (access(brightness_path.value().c_str(), R_OK | W_OK)) {
- LOG(WARNING) << "Can't write to " << brightness_path.value();
- return 0;
- }
-
- int64 max_level = 0;
- if (!ReadBrightnessLevelFromFile(max_brightness_path, &max_level))
- return 0;
- return max_level;
-}
-
-// static
-bool InternalBacklight::ReadBrightnessLevelFromFile(const base::FilePath& path,
- int64* level) {
- DCHECK(level);
-
- std::string level_str;
- if (!base::ReadFileToString(path, &level_str)) {
- LOG(ERROR) << "Unable to read brightness from " << path.value();
+bool InternalBacklight::SetResumeBrightnessLevel(int64 level) {
+ if (resume_brightness_path_.empty()) {
+ LOG(ERROR) << "Cannot find backlight resume brightness file.";
return false;
}
- TrimWhitespaceASCII(level_str, TRIM_TRAILING, &level_str);
- if (!base::StringToInt64(level_str, level)) {
- LOG(ERROR) << "Unable to parse brightness \"" << level_str << "\" from "
- << path.value();
- return false;
- }
-
- return true;
-}
-
-// static
-bool InternalBacklight::WriteBrightnessLevelToFile(const base::FilePath& path,
- int64 level) {
- std::string buf = base::Int64ToString(level);
- if (file_util::WriteFile(path, buf.data(), buf.size()) == -1) {
- LOG(ERROR) << "Unable to write brightness \"" << buf << "\" to "
- << path.value();
- return false;
- }
- return true;
+ return WriteBrightnessLevelToFile(resume_brightness_path_, level);
}
void InternalBacklight::HandleTransitionTimeout() {
diff --git a/powerd/system/internal_backlight.h b/powerd/system/internal_backlight.h
index 451bacd..37b05b6 100644
--- a/powerd/system/internal_backlight.h
+++ b/powerd/system/internal_backlight.h
@@ -19,7 +19,7 @@
namespace system {
-// Controls an LCD or keyboard backlight via sysfs.
+// Controls a panel or keyboard backlight via sysfs.
class InternalBacklight : public BacklightInterface {
public:
// Base names of backlight files within sysfs directories.
@@ -46,6 +46,9 @@
bool transition_timer_is_running() const {
return transition_timer_.IsRunning();
}
+ base::TimeTicks transition_timer_start_time() const {
+ return transition_timer_start_time_;
+ }
Clock* clock() { return clock_.get(); }
// Calls HandleTransitionTimeout() as if |transition_timer_| had fired
@@ -61,26 +64,6 @@
virtual bool SetResumeBrightnessLevel(int64 level) OVERRIDE;
private:
- // Generates FilePaths within |dir_path| for reading and writing
- // brightness information.
- static void GetBacklightFilePaths(const base::FilePath& dir_path,
- base::FilePath* actual_brightness_path,
- base::FilePath* brightness_path,
- base::FilePath* max_brightness_path,
- base::FilePath* resume_brightness_path);
-
- // Looks for the existence of required files and returns the max brightness.
- // Returns 0 if necessary files are missing.
- static int64 CheckBacklightFiles(const base::FilePath& dir_path);
-
- // Reads the value from |path| to |level|. Returns false on failure.
- static bool ReadBrightnessLevelFromFile(const base::FilePath& path,
- int64* level);
-
- // Writes |level| to |path|. Returns false on failure.
- static bool WriteBrightnessLevelToFile(const base::FilePath& path,
- int64 level);
-
// Sets the brightness level appropriately for the current point in the
// transition. When the transition is done, stops |transition_timer_|.
void HandleTransitionTimeout();
@@ -104,6 +87,9 @@
// Calls HandleTransitionTimeout().
base::RepeatingTimer<InternalBacklight> transition_timer_;
+ // Time at which |transition_timer_| was last started. Used for testing.
+ base::TimeTicks transition_timer_start_time_;
+
// Times at which the current transition started and is scheduled to end.
base::TimeTicks transition_start_time_;
base::TimeTicks transition_end_time_;
diff --git a/powerd/system/internal_backlight_unittest.cc b/powerd/system/internal_backlight_unittest.cc
index aa9a04e..b326e5f 100644
--- a/powerd/system/internal_backlight_unittest.cc
+++ b/powerd/system/internal_backlight_unittest.cc
@@ -80,7 +80,7 @@
base::FilePath test_path_;
};
-// A basic test of functionality
+// A basic test of functionality.
TEST_F(InternalBacklightTest, BasicTest) {
base::FilePath this_test_path = test_path_.Append("basic_test");
const int64 kBrightness = 128;
@@ -112,18 +112,14 @@
EXPECT_EQ(kMaxBrightness, backlight.GetMaxBrightnessLevel());
}
-// Test that we pick the one with the greatest granularity
+// Test that we pick the device with the greatest granularity.
TEST_F(InternalBacklightTest, GranularityTest) {
- base::FilePath this_test_path = test_path_.Append("granularity_test");
-
- // Make sure the middle one is the most granular so we're not just
- // getting lucky. Middle in terms of order created and alphabet, since I
- // don't know how enumaration might be happening.
- base::FilePath a_path = this_test_path.Append("a");
+ const base::FilePath this_test_path = test_path_.Append("granularity_test");
+ const base::FilePath a_path = this_test_path.Append("a");
PopulateBacklightDir(a_path, 10, 127, 11);
- base::FilePath b_path = this_test_path.Append("b");
+ const base::FilePath b_path = this_test_path.Append("b");
PopulateBacklightDir(b_path, 20, 255, 21);
- base::FilePath c_path = this_test_path.Append("c");
+ const base::FilePath c_path = this_test_path.Append("c");
PopulateBacklightDir(c_path, 30, 63, 31);
InternalBacklight backlight;
@@ -132,12 +128,12 @@
EXPECT_EQ(255, backlight.GetMaxBrightnessLevel());
}
-// Test ignore directories starting with a "."
+// Test that directories starting with a "." are ignored.
TEST_F(InternalBacklightTest, NoDotDirsTest) {
base::FilePath this_test_path = test_path_.Append("no_dot_dirs_test");
// We'll just create one dir and it will have a dot in it. Then, we can
- // be sure that we didn't just get luckly...
+ // be sure that we didn't just get lucky...
base::FilePath my_path = this_test_path.Append(".pwm-backlight");
PopulateBacklightDir(my_path, 128, 255, 127);
@@ -192,6 +188,8 @@
// If the timeout fires at this point, we should still be at the maximum
// level.
EXPECT_TRUE(backlight.transition_timer_is_running());
+ EXPECT_EQ(kStartTime.ToInternalValue(),
+ backlight.transition_timer_start_time().ToInternalValue());
EXPECT_TRUE(backlight.TriggerTransitionTimeoutForTesting());
EXPECT_EQ(kMaxBrightness, ReadBrightness(backlight_dir));
EXPECT_EQ(kMaxBrightness, backlight.GetCurrentBrightnessLevel());
@@ -213,5 +211,66 @@
EXPECT_EQ(kHalfBrightness, backlight.GetCurrentBrightnessLevel());
}
+TEST_F(InternalBacklightTest, InterruptTransition) {
+ // Make the backlight start at its max level.
+ const int kMaxBrightness = 100;
+ base::FilePath backlight_dir = test_path_.Append("backlight");
+ PopulateBacklightDir(
+ backlight_dir, kMaxBrightness, kMaxBrightness, kMaxBrightness);
+ InternalBacklight backlight;
+ backlight.clock()->set_current_time_for_testing(
+ base::TimeTicks::FromInternalValue(10000));
+ ASSERT_TRUE(backlight.Init(test_path_, "*"));
+
+ // Start a one-second transition to 0.
+ const base::TimeDelta kDuration = base::TimeDelta::FromSeconds(1);
+ backlight.SetBrightnessLevel(0, kDuration);
+
+ // Let the timer fire after half a second. The backlight should be at half
+ // brightness.
+ backlight.clock()->set_current_time_for_testing(
+ backlight.clock()->GetCurrentTime() + kDuration / 2);
+ EXPECT_TRUE(backlight.TriggerTransitionTimeoutForTesting());
+ const int kHalfBrightness = kMaxBrightness / 2;
+ EXPECT_EQ(kHalfBrightness, ReadBrightness(backlight_dir));
+
+ // If a request to use half brightness arrives at this point, the timer should
+ // be stopped.
+ backlight.SetBrightnessLevel(kHalfBrightness, kDuration);
+ EXPECT_FALSE(backlight.transition_timer_is_running());
+ EXPECT_EQ(kHalfBrightness, ReadBrightness(backlight_dir));
+
+ // Set the brightness to 0 immediately and start a one-second transition to
+ // the maximum level.
+ backlight.SetBrightnessLevel(0, base::TimeDelta());
+ const base::TimeTicks kInterruptedTransitionStartTime =
+ backlight.clock()->GetCurrentTime();
+ backlight.SetBrightnessLevel(kMaxBrightness, kDuration);
+ EXPECT_TRUE(backlight.transition_timer_is_running());
+ EXPECT_EQ(0, ReadBrightness(backlight_dir));
+
+ // At the halfway point, interrupt the transition with a new request to go to
+ // 75% of the max over a second.
+ backlight.clock()->set_current_time_for_testing(
+ backlight.clock()->GetCurrentTime() + kDuration / 2);
+ EXPECT_TRUE(backlight.TriggerTransitionTimeoutForTesting());
+ EXPECT_EQ(kHalfBrightness, ReadBrightness(backlight_dir));
+ const int kThreeQuartersBrightness =
+ kHalfBrightness + (kMaxBrightness - kHalfBrightness) / 2;
+ backlight.SetBrightnessLevel(kThreeQuartersBrightness, kDuration);
+ EXPECT_EQ(kHalfBrightness, ReadBrightness(backlight_dir));
+
+ // Since the timer was already running, it shouldn't be restarted.
+ EXPECT_EQ(kInterruptedTransitionStartTime.ToInternalValue(),
+ backlight.transition_timer_start_time().ToInternalValue());
+ EXPECT_TRUE(backlight.transition_timer_is_running());
+
+ // After a second, the backlight should be at 75% and the timer should stop.
+ backlight.clock()->set_current_time_for_testing(
+ backlight.clock()->GetCurrentTime() + kDuration);
+ EXPECT_FALSE(backlight.TriggerTransitionTimeoutForTesting());
+ EXPECT_EQ(kThreeQuartersBrightness, ReadBrightness(backlight_dir));
+}
+
} // namespace system
} // namespace power_manager
