Skip to content

GitLab

T
tangled
Commit 75806208 authored by Nazih Almalki's avatar Nazih Almalki Committed by Commit Bot
Browse files
Options

revert Early Ack Pending Queued Surfaces 

Bug: 1060058
Bug: 1087865
Bug: 1091977
Bug: 1091555
Bug: 1091454
Bug: 1091453
Bug: 1091076
Change-Id: I1e6a67c7704b0df984df8fb18af7f236fa6e4ffc
Bug: 1087865
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2235653Reviewed-by: default avatarJonathan Ross <jonross@chromium.org>
Commit-Queue: Nazih Almalki <nalmalki@google.com>
Cr-Commit-Position: refs/heads/master@{#776247}
parent 3e4a14ad
Hide whitespace changes
Inline Side-by-side
Showing with 23 additions and 146 deletions
components/viz/service/frame_sinks/compositor_frame_sink_support.cc
View file @ 75806208
......@@ -184,7 +184,7 @@ void CompositorFrameSinkSupport::OnFrameTokenChanged(uint32_t frame_token) {
}
void CompositorFrameSinkSupport::OnSurfaceProcessed(Surface* surface) {
DidReceiveCompositorFrameAck(/*early_ack*/ false);
DidReceiveCompositorFrameAck();
}
void CompositorFrameSinkSupport::OnSurfaceAggregatedDamage(
......@@ -556,37 +556,6 @@ SubmitResult CompositorFrameSinkSupport::MaybeSubmitCompositorFrame(
// Make sure we periodically check if the frame should activate.
pending_surfaces_.insert(current_surface);
UpdateNeedsBeginFramesInternal();
#if defined(OS_ANDROID)
// Ack a pending surface early the client's scheduler to schedule frames
// for future surface changes This case happens when screen rotation takes
// place while there is an active frame when this takes place it has a
// unique signature of 2 consecutive submit frame requests with
// clients_needs_begin_frame_ is set (high) and 1st frame is not a new
// surface and 2nd frame is a new surface. For only this sequence send an
// early ack to the client by using OnSurfaceProcessed. Setting the
// early_ack during the call to denote that an early ACK took place. This
// prevents sending a second ack for the frame when it is activated. To
// ensure only this sequence is early acked, the
// last_pending_frame_was_new_surface_ flag is used to ensure the sequence
// is correct using current_surface == prev_surface => NOT new_surface ID,
// to limit the when the early ack is sent.
// In some cases the 2 frame comes and require early ack before the
// surface sync work is completed add counter early_acked_count_ to make
// sure right ack get cleared
if (client_needs_begin_frame_) {
if ((current_surface == prev_surface) ||
last_pending_frame_was_new_surface_) {
early_acked_count_++;
DidReceiveCompositorFrameAck(/*early_ack*/ true);
last_pending_frame_was_new_surface_ =
!last_pending_frame_was_new_surface_;
} else {
if (current_surface != prev_surface) {
last_pending_frame_was_new_surface_ = false;
}
}
}
#endif
break;
case Surface::QueueFrameResult::ACCEPTED_ACTIVE:
// Nothing to do here.
......@@ -617,39 +586,22 @@ void CompositorFrameSinkSupport::HandleCallback() {
surface_returned_resources_.clear();
}
void CompositorFrameSinkSupport::DidReceiveCompositorFrameAck(bool early_ack) {
// Only do early ack on frame that don't have callback
if (early_ack) {
DCHECK_GT(early_acked_count_, 0);
if (!client_ || compositor_frame_callback_)
return;
// early Ack client
client_->DidReceiveCompositorFrameAck(surface_returned_resources_);
// don't clear resources_ as it is still been used by surface_manager
} else { // actual ack
DCHECK_GT(ack_pending_count_, 0);
ack_pending_count_--;
if (!client_)
return;
// If we have a callback, we only return the resource on onBeginFrame.
if (compositor_frame_callback_) {
if (early_acked_count_ > 0) // if there was an early ack decrement it.
early_acked_count_--;
callback_received_receive_ack_ = true;
UpdateNeedsBeginFramesInternal();
HandleCallback();
return;
}
void CompositorFrameSinkSupport::DidReceiveCompositorFrameAck() {
DCHECK_GT(ack_pending_count_, 0);
ack_pending_count_--;
if (!client_)
return;
if (early_acked_count_ == 0) { // no early ACK has been sent, send an ACK
client_->DidReceiveCompositorFrameAck(surface_returned_resources_);
} else { // early count is bigger than zero, decrement it
early_acked_count_--;
}
// for any ack request, that is not early clear resources
surface_returned_resources_.clear();
// If we have a callback, we only return the resource on onBeginFrame.
if (compositor_frame_callback_) {
callback_received_receive_ack_ = true;
UpdateNeedsBeginFramesInternal();
HandleCallback();
return;
}
client_->DidReceiveCompositorFrameAck(surface_returned_resources_);
surface_returned_resources_.clear();
}
void CompositorFrameSinkSupport::DidPresentCompositorFrame(
......@@ -688,15 +640,15 @@ void CompositorFrameSinkSupport::DidRejectCompositorFrame(
std::vector<ui::LatencyInfo> latency_info) {
TRACE_EVENT_INSTANT0("viz", "DidRejectCompositorFrame",
TRACE_EVENT_SCOPE_THREAD);
// TODO(eseckler): Should these be stored and attached to the next
// successful frame submission instead?
// TODO(eseckler): Should these be stored and attached to the next successful
// frame submission instead?
for (ui::LatencyInfo& info : latency_info)
info.Terminate();
std::vector<ReturnedResource> resources =
TransferableResource::ReturnResources(frame_resource_list);
ReturnResources(resources);
DidReceiveCompositorFrameAck(/*early_ack*/ false);
DidReceiveCompositorFrameAck();
DidPresentCompositorFrame(frame_token, base::TimeTicks(), gfx::SwapTimings(),
gfx::PresentationFeedback::Failure());
}
......@@ -883,8 +835,8 @@ bool CompositorFrameSinkSupport::ShouldSendBeginFrame(
return false;
}
// We might throttle this OnBeginFrame() if it's been less than a second
// since the last one was sent.
// We might throttle this OnBeginFrame() if it's been less than a second since
// the last one was sent.
bool can_throttle =
(frame_time - last_frame_time_) < base::TimeDelta::FromSeconds(1);
......@@ -900,8 +852,8 @@ bool CompositorFrameSinkSupport::ShouldSendBeginFrame(
return true;
}
// We should never throttle BeginFrames if there is another client waiting
// for this client to submit a frame.
// We should never throttle BeginFrames if there is another client waiting for
// this client to submit a frame.
if (surface_manager_->HasBlockedEmbedder(frame_sink_id_)) {
RecordShouldSendBeginFrame(SendBeginFrameResult::kSendBlockedEmbedded);
return true;
......
components/viz/service/frame_sinks/compositor_frame_sink_support.h
View file @ 75806208
......@@ -208,7 +208,7 @@ class VIZ_SERVICE_EXPORT CompositorFrameSinkSupport
// Creates a surface reference from the top-level root to |surface_id|.
SurfaceReference MakeTopLevelRootReference(const SurfaceId& surface_id);
void DidReceiveCompositorFrameAck(bool early_ack);
void DidReceiveCompositorFrameAck();
void DidPresentCompositorFrame(uint32_t frame_token,
base::TimeTicks draw_start_timestamp,
const gfx::SwapTimings& swap_timings,
......@@ -322,8 +322,6 @@ class VIZ_SERVICE_EXPORT CompositorFrameSinkSupport
compositor_frame_callback_;
bool callback_received_begin_frame_ = true;
bool callback_received_receive_ack_ = true;
bool last_pending_frame_was_new_surface_ = false;
uint8_t early_acked_count_ = 0;
uint32_t trace_sequence_ = 0;
BeginFrameTracker begin_frame_tracker_;
......
components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc
View file @ 75806208
......@@ -112,7 +112,6 @@ class CompositorFrameSinkSupportTest : public testing::Test {
manager_.InvalidateFrameSinkId(kArbitraryFrameSinkId);
manager_.surface_manager()->RemoveObserver(&surface_observer_);
}
bool GetEarlyAckCount() const { return support_->early_acked_count_; }
void AddResourcesToFrame(CompositorFrame* frame,
ResourceId* resource_ids,
......@@ -1439,76 +1438,4 @@ TEST_F(CompositorFrameSinkSupportTest, ThrottleUnresponsiveClient) {
support->SetNeedsBeginFrame(false);
}
#if defined(OS_ANDROID)
// Check CompositorFrame get an early ack if the surface id needs to have
// pending child needsBeginFrame = true, and NOT new surface ID, then followed
// by needsBeginFrame = true, and new Surface ID, other classes should not get
// an early ACK. crbug.com/1060058)
TEST_F(CompositorFrameSinkSupportTest, PassesEarlyAcks) {
SurfaceId id(support_->frame_sink_id(), local_surface_id_);
LocalSurfaceId child_local_surface_id(1, kAnotherArbitraryToken);
SurfaceId child_id(kAnotherArbitraryFrameSinkId, child_local_surface_id);
MockCompositorFrameSinkClient mock_client;
uint8_t EarlyAckCount;
support_->SetNeedsBeginFrame(true);
// Submit a frame with size (5,5).
auto frame = CompositorFrameBuilder()
.AddRenderPass(gfx::Rect(5, 5), gfx::Rect())
.SetActivationDependencies({child_id})
.Build();
auto result = support_->MaybeSubmitCompositorFrame(
local_surface_id_, std::move(frame), base::nullopt, 0,
mojom::CompositorFrameSink::SubmitCompositorFrameSyncCallback());
// should get NOT early ACK, NeedsBeginFrame= true, and new surface ID (need
// to have a NOT new Surface ID)
EXPECT_EQ(GetEarlyAckCount(), 0);
// send a same surface id with new dependencies.
LocalSurfaceId child_local_surface_id2(2, kAnotherArbitraryToken);
SurfaceId child_id2(kAnotherArbitraryFrameSinkId, child_local_surface_id2);
frame = CompositorFrameBuilder()
.AddRenderPass(gfx::Rect(5, 5), gfx::Rect())
.SetActivationDependencies({child_id2})
.Build();
result = support_->MaybeSubmitCompositorFrame(
local_surface_id_, std::move(frame), base::nullopt, 0,
mojom::CompositorFrameSink::SubmitCompositorFrameSyncCallback());
EXPECT_EQ(SubmitResult::ACCEPTED, result);
// should get early ACK, count should be 1, NeedsBeginFrame= true, and not new
// surface ID
EXPECT_EQ(GetEarlyAckCount(), 1);
EarlyAckCount = GetEarlyAckCount();
// send new surface id with dependencies.
LocalSurfaceId local_surface_id2(6, kArbitraryToken);
LocalSurfaceId child_local_surface_id3(2, kAnotherArbitraryToken);
SurfaceId child_id3(kAnotherArbitraryFrameSinkId, child_local_surface_id3);
frame = CompositorFrameBuilder()
.AddRenderPass(gfx::Rect(5, 5), gfx::Rect())
.SetActivationDependencies({child_id3})
.Build();
result = support_->MaybeSubmitCompositorFrame(
local_surface_id2, std::move(frame), base::nullopt, 0,
mojom::CompositorFrameSink::SubmitCompositorFrameSyncCallback());
// should get early ACK, NeedsBeginFrame= true, and 1st new surface ID
// hence the count should not drop, it be either 1 or 2
EXPECT_GE(GetEarlyAckCount(), EarlyAckCount);
EarlyAckCount = GetEarlyAckCount();
// set a surface with needbeginframe = false, should not get early ack
support_->SetNeedsBeginFrame(false);
frame = CompositorFrameBuilder()
.AddRenderPass(gfx::Rect(5, 5), gfx::Rect())
.Build();
result = support_->MaybeSubmitCompositorFrame(
local_surface_id2, std::move(frame), base::nullopt, 0,
mojom::CompositorFrameSink::SubmitCompositorFrameSyncCallback());
// should get early NOT ACK, hence it the count should not increase, it could
// decrease
EXPECT_LE(GetEarlyAckCount(), EarlyAckCount);
}
#endif
} // namespace viz
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment