Commit 033d9e89 authored by fsamuel's avatar fsamuel Committed by Commit bot

Retain references to surfaces from both active AND pending CompositorFrames

A Surface may hold both an active and a pending CompositorFrame at once.
Surfaces referenced by either CompositorFrame must be retained in order
to ensure correctness once the pending frame is activated. This CL
tracks references from both pending and active CompositorFrames within
a surface.

This CL also fixes a tear down issue in SurfaceDependencyTracker where Surfaces
could continue to call back into SurfaceDependencyTracker after it was
destroyed. SurfaceDependencyTracker stops observing Surfaces in its destructor.

BUG=672062
TBR=jam@chromium.org for mechanical renaming in content/
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2688043002
Cr-Commit-Position: refs/heads/master@{#449543}
parent ee80e680
......@@ -76,25 +76,34 @@ void CompositorFrameSinkSupport::SubmitCompositorFrame(
++ack_pending_count_;
float device_scale_factor = frame.metadata.device_scale_factor;
surface_factory_.SubmitCompositorFrame(
local_surface_id, std::move(frame),
base::Bind(&CompositorFrameSinkSupport::DidReceiveCompositorFrameAck,
weak_factory_.GetWeakPtr()));
if (surface_manager_->using_surface_references()) {
SurfaceId last_surface_id = reference_tracker_.current_surface_id();
// Populate list of surface references to add and remove based on reference
// surfaces in current frame compared with the last frame.
reference_tracker_.UpdateReferences(local_surface_id,
frame.metadata.referenced_surfaces);
surface_factory_.SubmitCompositorFrame(
local_surface_id, std::move(frame),
base::Bind(&CompositorFrameSinkSupport::DidReceiveCompositorFrameAck,
weak_factory_.GetWeakPtr()));
// surfaces in current frame compared with the last frame. The list of
// surface references includes references from both the pending and active
// frame if any.
SurfaceId current_surface_id(frame_sink_id_, local_surface_id);
Surface* surface = surface_manager_->GetSurfaceForId(current_surface_id);
// TODO(fsamuel): This is pretty inefficent. We copy over referenced
// surfaces. Then we pass them into the ReferencedSurfaceTracker to copy
// them again into a set. ReferencedSurfaceTracker should just take in two
// vectors, one for pending referenced surfaces and one for active
// referenced surfaces.
std::vector<SurfaceId> referenced_surfaces =
surface->active_referenced_surfaces();
referenced_surfaces.insert(referenced_surfaces.end(),
surface->pending_referenced_surfaces().begin(),
surface->pending_referenced_surfaces().end());
reference_tracker_.UpdateReferences(local_surface_id, referenced_surfaces);
UpdateSurfaceReferences(last_surface_id, local_surface_id);
} else {
surface_factory_.SubmitCompositorFrame(
local_surface_id, std::move(frame),
base::Bind(&CompositorFrameSinkSupport::DidReceiveCompositorFrameAck,
weak_factory_.GetWeakPtr()));
}
if (display_)
......
......@@ -46,6 +46,10 @@ class CC_SURFACES_EXPORT CompositorFrameSinkSupport
return surface_factory_.current_surface_for_testing();
}
const ReferencedSurfaceTracker& ReferenceTrackerForTesting() const {
return reference_tracker_;
}
void EvictFrame();
void SetNeedsBeginFrame(bool needs_begin_frame);
void SubmitCompositorFrame(const LocalSurfaceId& local_surface_id,
......
......@@ -50,13 +50,17 @@ CompositorFrame MakeCompositorFrame(
class CompositorFrameSinkSupportTest : public testing::Test,
public CompositorFrameSinkSupportClient {
public:
CompositorFrameSinkSupportTest() {}
CompositorFrameSinkSupportTest()
: surface_manager_(SurfaceManager::LifetimeType::REFERENCES) {}
~CompositorFrameSinkSupportTest() override {}
CompositorFrameSinkSupport& parent_support() { return *supports_[0]; }
Surface* parent_surface() {
return parent_support().current_surface_for_testing();
}
const ReferencedSurfaceTracker& parent_reference_tracker() {
return parent_support().ReferenceTrackerForTesting();
}
CompositorFrameSinkSupport& child_support1() { return *supports_[1]; }
Surface* child_surface1() {
......@@ -105,7 +109,11 @@ class CompositorFrameSinkSupportTest : public testing::Test,
void TearDown() override {
surface_manager_.SetDependencyTracker(nullptr);
// SurfaceDependencyTracker depends on this BeginFrameSource and so it must
// be destroyed AFTER the dependency tracker is destroyed.
begin_frame_source_.reset();
supports_.clear();
}
......@@ -387,5 +395,53 @@ TEST_F(CompositorFrameSinkSupportTest,
EXPECT_THAT(parent_surface()->blocking_surfaces_for_testing(), IsEmpty());
}
// This test verifies that the set of references from a Surface includes both
// the pending and active CompositorFrames.
TEST_F(CompositorFrameSinkSupportTest,
DisplayCompositorLockingReferencesFromPendingAndActiveFrames) {
const SurfaceId parent_id = MakeSurfaceId(kParentFrameSink, 1);
const SurfaceId child_id = MakeSurfaceId(kChildFrameSink1, 1);
const SurfaceId arbitrary_id = MakeSurfaceId(kArbitraryFrameSink, 1);
const SurfaceReference parent_child_reference(parent_id, child_id);
const SurfaceReference parent_arbitrary_reference(parent_id, arbitrary_id);
// child_support1 submits a CompositorFrame without any dependencies.
child_support1().SubmitCompositorFrame(
child_id.local_surface_id(), MakeCompositorFrame(empty_surface_ids()));
// Verify that the child surface is not blocked.
EXPECT_TRUE(child_surface1()->HasActiveFrame());
EXPECT_FALSE(child_surface1()->HasPendingFrame());
EXPECT_THAT(child_surface1()->blocking_surfaces_for_testing(), IsEmpty());
// parent_support submits a CompositorFrame that depends on |child_id1|
// which is already active. Thus, this CompositorFrame should activate
// immediately.
parent_support().SubmitCompositorFrame(parent_id.local_surface_id(),
MakeCompositorFrame({child_id}));
EXPECT_TRUE(parent_surface()->HasActiveFrame());
EXPECT_FALSE(parent_surface()->HasPendingFrame());
EXPECT_THAT(parent_surface()->blocking_surfaces_for_testing(), IsEmpty());
// Verify that the parent will add a reference to the |child_id|.
EXPECT_THAT(parent_reference_tracker().references_to_add(),
UnorderedElementsAre(parent_child_reference));
EXPECT_THAT(parent_reference_tracker().references_to_remove(), IsEmpty());
// parent_support now submits another CompositorFrame to the same surface
// but depends on arbitrary_id. The parent surface should now have both
// a pending and active CompositorFrame.
parent_support().SubmitCompositorFrame(parent_id.local_surface_id(),
MakeCompositorFrame({arbitrary_id}));
EXPECT_TRUE(parent_surface()->HasActiveFrame());
EXPECT_TRUE(parent_surface()->HasPendingFrame());
EXPECT_THAT(parent_surface()->blocking_surfaces_for_testing(),
UnorderedElementsAre(arbitrary_id));
// Verify that the parent will add a reference to |arbitrary_id| and will not
// remove a reference to |child_id|.
EXPECT_THAT(parent_reference_tracker().references_to_add(),
UnorderedElementsAre(parent_arbitrary_reference));
EXPECT_THAT(parent_reference_tracker().references_to_remove(), IsEmpty());
}
} // namespace test
} // namespace cc
......@@ -63,6 +63,8 @@ void Surface::QueueFrame(CompositorFrame frame, const DrawCallback& callback) {
pending_frame_ = std::move(frame);
if (pending_frame_) {
factory_->ReceiveFromChild(pending_frame_->resource_list);
pending_referenced_surfaces_ =
pending_frame_->metadata.referenced_surfaces;
// Ask the surface manager to inform |this| when its dependencies are
// resolved.
factory_->manager()->RequestSurfaceResolution(this);
......@@ -149,6 +151,7 @@ void Surface::ActivatePendingFrame() {
DCHECK(pending_frame_);
ActivateFrame(std::move(pending_frame_.value()));
pending_frame_.reset();
pending_referenced_surfaces_.clear();
// ActiveFrame resources are now double ref-ed. Unref.
UnrefFrameResources(*active_frame_);
}
......@@ -177,7 +180,7 @@ void Surface::ActivateFrame(CompositorFrame frame) {
if (previous_frame)
UnrefFrameResources(*previous_frame);
referenced_surfaces_ = active_frame_->metadata.referenced_surfaces;
active_referenced_surfaces_ = active_frame_->metadata.referenced_surfaces;
for (auto& observer : observers_)
observer.OnSurfaceActivated(this);
......
......@@ -101,8 +101,12 @@ class CC_SURFACES_EXPORT Surface {
return destruction_dependencies_.size();
}
const std::vector<SurfaceId>& referenced_surfaces() const {
return referenced_surfaces_;
const std::vector<SurfaceId>& active_referenced_surfaces() const {
return active_referenced_surfaces_;
}
const std::vector<SurfaceId>& pending_referenced_surfaces() const {
return pending_referenced_surfaces_;
}
const SurfaceDependencies& blocking_surfaces_for_testing() const {
......@@ -140,8 +144,19 @@ class CC_SURFACES_EXPORT Surface {
// on multiple Displays.
std::set<BeginFrameSource*> begin_frame_sources_;
// The total set of CompositorFrames referenced by the active CompositorFrame.
std::vector<SurfaceId> referenced_surfaces_;
// The set of SurfaceIds referenced by the active CompositorFrame.
// TODO(fsamuel): It seems unnecessary to copy this vector over
// from CompostiorFrameMetadata to store locally here. We can simply
// provide an accessor to the referenced surfaces directly from
// CompositorFrameMetadata.
std::vector<SurfaceId> active_referenced_surfaces_;
// The set of SurfaceIds referenced by the pending CompositorFrame.
// TODO(fsamuel): It seems unnecessary to copy this vector over
// from CompostiorFrameMetadata to store locally here. We can simply
// provide an accessor to the referenced surfaces directly from
// CompositorFrameMetadata.
std::vector<SurfaceId> pending_referenced_surfaces_;
SurfaceDependencies blocking_surfaces_;
base::ObserverList<PendingFrameObserver, true> observers_;
......
......@@ -26,6 +26,9 @@ SurfaceDependencyTracker::SurfaceDependencyTracker(
SurfaceDependencyTracker::~SurfaceDependencyTracker() {
surface_manager_->RemoveObserver(this);
begin_frame_source_->RemoveObserver(this);
for (Surface* pending_surface : pending_surfaces_)
pending_surface->RemoveObserver(this);
pending_surfaces_.clear();
}
void SurfaceDependencyTracker::RequestSurfaceResolution(Surface* surface) {
......
......@@ -327,7 +327,9 @@ SurfaceManager::SurfaceIdSet SurfaceManager::GetLiveSurfacesForSequences() {
Surface* surf = surface_map_[live_surfaces[i]];
DCHECK(surf);
for (const SurfaceId& id : surf->referenced_surfaces()) {
// TODO(fsamuel): We should probably keep alive pending referenced surfaces
// too.
for (const SurfaceId& id : surf->active_referenced_surfaces()) {
if (live_surfaces_set.count(id))
continue;
......
......@@ -1171,9 +1171,9 @@ bool ContainsSurfaceId(cc::SurfaceId container_surface_id,
RenderWidgetHostViewChildFrame* target_view) {
if (!container_surface_id.is_valid())
return false;
for (cc::SurfaceId id :
GetSurfaceManager()->GetSurfaceForId(container_surface_id)
->referenced_surfaces()) {
for (cc::SurfaceId id : GetSurfaceManager()
->GetSurfaceForId(container_surface_id)
->active_referenced_surfaces()) {
if (id == target_view->SurfaceIdForTesting() ||
ContainsSurfaceId(id, target_view))
return true;
......
......@@ -324,7 +324,7 @@ bool SurfaceHitTestReadyNotifier::ContainsSurfaceId(
return false;
for (cc::SurfaceId id :
surface_manager_->GetSurfaceForId(container_surface_id)
->referenced_surfaces()) {
->active_referenced_surfaces()) {
if (id == target_view_->SurfaceIdForTesting() || ContainsSurfaceId(id))
return true;
}
......
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