Scene: increase the number of FPS samples, use fixed-point arithmetic

Using a higher number of samples (enough samples to account for the last
second or two of playback for e.g.) can be useful when comparing minor
changes in overall playback speed, where the behavior of multi-threaded
operations can make the value jitter with 8 samples (default).

Using fixed-point arithmetic means the average FPS can be updated
by subtracting the oldest FPS sample before adding the new value,
instead of having to average an array of floats every draw.
Increasing the number of samples now only uses a little more memory
(20kb at most).

The error margin from using fixed-point arithmetic is under 0.5
microseconds per frame - more than enough precision for FPS display.
A commented define is included that shows the error margin when enabled.

source/blender/editors/include/ED_scene.hh

@@ -50,7 +50,7 @@ void ED_operatortypes_scene();
  * \param ltime: Time since the last update,
  * compatible with the result of #PIL_check_seconds_timer.
  */
-void ED_scene_fps_average_accumulate(struct Scene *scene, uchar fps_samples, double ltime)
+void ED_scene_fps_average_accumulate(struct Scene *scene, short fps_samples, double ltime)
     ATTR_NONNULL(1);
 /**
  * Calculate an average (if it's not already calculated).

source/blender/editors/scene/scene_fps.cc

@@ -16,6 +16,26 @@
 
 #include "MEM_guardedalloc.h"
 
+using FrameSampleT = uint32_t;
+using FrameSumT = uint64_t;
+
+/**
+ * This values gives enough precision while not overflowing a 64-bit integer when accumulating.
+ * Compared to the same calculation with `double` the error rate is less than 0.5 microsecond,
+ * more than enough precision for FPS display.
+ */
+#define FIXED_UNIT FrameSampleT(65535)
+
+/** Use report the difference (error) between fixed-point arithmetic and double-precision. */
+// #define USE_DEBUG_REPORT_ERROR_MARGIN
+
+struct FrameSample {
+  FrameSampleT value;
+#ifdef USE_DEBUG_REPORT_ERROR_MARGIN
+  double value_db;
+#endif
+};
+
 /**
  * For playback frame-rate info stored during runtime as `scene->fps_info`.
  */
