Skip to content

Commit

Permalink
Fix off-by-one in fps_meter, fix unit test to catch this.
Browse files Browse the repository at this point in the history
The FPS counter returned bogus negative values as soon as the
sample count reached kNumFrameTimes since it subtracted a zero value
that was never set due to an off-by-one error.

Unfortunately the unit test didn't catch this since it never filled
the sample buffer. Fixed the test to cover this.

Also reduce kNumFrameTimes to 10, smoothing over 200 frames seems
excessive. We just want a bit of smoothing but should be able to
see some reaction to temporary glitches since these are quite visible
in VR.

BUG=

Review-Url: https://codereview.chromium.org/2892813002
Cr-Commit-Position: refs/heads/master@{#473810}
  • Loading branch information
klausw authored and Commit bot committed May 23, 2017
1 parent 26fe0da commit c90c344
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 3 deletions.
8 changes: 6 additions & 2 deletions chrome/browser/android/vr_shell/fps_meter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace vr_shell {

namespace {

static constexpr size_t kNumFrameTimes = 200;
static constexpr size_t kNumFrameTimes = 10;

} // namepsace

Expand All @@ -20,6 +20,10 @@ FPSMeter::FPSMeter() : total_time_us_(0) {

FPSMeter::~FPSMeter() {}

size_t FPSMeter::GetNumFrameTimes() {
return kNumFrameTimes;
}

void FPSMeter::AddFrame(const base::TimeTicks& time_stamp) {
if (last_time_stamp_.is_null()) {
last_time_stamp_ = time_stamp;
Expand All @@ -31,7 +35,7 @@ void FPSMeter::AddFrame(const base::TimeTicks& time_stamp) {

total_time_us_ += delta.InMicroseconds();

if (frame_times_.size() + 1 < kNumFrameTimes) {
if (frame_times_.size() < kNumFrameTimes) {
frame_times_.push_back(delta);
} else {
total_time_us_ -= frame_times_[current_index_].InMicroseconds();
Expand Down
3 changes: 3 additions & 0 deletions chrome/browser/android/vr_shell/fps_meter.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ class FPSMeter {

double GetFPS() const;

// Get sliding window size for tests.
size_t GetNumFrameTimes();

private:
size_t current_index_;
int64_t total_time_us_;
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/android/vr_shell/fps_meter_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ TEST(FPSMeter, AccurateFPSWithManyFrames) {
EXPECT_FALSE(meter.CanComputeFPS());
EXPECT_FLOAT_EQ(0.0, meter.GetFPS());

for (int i = 0; i < 5; ++i) {
for (size_t i = 0; i < 2 * meter.GetNumFrameTimes(); ++i) {
now += frame_time;
meter.AddFrame(now);
EXPECT_TRUE(meter.CanComputeFPS());
Expand Down

0 comments on commit c90c344

Please sign in to comment.