Skip to content

Commit

Permalink
Fix BeginFrame throttling logic (round 2).
Browse files Browse the repository at this point in the history 
This is another attempt at fixing the issue addressed here:
https://chromium-review.googlesource.com/c/chromium/src/+/3936368

That fix works for a throttled frame rate of 30Hz, but does not work
for 20Hz. Why: 20Hz yields a frame interval of exactly 50000 us, which
is already a multiple of 100 microseconds (so the flooring in that
solution doesn't apply). On a 60Hz display, three frames span
16666 us * 3 = 49998 us, which is less than 50000 us. Ultimately, the
third frame in the sequence gets throttled incorrectly, yielding a
frame rate of 1 / (16666us * 4) = 15Hz instead of 20Hz.

Bug: b:249539848
Change-Id: Iaf7815b2e49622388bef8260a01ef42d9523a629
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3994188
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Reviewed-by: Anthony Vallée-Dubois <anthonyvd@chromium.org>
Commit-Queue: Eric Sum <esum@google.com>
Cr-Commit-Position: refs/heads/main@{#1067335}
  • Loading branch information
@esum26
esum26 authored and Chromium LUCI CQ committed Nov 4, 2022
1 parent be41d94 commit b5c59b7
Show file tree
Hide file tree
Showing 4 changed files with 132 additions and 51 deletions.
24 changes: 23 additions & 1 deletion components/viz/service/frame_sinks/compositor_frame_sink_support.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1141,8 +1141,30 @@ void CompositorFrameSinkSupport::CheckPendingSurfaces() {

bool CompositorFrameSinkSupport::ShouldThrottleBeginFrameAsRequested(
base::TimeTicks frame_time) {
// It is not good enough to only check whether
// |time_since_last_frame| < |begin_frame_interval_|. There are 2 factors
// complicating this (examples assume a 30Hz throttled frame rate):
// 1) The precision of timing between frames is in microseconds, which
// can result in error accumulation over several throttled frames. For
// instance, on a 60Hz display, the first frame is produced at 0.016666
// seconds, and the second at (0.016666 + 0.016666 = 0.033332) seconds.
// base::Hertz(30) is 0.033333 seconds, so the second frame is considered
// to have been produced too fast, and is therefore throttled.
// 2) Small system error in the frame timestamps (often on the order of a few
// microseconds). For example, the first frame may be produced at 0.016662
// seconds (instead of 0.016666), so the second frame's timestamp is
// 0.016662 + 0.016666 = 0.033328 and incorrectly gets throttled.
//
// To correct for this: Ceil the time since last frame to the nearest 100us.
// Building off the example above:
// Frame 1 time -> 0.016662 -> 0.0167 -> Throttle
// Frame 2 time -> 0.016662 + 0.016666 = 0.033328 -> 0.0334 -> Don't Throttle
static constexpr base::TimeDelta kFrameTimeQuantization =
base::Microseconds(100);
base::TimeDelta time_since_last_frame = frame_time - last_frame_time_;
return begin_frame_interval_.is_positive() &&
(frame_time - last_frame_time_) < begin_frame_interval_;
time_since_last_frame.CeilToMultiple(kFrameTimeQuantization) <
begin_frame_interval_;
}

void CompositorFrameSinkSupport::ProcessCompositorFrameTransitionDirective(
Expand Down
132 changes: 104 additions & 28 deletions components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <utility>

#include "base/bind.h"
#include "base/numerics/safe_conversions.h"
#include "base/test/scoped_feature_list.h"
#include "base/test/simple_test_tick_clock.h"
#include "base/time/time.h"
Expand Down Expand Up @@ -1569,9 +1570,9 @@ TEST_F(CompositorFrameSinkSupportTest, ThrottleUnresponsiveClient) {
support->SetNeedsBeginFrame(false);
}

// Verifies that when CompositorFrameSinkSupport has its |begin_frame_interval_|
// set, any BeginFrame would be sent only after this interval has passed from
// the time when the last BeginFrame was sent.
// Verifies that when CompositorFrameSinkSupport has its
// |begin_frame_interval_| set, any BeginFrame would be sent only after this
// interval has passed from the time when the last BeginFrame was sent.
TEST_F(ThrottledBeginFrameCompositorFrameSinkSupportTest, BeginFrameInterval) {
FakeExternalBeginFrameSource begin_frame_source(0.f, false);

Expand All @@ -1581,48 +1582,123 @@ TEST_F(ThrottledBeginFrameCompositorFrameSinkSupportTest, BeginFrameInterval) {
SurfaceId id(kAnotherArbitraryFrameSinkId, local_surface_id_);
support->SetBeginFrameSource(&begin_frame_source);
support->SetNeedsBeginFrame(true);
constexpr uint8_t fps = 5;
constexpr int fps = 5;
constexpr base::TimeDelta throttled_interval = base::Seconds(1) / fps;
support->ThrottleBeginFrame(throttled_interval);

constexpr base::TimeDelta interval = BeginFrameArgs::DefaultInterval();
const int num_expected_skipped_frames =
(base::ClampRound<int>(interval.ToHz()) / fps) - 1;
base::TimeTicks frame_time;
uint64_t sequence_number = 1;
int sequence_number = 1;
int sent_frames = 0;
BeginFrameArgs args;
uint64_t frames_throttled_since_last = 0;
int frames_throttled_since_last = 0;
const base::TimeTicks end_time = frame_time + base::Seconds(2);

base::TimeTicks next_expected_begin_frame = frame_time;
while (frame_time < end_time) {
args = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0,
sequence_number++, frame_time);
if (frame_time < next_expected_begin_frame) {
EXPECT_CALL(mock_client, OnBeginFrame(_, _)).Times(0);
++frames_throttled_since_last;
} else {
BeginFrameArgs expected_args(args);
expected_args.interval = throttled_interval;
expected_args.deadline =
frame_time + throttled_interval -
BeginFrameArgs::DefaultEstimatedDisplayDrawTime(interval);
expected_args.frames_throttled_since_last = frames_throttled_since_last;
frames_throttled_since_last = 0;
EXPECT_CALL(mock_client, OnBeginFrame(expected_args, _)).WillOnce([&]() {
support->SubmitCompositorFrame(local_surface_id_,
MakeDefaultCompositorFrame());
GetSurfaceForId(id)->MarkAsDrawn();
++sent_frames;
});
next_expected_begin_frame = throttled_interval + frame_time;
}

BeginFrameArgs expected_args(args);
expected_args.interval = throttled_interval;
expected_args.deadline =
frame_time + throttled_interval -
BeginFrameArgs::DefaultEstimatedDisplayDrawTime(interval);
expected_args.frames_throttled_since_last = frames_throttled_since_last;
bool sent_frame = false;
ON_CALL(mock_client, OnBeginFrame(_, _))
.WillByDefault([&](const BeginFrameArgs& actual_args,
const FrameTimingDetailsMap&) {
EXPECT_THAT(actual_args, Eq(expected_args));
support->SubmitCompositorFrame(local_surface_id_,
MakeDefaultCompositorFrame());
GetSurfaceForId(id)->MarkAsDrawn();
sent_frame = true;
++sent_frames;
if (!frame_time.is_null()) {
EXPECT_THAT(frames_throttled_since_last,
Eq(num_expected_skipped_frames));
}
frames_throttled_since_last = 0;
});

begin_frame_source.TestOnBeginFrame(args);
testing::Mock::VerifyAndClearExpectations(&mock_client);

if (!sent_frame) {
++frames_throttled_since_last;
}
frame_time += interval;
}
// In total 10 frames should have been sent (5fps x 2 seconds).
EXPECT_EQ(sent_frames, 10);
// In total 11 frames should have been sent (5fps x 2 seconds) + 1 frame at
// time 0.
EXPECT_EQ(sent_frames, 11);
support->SetNeedsBeginFrame(false);
}

TEST_F(ThrottledBeginFrameCompositorFrameSinkSupportTest,
HandlesSmallErrorInBeginFrameTimes) {
FakeExternalBeginFrameSource begin_frame_source(0.f, false);

testing::NiceMock<MockCompositorFrameSinkClient> mock_client;
auto support = std::make_unique<CompositorFrameSinkSupport>(
&mock_client, &manager_, kAnotherArbitraryFrameSinkId,
/*is_root=*/true);
SurfaceId id(kAnotherArbitraryFrameSinkId, local_surface_id_);
support->SetBeginFrameSource(&begin_frame_source);
support->SetNeedsBeginFrame(true);
constexpr base::TimeDelta kNativeInterval = BeginFrameArgs::DefaultInterval();
constexpr base::TimeDelta kThrottledInterval = kNativeInterval * 2;
support->ThrottleBeginFrame(kThrottledInterval);
constexpr base::TimeDelta kEpsilon = base::Microseconds(2);

base::TimeTicks frame_time;
int sequence_number = 1;

auto submit_compositor_frame = [&]() {
support->SubmitCompositorFrame(local_surface_id_,
MakeDefaultCompositorFrame());
GetSurfaceForId(id)->MarkAsDrawn();
};

// T: 0 (Should always draw)
EXPECT_CALL(mock_client, OnBeginFrame(_, _))
.WillOnce(submit_compositor_frame);
begin_frame_source.TestOnBeginFrame(CreateBeginFrameArgsForTesting(
BEGINFRAME_FROM_HERE, 0, sequence_number++, frame_time));
testing::Mock::VerifyAndClearExpectations(&mock_client);

// T: 1 native interval
frame_time += kNativeInterval;
EXPECT_CALL(mock_client, OnBeginFrame(_, _)).Times(0);
begin_frame_source.TestOnBeginFrame(CreateBeginFrameArgsForTesting(
BEGINFRAME_FROM_HERE, 0, sequence_number++, frame_time));
testing::Mock::VerifyAndClearExpectations(&mock_client);

// T: 2 native intervals - epsilon
frame_time += (kNativeInterval - kEpsilon);
EXPECT_CALL(mock_client, OnBeginFrame(_, _))
.WillOnce(submit_compositor_frame);
begin_frame_source.TestOnBeginFrame(CreateBeginFrameArgsForTesting(
BEGINFRAME_FROM_HERE, 0, sequence_number++, frame_time));
testing::Mock::VerifyAndClearExpectations(&mock_client);

// T: 3 native intervals
frame_time += kNativeInterval;
EXPECT_CALL(mock_client, OnBeginFrame(_, _)).Times(0);
begin_frame_source.TestOnBeginFrame(CreateBeginFrameArgsForTesting(
BEGINFRAME_FROM_HERE, 0, sequence_number++, frame_time));
testing::Mock::VerifyAndClearExpectations(&mock_client);

// T: 4 native intervals + epsilon
frame_time += kNativeInterval + 2 * kEpsilon;
EXPECT_CALL(mock_client, OnBeginFrame(_, _))
.WillOnce(submit_compositor_frame);
begin_frame_source.TestOnBeginFrame(CreateBeginFrameArgsForTesting(
BEGINFRAME_FROM_HERE, 0, sequence_number++, frame_time));
testing::Mock::VerifyAndClearExpectations(&mock_client);

support->SetNeedsBeginFrame(false);
}

Expand Down
15 changes: 1 addition & 14 deletions components/viz/service/frame_sinks/frame_sink_manager_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -751,20 +751,7 @@ void FrameSinkManagerImpl::Throttle(const std::vector<FrameSinkId>& ids,

void FrameSinkManagerImpl::StartThrottlingAllFrameSinks(
base::TimeDelta interval) {
// Floor the requested interval to the nearest 10th of a millisecond. This is
// because the precision of timing between frames is in microseconds, which
// can result in error accumulation over several throttled frames.
//
// For instance, on a 60Hz display, the first frame is produced at 0.016666
// seconds, and the second at (0.016666 + 0.016666 = 0.033332) seconds.
// base::Hertz(30) is 0.033333 seconds, so the second frame is considered to
// have been produced too fast, and is therefore throttled. This results in a
// 20Hz refresh rate instead of the desired 30Hz.
//
// Flooring at the nearest 10th of a millisecond produces correct throttling
// results for frame rates up to 960Hz, after which either flooring to
// milliseconds or a more precise between-frames measurement is required.
global_throttle_interval_ = interval.FloorToMultiple(base::Microseconds(100));
global_throttle_interval_ = interval;
UpdateThrottling();
}

Expand Down
12 changes: 4 additions & 8 deletions components/viz/service/frame_sinks/frame_sink_manager_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -535,10 +535,6 @@ TEST_F(FrameSinkManagerTest, GlobalThrottle) {

constexpr base::TimeDelta global_interval = base::Hertz(30);
constexpr base::TimeDelta interval = base::Hertz(20);
// The global throttle interval is floored to avoid precision-related
// accumulated error. See the comment on `StartThrottlingAllFrameSinks`
const base::TimeDelta expected_global_interval =
base::Hertz(30).FloorToMultiple(base::Microseconds(100));

std::vector<FrameSinkId> ids{kFrameSinkIdRoot, kFrameSinkIdA, kFrameSinkIdB,
kFrameSinkIdC, kFrameSinkIdD};
Expand All @@ -549,20 +545,20 @@ TEST_F(FrameSinkManagerTest, GlobalThrottle) {

// Starting global throttling should throttle the entire hierarchy.
manager_.StartThrottlingAllFrameSinks(global_interval);
VerifyThrottling(expected_global_interval, ids);
VerifyThrottling(global_interval, ids);

// Throttling more aggressively on top of global throttling should further
// throttle the specified frame sink hierarchy, but preserve global throttling
// on the unaffected framesinks.
manager_.Throttle({kFrameSinkIdC}, interval);
VerifyThrottling(expected_global_interval,
VerifyThrottling(global_interval,
{kFrameSinkIdRoot, kFrameSinkIdA, kFrameSinkIdB});
VerifyThrottling(interval, {kFrameSinkIdC, kFrameSinkIdD});

// Attempting to per-sink throttle to an interval shorter than the global
// throttling should still throttle all frame sinks to the global interval.
manager_.Throttle({kFrameSinkIdA}, base::Hertz(40));
VerifyThrottling(expected_global_interval, ids);
VerifyThrottling(global_interval, ids);

// Add a new branch to the hierarchy. These new frame sinks should be globally
// throttled immediately. root -> A -> B
Expand All @@ -575,7 +571,7 @@ TEST_F(FrameSinkManagerTest, GlobalThrottle) {
manager_.RegisterFrameSinkHierarchy(client_e->frame_sink_id(),
client_f->frame_sink_id());
VerifyThrottling(
expected_global_interval,
global_interval,
{kFrameSinkIdRoot, kFrameSinkIdA, kFrameSinkIdB, kFrameSinkIdC,
kFrameSinkIdD, kFrameSinkIdE, kFrameSinkIdF});

Expand Down

0 comments on commit b5c59b7

Please sign in to comment.