Battery saving sleep heuristic algorithm is not good enough

[juj]

Stumbled upon this thread https://forum.freeplaytech.com/showthread.php?tid=4988 where a number of users are discussing the observation that the "display is idle" heuristic is producing unwanted visual quality, e.g.

It's jarring is all. Kinda makes my fancy $300 hobby device look "broken" if that makes any sense.

The root reason why a "display is idle" power saving throttling heuristic has been added is due to issue raspberrypi/userland#440 .

Marking this issue down as a TODO to see if the sleep heuristic can be tweaked/improved.

The throttling heuristic can be disabled via

diff --git a/config.h b/config.h
index 75b802b..fd343b2 100644
--- a/config.h
+++ b/config.h
@@ -146,7 +146,7 @@
 
 // Detects when the activity on the screen is mostly idle, and goes to low power mode, in which new
 // frames will be polled first at 10fps, and ultimately at only 2fps.
-#define SAVE_BATTERY_BY_SLEEPING_WHEN_IDLE
+// #define SAVE_BATTERY_BY_SLEEPING_WHEN_IDLE
 
 // Builds a histogram of observed frame intervals and uses that to sync to a known update rate. This aims
 // to detect if an application uses a non-60Hz update rate, and synchronizes to that instead.

One way to tweak the heuristic is to change the timings:

diff --git a/gpu.cpp b/gpu.cpp
index 61b4d42..b104575 100644
--- a/gpu.cpp
+++ b/gpu.cpp
@@ -281,8 +281,8 @@ uint64_t EstimateFrameRateInterval()
   uint64_t timeNow = tick();
 #ifdef SAVE_BATTERY_BY_SLEEPING_WHEN_IDLE
   // "Deep sleep" options: is user leaves the device with static content on screen for a long time.
-  if (timeNow - mostRecentFrame > 60000000) { histogramSize = 1; return 500000; } // if it's been more than one minute since last seen update, assume interval of 500ms.
-  if (timeNow - mostRecentFrame > 5000000) return lastFramePollTime + 100000; // if it's been more than 5 seconds since last seen update, assume interval of 100ms.
+  if (timeNow - mostRecentFrame > 60000000) { histogramSize = 1; return 100000; } // if it's been more than one minute since last seen update, assume interval of 500ms.
+  if (timeNow - mostRecentFrame > 5000000) return lastFramePollTime + 50000; // if it's been more than 5 seconds since last seen update, assume interval of 100ms.
 #endif
 
 #ifndef SAVE_BATTERY_BY_PREDICTING_FRAME_ARRIVAL_TIMES
@@ -319,7 +319,7 @@ uint64_t PredictNextFrameArrivalTime()
 #ifdef SAVE_BATTERY_BY_SLEEPING_WHEN_IDLE
   // "Deep sleep" options: is user leaves the device with static content on screen for a long time.
   if (timeNow - mostRecentFrame > 60000000) { histogramSize = 1; return lastFramePollTime + 100000; } // if it's been more than one minute since last seen update, assume interval of 500ms.
-  if (timeNow - mostRecentFrame > 5000000) return lastFramePollTime + 100000; // if it's been more than 5 seconds since last seen update, assume interval of 100ms.
+  if (timeNow - mostRecentFrame > 5000000) return lastFramePollTime + 50000; // if it's been more than 5 seconds since last seen update, assume interval of 100ms.
 #endif
   uint64_t interval = EstimateFrameRateInterval();

Perhaps more aggressive timings for the sleep can produce a more desirable balance for users.

[juj]

CC @Mootikins for visibility.

[Mootikins]

Just built with your suggested values, as well as another set at half of them, with the second naturally giving the best results, though the difference only seems to be marginal. Appreciate the suggestion.