Implement OnBeginFrameAcks
This change updates CompositorFrameSinkClient::OnBeginFrame to also take
an array of ReturnedResource.
When the Viz feature::OnBeginFrameAcks is enabled the OnBeginFrame
message will act as both DidReceiveCompositorFrameAck and
ReclaimResources. Except during teardown, when ReclaimResources will be
sent directly.
CompositorFrameSinkSupport has been updated to have some initial
differentiation for handling BeginFrameArgs::kManualSourceId. Some tests
such as blink_web_tests are using two BeginFrameSources. One is manual,
and the tests are performing updates in response to the Ack. For this
testing style we still send DidReceiveCompositorFrameAck immediately, as
otherwise the manual frames can get too backed up.
Clients have been updated to call their own DidReceiveCompositorFrameAck
and ReclaimResources in response to the OnBeginFrame. In future work we
will analysis the side-effects of these methods to see abouth further
reduction in post-tasks.
viz_unittests have been updated to be parameterized to test both modes,
with changed mock call expectations.
Bug: 1399880
Change-Id: Idf003d04474391ba0afeade30ba5b72797670e49
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4107129
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Kyle Charbonneau <kylechar@chromium.org>
Reviewed-by: Bo Liu <boliu@chromium.org>
Reviewed-by: Brandon Jones <bajones@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1106819}
diff --git a/android_webview/browser/gfx/root_frame_sink.cc b/android_webview/browser/gfx/root_frame_sink.cc
index c5cbd9c..3d83337 100644
--- a/android_webview/browser/gfx/root_frame_sink.cc
+++ b/android_webview/browser/gfx/root_frame_sink.cc
@@ -49,7 +49,9 @@
ReclaimResources(std::move(resources));
}
void OnBeginFrame(const viz::BeginFrameArgs& args,
- const viz::FrameTimingDetailsMap& feedbacks) override {}
+ const viz::FrameTimingDetailsMap& feedbacks,
+ bool frame_ack,
+ std::vector<viz::ReturnedResource> resources) override {}
void OnBeginFramePausedChanged(bool paused) override {}
void ReclaimResources(std::vector<viz::ReturnedResource> resources) override {
owner_->ReturnResources(frame_sink_id_, layer_tree_frame_sink_id_,
diff --git a/android_webview/browser/gfx/root_frame_sink.h b/android_webview/browser/gfx/root_frame_sink.h
index 3d51f9c..1ca1429 100644
--- a/android_webview/browser/gfx/root_frame_sink.h
+++ b/android_webview/browser/gfx/root_frame_sink.h
@@ -84,7 +84,9 @@
void DidReceiveCompositorFrameAck(
std::vector<viz::ReturnedResource> resources) override;
void OnBeginFrame(const viz::BeginFrameArgs& args,
- const viz::FrameTimingDetailsMap& feedbacks) override {}
+ const viz::FrameTimingDetailsMap& feedbacks,
+ bool frame_ack,
+ std::vector<viz::ReturnedResource> resources) override {}
void OnBeginFramePausedChanged(bool paused) override {}
void ReclaimResources(std::vector<viz::ReturnedResource> resources) override;
void OnCompositorFrameTransitionDirectiveProcessed(
diff --git a/android_webview/browser/gfx/test/invalidate_test.cc b/android_webview/browser/gfx/test/invalidate_test.cc
index 510acec5..861fc9a 100644
--- a/android_webview/browser/gfx/test/invalidate_test.cc
+++ b/android_webview/browser/gfx/test/invalidate_test.cc
@@ -14,6 +14,7 @@
#include "base/task/single_thread_task_runner.h"
#include "base/test/task_environment.h"
#include "base/time/time.h"
+#include "components/viz/common/features.h"
#include "components/viz/common/quads/solid_color_draw_quad.h"
#include "components/viz/common/quads/surface_draw_quad.h"
#include "components/viz/service/frame_sinks/compositor_frame_sink_support.h"
@@ -158,7 +159,12 @@
pending_frames_--;
}
void OnBeginFrame(const viz::BeginFrameArgs& args,
- const viz::FrameTimingDetailsMap& feedbacks) override {
+ const viz::FrameTimingDetailsMap& feedbacks,
+ bool frame_ack,
+ std::vector<viz::ReturnedResource> resources) override {
+ if (features::IsOnBeginFrameAcksEnabled() && pending_frames_) {
+ DidReceiveCompositorFrameAck(std::move(resources));
+ }
for (const auto& feedback : feedbacks) {
DCHECK(!feedbacks_.contains(feedback.first));
feedbacks_[feedback.first] = feedback.second;
diff --git a/cc/mojo_embedder/async_layer_tree_frame_sink.cc b/cc/mojo_embedder/async_layer_tree_frame_sink.cc
index 67832f00..27da27f 100644
--- a/cc/mojo_embedder/async_layer_tree_frame_sink.cc
+++ b/cc/mojo_embedder/async_layer_tree_frame_sink.cc
@@ -255,7 +255,17 @@
void AsyncLayerTreeFrameSink::OnBeginFrame(
const viz::BeginFrameArgs& args,
- const viz::FrameTimingDetailsMap& timing_details) {
+ const viz::FrameTimingDetailsMap& timing_details,
+ bool frame_ack,
+ std::vector<viz::ReturnedResource> resources) {
+ if (features::IsOnBeginFrameAcksEnabled()) {
+ if (frame_ack) {
+ DidReceiveCompositorFrameAck(std::move(resources));
+ } else if (!resources.empty()) {
+ ReclaimResources(std::move(resources));
+ }
+ }
+
for (const auto& pair : timing_details) {
client_->DidPresentCompositorFrame(pair.first, pair.second);
}
diff --git a/cc/mojo_embedder/async_layer_tree_frame_sink.h b/cc/mojo_embedder/async_layer_tree_frame_sink.h
index 1bc5739..6cdb257 100644
--- a/cc/mojo_embedder/async_layer_tree_frame_sink.h
+++ b/cc/mojo_embedder/async_layer_tree_frame_sink.h
@@ -109,7 +109,9 @@
void DidReceiveCompositorFrameAck(
std::vector<viz::ReturnedResource> resources) override;
void OnBeginFrame(const viz::BeginFrameArgs& begin_frame_args,
- const viz::FrameTimingDetailsMap& timing_details) override;
+ const viz::FrameTimingDetailsMap& timing_details,
+ bool frame_ack,
+ std::vector<viz::ReturnedResource> resources) override;
void OnBeginFramePausedChanged(bool paused) override;
void ReclaimResources(std::vector<viz::ReturnedResource> resources) override;
void OnCompositorFrameTransitionDirectiveProcessed(
diff --git a/cc/slim/frame_sink_impl.cc b/cc/slim/frame_sink_impl.cc
index d151eba..c83845b2 100644
--- a/cc/slim/frame_sink_impl.cc
+++ b/cc/slim/frame_sink_impl.cc
@@ -15,6 +15,7 @@
#include "base/trace_event/trace_event.h"
#include "build/build_config.h"
#include "cc/slim/frame_sink_impl_client.h"
+#include "components/viz/common/features.h"
#include "components/viz/common/resources/platform_color.h"
#include "components/viz/common/resources/resource_format_utils.h"
#include "components/viz/common/resources/resource_id.h"
@@ -208,7 +209,17 @@
void FrameSinkImpl::OnBeginFrame(
const viz::BeginFrameArgs& begin_frame_args,
- const viz::FrameTimingDetailsMap& timing_details) {
+ const viz::FrameTimingDetailsMap& timing_details,
+ bool frame_ack,
+ std::vector<viz::ReturnedResource> resources) {
+ if (features::IsOnBeginFrameAcksEnabled()) {
+ if (frame_ack) {
+ DidReceiveCompositorFrameAck(std::move(resources));
+ } else if (!resources.empty()) {
+ ReclaimResources(std::move(resources));
+ }
+ }
+
for (const auto& pair : timing_details) {
client_->DidPresentCompositorFrame(pair.first, pair.second);
}
diff --git a/cc/slim/frame_sink_impl.h b/cc/slim/frame_sink_impl.h
index c5ffae2..61db3293 100644
--- a/cc/slim/frame_sink_impl.h
+++ b/cc/slim/frame_sink_impl.h
@@ -76,7 +76,9 @@
void DidReceiveCompositorFrameAck(
std::vector<viz::ReturnedResource> resources) override;
void OnBeginFrame(const viz::BeginFrameArgs& begin_frame_args,
- const viz::FrameTimingDetailsMap& timing_details) override;
+ const viz::FrameTimingDetailsMap& timing_details,
+ bool frame_ack,
+ std::vector<viz::ReturnedResource> resources) override;
void OnBeginFramePausedChanged(bool paused) override {}
void ReclaimResources(std::vector<viz::ReturnedResource> resources) override;
void OnCompositorFrameTransitionDirectiveProcessed(
diff --git a/cc/test/layer_tree_test.cc b/cc/test/layer_tree_test.cc
index f09d985..5f4bff4 100644
--- a/cc/test/layer_tree_test.cc
+++ b/cc/test/layer_tree_test.cc
@@ -113,6 +113,10 @@
}
void DidReceiveCompositorFrameAck(
std::vector<viz::ReturnedResource> resources) override {
+ if (!frame_ack_pending_) {
+ DCHECK(resources.empty());
+ return;
+ }
DCHECK(frame_ack_pending_);
frame_ack_pending_ = false;
TestLayerTreeFrameSink::DidReceiveCompositorFrameAck(std::move(resources));
diff --git a/cc/test/test_layer_tree_frame_sink.cc b/cc/test/test_layer_tree_frame_sink.cc
index f024890..fa1f3b9c 100644
--- a/cc/test/test_layer_tree_frame_sink.cc
+++ b/cc/test/test_layer_tree_frame_sink.cc
@@ -15,6 +15,7 @@
#include "cc/trees/layer_tree_frame_sink_client.h"
#include "cc/trees/single_thread_proxy.h"
#include "cc/trees/task_runner_provider.h"
+#include "components/viz/common/features.h"
#include "components/viz/common/frame_sinks/begin_frame_args.h"
#include "components/viz/common/resources/bitmap_allocation.h"
#include "components/viz/service/display/direct_renderer.h"
@@ -264,7 +265,18 @@
void TestLayerTreeFrameSink::OnBeginFrame(
const viz::BeginFrameArgs& args,
- const viz::FrameTimingDetailsMap& timing_details) {
+ const viz::FrameTimingDetailsMap& timing_details,
+ bool frame_ack,
+ std::vector<viz::ReturnedResource> resources) {
+ // We do not want to Ack the first OnBeginFrame. Only deliver Acks once there
+ // is a valid activated surface, and we have pending frames.
+ if (features::IsOnBeginFrameAcksEnabled()) {
+ if (frame_ack) {
+ DidReceiveCompositorFrameAck(std::move(resources));
+ } else if (!resources.empty()) {
+ ReclaimResources(std::move(resources));
+ }
+ }
DebugScopedSetImplThread impl(task_runner_provider_);
for (const auto& pair : timing_details)
client_->DidPresentCompositorFrame(pair.first, pair.second);
diff --git a/cc/test/test_layer_tree_frame_sink.h b/cc/test/test_layer_tree_frame_sink.h
index eb03f67..4e0af4c8 100644
--- a/cc/test/test_layer_tree_frame_sink.h
+++ b/cc/test/test_layer_tree_frame_sink.h
@@ -105,7 +105,9 @@
void DidReceiveCompositorFrameAck(
std::vector<viz::ReturnedResource> resources) override;
void OnBeginFrame(const viz::BeginFrameArgs& args,
- const viz::FrameTimingDetailsMap& timing_details) override;
+ const viz::FrameTimingDetailsMap& timing_details,
+ bool frame_ack,
+ std::vector<viz::ReturnedResource> resources) override;
void ReclaimResources(std::vector<viz::ReturnedResource> resources) override;
void OnBeginFramePausedChanged(bool paused) override;
void OnCompositorFrameTransitionDirectiveProcessed(
diff --git a/components/exo/wayland/clients/perftests.cc b/components/exo/wayland/clients/perftests.cc
index b819dc7..d4ca122 100644
--- a/components/exo/wayland/clients/perftests.cc
+++ b/components/exo/wayland/clients/perftests.cc
@@ -4,14 +4,29 @@
#include "base/command_line.h"
#include "base/strings/stringprintf.h"
+#include "base/test/scoped_feature_list.h"
#include "components/exo/wayland/clients/blur.h"
#include "components/exo/wayland/clients/simple.h"
#include "components/exo/wayland/clients/test/wayland_client_test.h"
+#include "components/viz/common/features.h"
#include "testing/perf/perf_result_reporter.h"
namespace {
-using WaylandClientPerfTests = exo::WaylandClientTest;
+class WaylandClientPerfTests : public exo::WaylandClientTest {
+ public:
+ WaylandClientPerfTests();
+ ~WaylandClientPerfTests() override = default;
+
+ private:
+ base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+WaylandClientPerfTests::WaylandClientPerfTests() {
+ // TODO(crbug.com/1399591): Figure out the missing/misordered
+ // PresentationFeedback when using this feature.
+ scoped_feature_list_.InitAndDisableFeature(features::kOnBeginFrameAcks);
+}
constexpr char kMetricPrefixWaylandClient[] = "WaylandClient.";
constexpr char kMetricFramerate[] = "framerate";
diff --git a/components/viz/common/features.cc b/components/viz/common/features.cc
index 97f8ca0..3046742 100644
--- a/components/viz/common/features.cc
+++ b/components/viz/common/features.cc
@@ -225,6 +225,13 @@
// begin also evicting the entire FrameTree.
BASE_FEATURE(kEvictSubtree, "EvictSubtree", base::FEATURE_DISABLED_BY_DEFAULT);
#endif
+// If enabled, CompositorFrameSinkClient::OnBeginFrame is also treated as the
+// DidReceiveCompositorFrameAck. Both in providing the Ack for the previous
+// frame, and in returning resources. While enabled the separate Ack and
+// ReclaimResources signals will not be sent.
+BASE_FEATURE(kOnBeginFrameAcks,
+ "OnBeginFrameAcks",
+ base::FEATURE_DISABLED_BY_DEFAULT);
bool IsDelegatedCompositingEnabled() {
return base::FeatureList::IsEnabled(kDelegatedCompositing);
@@ -368,4 +375,8 @@
return base::FeatureList::IsEnabled(kRendererAllocatesImages);
}
+bool IsOnBeginFrameAcksEnabled() {
+ return base::FeatureList::IsEnabled(features::kOnBeginFrameAcks);
+}
+
} // namespace features
diff --git a/components/viz/common/features.h b/components/viz/common/features.h
index 5f40d60..0bbf3ee 100644
--- a/components/viz/common/features.h
+++ b/components/viz/common/features.h
@@ -63,6 +63,7 @@
#if BUILDFLAG(IS_ANDROID)
VIZ_COMMON_EXPORT BASE_DECLARE_FEATURE(kEvictSubtree);
#endif
+VIZ_COMMON_EXPORT BASE_DECLARE_FEATURE(kOnBeginFrameAcks);
VIZ_COMMON_EXPORT extern const char kDraw1Point12Ms[];
VIZ_COMMON_EXPORT extern const char kDraw2Points6Ms[];
@@ -99,6 +100,7 @@
VIZ_COMMON_EXPORT bool ShouldVideoDetectorIgnoreNonVideoFrames();
VIZ_COMMON_EXPORT bool ShouldOverrideThrottledFrameRateParams();
VIZ_COMMON_EXPORT bool ShouldRendererAllocateImages();
+VIZ_COMMON_EXPORT bool IsOnBeginFrameAcksEnabled();
} // namespace features
diff --git a/components/viz/demo/client/demo_client.cc b/components/viz/demo/client/demo_client.cc
index 14b504f..90a71116 100644
--- a/components/viz/demo/client/demo_client.cc
+++ b/components/viz/demo/client/demo_client.cc
@@ -168,9 +168,10 @@
// See documentation in mojom for how this can be used.
}
-void DemoClient::OnBeginFrame(
- const viz::BeginFrameArgs& args,
- const viz::FrameTimingDetailsMap& timing_details) {
+void DemoClient::OnBeginFrame(const viz::BeginFrameArgs& args,
+ const viz::FrameTimingDetailsMap& timing_details,
+ bool frame_ack,
+ std::vector<viz::ReturnedResource> resources) {
// Generate a new compositor-frame for each begin-frame. This demo client
// generates and submits the compositor-frame immediately. But it is possible
// for the client to delay sending the compositor-frame. |args| includes the
diff --git a/components/viz/demo/client/demo_client.h b/components/viz/demo/client/demo_client.h
index f445baa..46daf68 100644
--- a/components/viz/demo/client/demo_client.h
+++ b/components/viz/demo/client/demo_client.h
@@ -102,7 +102,9 @@
void DidReceiveCompositorFrameAck(
std::vector<viz::ReturnedResource> resources) override;
void OnBeginFrame(const viz::BeginFrameArgs& args,
- const viz::FrameTimingDetailsMap& timing_details) override;
+ const viz::FrameTimingDetailsMap& timing_details,
+ bool frame_ack,
+ std::vector<viz::ReturnedResource> resources) override;
void OnBeginFramePausedChanged(bool paused) override;
void ReclaimResources(std::vector<viz::ReturnedResource> resources) override;
void OnCompositorFrameTransitionDirectiveProcessed(
diff --git a/components/viz/service/display/display_unittest.cc b/components/viz/service/display/display_unittest.cc
index cd4302c6..0151fc6 100644
--- a/components/viz/service/display/display_unittest.cc
+++ b/components/viz/service/display/display_unittest.cc
@@ -82,6 +82,8 @@
static constexpr FrameSinkId kAnotherFrameSinkId(4, 4);
static constexpr FrameSinkId kAnotherFrameSinkId2(5, 5);
+const uint64_t kBeginFrameSourceId = 1337;
+
class TestSoftwareOutputDevice : public SoftwareOutputDevice {
public:
gfx::Rect damage_rect() const { return damage_rect_; }
@@ -3432,7 +3434,31 @@
}
}
-TEST_F(DisplayTest, CompositorFrameWithPresentationToken) {
+// Supports testing features::OnBeginFrameAcks, which changes the expectations
+// of what IPCs are sent to the CompositorFrameSinkClient. When enabled
+// OnBeginFrame also handles ReturnResources as well as
+// DidReceiveCompositorFrameAck.
+class OnBeginFrameAcksDisplayTest : public DisplayTest,
+ public testing::WithParamInterface<bool> {
+ public:
+ OnBeginFrameAcksDisplayTest();
+ ~OnBeginFrameAcksDisplayTest() override = default;
+
+ bool BeginFrameAcksEnabled() const { return GetParam(); }
+
+ private:
+ base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+OnBeginFrameAcksDisplayTest::OnBeginFrameAcksDisplayTest() {
+ if (BeginFrameAcksEnabled()) {
+ scoped_feature_list_.InitAndEnableFeature(features::kOnBeginFrameAcks);
+ } else {
+ scoped_feature_list_.InitAndDisableFeature(features::kOnBeginFrameAcks);
+ }
+}
+
+TEST_P(OnBeginFrameAcksDisplayTest, CompositorFrameWithPresentationToken) {
RendererSettings settings;
id_allocator_.GenerateId();
const LocalSurfaceId local_surface_id(
@@ -3463,8 +3489,10 @@
CompositorFrame frame =
CompositorFrameBuilder()
.AddRenderPass(gfx::Rect(sub_surface_size), gfx::Rect())
+ .SetBeginFrameSourceId(kBeginFrameSourceId)
.Build();
- EXPECT_CALL(sub_client, DidReceiveCompositorFrameAck(_)).Times(1);
+ EXPECT_CALL(sub_client, DidReceiveCompositorFrameAck(_))
+ .Times(BeginFrameAcksEnabled() ? 0 : 1);
frame_token_1 = frame.metadata.frame_token;
sub_support->SubmitCompositorFrame(sub_local_surface_id, std::move(frame));
}
@@ -3514,10 +3542,12 @@
CompositorFrame frame = CompositorFrameBuilder()
.AddRenderPass(gfx::Rect(sub_surface_size),
gfx::Rect(sub_surface_size))
+ .SetBeginFrameSourceId(kBeginFrameSourceId)
.Build();
frame_token_2 = frame.metadata.frame_token;
- EXPECT_CALL(sub_client, DidReceiveCompositorFrameAck(_)).Times(1);
+ EXPECT_CALL(sub_client, DidReceiveCompositorFrameAck(_))
+ .Times(BeginFrameAcksEnabled() ? 0 : 1);
sub_support->SubmitCompositorFrame(sub_local_surface_id, std::move(frame));
display_->DrawAndSwap({base::TimeTicks::Now(), base::TimeTicks::Now()});
@@ -3533,9 +3563,11 @@
CompositorFrame frame =
CompositorFrameBuilder()
.AddRenderPass(gfx::Rect(sub_surface_size), gfx::Rect())
+ .SetBeginFrameSourceId(kBeginFrameSourceId)
.Build();
- EXPECT_CALL(sub_client, DidReceiveCompositorFrameAck(_)).Times(1);
+ EXPECT_CALL(sub_client, DidReceiveCompositorFrameAck(_))
+ .Times(BeginFrameAcksEnabled() ? 0 : 1);
sub_support->SubmitCompositorFrame(sub_local_surface_id, std::move(frame));
display_->DrawAndSwap({base::TimeTicks::Now(), base::TimeTicks::Now()});
@@ -3543,6 +3575,14 @@
}
}
+INSTANTIATE_TEST_SUITE_P(,
+ OnBeginFrameAcksDisplayTest,
+ testing::Bool(),
+ [](auto& info) {
+ return info.param ? "BeginFrameAcks"
+ : "CompositoFrameAcks";
+ });
+
TEST_F(DisplayTest, BeginFrameThrottling) {
id_allocator_.GenerateId();
SetUpGpuDisplay(RendererSettings());
diff --git a/components/viz/service/frame_sinks/compositor_frame_sink_impl.cc b/components/viz/service/frame_sinks/compositor_frame_sink_impl.cc
index 5d0b821bb..a9c7bb4 100644
--- a/components/viz/service/frame_sinks/compositor_frame_sink_impl.cc
+++ b/components/viz/service/frame_sinks/compositor_frame_sink_impl.cc
@@ -49,10 +49,13 @@
}
void OnBeginFrame(const BeginFrameArgs& args,
- const FrameTimingDetailsMap& timing_details) override {
+ const FrameTimingDetailsMap& timing_details,
+ bool frame_ack,
+ std::vector<ReturnedResource> resources) override {
if (auto* bundle = GetBundle()) {
bundle->EnqueueOnBeginFrame(frame_sink_id_.sink_id(), args,
- timing_details);
+ timing_details, frame_ack,
+ std::move(resources));
}
}
diff --git a/components/viz/service/frame_sinks/compositor_frame_sink_support.cc b/components/viz/service/frame_sinks/compositor_frame_sink_support.cc
index 842409c..f85e259 100644
--- a/components/viz/service/frame_sinks/compositor_frame_sink_support.cc
+++ b/components/viz/service/frame_sinks/compositor_frame_sink_support.cc
@@ -312,6 +312,13 @@
if (surface->surface_id() == last_created_surface_id_)
last_created_surface_id_ = SurfaceId();
+
+ if (!features::IsOnBeginFrameAcksEnabled() || !client_ ||
+ surface_returned_resources_.empty()) {
+ return;
+ }
+ client_->ReclaimResources(std::move(surface_returned_resources_));
+ surface_returned_resources_.clear();
}
void CompositorFrameSinkSupport::OnSurfacePresented(
@@ -344,7 +351,20 @@
std::vector<ReturnedResource> resources) {
if (resources.empty())
return;
- if (!ack_pending_count_ && client_) {
+
+ // When features::OnBeginFrameAcks is disabled we attempt to return resources
+ // in DidReceiveCompositorFrameAck. However if there is no
+ // `ack_pending_count_` then we don't expect that signal soon. In which case
+ // we return the resources to the `client_` now.
+ //
+ // When features::OnBeginFrameAcks is enabled we attempt to return resources
+ // during the next OnBeginFrame. However if we currently do not
+ // `needs_begin_frame_` or if we have been disconnected from a
+ // `begin_frame_source_` then we don't expect that signal soon. In which case
+ // we return the resources to the `client_` now.
+ if (!ack_pending_count_ && client_ &&
+ (!features::IsOnBeginFrameAcksEnabled() ||
+ (!needs_begin_frame_ || !begin_frame_source_))) {
client_->ReclaimResources(std::move(resources));
return;
}
@@ -533,6 +553,11 @@
begin_frame_tracker_.ReceivedAck(frame.metadata.begin_frame_ack);
++ack_pending_count_;
+ if (frame.metadata.begin_frame_ack.frame_id.source_id ==
+ BeginFrameArgs::kManualSourceId) {
+ pending_manual_begin_frame_source_ = true;
+ }
+
compositor_frame_callback_ = std::move(callback);
if (compositor_frame_callback_) {
callback_received_begin_frame_ = false;
@@ -705,7 +730,12 @@
void CompositorFrameSinkSupport::DidReceiveCompositorFrameAck() {
DCHECK_GT(ack_pending_count_, 0);
+ bool was_pending_manual_begin_frame_source_ =
+ pending_manual_begin_frame_source_;
ack_pending_count_--;
+ if (!ack_pending_count_) {
+ pending_manual_begin_frame_source_ = false;
+ }
if (!client_)
return;
@@ -717,6 +747,11 @@
return;
}
+ if (features::IsOnBeginFrameAcksEnabled() &&
+ !was_pending_manual_begin_frame_source_) {
+ return;
+ }
+
client_->DidReceiveCompositorFrameAck(std::move(surface_returned_resources_));
surface_returned_resources_.clear();
}
@@ -843,7 +878,16 @@
frames_throttled_since_last_ = 0;
last_frame_time_ = adjusted_args.frame_time;
- client_->OnBeginFrame(adjusted_args, std::move(frame_timing_details_));
+ if (features::IsOnBeginFrameAcksEnabled()) {
+ client_->OnBeginFrame(adjusted_args, std::move(frame_timing_details_),
+ !ack_pending_count_,
+ std::move(surface_returned_resources_));
+ surface_returned_resources_.clear();
+ } else {
+ client_->OnBeginFrame(adjusted_args, std::move(frame_timing_details_),
+ /*frame_ack=*/false,
+ std::vector<ReturnedResource>());
+ }
begin_frame_tracker_.SentBeginFrame(adjusted_args);
frame_sink_manager_->DidBeginFrame(frame_sink_id_, adjusted_args);
frame_timing_details_.clear();
@@ -866,11 +910,14 @@
return;
// We require a begin frame if there's a callback pending, or if the client
- // requested it, or if the client needs to get some frame timing details.
+ // requested it, or if the client needs to get some frame timing details, or
+ // if there are resources to return.
needs_begin_frame_ =
(client_needs_begin_frame_ || !frame_timing_details_.empty() ||
!pending_surfaces_.empty() ||
- (compositor_frame_callback_ && !callback_received_begin_frame_));
+ (compositor_frame_callback_ && !callback_received_begin_frame_) ||
+ (features::IsOnBeginFrameAcksEnabled() &&
+ !surface_returned_resources_.empty()));
if (bundle_id_.has_value()) {
// When bundled with other sinks, observation of BeginFrame notifications is
diff --git a/components/viz/service/frame_sinks/compositor_frame_sink_support.h b/components/viz/service/frame_sinks/compositor_frame_sink_support.h
index 1982e76..c8350efc 100644
--- a/components/viz/service/frame_sinks/compositor_frame_sink_support.h
+++ b/components/viz/service/frame_sinks/compositor_frame_sink_support.h
@@ -249,6 +249,7 @@
friend class CompositorFrameSinkSupportTest;
friend class DisplayTest;
friend class FrameSinkManagerTest;
+ friend class OnBeginFrameAcksCompositorFrameSinkSupportTest;
friend class SurfaceAggregatorWithResourcesTest;
// Creates a surface reference from the top-level root to |surface_id|.
@@ -333,6 +334,16 @@
// Counts the number of CompositorFrames that have been submitted and have not
// yet received an ACK.
int ack_pending_count_ = 0;
+
+ // When `true` we have received frames from a client using its own
+ // BeginFrameSource. While dealing with frames from multiple sources we cannot
+ // fely on `ack_pending_count_` to throttle frame production.
+ //
+ // TODO(crbug.com/1396081): Track acks, presentation feedback, and resources
+ // being returned, on a per BeginFrameSource basis. For
+ // BeginFrameArgs::kManualSourceId the feedback and resources should not be
+ // tied to the current `begin_frame_source_`;
+ bool pending_manual_begin_frame_source_ = false;
std::vector<ReturnedResource> surface_returned_resources_;
// The begin frame source being observered. Null if none.
diff --git a/components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc b/components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc
index 326ba0ac..ffed289 100644
--- a/components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc
+++ b/components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc
@@ -4,6 +4,7 @@
#include "components/viz/service/frame_sinks/compositor_frame_sink_support.h"
+#include <string>
#include <utility>
#include "base/functional/bind.h"
@@ -59,6 +60,8 @@
const base::UnguessableToken kAnotherArbitraryToken =
base::UnguessableToken::CreateForTesting(2, 2);
+const uint64_t kBeginFrameSourceId = 1337;
+
// Matches a SurfaceInfo for |surface_id|.
MATCHER_P(SurfaceInfoWithId, surface_id, "") {
return arg.id() == surface_id;
@@ -78,6 +81,10 @@
return first.frame_id == second.frame_id;
}
+std::string PostTestCaseName(const ::testing::TestParamInfo<bool>& info) {
+ return info.param ? "BeginFrameAcks" : "CompositoFrameAcks";
+}
+
} // namespace
class MockFrameSinkManagerClient : public mojom::FrameSinkManagerClient {
@@ -141,7 +148,7 @@
void SubmitCompositorFrameWithResources(ResourceId* resource_ids,
size_t num_resource_ids) {
- auto frame = MakeDefaultCompositorFrame();
+ auto frame = MakeDefaultCompositorFrame(kBeginFrameSourceId);
AddResourcesToFrame(&frame, resource_ids, num_resource_ids);
support_->SubmitCompositorFrame(local_surface_id_, std::move(frame));
EXPECT_EQ(surface_observer_.last_created_surface_id().local_surface_id(),
@@ -271,20 +278,82 @@
const gpu::SyncToken consumer_sync_token_;
};
+// Supports testing features::OnBeginFrameAcks, which changes the expectations
+// of what IPCs are sent to the CompositorFrameSinkClient. When enabled
+// OnBeginFrame also handles ReturnResources as well as
+// DidReceiveCompositorFrameAck.
+class OnBeginFrameAcksCompositorFrameSinkSupportTest
+ : public CompositorFrameSinkSupportTest,
+ public testing::WithParamInterface<bool> {
+ public:
+ explicit OnBeginFrameAcksCompositorFrameSinkSupportTest(
+ bool override_throttled_frame_rate_params = false);
+ ~OnBeginFrameAcksCompositorFrameSinkSupportTest() override = default;
+
+ // When features::OnBeginFrameAcks is enabled resources are only returned
+ // after a frame has been Acked, and during the next OnBeginFrame. When this
+ // is off the resources are returned immediately.
+ //
+ // These methods will submit the according Ack/BeginFrames when
+ // features::OnBeginFrameAcks is enabled, to ensure the resource return path
+ // is triggered.
+ void MaybeSendCompositorFrameAck();
+ void MaybeTestOnBeginFrame(uint64_t sequence_number);
+
+ bool BeginFrameAcksEnabled() const { return GetParam(); }
+
+ int ack_pending_count(const CompositorFrameSinkSupport* support) const {
+ return support->ack_pending_count_;
+ }
+
+ private:
+ base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+OnBeginFrameAcksCompositorFrameSinkSupportTest::
+ OnBeginFrameAcksCompositorFrameSinkSupportTest(
+ bool override_throttled_frame_rate_params)
+ : CompositorFrameSinkSupportTest(override_throttled_frame_rate_params) {
+ if (BeginFrameAcksEnabled()) {
+ scoped_feature_list_.InitAndEnableFeature(features::kOnBeginFrameAcks);
+ } else {
+ scoped_feature_list_.InitAndDisableFeature(features::kOnBeginFrameAcks);
+ }
+}
+
+void OnBeginFrameAcksCompositorFrameSinkSupportTest::
+ MaybeSendCompositorFrameAck() {
+ if (!BeginFrameAcksEnabled()) {
+ return;
+ }
+ support_->SendCompositorFrameAck();
+}
+
+void OnBeginFrameAcksCompositorFrameSinkSupportTest::MaybeTestOnBeginFrame(
+ uint64_t sequence_number) {
+ if (!BeginFrameAcksEnabled()) {
+ return;
+ }
+ BeginFrameArgs args =
+ CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0, sequence_number);
+ begin_frame_source_.TestOnBeginFrame(args);
+}
+
class ThrottledBeginFrameCompositorFrameSinkSupportTest
- : public CompositorFrameSinkSupportTest {
+ : public OnBeginFrameAcksCompositorFrameSinkSupportTest {
protected:
ThrottledBeginFrameCompositorFrameSinkSupportTest()
- : CompositorFrameSinkSupportTest(
+ : OnBeginFrameAcksCompositorFrameSinkSupportTest(
/*override_throttled_frame_rate_params=*/true) {}
};
// Tests submitting a frame with resources followed by one with no resources
// with no resource provider action in between.
-TEST_F(CompositorFrameSinkSupportTest, ResourceLifetimeSimple) {
+TEST_P(OnBeginFrameAcksCompositorFrameSinkSupportTest, ResourceLifetimeSimple) {
ResourceId first_frame_ids[] = {ResourceId(1), ResourceId(2), ResourceId(3)};
SubmitCompositorFrameWithResources(first_frame_ids,
std::size(first_frame_ids));
+ MaybeSendCompositorFrameAck();
// All of the resources submitted in the first frame are still in use at this
// time by virtue of being in the pending frame, so none can be returned to
@@ -296,6 +365,7 @@
// make all resources of first frame available to be returned.
SubmitCompositorFrameWithResources(nullptr, 0);
+ MaybeTestOnBeginFrame(1);
ResourceId expected_returned_ids[] = {ResourceId(1), ResourceId(2),
ResourceId(3)};
int expected_returned_counts[] = {1, 1, 1};
@@ -320,6 +390,7 @@
SubmitCompositorFrameWithResources(forth_frame_ids,
std::size(forth_frame_ids));
+ MaybeTestOnBeginFrame(2);
ResourceId forth_expected_returned_ids[] = {ResourceId(4), ResourceId(5),
ResourceId(6)};
int forth_expected_returned_counts[] = {1, 1, 1};
@@ -331,11 +402,12 @@
// Tests submitting a frame with resources followed by one with no resources
// with the resource provider holding everything alive.
-TEST_F(CompositorFrameSinkSupportTest,
+TEST_P(OnBeginFrameAcksCompositorFrameSinkSupportTest,
ResourceLifetimeSimpleWithProviderHoldingAlive) {
ResourceId first_frame_ids[] = {ResourceId(1), ResourceId(2), ResourceId(3)};
SubmitCompositorFrameWithResources(first_frame_ids,
std::size(first_frame_ids));
+ MaybeSendCompositorFrameAck();
// All of the resources submitted in the first frame are still in use at this
// time by virtue of being in the pending frame, so none can be returned to
@@ -363,6 +435,7 @@
// Submitting an empty frame causes previous resources referenced by the
// previous frame to be returned to client.
SubmitCompositorFrameWithResources(nullptr, 0);
+ MaybeTestOnBeginFrame(1);
ResourceId expected_returned_ids[] = {ResourceId(1), ResourceId(2),
ResourceId(3)};
int expected_returned_counts[] = {1, 1, 1};
@@ -373,10 +446,12 @@
// Tests referencing a resource, unref'ing it to zero, then using it again
// before returning it to the client.
-TEST_F(CompositorFrameSinkSupportTest, ResourceReusedBeforeReturn) {
+TEST_P(OnBeginFrameAcksCompositorFrameSinkSupportTest,
+ ResourceReusedBeforeReturn) {
ResourceId first_frame_ids[] = {ResourceId(7)};
SubmitCompositorFrameWithResources(first_frame_ids,
std::size(first_frame_ids));
+ MaybeSendCompositorFrameAck();
// This removes all references to resource id 7.
SubmitCompositorFrameWithResources(nullptr, 0);
@@ -388,6 +463,7 @@
// This removes it again.
SubmitCompositorFrameWithResources(nullptr, 0);
+ MaybeTestOnBeginFrame(2);
// Now it should be returned.
// We don't care how many entries are in the returned array for 7, so long as
// the total returned count matches the submitted count.
@@ -403,10 +479,12 @@
// Tests having resources referenced multiple times, as if referenced by
// multiple providers.
-TEST_F(CompositorFrameSinkSupportTest, ResourceRefMultipleTimes) {
+TEST_P(OnBeginFrameAcksCompositorFrameSinkSupportTest,
+ ResourceRefMultipleTimes) {
ResourceId first_frame_ids[] = {ResourceId(3), ResourceId(4)};
SubmitCompositorFrameWithResources(first_frame_ids,
std::size(first_frame_ids));
+ MaybeSendCompositorFrameAck();
// Ref resources from the first frame twice.
RefCurrentFrameResources();
@@ -443,6 +521,8 @@
UnrefResources(ids_to_unref, counts, std::size(ids_to_unref));
SubmitCompositorFrameWithResources(nullptr, 0);
+
+ MaybeTestOnBeginFrame(1);
ResourceId expected_returned_ids[] = {ResourceId(3)};
int expected_returned_counts[] = {1};
CheckReturnedResourcesMatchExpected(
@@ -460,6 +540,7 @@
UnrefResources(ids_to_unref, counts, std::size(ids_to_unref));
SubmitCompositorFrameWithResources(nullptr, 0);
+ MaybeTestOnBeginFrame(2);
ResourceId expected_returned_ids[] = {ResourceId(5)};
int expected_returned_counts[] = {1};
CheckReturnedResourcesMatchExpected(
@@ -476,6 +557,7 @@
UnrefResources(ids_to_unref, counts, std::size(ids_to_unref));
SubmitCompositorFrameWithResources(nullptr, 0);
+ MaybeTestOnBeginFrame(3);
ResourceId expected_returned_ids[] = {ResourceId(4)};
int expected_returned_counts[] = {2};
CheckReturnedResourcesMatchExpected(
@@ -484,11 +566,24 @@
}
}
-TEST_F(CompositorFrameSinkSupportTest, ResourceLifetime) {
+TEST_P(OnBeginFrameAcksCompositorFrameSinkSupportTest, ResourceLifetime) {
+ support_->SetNeedsBeginFrame(true);
ResourceId first_frame_ids[] = {ResourceId(1), ResourceId(2), ResourceId(3)};
SubmitCompositorFrameWithResources(first_frame_ids,
std::size(first_frame_ids));
+ // This test relied on CompositorFrameSinkSupport::ReturnResources to not send
+ // as long as there has been no DidReceiveCompositorFrameAck. Such that
+ // `ack_pending_count_` is always greater than 1.
+ //
+ // With features::kOnBeginFrameAcks we now return the resources during
+ // OnBeginFrame, however that is throttled while we await any ack.
+ //
+ // Renderers, the principle Viz Client, do not submit new CompositorFrames as
+ // long as there is a pending ack. So the original testing scenario here does
+ // not occur.
+ MaybeSendCompositorFrameAck();
+
// All of the resources submitted in the first frame are still in use at this
// time by virtue of being in the pending frame, so none can be returned to
// the client yet.
@@ -503,6 +598,7 @@
std::size(second_frame_ids));
{
SCOPED_TRACE("second frame");
+ MaybeTestOnBeginFrame(1);
ResourceId expected_returned_ids[] = {ResourceId(1)};
int expected_returned_counts[] = {1};
CheckReturnedResourcesMatchExpected(
@@ -521,6 +617,7 @@
{
SCOPED_TRACE("third frame");
+ MaybeTestOnBeginFrame(2);
ResourceId expected_returned_ids[] = {ResourceId(2), ResourceId(3),
ResourceId(4)};
int expected_returned_counts[] = {2, 2, 1};
@@ -577,6 +674,7 @@
{
SCOPED_TRACE("fourth frame, second unref");
+ MaybeTestOnBeginFrame(3);
ResourceId expected_returned_ids[] = {ResourceId(10), ResourceId(11),
ResourceId(12), ResourceId(13)};
int expected_returned_counts[] = {1, 1, 2, 2};
@@ -586,28 +684,41 @@
}
}
-TEST_F(CompositorFrameSinkSupportTest, AddDuringEviction) {
+TEST_P(OnBeginFrameAcksCompositorFrameSinkSupportTest, AddDuringEviction) {
manager_.RegisterFrameSinkId(kAnotherArbitraryFrameSinkId,
true /* report_activation */);
MockCompositorFrameSinkClient mock_client;
auto support = std::make_unique<CompositorFrameSinkSupport>(
&mock_client, &manager_, kAnotherArbitraryFrameSinkId, kIsRoot);
LocalSurfaceId local_surface_id(6, kArbitraryToken);
- support->SubmitCompositorFrame(local_surface_id,
- MakeDefaultCompositorFrame());
+ support->SubmitCompositorFrame(
+ local_surface_id, MakeDefaultCompositorFrame(kBeginFrameSourceId));
SurfaceManager* surface_manager = manager_.surface_manager();
- EXPECT_CALL(mock_client, DidReceiveCompositorFrameAck(_))
- .WillOnce(testing::InvokeWithoutArgs([&]() {
- LocalSurfaceId new_id(7, base::UnguessableToken::Create());
- support->SubmitCompositorFrame(new_id, MakeDefaultCompositorFrame());
- surface_manager->GarbageCollectSurfaces();
- }))
- .WillRepeatedly(testing::Return());
+ auto submit_compositor_frame = [&]() {
+ LocalSurfaceId new_id(7, base::UnguessableToken::Create());
+ support->SubmitCompositorFrame(
+ new_id, MakeDefaultCompositorFrame(kBeginFrameSourceId));
+ surface_manager->GarbageCollectSurfaces();
+ };
+ if (BeginFrameAcksEnabled()) {
+ EXPECT_CALL(mock_client, DidReceiveCompositorFrameAck(_)).Times(0);
+ } else {
+ EXPECT_CALL(mock_client, DidReceiveCompositorFrameAck(_))
+ .WillOnce(submit_compositor_frame)
+ .WillRepeatedly(testing::Return());
+ }
support->EvictSurface(local_surface_id);
ExpireAllTemporaryReferences();
manager_.InvalidateFrameSinkId(kAnotherArbitraryFrameSinkId);
+
+ if (BeginFrameAcksEnabled()) {
+ submit_compositor_frame();
+ testing::Mock::VerifyAndClearExpectations(&mock_client);
+ }
+
+ EXPECT_EQ(1, ack_pending_count(support.get()));
}
// Verifies that only monotonically increasing LocalSurfaceIds are accepted.
@@ -666,7 +777,8 @@
// Verifies that CopyOutputRequests submitted by unprivileged clients are
// rejected.
-TEST_F(CompositorFrameSinkSupportTest, ProhibitsUnprivilegedCopyRequests) {
+TEST_P(OnBeginFrameAcksCompositorFrameSinkSupportTest,
+ ProhibitsUnprivilegedCopyRequests) {
manager_.RegisterFrameSinkId(kAnotherArbitraryFrameSinkId,
true /* report_activation */);
MockCompositorFrameSinkClient mock_client;
@@ -699,6 +811,8 @@
aborted_copy_run_loop.Run();
EXPECT_TRUE(did_receive_aborted_copy_result);
+ MaybeTestOnBeginFrame(1);
+
// All the resources in the rejected frame should have been returned.
CheckReturnedResourcesMatchExpected(frame_resource_ids,
std::size(frame_resource_ids));
@@ -707,7 +821,8 @@
}
// Tests doing an EvictLastActivatedSurface before shutting down the factory.
-TEST_F(CompositorFrameSinkSupportTest, EvictLastActivatedSurface) {
+TEST_P(OnBeginFrameAcksCompositorFrameSinkSupportTest,
+ EvictLastActivatedSurface) {
manager_.RegisterFrameSinkId(kAnotherArbitraryFrameSinkId,
true /* report_activation */);
MockCompositorFrameSinkClient mock_client;
@@ -722,6 +837,7 @@
auto frame = CompositorFrameBuilder()
.AddDefaultRenderPass()
.AddTransferableResource(resource)
+ .SetBeginFrameSourceId(kBeginFrameSourceId)
.Build();
support->SubmitCompositorFrame(local_surface_id, std::move(frame));
EXPECT_EQ(surface_observer_.last_created_surface_id().local_surface_id(),
@@ -730,11 +846,18 @@
ResourceId returned_id = resource.ToReturnedResource().id;
EXPECT_TRUE(GetSurfaceForId(id));
- EXPECT_CALL(mock_client, DidReceiveCompositorFrameAck(_))
- .WillOnce([=](std::vector<ReturnedResource> got) {
- EXPECT_EQ(1u, got.size());
- EXPECT_EQ(returned_id, got[0].id);
- });
+ auto expected_returned_resources = [=](std::vector<ReturnedResource> got) {
+ EXPECT_EQ(1u, got.size());
+ EXPECT_EQ(returned_id, got[0].id);
+ };
+ if (BeginFrameAcksEnabled()) {
+ EXPECT_CALL(mock_client, DidReceiveCompositorFrameAck(_)).Times(0);
+ EXPECT_CALL(mock_client, ReclaimResources(_))
+ .WillOnce(expected_returned_resources);
+ } else {
+ EXPECT_CALL(mock_client, DidReceiveCompositorFrameAck(_))
+ .WillOnce(expected_returned_resources);
+ }
support->EvictSurface(local_surface_id);
ExpireAllTemporaryReferences();
manager_.surface_manager()->GarbageCollectSurfaces();
@@ -973,7 +1096,7 @@
// Check that if the size of a CompositorFrame doesn't match the size of the
// Surface it's being submitted to, we skip the frame.
-TEST_F(CompositorFrameSinkSupportTest, FrameSizeMismatch) {
+TEST_P(OnBeginFrameAcksCompositorFrameSinkSupportTest, FrameSizeMismatch) {
SurfaceId id(support_->frame_sink_id(), local_surface_id_);
// Submit a frame with size (5,5).
@@ -1002,6 +1125,9 @@
EXPECT_EQ(SubmitResult::SIZE_MISMATCH, result);
+ MaybeSendCompositorFrameAck();
+ MaybeTestOnBeginFrame(1);
+
// All the resources in the rejected frame should have been returned.
CheckReturnedResourcesMatchExpected(frame_resource_ids,
std::size(frame_resource_ids));
@@ -1071,7 +1197,7 @@
// Validates that if a client asked to stop receiving begin-frames, then it
// stops receiving begin-frames after receiving the presentation-feedback from
// the last submitted frame.
-TEST_F(CompositorFrameSinkSupportTest,
+TEST_P(OnBeginFrameAcksCompositorFrameSinkSupportTest,
NeedsBeginFrameResetAfterPresentationFeedback) {
// Request BeginFrames.
support_->SetNeedsBeginFrame(true);
@@ -1105,6 +1231,7 @@
// The presentation-feedback from the last submitted frame arrives. This
// results in the client immediately receiving a MISSED begin-frame.
+ support_->SendCompositorFrameAck();
SendPresentationFeedback(support_.get(), token);
received_args = GetLastUsedBeginFrameArgs(support_.get());
EXPECT_TRUE(BeginFrameArgsAreEquivalent(args, received_args));
@@ -1504,7 +1631,7 @@
args = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0,
sequence_number++, frametime);
- EXPECT_CALL(mock_client, OnBeginFrame(args, _));
+ EXPECT_CALL(mock_client, OnBeginFrame(args, _, _, _));
begin_frame_source.TestOnBeginFrame(args);
testing::Mock::VerifyAndClearExpectations(&mock_client);
}
@@ -1517,14 +1644,14 @@
frametime += interval;
args = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0,
sequence_number++, frametime);
- EXPECT_CALL(mock_client, OnBeginFrame(args, _)).Times(0);
+ EXPECT_CALL(mock_client, OnBeginFrame(args, _, _, _)).Times(0);
begin_frame_source.TestOnBeginFrame(args);
testing::Mock::VerifyAndClearExpectations(&mock_client);
frametime = unthrottle_time - base::Microseconds(1);
args = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0,
sequence_number++, frametime);
- EXPECT_CALL(mock_client, OnBeginFrame(args, _)).Times(0);
+ EXPECT_CALL(mock_client, OnBeginFrame(args, _, _, _)).Times(0);
begin_frame_source.TestOnBeginFrame(args);
testing::Mock::VerifyAndClearExpectations(&mock_client);
@@ -1532,7 +1659,7 @@
frametime = unthrottle_time;
args = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0,
sequence_number++, frametime);
- EXPECT_CALL(mock_client, OnBeginFrame(args, _));
+ EXPECT_CALL(mock_client, OnBeginFrame(args, _, _, _));
begin_frame_source.TestOnBeginFrame(args);
testing::Mock::VerifyAndClearExpectations(&mock_client);
}
@@ -1544,7 +1671,7 @@
frametime += base::Minutes(1);
args = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0,
sequence_number++, frametime);
- EXPECT_CALL(mock_client, OnBeginFrame(args, _)).Times(0);
+ EXPECT_CALL(mock_client, OnBeginFrame(args, _, _, _)).Times(0);
begin_frame_source.TestOnBeginFrame(args);
testing::Mock::VerifyAndClearExpectations(&mock_client);
@@ -1555,7 +1682,7 @@
frametime += interval;
args = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0,
sequence_number++, frametime);
- EXPECT_CALL(mock_client, OnBeginFrame(args, _));
+ EXPECT_CALL(mock_client, OnBeginFrame(args, _, _, _));
begin_frame_source.TestOnBeginFrame(args);
testing::Mock::VerifyAndClearExpectations(&mock_client);
@@ -1565,7 +1692,7 @@
// 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) {
+TEST_P(ThrottledBeginFrameCompositorFrameSinkSupportTest, BeginFrameInterval) {
FakeExternalBeginFrameSource begin_frame_source(0.f, false);
testing::NiceMock<MockCompositorFrameSinkClient> mock_client;
@@ -1599,14 +1726,21 @@
BeginFrameArgs::DefaultEstimatedDisplayDrawTime(interval);
expected_args.frames_throttled_since_last = frames_throttled_since_last;
bool sent_frame = false;
- ON_CALL(mock_client, OnBeginFrame(_, _))
+ ON_CALL(mock_client, OnBeginFrame(_, _, _, _))
.WillByDefault([&](const BeginFrameArgs& actual_args,
- const FrameTimingDetailsMap&) {
+ const FrameTimingDetailsMap&, bool frame_ack,
+ std::vector<ReturnedResource>) {
EXPECT_THAT(actual_args, Eq(expected_args));
support->SubmitCompositorFrame(local_surface_id_,
MakeDefaultCompositorFrame());
GetSurfaceForId(id)->MarkAsDrawn();
sent_frame = true;
+ // Ack the first submitted frame, as if activation completed.
+ // Subsequent frames are not sharing any resources, so the unref
+ // process will Ack the frame before activation.
+ if (!sent_frames) {
+ support->SendCompositorFrameAck();
+ }
++sent_frames;
if (!frame_time.is_null()) {
EXPECT_THAT(frames_throttled_since_last,
@@ -1629,7 +1763,7 @@
support->SetNeedsBeginFrame(false);
}
-TEST_F(ThrottledBeginFrameCompositorFrameSinkSupportTest,
+TEST_P(ThrottledBeginFrameCompositorFrameSinkSupportTest,
HandlesSmallErrorInBeginFrameTimes) {
FakeExternalBeginFrameSource begin_frame_source(0.f, false);
@@ -1655,22 +1789,23 @@
};
// T: 0 (Should always draw)
- EXPECT_CALL(mock_client, OnBeginFrame(_, _))
+ 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->SendCompositorFrameAck();
// T: 1 native interval
frame_time += kNativeInterval;
- EXPECT_CALL(mock_client, OnBeginFrame(_, _)).Times(0);
+ 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(_, _))
+ EXPECT_CALL(mock_client, OnBeginFrame(_, _, _, _))
.WillOnce(submit_compositor_frame);
begin_frame_source.TestOnBeginFrame(CreateBeginFrameArgsForTesting(
BEGINFRAME_FROM_HERE, 0, sequence_number++, frame_time));
@@ -1678,14 +1813,14 @@
// T: 3 native intervals
frame_time += kNativeInterval;
- EXPECT_CALL(mock_client, OnBeginFrame(_, _)).Times(0);
+ 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(_, _))
+ EXPECT_CALL(mock_client, OnBeginFrame(_, _, _, _))
.WillOnce(submit_compositor_frame);
begin_frame_source.TestOnBeginFrame(CreateBeginFrameArgsForTesting(
BEGINFRAME_FROM_HERE, 0, sequence_number++, frame_time));
@@ -1694,7 +1829,7 @@
support->SetNeedsBeginFrame(false);
}
-TEST_F(ThrottledBeginFrameCompositorFrameSinkSupportTest,
+TEST_P(ThrottledBeginFrameCompositorFrameSinkSupportTest,
UsesThrottledIntervalInPresentationFeedback) {
static constexpr base::TimeDelta kThrottledFrameInterval = base::Hertz(5);
// Request BeginFrames.
@@ -1717,6 +1852,7 @@
.Build();
auto token = frame.metadata.frame_token;
support_->SubmitCompositorFrame(local_surface_id_, std::move(frame));
+ support_->SendCompositorFrameAck();
// The presentation-feedback from the last submitted frame arrives.
SendPresentationFeedback(support_.get(), token);
@@ -1759,7 +1895,7 @@
EXPECT_CALL(mock_client,
OnBeginFrame(testing::Field(&BeginFrameArgs::animate_only,
testing::IsFalse()),
- _));
+ _, _, _));
begin_frame_source.TestOnBeginFrame(args_animate_only);
}
@@ -1846,4 +1982,13 @@
support_->GetCopyOutputRequestRegion(crop_id));
}
+INSTANTIATE_TEST_SUITE_P(,
+ OnBeginFrameAcksCompositorFrameSinkSupportTest,
+ testing::Bool(),
+ &PostTestCaseName);
+
+INSTANTIATE_TEST_SUITE_P(,
+ ThrottledBeginFrameCompositorFrameSinkSupportTest,
+ testing::Bool(),
+ &PostTestCaseName);
} // namespace viz
diff --git a/components/viz/service/frame_sinks/frame_sink_bundle_impl.cc b/components/viz/service/frame_sinks/frame_sink_bundle_impl.cc
index 998c1fa..0726275 100644
--- a/components/viz/service/frame_sinks/frame_sink_bundle_impl.cc
+++ b/components/viz/service/frame_sinks/frame_sink_bundle_impl.cc
@@ -101,9 +101,11 @@
void EnqueueOnBeginFrame(
uint32_t sink_id,
const BeginFrameArgs& args,
- const base::flat_map<uint32_t, FrameTimingDetails>& details) {
- pending_on_begin_frames_.push_back(
- mojom::BeginFrameInfo::New(sink_id, args, details));
+ const base::flat_map<uint32_t, FrameTimingDetails>& details,
+ bool frame_ack,
+ std::vector<ReturnedResource> resources) {
+ pending_on_begin_frames_.push_back(mojom::BeginFrameInfo::New(
+ sink_id, args, details, frame_ack, std::move(resources)));
if (!defer_on_begin_frames_) {
FlushMessages();
}
@@ -356,14 +358,18 @@
void FrameSinkBundleImpl::EnqueueOnBeginFrame(
uint32_t sink_id,
const BeginFrameArgs& args,
- const base::flat_map<uint32_t, FrameTimingDetails>& details) {
+ const base::flat_map<uint32_t, FrameTimingDetails>& details,
+ bool frame_ack,
+ std::vector<ReturnedResource> resources) {
if (auto* group = GetSinkGroup(sink_id)) {
- group->EnqueueOnBeginFrame(sink_id, args, details);
+ group->EnqueueOnBeginFrame(sink_id, args, details, frame_ack,
+ std::move(resources));
} else {
// The sink has no BeginFrameSource at the moment and therefore does not
// belong to a SinkGroup. Forward directly without batching.
std::vector<mojom::BeginFrameInfoPtr> begin_frames;
- begin_frames.push_back(mojom::BeginFrameInfo::New(sink_id, args, details));
+ begin_frames.push_back(mojom::BeginFrameInfo::New(
+ sink_id, args, details, frame_ack, std::move(resources)));
client_->FlushNotifications({}, std::move(begin_frames), {});
}
}
diff --git a/components/viz/service/frame_sinks/frame_sink_bundle_impl.h b/components/viz/service/frame_sinks/frame_sink_bundle_impl.h
index 1e4c549fe..8ae2071 100644
--- a/components/viz/service/frame_sinks/frame_sink_bundle_impl.h
+++ b/components/viz/service/frame_sinks/frame_sink_bundle_impl.h
@@ -85,7 +85,9 @@
void EnqueueOnBeginFrame(
uint32_t sink_id,
const BeginFrameArgs& args,
- const base::flat_map<uint32_t, FrameTimingDetails>& details);
+ const base::flat_map<uint32_t, FrameTimingDetails>& details,
+ bool frame_ack,
+ std::vector<ReturnedResource> resources);
void EnqueueReclaimResources(uint32_t sink_id,
std::vector<ReturnedResource> resources);
void SendOnBeginFramePausedChanged(uint32_t sink_id, bool paused);
diff --git a/components/viz/service/frame_sinks/frame_sink_bundle_impl_unittest.cc b/components/viz/service/frame_sinks/frame_sink_bundle_impl_unittest.cc
index 2d72429..f6405c7 100644
--- a/components/viz/service/frame_sinks/frame_sink_bundle_impl_unittest.cc
+++ b/components/viz/service/frame_sinks/frame_sink_bundle_impl_unittest.cc
@@ -15,9 +15,11 @@
#include "base/strings/string_number_conversions.h"
#include "base/task/sequenced_task_runner.h"
#include "base/test/bind.h"
+#include "base/test/scoped_feature_list.h"
#include "base/test/simple_test_tick_clock.h"
#include "base/test/task_environment.h"
#include "base/unguessable_token.h"
+#include "components/viz/common/features.h"
#include "components/viz/common/frame_sinks/begin_frame_source.h"
#include "components/viz/common/quads/compositor_frame.h"
#include "components/viz/common/resources/resource_id.h"
@@ -68,6 +70,8 @@
const LocalSurfaceId kSurfaceB{3, kSurfaceTokenB};
const LocalSurfaceId kSurfaceC{4, kSurfaceTokenC};
+const uint64_t kBeginFrameSourceId = 1337;
+
gpu::SyncToken MakeVerifiedSyncToken(int id) {
gpu::SyncToken token;
token.Set(gpu::CommandBufferNamespace::GPU_IO,
@@ -241,7 +245,7 @@
const FrameSinkId& frame_sink_id,
const LocalSurfaceId& surface_id,
std::vector<ResourceId> resource_ids = {}) {
- auto frame = MakeDefaultCompositorFrame();
+ auto frame = MakeDefaultCompositorFrame(kBeginFrameSourceId);
for (const auto& id : resource_ids) {
TransferableResource resource;
resource.id = id;
@@ -343,7 +347,41 @@
EXPECT_EQ(1u, begin_frame_source().num_observers());
}
-TEST_F(FrameSinkBundleImplTest, SubmitAndAck) {
+class OnBeginFrameAcksFrameSinkBundleImplTest
+ : public FrameSinkBundleImplTest,
+ public testing::WithParamInterface<bool> {
+ public:
+ OnBeginFrameAcksFrameSinkBundleImplTest();
+ ~OnBeginFrameAcksFrameSinkBundleImplTest() override = default;
+
+ // This will IssueOnBeginFrame if BeginFrameAcksEnabled is true. Since we no
+ // longer send the ack separately, we need the OnBeginFrame both to be the
+ // ack, and so there are messages to flush.
+ void MaybeIssueOnBeginFrame();
+
+ bool BeginFrameAcksEnabled() const { return GetParam(); }
+
+ private:
+ base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+OnBeginFrameAcksFrameSinkBundleImplTest::
+ OnBeginFrameAcksFrameSinkBundleImplTest() {
+ if (BeginFrameAcksEnabled()) {
+ scoped_feature_list_.InitAndEnableFeature(features::kOnBeginFrameAcks);
+ } else {
+ scoped_feature_list_.InitAndDisableFeature(features::kOnBeginFrameAcks);
+ }
+}
+
+void OnBeginFrameAcksFrameSinkBundleImplTest::MaybeIssueOnBeginFrame() {
+ if (!BeginFrameAcksEnabled()) {
+ return;
+ }
+ IssueOnBeginFrame();
+}
+
+TEST_P(OnBeginFrameAcksFrameSinkBundleImplTest, SubmitAndAck) {
TestFrameSink frame_a(manager(), kSubFrameA, kMainFrame, kBundleId);
TestFrameSink frame_b(manager(), kSubFrameB, kMainFrame, kBundleId);
TestFrameSink frame_c(manager(), kSubFrameC, kMainFrame, kBundleId);
@@ -357,13 +395,22 @@
bundle()->Submit(std::move(submissions));
std::vector<mojom::BundledReturnedResourcesPtr> acks;
- test_client().WaitForNextFlush(&acks, nullptr, nullptr);
-
- EXPECT_THAT(acks, ElementsAre(ForSink(kSubFrameA), ForSink(kSubFrameB),
- ForSink(kSubFrameC)));
+ std::vector<mojom::BeginFrameInfoPtr> begin_frames;
+ MaybeIssueOnBeginFrame();
+ test_client().WaitForNextFlush(&acks, &begin_frames, nullptr);
+ if (BeginFrameAcksEnabled()) {
+ EXPECT_TRUE(acks.empty());
+ EXPECT_THAT(begin_frames,
+ UnorderedElementsAre(ForSink(kSubFrameA), ForSink(kSubFrameB),
+ ForSink(kSubFrameC)));
+ } else {
+ EXPECT_THAT(acks, ElementsAre(ForSink(kSubFrameA), ForSink(kSubFrameB),
+ ForSink(kSubFrameC)));
+ EXPECT_TRUE(begin_frames.empty());
+ }
}
-TEST_F(FrameSinkBundleImplTest, NoAckIfDidNotProduceFrame) {
+TEST_P(OnBeginFrameAcksFrameSinkBundleImplTest, NoAckIfDidNotProduceFrame) {
TestFrameSink frame_a(manager(), kSubFrameA, kMainFrame, kBundleId);
TestFrameSink frame_b(manager(), kSubFrameB, kMainFrame, kBundleId);
TestFrameSink frame_c(manager(), kSubFrameC, kMainFrame, kBundleId);
@@ -376,11 +423,21 @@
bundle()->Submit(std::move(submissions));
std::vector<mojom::BundledReturnedResourcesPtr> acks;
- test_client().WaitForNextFlush(&acks, nullptr, nullptr);
- EXPECT_THAT(acks, ElementsAre(ForSink(kSubFrameA), ForSink(kSubFrameC)));
+ std::vector<mojom::BeginFrameInfoPtr> begin_frames;
+ MaybeIssueOnBeginFrame();
+ test_client().WaitForNextFlush(&acks, &begin_frames, nullptr);
+ if (BeginFrameAcksEnabled()) {
+ EXPECT_TRUE(acks.empty());
+ EXPECT_THAT(begin_frames,
+ UnorderedElementsAre(ForSink(kSubFrameA), ForSink(kSubFrameB),
+ ForSink(kSubFrameC)));
+ } else {
+ EXPECT_THAT(acks, ElementsAre(ForSink(kSubFrameA), ForSink(kSubFrameC)));
+ EXPECT_TRUE(begin_frames.empty());
+ }
}
-TEST_F(FrameSinkBundleImplTest, ReclaimResourcesOnAck) {
+TEST_P(OnBeginFrameAcksFrameSinkBundleImplTest, ReclaimResourcesOnAck) {
TestFrameSink frame_a(manager(), kSubFrameA, kMainFrame, kBundleId);
TestFrameSink frame_b(manager(), kSubFrameB, kMainFrame, kBundleId);
TestFrameSink frame_c(manager(), kSubFrameC, kMainFrame, kBundleId);
@@ -393,9 +450,19 @@
bundle()->Submit(std::move(submissions));
std::vector<mojom::BundledReturnedResourcesPtr> acks;
- test_client().WaitForNextFlush(&acks, nullptr, nullptr);
- EXPECT_THAT(acks, ElementsAre(ForSink(kSubFrameA), ForSink(kSubFrameB),
- ForSink(kSubFrameC)));
+ std::vector<mojom::BeginFrameInfoPtr> begin_frames;
+ MaybeIssueOnBeginFrame();
+ test_client().WaitForNextFlush(&acks, &begin_frames, nullptr);
+ if (BeginFrameAcksEnabled()) {
+ EXPECT_TRUE(acks.empty());
+ EXPECT_THAT(begin_frames,
+ UnorderedElementsAre(ForSink(kSubFrameA), ForSink(kSubFrameB),
+ ForSink(kSubFrameC)));
+ } else {
+ EXPECT_THAT(acks, ElementsAre(ForSink(kSubFrameA), ForSink(kSubFrameB),
+ ForSink(kSubFrameC)));
+ EXPECT_TRUE(begin_frames.empty());
+ }
// Now frame C will submit with resources to a dead surface and be rejected
// immediately. This should result in an ack which immediately returns the
@@ -408,12 +475,42 @@
bundle()->Submit(std::move(submissions));
acks.clear();
- test_client().WaitForNextFlush(&acks, nullptr, nullptr);
- EXPECT_THAT(acks, ElementsAre(ForSink(kSubFrameC)));
+ begin_frames.clear();
+ MaybeIssueOnBeginFrame();
+ test_client().WaitForNextFlush(&acks, &begin_frames, nullptr);
+ if (BeginFrameAcksEnabled()) {
+ // Without the OnBeginFrame there is no message waiting to flush. While
+ // `bundle()->Submit` executes during this RunLoop, the resources won't have
+ // been acked by the time we issue the OnBeginFrame.
+ EXPECT_THAT(begin_frames,
+ UnorderedElementsAre(ForSink(kSubFrameA), ForSink(kSubFrameB),
+ ForSink(kSubFrameC)));
- EXPECT_EQ(kSubFrameC.sink_id(), acks[0]->sink_id);
- EXPECT_THAT(acks[0]->resources, ElementsAre(ForResource(resource)));
+ // Resources are returned as a part of future OnBeginFrames, after the
+ // frame sink has internally Acked the frame. The `Submit` above will have
+ // enqueued the resources to return.
+ begin_frames.clear();
+ IssueOnBeginFrame();
+ test_client().WaitForNextFlush(nullptr, &begin_frames, nullptr);
+ EXPECT_THAT(begin_frames,
+ UnorderedElementsAre(ForSink(kSubFrameA), ForSink(kSubFrameB),
+ ForSink(kSubFrameC)));
+ EXPECT_EQ(kSubFrameC.sink_id(), begin_frames[2]->sink_id);
+ EXPECT_THAT(begin_frames[2]->resources, ElementsAre(ForResource(resource)));
+ } else {
+ EXPECT_THAT(acks, ElementsAre(ForSink(kSubFrameC)));
+
+ EXPECT_EQ(kSubFrameC.sink_id(), acks[0]->sink_id);
+ EXPECT_THAT(acks[0]->resources, ElementsAre(ForResource(resource)));
+ }
}
+INSTANTIATE_TEST_SUITE_P(,
+ OnBeginFrameAcksFrameSinkBundleImplTest,
+ testing::Bool(),
+ [](auto& info) {
+ return info.param ? "BeginFrameAcks"
+ : "CompositoFrameAcks";
+ });
} // namespace
} // namespace viz
diff --git a/components/viz/service/frame_sinks/surface_synchronization_unittest.cc b/components/viz/service/frame_sinks/surface_synchronization_unittest.cc
index 13a412c..db39eeb 100644
--- a/components/viz/service/frame_sinks/surface_synchronization_unittest.cc
+++ b/components/viz/service/frame_sinks/surface_synchronization_unittest.cc
@@ -3,7 +3,9 @@
// found in the LICENSE file.
#include "base/containers/flat_set.h"
+#include "base/test/scoped_feature_list.h"
#include "base/test/simple_test_tick_clock.h"
+#include "components/viz/common/features.h"
#include "components/viz/common/surfaces/surface_id.h"
#include "components/viz/service/display_embedder/server_shared_bitmap_manager.h"
#include "components/viz/service/frame_sinks/compositor_frame_sink_support.h"
@@ -37,6 +39,8 @@
constexpr FrameSinkId kChildFrameSink2(65564, 0);
constexpr FrameSinkId kArbitraryFrameSink(1337, 7331);
+const uint64_t kBeginFrameSourceId = 1337;
+
std::vector<SurfaceId> empty_surface_ids() {
return std::vector<SurfaceId>();
}
@@ -52,6 +56,7 @@
return CompositorFrameBuilder()
.AddDefaultRenderPass()
.SetActivationDependencies(std::move(activation_dependencies))
+ .SetBeginFrameSourceId(kBeginFrameSourceId)
.SetReferencedSurfaces(std::move(referenced_surfaces))
.SetTransferableResources(std::move(resource_list))
.SetDeadline(deadline)
@@ -278,8 +283,9 @@
const SurfaceId display_id_second = MakeSurfaceId(kDisplayFrameSink, 2);
// Submit a CompositorFrame for the first display root surface.
- display_support().SubmitCompositorFrame(display_id_first.local_surface_id(),
- MakeDefaultCompositorFrame());
+ display_support().SubmitCompositorFrame(
+ display_id_first.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// A surface reference from the top-level root is added and there shouldn't be
// a temporary reference.
@@ -289,8 +295,9 @@
UnorderedElementsAre(display_id_first));
// Submit a CompositorFrame for the second display root surface.
- display_support().SubmitCompositorFrame(display_id_second.local_surface_id(),
- MakeDefaultCompositorFrame());
+ display_support().SubmitCompositorFrame(
+ display_id_second.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// A surface reference from the top-level root to |display_id_second| should
// be added and the reference to |display_root_first| removed.
@@ -325,8 +332,9 @@
// Submit a CompositorFrame without any dependencies to |child_id1|.
// parent_support should now only be blocked on |child_id2|.
- child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id1.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
EXPECT_TRUE(parent_surface()->has_deadline());
EXPECT_FALSE(parent_surface()->HasActiveFrame());
@@ -336,8 +344,9 @@
// Submit a CompositorFrame without any dependencies to |child_id2|.
// parent_support should be activated.
- child_support2().SubmitCompositorFrame(child_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support2().SubmitCompositorFrame(
+ child_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
EXPECT_FALSE(child_surface2()->has_deadline());
EXPECT_TRUE(parent_surface()->HasActiveFrame());
@@ -447,8 +456,9 @@
// Submit a CompositorFrame without any dependencies to |child_id2|.
// parent_support should be activated.
- child_support2().SubmitCompositorFrame(child_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support2().SubmitCompositorFrame(
+ child_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
EXPECT_FALSE(child_surface2()->has_deadline());
@@ -574,8 +584,9 @@
EXPECT_THAT(parent_surface()->activation_dependencies(),
UnorderedElementsAre(child_id1));
- child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id1.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// parent_surface has been activated.
EXPECT_TRUE(parent_surface()->HasActiveFrame());
@@ -631,8 +642,9 @@
UnorderedElementsAre(arbitrary_id));
// Submit a CompositorFrame that has no dependencies.
- parent_support().SubmitCompositorFrame(parent_id.local_surface_id(),
- MakeDefaultCompositorFrame());
+ parent_support().SubmitCompositorFrame(
+ parent_id.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Verify that the CompositorFrame has been activated.
EXPECT_TRUE(parent_surface()->HasActiveFrame());
@@ -640,19 +652,48 @@
EXPECT_THAT(parent_surface()->activation_dependencies(), IsEmpty());
}
+// Supports testing features::OnBeginFrameAcks, which changes the expectations
+// of what IPCs are sent to the CompositorFrameSinkClient. When enabled
+// OnBeginFrame also handles ReturnResources as well as
+// DidReceiveCompositorFrameAck.
+class OnBeginFrameAcksSurfaceSynchronizationTest
+ : public SurfaceSynchronizationTest,
+ public testing::WithParamInterface<bool> {
+ public:
+ OnBeginFrameAcksSurfaceSynchronizationTest();
+ ~OnBeginFrameAcksSurfaceSynchronizationTest() override = default;
+
+ bool BeginFrameAcksEnabled() const { return GetParam(); }
+
+ private:
+ base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+OnBeginFrameAcksSurfaceSynchronizationTest::
+ OnBeginFrameAcksSurfaceSynchronizationTest() {
+ if (BeginFrameAcksEnabled()) {
+ scoped_feature_list_.InitAndEnableFeature(features::kOnBeginFrameAcks);
+ } else {
+ scoped_feature_list_.InitAndDisableFeature(features::kOnBeginFrameAcks);
+ }
+}
+
// This test verifies that a pending CompositorFrame does not affect surface
// references. A new surface from a child will continue to exist as a temporary
// reference until the parent's frame activates.
-TEST_F(SurfaceSynchronizationTest, OnlyActiveFramesAffectSurfaceReferences) {
+TEST_P(OnBeginFrameAcksSurfaceSynchronizationTest,
+ OnlyActiveFramesAffectSurfaceReferences) {
const SurfaceId parent_id = MakeSurfaceId(kParentFrameSink, 1);
const SurfaceId child_id1 = MakeSurfaceId(kChildFrameSink1, 1);
const SurfaceId child_id2 = MakeSurfaceId(kChildFrameSink2, 1);
// child_support1 submits a CompositorFrame without any dependencies.
// DidReceiveCompositorFrameAck should call on immediate activation.
- EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_)).Times(1);
- child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
- MakeDefaultCompositorFrame());
+ EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_))
+ .Times(BeginFrameAcksEnabled() ? 0 : 1);
+ child_support1().SubmitCompositorFrame(
+ child_id1.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
testing::Mock::VerifyAndClearExpectations(&support_client_);
// Verify that the child surface is not blocked.
@@ -687,9 +728,11 @@
// child_support2 submits a CompositorFrame without any dependencies.
// Both the child and the parent should immediately ACK CompositorFrames
// on activation.
- EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_)).Times(2);
- child_support2().SubmitCompositorFrame(child_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_))
+ .Times(BeginFrameAcksEnabled() ? 0 : 2);
+ child_support2().SubmitCompositorFrame(
+ child_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
testing::Mock::VerifyAndClearExpectations(&support_client_);
// Verify that the child surface is not blocked.
@@ -709,7 +752,7 @@
// This test verifies that we do not double count returned resources when a
// CompositorFrame starts out as pending, then becomes active, and then is
// replaced with another active CompositorFrame.
-TEST_F(SurfaceSynchronizationTest, ResourcesOnlyReturnedOnce) {
+TEST_P(OnBeginFrameAcksSurfaceSynchronizationTest, ResourcesOnlyReturnedOnce) {
const SurfaceId parent_id = MakeSurfaceId(kParentFrameSink, 1);
const SurfaceId child_id = MakeSurfaceId(kChildFrameSink1, 1);
@@ -747,13 +790,17 @@
EXPECT_FALSE(parent_surface()->HasPendingFrame());
EXPECT_THAT(parent_surface()->activation_dependencies(), IsEmpty());
- std::vector<ReturnedResource> returned_resources;
- ResourceId id = resource.ToReturnedResource().id;
- EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_))
- .WillOnce([=](std::vector<ReturnedResource> got) {
- EXPECT_EQ(1u, got.size());
- EXPECT_EQ(id, got[0].id);
- });
+ if (BeginFrameAcksEnabled()) {
+ EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_)).Times(0);
+ } else {
+ std::vector<ReturnedResource> returned_resources;
+ ResourceId id = resource.ToReturnedResource().id;
+ EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_))
+ .WillOnce([=](std::vector<ReturnedResource> got) {
+ EXPECT_EQ(1u, got.size());
+ EXPECT_EQ(id, got[0].id);
+ });
+ }
// The parent submits a CompositorFrame without any dependencies. That
// frame should activate immediately, replacing the earlier frame. The
@@ -765,6 +812,18 @@
EXPECT_TRUE(parent_surface()->HasActiveFrame());
EXPECT_FALSE(parent_surface()->HasPendingFrame());
EXPECT_THAT(parent_surface()->activation_dependencies(), IsEmpty());
+
+ if (BeginFrameAcksEnabled()) {
+ ResourceId id = resource.ToReturnedResource().id;
+ EXPECT_CALL(support_client_, OnBeginFrame(_, _, _, _))
+ .WillOnce([=](const BeginFrameArgs& args,
+ const FrameTimingDetailsMap& timing_details,
+ bool frame_ack, std::vector<ReturnedResource> got) {
+ EXPECT_EQ(1u, got.size());
+ EXPECT_EQ(id, got[0].id);
+ });
+ SendNextBeginFrame();
+ }
}
// This test verifies that if a surface has both a pending and active
@@ -772,7 +831,8 @@
// the existing active CompositorFrame, then the surface reference hierarchy
// will be updated allowing garbage collection of surfaces that are no longer
// referenced.
-TEST_F(SurfaceSynchronizationTest, DropStaleReferencesAfterActivation) {
+TEST_P(OnBeginFrameAcksSurfaceSynchronizationTest,
+ DropStaleReferencesAfterActivation) {
const SurfaceId parent_id = MakeSurfaceId(kParentFrameSink, 1);
const SurfaceId child_id1 = MakeSurfaceId(kChildFrameSink1, 1);
const SurfaceId child_id2 = MakeSurfaceId(kChildFrameSink2, 1);
@@ -798,9 +858,11 @@
// DidReceiveCompositorFrameAck should get called twice: once for the child
// and once for the now active parent CompositorFrame.
- EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_)).Times(2);
- child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
- MakeDefaultCompositorFrame());
+ EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_))
+ .Times(BeginFrameAcksEnabled() ? 0 : 2);
+ child_support1().SubmitCompositorFrame(
+ child_id1.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
testing::Mock::VerifyAndClearExpectations(&support_client_);
// Verify that the child CompositorFrame activates immediately.
@@ -848,8 +910,9 @@
UnorderedElementsAre(child_id2));
EXPECT_THAT(GetChildReferences(parent_id), UnorderedElementsAre(child_id1));
- child_support2().SubmitCompositorFrame(child_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support2().SubmitCompositorFrame(
+ child_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Verify that the parent Surface has activated and no longer has a
// pending CompositorFrame. Also verify that |child_id1| is no longer a
@@ -1001,7 +1064,7 @@
ui::LatencyInfo info;
info.AddLatencyNumber(latency_type1);
- CompositorFrame frame = MakeDefaultCompositorFrame();
+ CompositorFrame frame = MakeDefaultCompositorFrame(kBeginFrameSourceId);
frame.metadata.latency_info.push_back(info);
parent_support().SubmitCompositorFrame(parent_id1.local_surface_id(),
@@ -1025,8 +1088,9 @@
EXPECT_TRUE(old_surface->HasPendingFrame());
// Submit a frame with a new local surface id.
- parent_support().SubmitCompositorFrame(parent_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ parent_support().SubmitCompositorFrame(
+ parent_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Verify that the new surface has an active frame only.
Surface* surface = GetSurfaceForId(parent_id2);
@@ -1075,7 +1139,7 @@
ui::LatencyInfo info;
info.AddLatencyNumber(latency_type1);
- CompositorFrame frame = MakeDefaultCompositorFrame();
+ CompositorFrame frame = MakeDefaultCompositorFrame(kBeginFrameSourceId);
frame.metadata.latency_info.push_back(info);
parent_support().SubmitCompositorFrame(parent_id1.local_surface_id(),
@@ -1106,8 +1170,9 @@
EXPECT_FALSE(surface->HasActiveFrame());
// Resolve the dependencies. The frame in parent's surface must become active.
- child_support1().SubmitCompositorFrame(child_id.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
EXPECT_FALSE(surface->HasPendingFrame());
EXPECT_TRUE(surface->HasActiveFrame());
@@ -1181,8 +1246,9 @@
EXPECT_TRUE(old_surface2->HasPendingFrame());
// Submit a frame with no dependencies to the new surface parent_id3.
- parent_support().SubmitCompositorFrame(parent_id3.local_surface_id(),
- MakeDefaultCompositorFrame());
+ parent_support().SubmitCompositorFrame(
+ parent_id3.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Verify that the new surface has an active frame only.
Surface* surface = GetSurfaceForId(parent_id3);
@@ -1281,7 +1347,7 @@
}
// Checks that resources and ack are sent together if possible.
-TEST_F(SurfaceSynchronizationTest, ReturnResourcesWithAck) {
+TEST_P(OnBeginFrameAcksSurfaceSynchronizationTest, ReturnResourcesWithAck) {
const SurfaceId parent_id = MakeSurfaceId(kParentFrameSink, 1);
TransferableResource resource;
resource.id = ResourceId(1234);
@@ -1291,18 +1357,33 @@
{resource}));
ResourceId id = resource.ToReturnedResource().id;
EXPECT_CALL(support_client_, ReclaimResources(_)).Times(0);
- EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_))
- .WillOnce([=](std::vector<ReturnedResource> got) {
- EXPECT_EQ(1u, got.size());
- EXPECT_EQ(id, got[0].id);
- });
- parent_support().SubmitCompositorFrame(parent_id.local_surface_id(),
- MakeDefaultCompositorFrame());
+ if (BeginFrameAcksEnabled()) {
+ EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_)).Times(0);
+ } else {
+ EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_))
+ .WillOnce([=](std::vector<ReturnedResource> got) {
+ EXPECT_EQ(1u, got.size());
+ EXPECT_EQ(id, got[0].id);
+ });
+ }
+ parent_support().SubmitCompositorFrame(
+ parent_id.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
+ if (BeginFrameAcksEnabled()) {
+ EXPECT_CALL(support_client_, OnBeginFrame(_, _, _, _))
+ .WillOnce([=](const BeginFrameArgs& args,
+ const FrameTimingDetailsMap& timing_details,
+ bool frame_ack, std::vector<ReturnedResource> got) {
+ EXPECT_EQ(1u, got.size());
+ EXPECT_EQ(id, got[0].id);
+ });
+ SendNextBeginFrame();
+ }
}
// Verifies that arrival of a new CompositorFrame doesn't change the fact that a
// surface is marked for destruction.
-TEST_F(SurfaceSynchronizationTest, SubmitToDestroyedSurface) {
+TEST_P(OnBeginFrameAcksSurfaceSynchronizationTest, SubmitToDestroyedSurface) {
const SurfaceId parent_id = MakeSurfaceId(kParentFrameSink, 1);
const SurfaceId child_id = MakeSurfaceId(kChildFrameSink1, 3);
@@ -1335,17 +1416,20 @@
// destroyed. The frame is immediately rejected.
{
EXPECT_CALL(support_client_, ReclaimResources(_)).Times(0);
- EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_));
+ EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_))
+ .Times(BeginFrameAcksEnabled() ? 0 : 1);
surface_observer().Reset();
- child_support1().SubmitCompositorFrame(child_id.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
testing::Mock::VerifyAndClearExpectations(&support_client_);
}
// The parent stops referencing the child surface. This allows the child
// surface to be garbage collected.
- parent_support().SubmitCompositorFrame(parent_id.local_surface_id(),
- MakeDefaultCompositorFrame());
+ parent_support().SubmitCompositorFrame(
+ parent_id.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
{
ResourceId id = resource.ToReturnedResource().id;
@@ -1370,8 +1454,9 @@
const SurfaceId child_id = MakeSurfaceId(kChildFrameSink1, 3);
// Submit the first frame. Creates the surface.
- child_support1().SubmitCompositorFrame(child_id.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
EXPECT_NE(nullptr, GetSurfaceForId(child_id));
// Add a reference from parent.
@@ -1381,8 +1466,9 @@
std::vector<TransferableResource>()));
// Remove the reference from parant. This allows us to destroy the surface.
- parent_support().SubmitCompositorFrame(parent_id.local_surface_id(),
- MakeDefaultCompositorFrame());
+ parent_support().SubmitCompositorFrame(
+ parent_id.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Destroy the surface.
child_support1().EvictSurface(child_id.local_surface_id());
@@ -1392,8 +1478,9 @@
// Submit another frame with the same local surface id. The surface should not
// be recreated.
- child_support1().SubmitCompositorFrame(child_id.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
EXPECT_EQ(nullptr, GetSurfaceForId(child_id));
}
@@ -1455,8 +1542,9 @@
// Submitting a CompositorFrame will trigger garbage collection of the
// |parent_id1| subtree. This should not crash.
- child_support1().SubmitCompositorFrame(child_id.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
}
// This test verifies that a crash does not occur if garbage collection is
@@ -1545,8 +1633,9 @@
// Submitting a CompositorFrame with |parent_id2| so that the display
// CompositorFrame can hold a reference to it.
- parent_support().SubmitCompositorFrame(parent_id1.local_surface_id(),
- MakeDefaultCompositorFrame());
+ parent_support().SubmitCompositorFrame(
+ parent_id1.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
display_support().SubmitCompositorFrame(
display_id.local_surface_id(),
@@ -1567,8 +1656,9 @@
// Submitting a CompositorFrame with |parent_id2| should unblock the
// display CompositorFrame.
- parent_support().SubmitCompositorFrame(parent_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ parent_support().SubmitCompositorFrame(
+ parent_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
EXPECT_FALSE(display_surface()->has_deadline());
EXPECT_FALSE(display_surface()->HasPendingFrame());
@@ -1675,7 +1765,7 @@
// This test verifies that CompositorFrames submitted to a surface referenced
// by a parent CompositorFrame as a fallback will be ACK'ed immediately.
-TEST_F(SurfaceSynchronizationTest, FallbackSurfacesClosed) {
+TEST_P(OnBeginFrameAcksSurfaceSynchronizationTest, FallbackSurfacesClosed) {
const SurfaceId parent_id1 = MakeSurfaceId(kParentFrameSink, 1);
// This is the fallback child surface that the parent holds a reference to.
const SurfaceId child_id1 = MakeSurfaceId(kChildFrameSink1, 1);
@@ -1701,7 +1791,8 @@
// The parent is blocked on |child_id2| and references |child_id1|.
// |child_id1| should immediately activate and the ack must be sent.
- EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_));
+ EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_))
+ .Times(BeginFrameAcksEnabled() ? 0 : 1);
parent_support().SubmitCompositorFrame(
parent_id1.local_surface_id(),
MakeCompositorFrame({child_id2}, {SurfaceRange(child_id1, child_id2)},
@@ -1718,7 +1809,8 @@
// immediately so that the child can submit another frame and catch up with
// the parent.
SendNextBeginFrame();
- EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_));
+ EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_))
+ .Times(BeginFrameAcksEnabled() ? 0 : 1);
child_support1().SubmitCompositorFrame(
child_id1.local_surface_id(),
MakeCompositorFrame({arbitrary_id},
@@ -1736,13 +1828,15 @@
const SurfaceId child_id2 = MakeSurfaceId(kChildFrameSink2, 1);
const SurfaceId arbitrary_id = MakeSurfaceId(kArbitraryFrameSink, 1);
- child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id1.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
EXPECT_FALSE(child_surface1()->HasPendingFrame());
EXPECT_TRUE(child_surface1()->HasActiveFrame());
- child_support2().SubmitCompositorFrame(child_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support2().SubmitCompositorFrame(
+ child_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
EXPECT_FALSE(child_surface2()->HasPendingFrame());
EXPECT_TRUE(child_surface2()->HasActiveFrame());
@@ -1983,8 +2077,9 @@
const SurfaceId parent_id = MakeSurfaceId(kParentFrameSink, 1);
const SurfaceId child_id = MakeSurfaceId(kChildFrameSink1, 1);
- parent_support().SubmitCompositorFrame(parent_id.local_surface_id(),
- MakeDefaultCompositorFrame());
+ parent_support().SubmitCompositorFrame(
+ parent_id.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
EXPECT_FALSE(parent_surface()->has_deadline());
EXPECT_TRUE(parent_surface()->HasActiveFrame());
@@ -2022,8 +2117,9 @@
EXPECT_NE(nullptr, parent_surface);
// Submitting a new CompositorFrame to the display should free the parent.
- display_support().SubmitCompositorFrame(display_id.local_surface_id(),
- MakeDefaultCompositorFrame());
+ display_support().SubmitCompositorFrame(
+ display_id.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
frame_sink_manager().surface_manager()->GarbageCollectSurfaces();
@@ -2125,10 +2221,12 @@
EXPECT_FALSE(parent_surface()->HasActiveFrame());
EXPECT_EQ(0u, parent_surface()->GetActiveFrameIndex());
- child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
- MakeDefaultCompositorFrame());
- child_support2().SubmitCompositorFrame(child_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id1.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
+ child_support2().SubmitCompositorFrame(
+ child_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
EXPECT_TRUE(parent_surface()->HasActiveFrame());
uint64_t expected_index = kFrameIndexStart;
EXPECT_EQ(expected_index, parent_surface()->GetActiveFrameIndex());
@@ -2147,8 +2245,9 @@
const SurfaceId child_id3 = MakeSurfaceId(kChildFrameSink1, 2, 2);
const SurfaceId child_id4 = MakeSurfaceId(kChildFrameSink1, 2, 3);
- child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id1.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
parent_support().SubmitCompositorFrame(
parent_id.local_surface_id(),
@@ -2191,8 +2290,9 @@
// Submit a child CompositorFrame to a new SurfaceId and verify that
// GetLatestInFlightSurface returns the right surface.
- child_support1().SubmitCompositorFrame(child_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Verify that there is a temporary reference for child_id2 and there is
// a reference from the parent to child_id1.
@@ -2209,8 +2309,9 @@
// Submit a child CompositorFrame to a new SurfaceId and verify that
// GetLatestInFlightSurface returns the right surface.
- child_support1().SubmitCompositorFrame(child_id3.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id3.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Verify that there is a temporary reference for child_id3.
EXPECT_TRUE(HasTemporaryReference(child_id3));
@@ -2239,8 +2340,9 @@
const SurfaceId child_id1 = MakeSurfaceId(kChildFrameSink1, 1);
const SurfaceId child_id2 = MakeSurfaceId(kChildFrameSink1, 2);
- child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id1.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
parent_support().SubmitCompositorFrame(
parent_id.local_surface_id(),
@@ -2275,8 +2377,9 @@
const SurfaceId child_id1 = MakeSurfaceId(kChildFrameSink1, 1);
const SurfaceId child_id2 = MakeSurfaceId(kChildFrameSink1, 2);
- child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id1.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Verify that |child_id1| is active.
EXPECT_TRUE(child_surface1()->HasActiveFrame());
EXPECT_FALSE(child_surface1()->HasPendingFrame());
@@ -2302,8 +2405,9 @@
GetLatestInFlightSurface(SurfaceRange(absl::nullopt, child_id2)));
// Activate |child_id2|
- child_support1().SubmitCompositorFrame(child_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Verify that child2 is active.
EXPECT_TRUE(child_surface1()->HasActiveFrame());
EXPECT_FALSE(child_surface1()->HasPendingFrame());
@@ -2329,8 +2433,9 @@
const SurfaceId child_id4 = MakeSurfaceId(kChildFrameSink1, 4);
// Activate |child_id1|.
- child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id1.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Verify that |child_id1| CompositorFrames is active.
EXPECT_TRUE(child_surface1()->HasActiveFrame());
@@ -2352,8 +2457,9 @@
EXPECT_FALSE(HasTemporaryReference(child_id1));
// Activate |child_id2|.
- child_support1().SubmitCompositorFrame(child_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Verify that |child_id2| CompositorFrames is active and it has a temporary
// reference.
@@ -2363,8 +2469,9 @@
EXPECT_TRUE(HasTemporaryReference(child_id2));
// Activate |child_id3|.
- child_support1().SubmitCompositorFrame(child_id3.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id3.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Verify that |child_id3| CompositorFrames is active.
EXPECT_TRUE(child_surface1()->HasActiveFrame());
@@ -2410,12 +2517,14 @@
const SurfaceId child_id4 = MakeSurfaceId(kChildFrameSink2, 2);
// Activate |child_id1|.
- child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id1.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Activate |child_id2|.
- child_support1().SubmitCompositorFrame(child_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
parent_support().SubmitCompositorFrame(
parent_id.local_surface_id(),
@@ -2428,8 +2537,9 @@
GetLatestInFlightSurface(SurfaceRange(child_id1, child_id4)));
// Activate |child_id3| which is in different frame sink.
- child_support2().SubmitCompositorFrame(child_id3.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support2().SubmitCompositorFrame(
+ child_id3.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// |child_id3| is the latest in primary's frame sink.
EXPECT_EQ(GetSurfaceForId(child_id3),
@@ -2444,8 +2554,9 @@
const SurfaceId child_id2 = MakeSurfaceId(kChildFrameSink1, 2);
const SurfaceId child_id3 = MakeSurfaceId(kChildFrameSink1, 3);
- child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id1.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Create a reference from |parent_id| to |child_id|.
parent_support().SubmitCompositorFrame(
@@ -2453,14 +2564,16 @@
MakeCompositorFrame(empty_surface_ids(), {SurfaceRange(child_id1)},
std::vector<TransferableResource>()));
- child_support1().SubmitCompositorFrame(child_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
EXPECT_EQ(GetSurfaceForId(child_id2),
GetLatestInFlightSurface(SurfaceRange(child_id1, child_id3)));
- child_support1().SubmitCompositorFrame(child_id3.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id3.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// GetLatestInFlightSurface will return the primary surface ID if it's
// available.
@@ -2478,8 +2591,9 @@
const SurfaceId child_id3 = MakeSurfaceId(kChildFrameSink1, 3);
// Activate |child_id1|.
- child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id1.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// |child_id1| now should have a temporary reference.
EXPECT_TRUE(HasTemporaryReference(child_id1));
@@ -2488,8 +2602,9 @@
.empty());
// Activate |child_id2|.
- child_support1().SubmitCompositorFrame(child_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// |child_id2| now should have a temporary reference.
EXPECT_TRUE(HasTemporaryReference(child_id2));
@@ -2538,8 +2653,9 @@
const SurfaceId child_id5 =
SurfaceId(kChildFrameSink1, LocalSurfaceId(5, nonce3));
- child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id1.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Create a reference from |parent_id| to |child_id|.
parent_support().SubmitCompositorFrame(
@@ -2547,14 +2663,16 @@
MakeCompositorFrame(empty_surface_ids(), {SurfaceRange(child_id1)},
std::vector<TransferableResource>()));
- child_support1().SubmitCompositorFrame(child_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
EXPECT_EQ(GetSurfaceForId(child_id2),
GetLatestInFlightSurface(SurfaceRange(child_id1, child_id4)));
- child_support1().SubmitCompositorFrame(child_id3.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id3.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// GetLatestInFlightSurface will return child_id3 because the nonce
// matches that of child_id4.
@@ -2695,8 +2813,9 @@
const SurfaceId arbitrary_id = MakeSurfaceId(kArbitraryFrameSink, 1);
// Submit a CompositorFrame that has no dependencies.
- parent_support().SubmitCompositorFrame(parent_id1.local_surface_id(),
- MakeDefaultCompositorFrame());
+ parent_support().SubmitCompositorFrame(
+ parent_id1.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Verify that the CompositorFrame has been activated.
Surface* parent_surface1 = GetSurfaceForId(parent_id1);
@@ -2748,8 +2867,9 @@
// The parent CompositorFrame is not blocked on anything and so it should
// immediately activate.
EXPECT_FALSE(parent_support().last_activated_surface_id().is_valid());
- parent_support().SubmitCompositorFrame(parent_id.local_surface_id(),
- MakeDefaultCompositorFrame());
+ parent_support().SubmitCompositorFrame(
+ parent_id.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Verify that the parent CompositorFrame has activated.
EXPECT_FALSE(parent_surface()->has_deadline());
@@ -2781,8 +2901,9 @@
// The CompositorFrame in the evicted |parent_id| activates here because it
// was blocked on |child_id1|.
- child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id1.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// parent_support will be informed of the activation of a CompositorFrame
// associated with |parent_id|, but we clear |last_active_surface_id_| because
@@ -2796,8 +2917,9 @@
// This should not crash as the previous surface was cleared in
// CompositorFrameSinkSupport.
- parent_support().SubmitCompositorFrame(parent_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ parent_support().SubmitCompositorFrame(
+ parent_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
}
// This test verifies that when a surface activates that has the same
@@ -2822,8 +2944,9 @@
EXPECT_THAT(GetReferencesFrom(parent_id), empty_surface_ids());
// Activate |child_id2|.
- child_support2().SubmitCompositorFrame(child_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support2().SubmitCompositorFrame(
+ child_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Since |child_id2| has a different embed token than both primary and
// fallback, it should not be used as a reference even if it has the same
@@ -2831,8 +2954,9 @@
EXPECT_THAT(GetReferencesFrom(parent_id), empty_surface_ids());
// Activate |child_id3|.
- child_support2().SubmitCompositorFrame(child_id3.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support2().SubmitCompositorFrame(
+ child_id3.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Verify that a reference is acquired.
EXPECT_THAT(GetReferencesFrom(parent_id), UnorderedElementsAre(child_id3));
@@ -2859,29 +2983,33 @@
EXPECT_THAT(GetReferencesFrom(parent_id), empty_surface_ids());
// Activate |child_id1|.
- child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id1.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Verify that a reference is acquired.
EXPECT_THAT(GetReferencesFrom(parent_id), UnorderedElementsAre(child_id1));
// Activate |child_id2|.
- child_support1().SubmitCompositorFrame(child_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Verify that the reference is updated.
EXPECT_THAT(GetReferencesFrom(parent_id), UnorderedElementsAre(child_id2));
// Activate |child_id4| in a different frame sink.
- child_support2().SubmitCompositorFrame(child_id4.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support2().SubmitCompositorFrame(
+ child_id4.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Verify that the reference is updated.
EXPECT_THAT(GetReferencesFrom(parent_id), UnorderedElementsAre(child_id4));
// Activate |child_id3|.
- child_support1().SubmitCompositorFrame(child_id3.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id3.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Verify that the reference will not get updated since |child_id3| is in the
// fallback's FrameSinkId.
@@ -2921,8 +3049,9 @@
const SurfaceId child_id3 = MakeSurfaceId(kChildFrameSink1, 2, 2);
// Submit a CompositorFrame to |child_id1|.
- child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id1.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Evict |child_id1|. It should get marked for destruction immediately.
child_support1().EvictSurface(child_id1.local_surface_id());
@@ -2931,14 +3060,16 @@
// Submit a CompositorFrame to |child_id2|. This CompositorFrame should be
// immediately rejected because |child_id2| has the same parent sequence
// number as |child_id1|.
- child_support1().SubmitCompositorFrame(child_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
EXPECT_EQ(nullptr, GetSurfaceForId(child_id2));
// Submit a CompositorFrame to |child_id3|. It should not be accepted and not
// marked for destruction.
- child_support1().SubmitCompositorFrame(child_id3.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id3.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
ASSERT_NE(nullptr, GetSurfaceForId(child_id3));
EXPECT_FALSE(IsMarkedForDestruction(child_id3));
}
@@ -2953,8 +3084,9 @@
const SurfaceId arbitrary_id = MakeSurfaceId(kArbitraryFrameSink, 1);
// Submit a CompositorFrame to |child_id1|.
- child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ child_id1.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// Child 1 should not yet be active.
Surface* child_surface1 = GetSurfaceForId(child_id1);
@@ -2999,8 +3131,9 @@
EXPECT_FALSE(surface_manager()->GetAllocationGroupForSurfaceId(surface_id));
// Submit a CompositorFrame to |child_id1|.
- child_support1().SubmitCompositorFrame(surface_id.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1().SubmitCompositorFrame(
+ surface_id.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
// The allocation group should now exist.
EXPECT_TRUE(surface_manager()->GetAllocationGroupForSurfaceId(surface_id));
@@ -3040,8 +3173,9 @@
// Now the parent unembeds the child surface. The allocation group for child
// surface should be marked for destruction. However, it won't get actually
// destroyed until garbage collection time.
- parent_support().SubmitCompositorFrame(parent_id.local_surface_id(),
- MakeDefaultCompositorFrame());
+ parent_support().SubmitCompositorFrame(
+ parent_id.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
ASSERT_TRUE(surface_manager()->GetAllocationGroupForSurfaceId(child_id));
EXPECT_TRUE(surface_manager()
->GetAllocationGroupForSurfaceId(child_id)
@@ -3083,8 +3217,9 @@
// Make |child_id2| available. The allocation group for |child_id1| should be
// marked for destruction.
EXPECT_FALSE(allocation_groups_need_garbage_collection());
- child_support2().SubmitCompositorFrame(child_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support2().SubmitCompositorFrame(
+ child_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
ASSERT_TRUE(surface_manager()->GetAllocationGroupForSurfaceId(child_id1));
EXPECT_TRUE(surface_manager()
->GetAllocationGroupForSurfaceId(child_id1)
@@ -3118,8 +3253,9 @@
EXPECT_FALSE(surface_manager()->GetAllocationGroupForSurfaceId(child_id2));
// Make |child_id2| available. An allocation group should be created for it.
- child_support2().SubmitCompositorFrame(child_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support2().SubmitCompositorFrame(
+ child_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
EXPECT_FALSE(surface_manager()->GetAllocationGroupForSurfaceId(child_id1));
EXPECT_TRUE(surface_manager()->GetAllocationGroupForSurfaceId(child_id2));
@@ -3142,13 +3278,15 @@
const SurfaceId parent_id1 = MakeSurfaceId(kParentFrameSink, 2);
const SurfaceId parent_id2(
kParentFrameSink, LocalSurfaceId(1, base::UnguessableToken::Create()));
- parent_support().SubmitCompositorFrame(parent_id1.local_surface_id(),
- MakeDefaultCompositorFrame());
+ parent_support().SubmitCompositorFrame(
+ parent_id1.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
Surface* parent_surface1 = GetSurfaceForId(parent_id1);
EXPECT_TRUE(parent_surface1->HasActiveFrame());
EXPECT_FALSE(parent_surface1->HasPendingFrame());
- parent_support().SubmitCompositorFrame(parent_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ parent_support().SubmitCompositorFrame(
+ parent_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
Surface* parent_surface2 = GetSurfaceForId(parent_id2);
EXPECT_TRUE(parent_surface2->HasActiveFrame());
EXPECT_FALSE(parent_surface2->HasPendingFrame());
@@ -3166,10 +3304,10 @@
const SurfaceId id2 = MakeSurfaceId(kParentFrameSink, 2, 2);
const SurfaceId id3 = MakeSurfaceId(kParentFrameSink, 1, 3);
- parent_support().SubmitCompositorFrame(id1.local_surface_id(),
- MakeDefaultCompositorFrame());
- parent_support().SubmitCompositorFrame(id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ parent_support().SubmitCompositorFrame(
+ id1.local_surface_id(), MakeDefaultCompositorFrame(kBeginFrameSourceId));
+ parent_support().SubmitCompositorFrame(
+ id2.local_surface_id(), MakeDefaultCompositorFrame(kBeginFrameSourceId));
EXPECT_EQ(GetSurfaceForId(id1),
GetLatestInFlightSurface(SurfaceRange(absl::nullopt, id3)));
}
@@ -3301,7 +3439,7 @@
// Embedder has submitted new ActivationDependencies, that it is immediately
// ACKed, even if normally it would not be due to damage. This way we don't have
// an Embedder blocked on an unACKed frame. (https://crbug.com/1203804)
-TEST_F(SurfaceSynchronizationTest,
+TEST_P(OnBeginFrameAcksSurfaceSynchronizationTest,
UnAckedOldActivationDependencyArrivesAfterNewDependencies) {
TestSurfaceIdAllocator parent_id(kParentFrameSink);
TestSurfaceIdAllocator child_id(kChildFrameSink1);
@@ -3359,7 +3497,8 @@
surface_observer().set_damage_display(true);
// Submitting a CompositorFrame to the old SurfaceId, which is no longer the
// dependency, should lead to an immediate ACK.
- EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_)).Times(1);
+ EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_))
+ .Times(BeginFrameAcksEnabled() ? 0 : 1);
child_support1().SubmitCompositorFrame(
old_child_id,
MakeCompositorFrame(empty_surface_ids(), empty_surface_ranges(),
@@ -3374,4 +3513,12 @@
EXPECT_FALSE(child_surface1()->HasUnackedActiveFrame());
}
+INSTANTIATE_TEST_SUITE_P(,
+ OnBeginFrameAcksSurfaceSynchronizationTest,
+ testing::Bool(),
+ [](auto& info) {
+ return info.param ? "BeginFrameAcks"
+ : "CompositoFrameAcks";
+ });
+
} // namespace viz
diff --git a/components/viz/service/surfaces/surface.cc b/components/viz/service/surfaces/surface.cc
index b38717e0..1e477e96 100644
--- a/components/viz/service/surfaces/surface.cc
+++ b/components/viz/service/surfaces/surface.cc
@@ -99,8 +99,6 @@
Surface::~Surface() {
ClearCopyRequests();
- if (surface_client_)
- surface_client_->OnSurfaceDestroyed(this);
surface_manager_->SurfaceDestroyed(this);
UnrefFrameResourcesAndRunCallbacks(std::move(pending_frame_data_));
@@ -120,6 +118,9 @@
"Surface", this, "surface_info",
surface_info_.ToString());
allocation_group_->UnregisterSurface(this);
+ if (surface_client_) {
+ surface_client_->OnSurfaceDestroyed(this);
+ }
}
void Surface::SetDependencyDeadline(
diff --git a/components/viz/service/surfaces/surface_client.h b/components/viz/service/surfaces/surface_client.h
index e83e4311e..0120676 100644
--- a/components/viz/service/surfaces/surface_client.h
+++ b/components/viz/service/surfaces/surface_client.h
@@ -47,7 +47,7 @@
// Called when |surface| has a new CompositorFrame available for display.
virtual void OnSurfaceActivated(Surface* surface) = 0;
- // Called when |surface| is about to be destroyed.
+ // Called when |surface| has completed destruction.
virtual void OnSurfaceDestroyed(Surface* surface) = 0;
// Called when a |surface| is about to be drawn.
diff --git a/components/viz/service/surfaces/surface_unittest.cc b/components/viz/service/surfaces/surface_unittest.cc
index 0f0f304..3fc231f2 100644
--- a/components/viz/service/surfaces/surface_unittest.cc
+++ b/components/viz/service/surfaces/surface_unittest.cc
@@ -6,7 +6,9 @@
#include "base/functional/bind.h"
#include "base/run_loop.h"
+#include "base/test/scoped_feature_list.h"
#include "cc/test/scheduler_test_common.h"
+#include "components/viz/common/features.h"
#include "components/viz/common/frame_sinks/copy_output_result.h"
#include "components/viz/common/surfaces/parent_local_surface_id_allocator.h"
#include "components/viz/common/surfaces/subtree_capture_id.h"
@@ -29,6 +31,7 @@
constexpr FrameSinkId kArbitraryFrameSinkId(1, 1);
constexpr bool kIsRoot = true;
+const uint64_t kBeginFrameSourceId = 1337;
class SurfaceTest : public testing::Test {
public:
@@ -41,7 +44,31 @@
FrameSinkManagerImpl frame_sink_manager_;
};
-TEST_F(SurfaceTest, PresentationCallback) {
+// Supports testing features::OnBeginFrameAcks, which changes the expectations
+// of what IPCs are sent to the CompositorFrameSinkClient. When enabled
+// OnBeginFrame also handles ReturnResources as well as
+// DidReceiveCompositorFrameAck.
+class OnBeginFrameAcksSurfaceTest : public SurfaceTest,
+ public testing::WithParamInterface<bool> {
+ public:
+ OnBeginFrameAcksSurfaceTest();
+ ~OnBeginFrameAcksSurfaceTest() override = default;
+
+ bool BeginFrameAcksEnabled() const { return GetParam(); }
+
+ private:
+ base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+OnBeginFrameAcksSurfaceTest::OnBeginFrameAcksSurfaceTest() {
+ if (BeginFrameAcksEnabled()) {
+ scoped_feature_list_.InitAndEnableFeature(features::kOnBeginFrameAcks);
+ } else {
+ scoped_feature_list_.InitAndDisableFeature(features::kOnBeginFrameAcks);
+ }
+}
+
+TEST_P(OnBeginFrameAcksSurfaceTest, PresentationCallback) {
constexpr gfx::Size kSurfaceSize(300, 300);
constexpr gfx::Rect kDamageRect(0, 0);
const LocalSurfaceId local_surface_id(6, base::UnguessableToken::Create());
@@ -54,10 +81,12 @@
CompositorFrame frame =
CompositorFrameBuilder()
.AddRenderPass(gfx::Rect(kSurfaceSize), kDamageRect)
+ .SetBeginFrameSourceId(kBeginFrameSourceId)
.Build();
frame_token = frame.metadata.frame_token;
ASSERT_NE(frame_token, 0u);
- EXPECT_CALL(client, DidReceiveCompositorFrameAck(testing::_)).Times(1);
+ EXPECT_CALL(client, DidReceiveCompositorFrameAck(testing::_))
+ .Times(BeginFrameAcksEnabled() ? 0 : 1);
support->SubmitCompositorFrame(local_surface_id, std::move(frame));
testing::Mock::VerifyAndClearExpectations(&client);
}
@@ -68,8 +97,10 @@
CompositorFrame frame =
CompositorFrameBuilder()
.AddRenderPass(gfx::Rect(kSurfaceSize), kDamageRect)
+ .SetBeginFrameSourceId(kBeginFrameSourceId)
.Build();
- EXPECT_CALL(client, DidReceiveCompositorFrameAck(testing::_)).Times(1);
+ EXPECT_CALL(client, DidReceiveCompositorFrameAck(testing::_))
+ .Times(BeginFrameAcksEnabled() ? 0 : 1);
support->SubmitCompositorFrame(local_surface_id, std::move(frame));
ASSERT_EQ(1u, support->timing_details().size());
EXPECT_EQ(frame_token, support->timing_details().begin()->first);
@@ -77,6 +108,14 @@
}
}
+INSTANTIATE_TEST_SUITE_P(,
+ OnBeginFrameAcksSurfaceTest,
+ testing::Bool(),
+ [](auto& info) {
+ return info.param ? "BeginFrameAcks"
+ : "CompositoFrameAcks";
+ });
+
TEST_F(SurfaceTest, SurfaceIds) {
for (size_t i = 0; i < 3; ++i) {
ParentLocalSurfaceIdAllocator allocator;
@@ -104,7 +143,7 @@
LocalSurfaceId local_surface_id(6, base::UnguessableToken::Create());
SurfaceId surface_id(kArbitraryFrameSinkId, local_surface_id);
- CompositorFrame frame = MakeDefaultCompositorFrame();
+ CompositorFrame frame = MakeDefaultCompositorFrame(kBeginFrameSourceId);
support->SubmitCompositorFrame(local_surface_id, std::move(frame));
Surface* surface = surface_manager->GetSurfaceForId(surface_id);
ASSERT_TRUE(surface);
@@ -209,8 +248,9 @@
// Submit something to the second child surface and verify it's now included
// in active referenced surfaces.
- child_support2->SubmitCompositorFrame(child_surface_id2.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support2->SubmitCompositorFrame(
+ child_surface_id2.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
EXPECT_TRUE(surface_manager->GetSurfaceForId(child_surface_id2));
EXPECT_THAT(root_surface->active_referenced_surfaces(),
testing::ElementsAre(child_surface_id2));
@@ -220,8 +260,9 @@
// normally the |child_surface_id1| would have activated first if the browser
// is navigating away from it but if the first renderer is slow to produce
// content the order can be reversed.
- child_support1->SubmitCompositorFrame(child_surface_id1.local_surface_id(),
- MakeDefaultCompositorFrame());
+ child_support1->SubmitCompositorFrame(
+ child_surface_id1.local_surface_id(),
+ MakeDefaultCompositorFrame(kBeginFrameSourceId));
EXPECT_TRUE(surface_manager->GetSurfaceForId(child_surface_id1));
EXPECT_THAT(root_surface->active_referenced_surfaces(),
testing::ElementsAre(child_surface_id1, child_surface_id2));
diff --git a/components/viz/test/compositor_frame_helpers.cc b/components/viz/test/compositor_frame_helpers.cc
index 92b38f6..b6539e6 100644
--- a/components/viz/test/compositor_frame_helpers.cc
+++ b/components/viz/test/compositor_frame_helpers.cc
@@ -381,6 +381,12 @@
return *this;
}
+CompositorFrameBuilder& CompositorFrameBuilder::SetBeginFrameSourceId(
+ uint64_t source_id) {
+ frame_->metadata.begin_frame_ack.frame_id.source_id = source_id;
+ return *this;
+}
+
CompositorFrameBuilder& CompositorFrameBuilder::SetDeviceScaleFactor(
float device_scale_factor) {
frame_->metadata.device_scale_factor = device_scale_factor;
@@ -454,8 +460,11 @@
return copy_list;
}
-CompositorFrame MakeDefaultCompositorFrame() {
- return CompositorFrameBuilder().AddDefaultRenderPass().Build();
+CompositorFrame MakeDefaultCompositorFrame(uint64_t source_id) {
+ return CompositorFrameBuilder()
+ .AddDefaultRenderPass()
+ .SetBeginFrameSourceId(source_id)
+ .Build();
}
CompositorFrame MakeCompositorFrame(
diff --git a/components/viz/test/compositor_frame_helpers.h b/components/viz/test/compositor_frame_helpers.h
index 2375c03..b7b80c4e 100644
--- a/components/viz/test/compositor_frame_helpers.h
+++ b/components/viz/test/compositor_frame_helpers.h
@@ -222,6 +222,7 @@
// Sets the BeginFrameAck. This replaces the default BeginFrameAck.
CompositorFrameBuilder& SetBeginFrameAck(const BeginFrameAck& ack);
+ CompositorFrameBuilder& SetBeginFrameSourceId(uint64_t source_id);
CompositorFrameBuilder& SetDeviceScaleFactor(float device_scale_factor);
CompositorFrameBuilder& AddLatencyInfo(ui::LatencyInfo latency_info);
CompositorFrameBuilder& AddLatencyInfos(
@@ -249,7 +250,8 @@
// Creates a CompositorFrame that has a render pass with 20x20 output_rect and
// empty damage_rect. This CompositorFrame is valid and can be sent over IPC.
-CompositorFrame MakeDefaultCompositorFrame();
+CompositorFrame MakeDefaultCompositorFrame(
+ uint64_t source_id = BeginFrameArgs::kManualSourceId);
// Creates a CompositorFrame with provided render pass.
CompositorFrame MakeCompositorFrame(
diff --git a/components/viz/test/fake_compositor_frame_sink_client.cc b/components/viz/test/fake_compositor_frame_sink_client.cc
index fb9c36c..8b5f6c3 100644
--- a/components/viz/test/fake_compositor_frame_sink_client.cc
+++ b/components/viz/test/fake_compositor_frame_sink_client.cc
@@ -4,6 +4,7 @@
#include <utility>
+#include "components/viz/common/features.h"
#include "components/viz/test/fake_compositor_frame_sink_client.h"
namespace viz {
@@ -18,7 +19,16 @@
void FakeCompositorFrameSinkClient::OnBeginFrame(
const BeginFrameArgs& args,
- const FrameTimingDetailsMap& timing_details) {
+ const FrameTimingDetailsMap& timing_details,
+ bool frame_ack,
+ std::vector<ReturnedResource> resources) {
+ if (features::IsOnBeginFrameAcksEnabled()) {
+ if (frame_ack) {
+ DidReceiveCompositorFrameAck(std::move(resources));
+ } else if (!resources.empty()) {
+ ReclaimResources(std::move(resources));
+ }
+ }
for (const auto& [frame_token, timing] : timing_details) {
all_frame_timing_details_.insert_or_assign(frame_token, timing);
}
diff --git a/components/viz/test/fake_compositor_frame_sink_client.h b/components/viz/test/fake_compositor_frame_sink_client.h
index 86ca12e2..913dbcd 100644
--- a/components/viz/test/fake_compositor_frame_sink_client.h
+++ b/components/viz/test/fake_compositor_frame_sink_client.h
@@ -31,7 +31,9 @@
void DidReceiveCompositorFrameAck(
std::vector<ReturnedResource> resources) override;
void OnBeginFrame(const BeginFrameArgs& args,
- const FrameTimingDetailsMap& timing_details) override;
+ const FrameTimingDetailsMap& timing_details,
+ bool frame_ack,
+ std::vector<ReturnedResource> resources) override;
void ReclaimResources(std::vector<ReturnedResource> resources) override;
void OnBeginFramePausedChanged(bool paused) override;
void OnCompositorFrameTransitionDirectiveProcessed(
diff --git a/components/viz/test/mock_compositor_frame_sink_client.h b/components/viz/test/mock_compositor_frame_sink_client.h
index a72e317..b9457db4 100644
--- a/components/viz/test/mock_compositor_frame_sink_client.h
+++ b/components/viz/test/mock_compositor_frame_sink_client.h
@@ -38,8 +38,11 @@
// mojom::CompositorFrameSinkClient implementation.
MOCK_METHOD1(DidReceiveCompositorFrameAck,
void(std::vector<ReturnedResource>));
- MOCK_METHOD2(OnBeginFrame,
- void(const BeginFrameArgs&, const FrameTimingDetailsMap&));
+ MOCK_METHOD4(OnBeginFrame,
+ void(const BeginFrameArgs&,
+ const FrameTimingDetailsMap&,
+ bool frame_ack,
+ std::vector<ReturnedResource>));
MOCK_METHOD1(ReclaimResources, void(std::vector<ReturnedResource>));
MOCK_METHOD2(WillDrawSurface, void(const LocalSurfaceId&, const gfx::Rect&));
MOCK_METHOD1(OnBeginFramePausedChanged, void(bool paused));
diff --git a/content/test/fake_renderer_compositor_frame_sink.h b/content/test/fake_renderer_compositor_frame_sink.h
index 00066b6..ef6e826 100644
--- a/content/test/fake_renderer_compositor_frame_sink.h
+++ b/content/test/fake_renderer_compositor_frame_sink.h
@@ -39,8 +39,9 @@
void DidReceiveCompositorFrameAck(
std::vector<viz::ReturnedResource> resources) override;
void OnBeginFrame(const viz::BeginFrameArgs& args,
- const viz::FrameTimingDetailsMap& timing_details) override {
- }
+ const viz::FrameTimingDetailsMap& timing_details,
+ bool frame_ack,
+ std::vector<viz::ReturnedResource> resources) override {}
void OnBeginFramePausedChanged(bool paused) override {}
void ReclaimResources(std::vector<viz::ReturnedResource> resources) override;
void OnCompositorFrameTransitionDirectiveProcessed(
diff --git a/device/vr/android/arcore/ar_compositor_frame_sink.cc b/device/vr/android/arcore/ar_compositor_frame_sink.cc
index dfc7fdb..c96b488 100644
--- a/device/vr/android/arcore/ar_compositor_frame_sink.cc
+++ b/device/vr/android/arcore/ar_compositor_frame_sink.cc
@@ -9,6 +9,7 @@
#include "base/memory/raw_ptr.h"
#include "base/task/bind_post_task.h"
#include "base/task/single_thread_task_runner.h"
+#include "components/viz/common/features.h"
#include "components/viz/common/quads/compositor_frame.h"
#include "components/viz/common/quads/surface_draw_quad.h"
#include "components/viz/common/quads/texture_draw_quad.h"
@@ -293,7 +294,18 @@
void ArCompositorFrameSink::OnBeginFrame(
const viz::BeginFrameArgs& args,
- const viz::FrameTimingDetailsMap& timing_details) {
+ const viz::FrameTimingDetailsMap& timing_details,
+ bool frame_ack,
+ std::vector<viz::ReturnedResource> resources) {
+ // TODO(crbug.com/1401032): Determine why the timing of this Ack leads to
+ // frame production stopping in tests.
+ if (features::IsOnBeginFrameAcksEnabled()) {
+ if (frame_ack) {
+ DidReceiveCompositorFrameAck(std::move(resources));
+ } else if (!resources.empty()) {
+ ReclaimResources(std::move(resources));
+ }
+ }
on_begin_frame_.Run(args, timing_details);
}
diff --git a/device/vr/android/arcore/ar_compositor_frame_sink.h b/device/vr/android/arcore/ar_compositor_frame_sink.h
index 939d7d8..28de589 100644
--- a/device/vr/android/arcore/ar_compositor_frame_sink.h
+++ b/device/vr/android/arcore/ar_compositor_frame_sink.h
@@ -132,7 +132,9 @@
void DidReceiveCompositorFrameAck(
std::vector<viz::ReturnedResource> resources) override;
void OnBeginFrame(const viz::BeginFrameArgs& args,
- const viz::FrameTimingDetailsMap& timing_details) override;
+ const viz::FrameTimingDetailsMap& timing_details,
+ bool frame_ack,
+ std::vector<viz::ReturnedResource> resources) override;
// Callback that we bind when submitting a frame. It lets us know that viz
// will allow us to call "BeginFrame" again.
diff --git a/services/viz/public/mojom/compositing/compositor_frame_sink.mojom b/services/viz/public/mojom/compositing/compositor_frame_sink.mojom
index dc51074..f7fd878 100644
--- a/services/viz/public/mojom/compositing/compositor_frame_sink.mojom
+++ b/services/viz/public/mojom/compositing/compositor_frame_sink.mojom
@@ -113,9 +113,14 @@
// Notification for the client to generate a CompositorFrame. The client is
// required to respond with either SubmitCompositorFrame() or
// DidNotProduceFrame(). If the client is unresponsive then begin frames will
- // be throttled and eventually stopped entirely.
+ // be throttled and eventually stopped entirely. We also return whether this
+ // OnBeginFrame is to act as DidReceiveCompositorFrameAck. We also return a
+ // list of resources, previously sent to SubmitCompositorFrame, to be reused
+ // or freed.
OnBeginFrame(BeginFrameArgs args,
- map<uint32, FrameTimingDetails> details);
+ map<uint32, FrameTimingDetails> details,
+ bool frame_ack,
+ array<ReturnedResource> resources);
// Inform the client that OnBeginFrame may not be called for some time.
OnBeginFramePausedChanged(bool paused);
diff --git a/services/viz/public/mojom/compositing/frame_sink_bundle.mojom b/services/viz/public/mojom/compositing/frame_sink_bundle.mojom
index c3f1cae..eb4c66b 100644
--- a/services/viz/public/mojom/compositing/frame_sink_bundle.mojom
+++ b/services/viz/public/mojom/compositing/frame_sink_bundle.mojom
@@ -90,6 +90,8 @@
uint32 sink_id;
BeginFrameArgs args;
map<uint32, FrameTimingDetails> details;
+ bool frame_ack;
+ array<ReturnedResource> resources;
};
// Client interface for FrameSinkBundle. This behaves as an aggregated
diff --git a/third_party/blink/renderer/platform/graphics/begin_frame_provider.cc b/third_party/blink/renderer/platform/graphics/begin_frame_provider.cc
index 7b880c1..d1be7ee 100644
--- a/third_party/blink/renderer/platform/graphics/begin_frame_provider.cc
+++ b/third_party/blink/renderer/platform/graphics/begin_frame_provider.cc
@@ -13,6 +13,7 @@
#include "components/power_scheduler/power_mode.h"
#include "components/power_scheduler/power_mode_arbiter.h"
#include "components/power_scheduler/power_mode_voter.h"
+#include "components/viz/common/features.h"
#include "services/viz/public/mojom/compositing/frame_timing_details.mojom-blink.h"
#include "third_party/blink/public/common/thread_safe_browser_interface_broker_proxy.h"
#include "third_party/blink/public/platform/platform.h"
@@ -111,7 +112,9 @@
void BeginFrameProvider::OnBeginFrame(
const viz::BeginFrameArgs& args,
- const WTF::HashMap<uint32_t, viz::FrameTimingDetails>&) {
+ const WTF::HashMap<uint32_t, viz::FrameTimingDetails>&,
+ bool frame_ack,
+ WTF::Vector<viz::ReturnedResource> resources) {
TRACE_EVENT_WITH_FLOW0("blink", "BeginFrameProvider::OnBeginFrame",
TRACE_ID_GLOBAL(args.trace_id),
TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT);
diff --git a/third_party/blink/renderer/platform/graphics/begin_frame_provider.h b/third_party/blink/renderer/platform/graphics/begin_frame_provider.h
index 4670cc1c..570cff6 100644
--- a/third_party/blink/renderer/platform/graphics/begin_frame_provider.h
+++ b/third_party/blink/renderer/platform/graphics/begin_frame_provider.h
@@ -52,9 +52,10 @@
WTF::Vector<viz::ReturnedResource> resources) final {
NOTIMPLEMENTED();
}
- void OnBeginFrame(
- const viz::BeginFrameArgs&,
- const WTF::HashMap<uint32_t, viz::FrameTimingDetails>&) final;
+ void OnBeginFrame(const viz::BeginFrameArgs&,
+ const WTF::HashMap<uint32_t, viz::FrameTimingDetails>&,
+ bool frame_ack,
+ WTF::Vector<viz::ReturnedResource> resources) final;
void OnBeginFramePausedChanged(bool paused) final {}
void ReclaimResources(WTF::Vector<viz::ReturnedResource> resources) final {
NOTIMPLEMENTED();
diff --git a/third_party/blink/renderer/platform/graphics/canvas_resource_dispatcher.cc b/third_party/blink/renderer/platform/graphics/canvas_resource_dispatcher.cc
index 5cd33878..11f5d15 100644
--- a/third_party/blink/renderer/platform/graphics/canvas_resource_dispatcher.cc
+++ b/third_party/blink/renderer/platform/graphics/canvas_resource_dispatcher.cc
@@ -12,6 +12,7 @@
#include "components/power_scheduler/power_mode.h"
#include "components/power_scheduler/power_mode_arbiter.h"
#include "components/power_scheduler/power_mode_voter.h"
+#include "components/viz/common/features.h"
#include "components/viz/common/quads/compositor_frame.h"
#include "components/viz/common/quads/texture_draw_quad.h"
#include "components/viz/common/resources/release_callback.h"
@@ -388,7 +389,16 @@
void CanvasResourceDispatcher::OnBeginFrame(
const viz::BeginFrameArgs& begin_frame_args,
- const WTF::HashMap<uint32_t, viz::FrameTimingDetails>&) {
+ const WTF::HashMap<uint32_t, viz::FrameTimingDetails>&,
+ bool frame_ack,
+ WTF::Vector<viz::ReturnedResource> resources) {
+ if (features::IsOnBeginFrameAcksEnabled()) {
+ if (frame_ack) {
+ DidReceiveCompositorFrameAck(std::move(resources));
+ } else if (!resources.empty()) {
+ ReclaimResources(std::move(resources));
+ }
+ }
current_begin_frame_ack_ = viz::BeginFrameAck(begin_frame_args, false);
if (HasTooManyPendingFrames() ||
(begin_frame_args.type == viz::BeginFrameArgs::MISSED &&
diff --git a/third_party/blink/renderer/platform/graphics/canvas_resource_dispatcher.h b/third_party/blink/renderer/platform/graphics/canvas_resource_dispatcher.h
index 31cc680..48ecaa3 100644
--- a/third_party/blink/renderer/platform/graphics/canvas_resource_dispatcher.h
+++ b/third_party/blink/renderer/platform/graphics/canvas_resource_dispatcher.h
@@ -97,9 +97,10 @@
// viz::mojom::blink::CompositorFrameSinkClient implementation.
void DidReceiveCompositorFrameAck(
WTF::Vector<viz::ReturnedResource> resources) final;
- void OnBeginFrame(
- const viz::BeginFrameArgs&,
- const WTF::HashMap<uint32_t, viz::FrameTimingDetails>&) final;
+ void OnBeginFrame(const viz::BeginFrameArgs&,
+ const WTF::HashMap<uint32_t, viz::FrameTimingDetails>&,
+ bool frame_ack,
+ WTF::Vector<viz::ReturnedResource> resources) final;
void OnBeginFramePausedChanged(bool paused) final {}
void ReclaimResources(WTF::Vector<viz::ReturnedResource> resources) final;
void OnCompositorFrameTransitionDirectiveProcessed(
diff --git a/third_party/blink/renderer/platform/graphics/test/mock_compositor_frame_sink_client.h b/third_party/blink/renderer/platform/graphics/test/mock_compositor_frame_sink_client.h
index 2bb0f7d..dbad4066 100644
--- a/third_party/blink/renderer/platform/graphics/test/mock_compositor_frame_sink_client.h
+++ b/third_party/blink/renderer/platform/graphics/test/mock_compositor_frame_sink_client.h
@@ -17,9 +17,11 @@
// mojom::blink::CompositorFrameSinkClient implementation:
MOCK_METHOD1(DidReceiveCompositorFrameAck,
void(WTF::Vector<viz::ReturnedResource>));
- MOCK_METHOD2(OnBeginFrame,
+ MOCK_METHOD4(OnBeginFrame,
void(const viz::BeginFrameArgs&,
- const WTF::HashMap<uint32_t, viz::FrameTimingDetails>&));
+ const WTF::HashMap<uint32_t, viz::FrameTimingDetails>&,
+ bool frame_ack,
+ WTF::Vector<viz::ReturnedResource>));
MOCK_METHOD1(ReclaimResources, void(WTF::Vector<viz::ReturnedResource>));
MOCK_METHOD2(WillDrawSurface,
void(const viz::LocalSurfaceId&, const gfx::Rect&));
diff --git a/third_party/blink/renderer/platform/graphics/video_frame_sink_bundle.cc b/third_party/blink/renderer/platform/graphics/video_frame_sink_bundle.cc
index aa7ff6b..0f2bfb3 100644
--- a/third_party/blink/renderer/platform/graphics/video_frame_sink_bundle.cc
+++ b/third_party/blink/renderer/platform/graphics/video_frame_sink_bundle.cc
@@ -248,7 +248,8 @@
auto it = clients_.find(entry->sink_id);
if (it == clients_.end())
continue;
- it->value->OnBeginFrame(std::move(entry->args), std::move(entry->details));
+ it->value->OnBeginFrame(std::move(entry->args), std::move(entry->details),
+ entry->frame_ack, std::move(entry->resources));
}
defer_submissions_ = false;
diff --git a/third_party/blink/renderer/platform/graphics/video_frame_sink_bundle_test.cc b/third_party/blink/renderer/platform/graphics/video_frame_sink_bundle_test.cc
index 9dba75e..a5250d6 100644
--- a/third_party/blink/renderer/platform/graphics/video_frame_sink_bundle_test.cc
+++ b/third_party/blink/renderer/platform/graphics/video_frame_sink_bundle_test.cc
@@ -68,7 +68,8 @@
viz::BeginFrameArgs::Create(BEGINFRAME_FROM_HERE, 1, 1, base::TimeTicks(),
base::TimeTicks(), base::TimeDelta(),
viz::BeginFrameArgs::NORMAL),
- WTF::HashMap<uint32_t, viz::FrameTimingDetails>());
+ WTF::HashMap<uint32_t, viz::FrameTimingDetails>(),
+ /*frame_ack=*/false, WTF::Vector<viz::ReturnedResource>());
}
class MockFrameSinkBundleClient
diff --git a/third_party/blink/renderer/platform/graphics/video_frame_submitter.cc b/third_party/blink/renderer/platform/graphics/video_frame_submitter.cc
index 4b102c0..0877ecf7 100644
--- a/third_party/blink/renderer/platform/graphics/video_frame_submitter.cc
+++ b/third_party/blink/renderer/platform/graphics/video_frame_submitter.cc
@@ -20,6 +20,7 @@
#include "components/power_scheduler/power_mode.h"
#include "components/power_scheduler/power_mode_arbiter.h"
#include "components/power_scheduler/power_mode_voter.h"
+#include "components/viz/common/features.h"
#include "components/viz/common/resources/resource_id.h"
#include "components/viz/common/resources/returned_resource.h"
#include "components/viz/common/surfaces/frame_sink_bundle_id.h"
@@ -331,7 +332,17 @@
void VideoFrameSubmitter::OnBeginFrame(
const viz::BeginFrameArgs& args,
- const WTF::HashMap<uint32_t, viz::FrameTimingDetails>& timing_details) {
+ const WTF::HashMap<uint32_t, viz::FrameTimingDetails>& timing_details,
+ bool frame_ack,
+ WTF::Vector<viz::ReturnedResource> resources) {
+ if (features::IsOnBeginFrameAcksEnabled()) {
+ if (frame_ack) {
+ DidReceiveCompositorFrameAck(std::move(resources));
+ } else if (!resources.empty()) {
+ ReclaimResources(std::move(resources));
+ }
+ }
+
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
TRACE_EVENT0("media", "VideoFrameSubmitter::OnBeginFrame");
diff --git a/third_party/blink/renderer/platform/graphics/video_frame_submitter.h b/third_party/blink/renderer/platform/graphics/video_frame_submitter.h
index e89edaf..92940b1 100644
--- a/third_party/blink/renderer/platform/graphics/video_frame_submitter.h
+++ b/third_party/blink/renderer/platform/graphics/video_frame_submitter.h
@@ -74,9 +74,10 @@
// cc::mojom::CompositorFrameSinkClient implementation.
void DidReceiveCompositorFrameAck(
WTF::Vector<viz::ReturnedResource> resources) override;
- void OnBeginFrame(
- const viz::BeginFrameArgs&,
- const WTF::HashMap<uint32_t, viz::FrameTimingDetails>&) override;
+ void OnBeginFrame(const viz::BeginFrameArgs&,
+ const WTF::HashMap<uint32_t, viz::FrameTimingDetails>&,
+ bool frame_ack,
+ WTF::Vector<viz::ReturnedResource> resources) override;
void OnBeginFramePausedChanged(bool paused) override {}
void ReclaimResources(WTF::Vector<viz::ReturnedResource> resources) override;
void OnCompositorFrameTransitionDirectiveProcessed(
diff --git a/third_party/blink/renderer/platform/graphics/video_frame_submitter_test.cc b/third_party/blink/renderer/platform/graphics/video_frame_submitter_test.cc
index 56e48af8..6ef6725f 100644
--- a/third_party/blink/renderer/platform/graphics/video_frame_submitter_test.cc
+++ b/third_party/blink/renderer/platform/graphics/video_frame_submitter_test.cc
@@ -13,6 +13,7 @@
#include "base/memory/ptr_util.h"
#include "base/memory/read_only_shared_memory_region.h"
#include "base/test/bind.h"
+#include "base/test/scoped_feature_list.h"
#include "base/test/simple_test_tick_clock.h"
#include "base/test/task_environment.h"
#include "base/time/time.h"
@@ -21,6 +22,7 @@
#include "cc/test/layer_test_common.h"
#include "cc/trees/layer_tree_settings.h"
#include "cc/trees/task_runner_provider.h"
+#include "components/viz/common/features.h"
#include "components/viz/test/fake_external_begin_frame_source.h"
#include "components/viz/test/test_context_provider.h"
#include "media/base/video_frame.h"
@@ -155,13 +157,23 @@
};
} // namespace
-class VideoFrameSubmitterTest : public testing::Test {
+// Supports testing features::OnBeginFrameAcks, which changes the expectations
+// of what IPCs are sent to the CompositorFrameSinkClient. When enabled
+// OnBeginFrame also handles ReturnResources as well as
+// DidReceiveCompositorFrameAck.
+class VideoFrameSubmitterTest : public testing::Test,
+ public testing::WithParamInterface<bool> {
public:
VideoFrameSubmitterTest()
: now_src_(new base::SimpleTestTickClock()),
begin_frame_source_(new viz::FakeExternalBeginFrameSource(0.f, false)),
video_frame_provider_(new StrictMock<MockVideoFrameProvider>()),
context_provider_(viz::TestContextProvider::Create()) {
+ if (GetParam()) {
+ scoped_feature_list_.InitAndEnableFeature(features::kOnBeginFrameAcks);
+ } else {
+ scoped_feature_list_.InitAndDisableFeature(features::kOnBeginFrameAcks);
+ }
context_provider_->BindToCurrentSequence();
MakeSubmitter();
task_environment_.RunUntilIdle();
@@ -229,6 +241,18 @@
submitter_->DidReceiveCompositorFrameAck(std::move(resources));
}
+ void OnBeginFrame(
+ const viz::BeginFrameArgs& args,
+ const WTF::HashMap<uint32_t, viz::FrameTimingDetails>& timing_details,
+ bool frame_ack,
+ WTF::Vector<viz::ReturnedResource> resources) {
+ if (GetParam()) {
+ EXPECT_CALL(*resource_provider_, ReceiveReturnsFromParent(_));
+ }
+ submitter_->OnBeginFrame(args, timing_details, frame_ack,
+ std::move(resources));
+ }
+
protected:
base::test::TaskEnvironment task_environment_;
std::unique_ptr<base::SimpleTestTickClock> now_src_;
@@ -238,6 +262,9 @@
StrictMock<MockVideoFrameResourceProvider>* resource_provider_;
scoped_refptr<viz::TestContextProvider> context_provider_;
std::unique_ptr<VideoFrameSubmitter> submitter_;
+
+ private:
+ base::test::ScopedFeatureList scoped_feature_list_;
};
enum class SubmissionType {
@@ -270,7 +297,7 @@
EXPECT_CALL(*resource_provider_, ReleaseFrameResources()); \
} while (0)
-TEST_F(VideoFrameSubmitterTest, StatRenderingFlipsBits) {
+TEST_P(VideoFrameSubmitterTest, StatRenderingFlipsBits) {
EXPECT_FALSE(IsRendering());
EXPECT_CALL(*sink_, SetNeedsBeginFrame(true));
@@ -281,7 +308,7 @@
EXPECT_TRUE(IsRendering());
}
-TEST_F(VideoFrameSubmitterTest, StopRenderingSkipsUpdateCurrentFrame) {
+TEST_P(VideoFrameSubmitterTest, StopRenderingSkipsUpdateCurrentFrame) {
EXPECT_FALSE(IsRendering());
EXPECT_CALL(*sink_, SetNeedsBeginFrame(true));
@@ -295,7 +322,7 @@
EXPECT_SUBMISSION(SubmissionType::kBeginFrame);
viz::BeginFrameArgs args = begin_frame_source_->CreateBeginFrameArgs(
BEGINFRAME_FROM_HERE, now_src_.get());
- submitter_->OnBeginFrame(args, {});
+ OnBeginFrame(args, {}, true, WTF::Vector<viz::ReturnedResource>());
task_environment_.RunUntilIdle();
AckSubmittedFrame();
@@ -310,11 +337,11 @@
EXPECT_CALL(*sink_, DidNotProduceFrame(_));
args = begin_frame_source_->CreateBeginFrameArgs(BEGINFRAME_FROM_HERE,
now_src_.get());
- submitter_->OnBeginFrame(args, {});
+ OnBeginFrame(args, {}, true, WTF::Vector<viz::ReturnedResource>());
task_environment_.RunUntilIdle();
}
-TEST_F(VideoFrameSubmitterTest, StopUsingProviderNullsProvider) {
+TEST_P(VideoFrameSubmitterTest, StopUsingProviderNullsProvider) {
EXPECT_FALSE(IsRendering());
EXPECT_EQ(video_frame_provider_.get(), GetProvider());
@@ -323,7 +350,7 @@
EXPECT_EQ(nullptr, GetProvider());
}
-TEST_F(VideoFrameSubmitterTest,
+TEST_P(VideoFrameSubmitterTest,
StopUsingProviderSubmitsFrameAndStopsRendering) {
EXPECT_CALL(*sink_, SetNeedsBeginFrame(true));
submitter_->StartRendering();
@@ -340,7 +367,7 @@
EXPECT_FALSE(IsRendering());
}
-TEST_F(VideoFrameSubmitterTest, DidReceiveFrameStillSubmitsIfRendering) {
+TEST_P(VideoFrameSubmitterTest, DidReceiveFrameStillSubmitsIfRendering) {
EXPECT_CALL(*sink_, SetNeedsBeginFrame(true));
submitter_->StartRendering();
@@ -353,7 +380,7 @@
task_environment_.RunUntilIdle();
}
-TEST_F(VideoFrameSubmitterTest, DidReceiveFrameSubmitsFrame) {
+TEST_P(VideoFrameSubmitterTest, DidReceiveFrameSubmitsFrame) {
EXPECT_FALSE(IsRendering());
EXPECT_SUBMISSION(SubmissionType::kManual);
@@ -361,7 +388,7 @@
task_environment_.RunUntilIdle();
}
-TEST_F(VideoFrameSubmitterTest, ShouldSubmitPreventsSubmission) {
+TEST_P(VideoFrameSubmitterTest, ShouldSubmitPreventsSubmission) {
EXPECT_CALL(*sink_, SetNeedsBeginFrame(false));
submitter_->SetIsSurfaceVisible(false);
task_environment_.RunUntilIdle();
@@ -399,7 +426,7 @@
// Tests that when set to true SetForceSubmit forces frame submissions.
// regardless of the internal submit state.
-TEST_F(VideoFrameSubmitterTest, SetForceSubmitForcesSubmission) {
+TEST_P(VideoFrameSubmitterTest, SetForceSubmitForcesSubmission) {
EXPECT_CALL(*sink_, SetNeedsBeginFrame(false));
submitter_->SetIsSurfaceVisible(false);
task_environment_.RunUntilIdle();
@@ -434,7 +461,7 @@
task_environment_.RunUntilIdle();
}
-TEST_F(VideoFrameSubmitterTest, RotationInformationPassedToResourceProvider) {
+TEST_P(VideoFrameSubmitterTest, RotationInformationPassedToResourceProvider) {
// Check to see if rotation is communicated pre-rendering.
EXPECT_FALSE(IsRendering());
@@ -485,7 +512,7 @@
viz::BeginFrameArgs args = begin_frame_source_->CreateBeginFrameArgs(
BEGINFRAME_FROM_HERE, now_src_.get());
- submitter_->OnBeginFrame(args, {});
+ OnBeginFrame(args, {}, true, WTF::Vector<viz::ReturnedResource>());
task_environment_.RunUntilIdle();
AckSubmittedFrame();
@@ -510,11 +537,11 @@
args = begin_frame_source_->CreateBeginFrameArgs(BEGINFRAME_FROM_HERE,
now_src_.get());
- submitter_->OnBeginFrame(args, {});
+ OnBeginFrame(args, {}, true, WTF::Vector<viz::ReturnedResource>());
task_environment_.RunUntilIdle();
}
-TEST_F(VideoFrameSubmitterTest, FrameTransformTakesPrecedent) {
+TEST_P(VideoFrameSubmitterTest, FrameTransformTakesPrecedent) {
EXPECT_FALSE(IsRendering());
submitter_->SetTransform(media::VideoRotation::VIDEO_ROTATION_90);
@@ -561,12 +588,12 @@
viz::BeginFrameArgs args = begin_frame_source_->CreateBeginFrameArgs(
BEGINFRAME_FROM_HERE, now_src_.get());
- submitter_->OnBeginFrame(args, {});
+ OnBeginFrame(args, {}, true, WTF::Vector<viz::ReturnedResource>());
task_environment_.RunUntilIdle();
AckSubmittedFrame();
}
-TEST_F(VideoFrameSubmitterTest, OnBeginFrameSubmitsFrame) {
+TEST_P(VideoFrameSubmitterTest, OnBeginFrameSubmitsFrame) {
EXPECT_CALL(*sink_, SetNeedsBeginFrame(true));
submitter_->StartRendering();
@@ -575,32 +602,32 @@
EXPECT_SUBMISSION(SubmissionType::kBeginFrame);
viz::BeginFrameArgs args = begin_frame_source_->CreateBeginFrameArgs(
BEGINFRAME_FROM_HERE, now_src_.get());
- submitter_->OnBeginFrame(args, {});
+ OnBeginFrame(args, {}, true, WTF::Vector<viz::ReturnedResource>());
task_environment_.RunUntilIdle();
}
-TEST_F(VideoFrameSubmitterTest, MissedFrameArgDoesNotProduceFrame) {
+TEST_P(VideoFrameSubmitterTest, MissedFrameArgDoesNotProduceFrame) {
EXPECT_CALL(*sink_, DidNotProduceFrame(_));
viz::BeginFrameArgs args = begin_frame_source_->CreateBeginFrameArgs(
BEGINFRAME_FROM_HERE, now_src_.get());
args.type = viz::BeginFrameArgs::MISSED;
- submitter_->OnBeginFrame(args, {});
+ OnBeginFrame(args, {}, true, WTF::Vector<viz::ReturnedResource>());
task_environment_.RunUntilIdle();
}
-TEST_F(VideoFrameSubmitterTest, MissingProviderDoesNotProduceFrame) {
+TEST_P(VideoFrameSubmitterTest, MissingProviderDoesNotProduceFrame) {
submitter_->StopUsingProvider();
EXPECT_CALL(*sink_, DidNotProduceFrame(_));
viz::BeginFrameArgs args = begin_frame_source_->CreateBeginFrameArgs(
BEGINFRAME_FROM_HERE, now_src_.get());
- submitter_->OnBeginFrame(args, {});
+ OnBeginFrame(args, {}, true, WTF::Vector<viz::ReturnedResource>());
task_environment_.RunUntilIdle();
}
-TEST_F(VideoFrameSubmitterTest, NoUpdateOnFrameDoesNotProduceFrame) {
+TEST_P(VideoFrameSubmitterTest, NoUpdateOnFrameDoesNotProduceFrame) {
EXPECT_CALL(*sink_, SetNeedsBeginFrame(true));
submitter_->StartRendering();
@@ -610,11 +637,11 @@
viz::BeginFrameArgs args = begin_frame_source_->CreateBeginFrameArgs(
BEGINFRAME_FROM_HERE, now_src_.get());
- submitter_->OnBeginFrame(args, {});
+ OnBeginFrame(args, {}, true, WTF::Vector<viz::ReturnedResource>());
task_environment_.RunUntilIdle();
}
-TEST_F(VideoFrameSubmitterTest, NotRenderingDoesNotProduceFrame) {
+TEST_P(VideoFrameSubmitterTest, NotRenderingDoesNotProduceFrame) {
// We don't care if UpdateCurrentFrame is called or not; it doesn't matter
// if we're not rendering.
EXPECT_CALL(*video_frame_provider_, UpdateCurrentFrame(_, _))
@@ -623,18 +650,18 @@
viz::BeginFrameArgs args = begin_frame_source_->CreateBeginFrameArgs(
BEGINFRAME_FROM_HERE, now_src_.get());
- submitter_->OnBeginFrame(args, {});
+ OnBeginFrame(args, {}, true, WTF::Vector<viz::ReturnedResource>());
task_environment_.RunUntilIdle();
}
-TEST_F(VideoFrameSubmitterTest, ReturnsResourceOnCompositorAck) {
+TEST_P(VideoFrameSubmitterTest, ReturnsResourceOnCompositorAck) {
AckSubmittedFrame();
task_environment_.RunUntilIdle();
}
// Tests that after submitting a frame, no frame will be submitted until an ACK
// was received. This is tested by simulating another BeginFrame message.
-TEST_F(VideoFrameSubmitterTest, WaitingForAckPreventsNewFrame) {
+TEST_P(VideoFrameSubmitterTest, WaitingForAckPreventsNewFrame) {
EXPECT_CALL(*sink_, SetNeedsBeginFrame(true));
submitter_->StartRendering();
@@ -643,7 +670,7 @@
EXPECT_SUBMISSION(SubmissionType::kBeginFrame);
viz::BeginFrameArgs args = begin_frame_source_->CreateBeginFrameArgs(
BEGINFRAME_FROM_HERE, now_src_.get());
- submitter_->OnBeginFrame(args, {});
+ OnBeginFrame(args, {}, true, WTF::Vector<viz::ReturnedResource>());
task_environment_.RunUntilIdle();
// DidNotProduceFrame should be called because no frame will be submitted
@@ -658,12 +685,12 @@
std::make_unique<base::SimpleTestTickClock>();
args = begin_frame_source_->CreateBeginFrameArgs(BEGINFRAME_FROM_HERE,
new_time.get());
- submitter_->OnBeginFrame(args, {});
+ OnBeginFrame(args, {}, true, WTF::Vector<viz::ReturnedResource>());
task_environment_.RunUntilIdle();
}
// Similar to above but verifies the single-frame paint path.
-TEST_F(VideoFrameSubmitterTest, WaitingForAckPreventsSubmitSingleFrame) {
+TEST_P(VideoFrameSubmitterTest, WaitingForAckPreventsSubmitSingleFrame) {
EXPECT_CALL(*sink_, SetNeedsBeginFrame(true));
submitter_->StartRendering();
@@ -686,7 +713,7 @@
// Test that after context is lost, the CompositorFrameSink is recreated but the
// SurfaceEmbedder isn't.
-TEST_F(VideoFrameSubmitterTest, RecreateCompositorFrameSinkAfterContextLost) {
+TEST_P(VideoFrameSubmitterTest, RecreateCompositorFrameSinkAfterContextLost) {
MockEmbeddedFrameSinkProvider mock_embedded_frame_sink_provider;
mojo::ReceiverSet<mojom::blink::EmbeddedFrameSinkProvider>
embedded_frame_sink_provider_receivers;
@@ -707,7 +734,7 @@
// Test that after context is lost, the CompositorFrameSink is recreated but the
// SurfaceEmbedder isn't even with software compositing.
-TEST_F(VideoFrameSubmitterTest,
+TEST_P(VideoFrameSubmitterTest,
RecreateCompositorFrameSinkAfterContextLostSoftwareCompositing) {
MockEmbeddedFrameSinkProvider mock_embedded_frame_sink_provider;
mojo::ReceiverSet<mojom::blink::EmbeddedFrameSinkProvider>
@@ -729,7 +756,7 @@
// This test simulates a race condition in which the |video_frame_provider_| is
// destroyed before OnReceivedContextProvider returns.
-TEST_F(VideoFrameSubmitterTest, StopUsingProviderDuringContextLost) {
+TEST_P(VideoFrameSubmitterTest, StopUsingProviderDuringContextLost) {
EXPECT_CALL(*sink_, SetNeedsBeginFrame(true));
submitter_->StartRendering();
@@ -751,7 +778,7 @@
// Test the behaviour of the ChildLocalSurfaceIdAllocator instance. It checks
// that the LocalSurfaceId is properly set at creation and updated when the
// video frames change.
-TEST_F(VideoFrameSubmitterTest, FrameSizeChangeUpdatesLocalSurfaceId) {
+TEST_P(VideoFrameSubmitterTest, FrameSizeChangeUpdatesLocalSurfaceId) {
{
viz::LocalSurfaceId local_surface_id =
child_local_surface_id_allocator().GetCurrentLocalSurfaceId();
@@ -806,7 +833,7 @@
}
}
-TEST_F(VideoFrameSubmitterTest, VideoRotationOutputRect) {
+TEST_P(VideoFrameSubmitterTest, VideoRotationOutputRect) {
MakeSubmitter();
EXPECT_CALL(*sink_, SetNeedsBeginFrame(true));
submitter_->StartRendering();
@@ -837,7 +864,7 @@
viz::BeginFrameArgs args = begin_frame_source_->CreateBeginFrameArgs(
BEGINFRAME_FROM_HERE, now_src_.get());
- submitter_->OnBeginFrame(args, {});
+ OnBeginFrame(args, {}, true, WTF::Vector<viz::ReturnedResource>());
task_environment_.RunUntilIdle();
EXPECT_EQ(sink_->last_submitted_compositor_frame().size_in_pixels(),
@@ -867,7 +894,7 @@
viz::BeginFrameArgs args = begin_frame_source_->CreateBeginFrameArgs(
BEGINFRAME_FROM_HERE, now_src_.get());
- submitter_->OnBeginFrame(args, {});
+ OnBeginFrame(args, {}, true, WTF::Vector<viz::ReturnedResource>());
task_environment_.RunUntilIdle();
// 180 deg rotation has same size.
@@ -898,7 +925,7 @@
viz::BeginFrameArgs args = begin_frame_source_->CreateBeginFrameArgs(
BEGINFRAME_FROM_HERE, now_src_.get());
- submitter_->OnBeginFrame(args, {});
+ OnBeginFrame(args, {}, true, WTF::Vector<viz::ReturnedResource>());
task_environment_.RunUntilIdle();
EXPECT_EQ(sink_->last_submitted_compositor_frame().size_in_pixels(),
@@ -908,7 +935,7 @@
}
}
-TEST_F(VideoFrameSubmitterTest, PageVisibilityControlsSubmission) {
+TEST_P(VideoFrameSubmitterTest, PageVisibilityControlsSubmission) {
// Hide the page and ensure no begin frames are issued.
EXPECT_CALL(*sink_, SetNeedsBeginFrame(false));
submitter_->SetIsPageVisible(false);
@@ -934,7 +961,7 @@
task_environment_.RunUntilIdle();
}
-TEST_F(VideoFrameSubmitterTest, PreferredInterval) {
+TEST_P(VideoFrameSubmitterTest, PreferredInterval) {
video_frame_provider_->preferred_interval = base::Seconds(1);
EXPECT_CALL(*sink_, SetNeedsBeginFrame(true));
@@ -945,7 +972,7 @@
EXPECT_SUBMISSION(SubmissionType::kBeginFrame);
viz::BeginFrameArgs args = begin_frame_source_->CreateBeginFrameArgs(
BEGINFRAME_FROM_HERE, now_src_.get());
- submitter_->OnBeginFrame(args, {});
+ OnBeginFrame(args, {}, true, WTF::Vector<viz::ReturnedResource>());
task_environment_.RunUntilIdle();
EXPECT_EQ(sink_->last_submitted_compositor_frame()
@@ -953,7 +980,7 @@
video_frame_provider_->preferred_interval);
}
-TEST_F(VideoFrameSubmitterTest, NoDuplicateFramesOnBeginFrame) {
+TEST_P(VideoFrameSubmitterTest, NoDuplicateFramesOnBeginFrame) {
EXPECT_CALL(*sink_, SetNeedsBeginFrame(true));
submitter_->StartRendering();
task_environment_.RunUntilIdle();
@@ -973,7 +1000,7 @@
EXPECT_CALL(*resource_provider_, ReleaseFrameResources());
viz::BeginFrameArgs args = begin_frame_source_->CreateBeginFrameArgs(
BEGINFRAME_FROM_HERE, now_src_.get());
- submitter_->OnBeginFrame(args, {});
+ OnBeginFrame(args, {}, true, WTF::Vector<viz::ReturnedResource>());
task_environment_.RunUntilIdle();
AckSubmittedFrame();
@@ -985,11 +1012,11 @@
EXPECT_CALL(*sink_, DidNotProduceFrame(_));
args = begin_frame_source_->CreateBeginFrameArgs(BEGINFRAME_FROM_HERE,
now_src_.get());
- submitter_->OnBeginFrame(args, {});
+ OnBeginFrame(args, {}, true, WTF::Vector<viz::ReturnedResource>());
task_environment_.RunUntilIdle();
}
-TEST_F(VideoFrameSubmitterTest, NoDuplicateFramesDidReceiveFrame) {
+TEST_P(VideoFrameSubmitterTest, NoDuplicateFramesDidReceiveFrame) {
auto vf = media::VideoFrame::CreateFrame(
media::PIXEL_FORMAT_YV12, gfx::Size(8, 8), gfx::Rect(gfx::Size(8, 8)),
gfx::Size(8, 8), base::TimeDelta());
@@ -1010,7 +1037,7 @@
task_environment_.RunUntilIdle();
}
-TEST_F(VideoFrameSubmitterTest, ZeroSizedFramesAreNotSubmitted) {
+TEST_P(VideoFrameSubmitterTest, ZeroSizedFramesAreNotSubmitted) {
auto vf = media::VideoFrame::CreateEOSFrame();
ASSERT_TRUE(vf->natural_size().IsEmpty());
@@ -1023,7 +1050,7 @@
// Check that given enough frames with wallclock duration and enough
// presentation feedback data, VideoFrameSubmitter will call the video roughness
// reporting callback.
-TEST_F(VideoFrameSubmitterTest, ProcessTimingDetails) {
+TEST_P(VideoFrameSubmitterTest, ProcessTimingDetails) {
int fps = 24;
int reports = 0;
base::TimeDelta frame_duration = base::Seconds(1.0 / fps);
@@ -1073,7 +1100,8 @@
auto args = begin_frame_source_->CreateBeginFrameArgs(BEGINFRAME_FROM_HERE,
now_src_.get());
- submitter_->OnBeginFrame(args, timing_details);
+ OnBeginFrame(args, timing_details, true,
+ WTF::Vector<viz::ReturnedResource>());
task_environment_.RunUntilIdle();
AckSubmittedFrame();
}
@@ -1081,4 +1109,12 @@
EXPECT_EQ(reports, 1);
}
+INSTANTIATE_TEST_SUITE_P(,
+ VideoFrameSubmitterTest,
+ testing::Bool(),
+ [](auto& info) {
+ return info.param ? "BeginFrameAcks"
+ : "CompositoFrameAcks";
+ });
+
} // namespace blink
diff --git a/third_party/blink/renderer/platform/widget/compositing/android_webview/synchronous_layer_tree_frame_sink.cc b/third_party/blink/renderer/platform/widget/compositing/android_webview/synchronous_layer_tree_frame_sink.cc
index 31c9155..28557b37 100644
--- a/third_party/blink/renderer/platform/widget/compositing/android_webview/synchronous_layer_tree_frame_sink.cc
+++ b/third_party/blink/renderer/platform/widget/compositing/android_webview/synchronous_layer_tree_frame_sink.cc
@@ -86,7 +86,10 @@
DCHECK(resources.empty());
}
void OnBeginFrame(const viz::BeginFrameArgs& args,
- const viz::FrameTimingDetailsMap& timing_details) override {
+ const viz::FrameTimingDetailsMap& timing_details,
+ bool frame_ack,
+ std::vector<viz::ReturnedResource> resources) override {
+ DCHECK(resources.empty());
}
void ReclaimResources(std::vector<viz::ReturnedResource> resources) override {
DCHECK(resources.empty());
@@ -587,8 +590,17 @@
void SynchronousLayerTreeFrameSink::OnBeginFrame(
const viz::BeginFrameArgs& args,
- const HashMap<uint32_t, viz::FrameTimingDetails>& timing_details) {
+ const HashMap<uint32_t, viz::FrameTimingDetails>& timing_details,
+ bool frame_ack,
+ Vector<viz::ReturnedResource> resources) {
DCHECK(viz_frame_submission_enabled_);
+ if (features::IsOnBeginFrameAcksEnabled()) {
+ if (frame_ack) {
+ DidReceiveCompositorFrameAck(std::move(resources));
+ } else if (!resources.empty()) {
+ ReclaimResources(std::move(resources));
+ }
+ }
// We do not receive BeginFrames via CompositorFrameSink, so we do not forward
// it to cc. We still might get one with FrameTimingDetailsMap, so we report
diff --git a/third_party/blink/renderer/platform/widget/compositing/android_webview/synchronous_layer_tree_frame_sink.h b/third_party/blink/renderer/platform/widget/compositing/android_webview/synchronous_layer_tree_frame_sink.h
index a9257ea..37d8d7d 100644
--- a/third_party/blink/renderer/platform/widget/compositing/android_webview/synchronous_layer_tree_frame_sink.h
+++ b/third_party/blink/renderer/platform/widget/compositing/android_webview/synchronous_layer_tree_frame_sink.h
@@ -115,9 +115,11 @@
// viz::mojom::CompositorFrameSinkClient implementation.
void DidReceiveCompositorFrameAck(
Vector<viz::ReturnedResource> resources) override;
- void OnBeginFrame(const viz::BeginFrameArgs& args,
- const HashMap<uint32_t, viz::FrameTimingDetails>&
- timing_details) override;
+ void OnBeginFrame(
+ const viz::BeginFrameArgs& args,
+ const HashMap<uint32_t, viz::FrameTimingDetails>& timing_details,
+ bool frame_ack,
+ Vector<viz::ReturnedResource> resources) override;
void ReclaimResources(Vector<viz::ReturnedResource> resources) override;
void OnBeginFramePausedChanged(bool paused) override;
void OnCompositorFrameTransitionDirectiveProcessed(
diff --git a/ui/compositor/layer_unittest.cc b/ui/compositor/layer_unittest.cc
index f726a23..7223fe6 100644
--- a/ui/compositor/layer_unittest.cc
+++ b/ui/compositor/layer_unittest.cc
@@ -1876,6 +1876,7 @@
// Moving, but not resizing, a layer should alert the observers.
observer.Reset();
l2->SetBounds(gfx::Rect(0, 0, 350, 350));
+ WaitForDraw();
WaitForSwap();
EXPECT_TRUE(observer.notified());
diff --git a/ui/compositor/test/direct_layer_tree_frame_sink.cc b/ui/compositor/test/direct_layer_tree_frame_sink.cc
index ba2781d..2510b1be 100644
--- a/ui/compositor/test/direct_layer_tree_frame_sink.cc
+++ b/ui/compositor/test/direct_layer_tree_frame_sink.cc
@@ -13,6 +13,7 @@
#include "build/build_config.h"
#include "cc/tiles/image_decode_cache_utils.h"
#include "cc/trees/layer_tree_frame_sink_client.h"
+#include "components/viz/common/features.h"
#include "components/viz/common/hit_test/hit_test_region_list.h"
#include "components/viz/common/quads/compositor_frame.h"
#include "components/viz/common/surfaces/frame_sink_id.h"
@@ -178,7 +179,16 @@
void DirectLayerTreeFrameSink::OnBeginFrame(
const viz::BeginFrameArgs& args,
- const viz::FrameTimingDetailsMap& timing_details) {
+ const viz::FrameTimingDetailsMap& timing_details,
+ bool frame_ack,
+ std::vector<viz::ReturnedResource> resources) {
+ if (features::IsOnBeginFrameAcksEnabled()) {
+ if (frame_ack) {
+ DidReceiveCompositorFrameAck(std::move(resources));
+ } else if (!resources.empty()) {
+ ReclaimResources(std::move(resources));
+ }
+ }
for (const auto& pair : timing_details)
client_->DidPresentCompositorFrame(pair.first, pair.second);
diff --git a/ui/compositor/test/direct_layer_tree_frame_sink.h b/ui/compositor/test/direct_layer_tree_frame_sink.h
index 177ef66..807fe70 100644
--- a/ui/compositor/test/direct_layer_tree_frame_sink.h
+++ b/ui/compositor/test/direct_layer_tree_frame_sink.h
@@ -80,7 +80,9 @@
void DidReceiveCompositorFrameAck(
std::vector<viz::ReturnedResource> resources) override;
void OnBeginFrame(const viz::BeginFrameArgs& args,
- const viz::FrameTimingDetailsMap& timing_details) override;
+ const viz::FrameTimingDetailsMap& timing_details,
+ bool frame_ack,
+ std::vector<viz::ReturnedResource> resource) override;
void ReclaimResources(std::vector<viz::ReturnedResource> resources) override;
void OnBeginFramePausedChanged(bool paused) override;
void OnCompositorFrameTransitionDirectiveProcessed(