@@ -23,6 +43,11 @@ struct ScreenFrameRateInfo {
   double time_curr;
   double time_prev;
 
+#ifdef USE_DEBUG_REPORT_ERROR_MARGIN
+  double error_sum;
+  int error_samples;
+#endif
+
   /** The target FPS, use to reset on change. */
   float fps_target;
   /** Final result, ignore when -1.0. */
@@ -32,19 +57,37 @@ struct ScreenFrameRateInfo {
   int times_fps_num;
   int times_fps_num_set;
 
-  /** Over allocate. */
-  float times_fps[0];
+  FrameSumT times_fps_sum;
+
+  /** Over allocate (containing `times_fps_num` elements). */
+  FrameSample times_fps[0];
 };
 
 void ED_scene_fps_average_clear(Scene *scene)
 {
-  MEM_SAFE_FREE(scene->fps_info);
+  if (scene->fps_info == nullptr) {
+    return;
+  }
+
+#ifndef NDEBUG
+  /* Assert this value has not somehow become out of sync as this would mean
+   * the reported frame-rate would be wrong. */
+  ScreenFrameRateInfo *fpsi = static_cast<ScreenFrameRateInfo *>(scene->fps_info);
+  FrameSumT times_fps_sum_cmp = 0;
+  for (int i = 0; i < fpsi->times_fps_num_set; i++) {
+    times_fps_sum_cmp += fpsi->times_fps[i].value;
+  }
+  BLI_assert(fpsi->times_fps_sum == times_fps_sum_cmp);
+#endif
+
+  MEM_freeN(scene->fps_info);
+  scene->fps_info = nullptr;
 }
 
-void ED_scene_fps_average_accumulate(Scene *scene, const uchar fps_samples, const double ltime)
+void ED_scene_fps_average_accumulate(Scene *scene, const short fps_samples, const double ltime)
 {
   const float fps_target = float(FPS);
-  const int times_fps_num = fps_samples ? fps_samples : max_ii(1, int(ceilf(fps_target)));
+  const int times_fps_num = (fps_samples > 0) ? fps_samples : max_ii(1, int(ceilf(fps_target)));
 
   ScreenFrameRateInfo *fpsi = static_cast<ScreenFrameRateInfo *>(scene->fps_info);
   if (fpsi) {
@@ -60,8 +103,8 @@ void ED_scene_fps_average_accumulate(Scene *scene, const uchar fps_samples, cons
 
   /* If there isn't any info, initialize it first. */
   if (fpsi == nullptr) {
-    scene->fps_info = MEM_callocN(sizeof(ScreenFrameRateInfo) + (sizeof(float) * times_fps_num),
-                                  __func__);
+    scene->fps_info = MEM_callocN(
+        sizeof(ScreenFrameRateInfo) + (sizeof(FrameSample) * times_fps_num), __func__);
     fpsi = static_cast<ScreenFrameRateInfo *>(scene->fps_info);
     fpsi->fps_target = fps_target;
     fpsi->times_fps_num = times_fps_num;
@@ -94,17 +137,52 @@ float ED_scene_fps_average_calc(const Scene *scene, float *r_fps_target)
     }
 
     /* Doing an average for a more robust calculation. */
-    fpsi->times_fps[fpsi->times_fps_index] = float(1.0 / (fpsi->time_prev - fpsi->time_curr));
+    const double fps_sample = 1.0 / (fpsi->time_prev - fpsi->time_curr);
+
+    {
+      const FrameSampleT fps_prev = (fpsi->times_fps_num_set == fpsi->times_fps_num) ?
+                                        fpsi->times_fps[fpsi->times_fps_index].value :
+                                        FrameSampleT(0);
+      const double fps_curr_db = std::round(fps_sample * double(FIXED_UNIT));
+      BLI_assert(fps_curr_db >= 0.0f);
+      const FrameSampleT fps_curr = FrameSampleT(fps_curr_db);
+      fpsi->times_fps[fpsi->times_fps_index].value = fps_curr;
+      fpsi->times_fps_sum -= fps_prev;
+      fpsi->times_fps_sum += fps_curr;
+    }
+#ifdef USE_DEBUG_REPORT_ERROR_MARGIN
+    fpsi->times_fps[fpsi->times_fps_index].value_db = fps_sample;
+#endif
+
     fpsi->times_fps_index++;
     if (fpsi->times_fps_index > fpsi->times_fps_num_set) {
       fpsi->times_fps_num_set = fpsi->times_fps_index;
     }
     BLI_assert(fpsi->times_fps_num_set > 0);
-    float fps = 0.0f;
-    for (int i = 0; i < fpsi->times_fps_num_set; i++) {
-      fps += fpsi->times_fps[i];
+
+    fpsi->fps_average = float((double(fpsi->times_fps_sum) / double(fpsi->times_fps_num_set)) /
+                              FIXED_UNIT);
+
+#ifdef USE_DEBUG_REPORT_ERROR_MARGIN
+    {
+      double fps_average_ref = 0.0f;
+      for (int i = 0; i < fpsi->times_fps_num_set; i++) {
+        fps_average_ref += fpsi->times_fps[i].value_db;
+      }
+      fps_average_ref = float(fps_average_ref / double(fpsi->times_fps_num_set));
+
+      const float error = float(fps_average_ref) - fpsi->fps_average;
+      fpsi->error_sum += error;
+      fpsi->error_samples += 1;
+      if ((fpsi->error_samples % 100) == 0) {
+        printf("%s error: %.16f over %d samples (average %.16f)\n",
+               __func__,
+               fpsi->error_sum,
+               fpsi->error_samples,
+               fpsi->error_sum / double(fpsi->error_samples));
+      }
     }
-    fpsi->fps_average = fps / float(fpsi->times_fps_num_set);
+#endif /* USE_DEBUG_REPORT_ERROR_MARGIN */
   }
 
   *r_fps_target = fpsi->fps_target;

source/blender/makesdna/DNA_userdef_types.h

@@ -924,9 +924,9 @@ typedef struct UserDef {
   short gpu_backend;
 
   /** Number of samples for FPS display calculations. */
-  uchar playback_fps_samples;
+  short playback_fps_samples;
 
-  char _pad7[3];
+  char _pad7[2];
 
   /** Private, defaults to 20 for 72 DPI setting. */
   short widget_unit;

source/blender/makesrna/intern/rna_userdef.cc

@@ -4835,7 +4835,10 @@ static void rna_def_userdef_view(BlenderRNA *brna)
 
   prop = RNA_def_property(srna, "playback_fps_samples", PROP_INT, PROP_NONE);
   RNA_def_property_int_sdna(prop, nullptr, "playback_fps_samples");
-  RNA_def_property_range(prop, 0, 200);
+  /* NOTE(@ideasman42): this maximum is arbitrary, 5000 samples averages over the last 10 seconds
+   * for an animation playing back at 500fps, which seems like more than enough. */
+  RNA_def_property_range(prop, 0, 5000);
+  RNA_def_property_ui_range(prop, 0, 500, 1, 3);
   RNA_def_property_ui_text(
       prop,
       "FPS Average Samples",