Skip to content

Commit

Permalink
Move logic for GetLatestInFlightSurface to SurfaceAllocationGroup
Browse files Browse the repository at this point in the history 
The current logic is probably wrong if a client changes its embed
token.

Bug: 931801
Change-Id: I9ce49fa2fd8efccd5a556483d6c24bb81d95a783
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1504212
Auto-Submit: Saman Sami <samans@chromium.org>
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Commit-Queue: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#638098}
  • Loading branch information
Saman Sami authored and Commit Bot committed Mar 6, 2019
1 parent 24a287c commit edfa68c
Show file tree
Hide file tree
Showing 5 changed files with 148 additions and 119 deletions.
40 changes: 23 additions & 17 deletions components/viz/service/frame_sinks/surface_synchronization_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,9 @@ class SurfaceSynchronizationTest : public testing::Test {
}

FrameSinkManagerImpl& frame_sink_manager() { return frame_sink_manager_; }
SurfaceManager* surface_manager() {
return frame_sink_manager_.surface_manager();
}

// Returns all the references where |surface_id| is the parent.
const base::flat_set<SurfaceId>& GetChildReferences(
Expand All @@ -135,12 +138,6 @@ class SurfaceSynchronizationTest : public testing::Test {
surface_id);
}

// Returns true if there is a Persistent reference for |surface_id|.
bool HasPersistentReference(const SurfaceId& surface_id) {
return frame_sink_manager().surface_manager()->HasPersistentReference(
surface_id);
}

Surface* GetLatestInFlightSurface(const SurfaceRange& surface_range) {
return frame_sink_manager().surface_manager()->GetLatestInFlightSurface(
surface_range);
Expand Down Expand Up @@ -2058,9 +2055,11 @@ TEST_F(SurfaceSynchronizationTest, LatestInFlightSurface) {
GetLatestInFlightSurface(SurfaceRange(child_id1, child_id3)));
}

// This test verifies that GetLatestInFlightSurface will return nullptr
// if it has a bogus fallback SurfaceID.
TEST_F(SurfaceSynchronizationTest, LatestInFlightSurfaceWithBogusFallback) {
// This test verifies that GetLatestInFlightSurface will return nullptr when the
// start of the range is newer than its end, even if a surface matching the end
// exists.
TEST_F(SurfaceSynchronizationTest,
LatestInFlightSurfaceWithInvalidSurfaceRange) {
const SurfaceId parent_id = MakeSurfaceId(kParentFrameSink, 1);
const SurfaceId child_id1 = MakeSurfaceId(kChildFrameSink1, 1);
const SurfaceId child_id2 = MakeSurfaceId(kChildFrameSink1, 2);
Expand All @@ -2084,12 +2083,11 @@ TEST_F(SurfaceSynchronizationTest, LatestInFlightSurfaceWithBogusFallback) {

const SurfaceId bogus_child_id = MakeSurfaceId(kChildFrameSink1, 10);

// If primary exists and active return it regardless of the fallback.
EXPECT_EQ(GetSurfaceForId(child_id1),
// The end exists but don't return it because the start is newer than the end.
EXPECT_EQ(nullptr,
GetLatestInFlightSurface(SurfaceRange(bogus_child_id, child_id1)));

// If primary is not active and fallback is doesn't exist, we always return
// nullptr.
// In this case, the end doesn't exist either. Still return nullptr.
EXPECT_EQ(nullptr,
GetLatestInFlightSurface(SurfaceRange(bogus_child_id, child_id2)));
}
Expand Down Expand Up @@ -2310,15 +2308,19 @@ TEST_F(SurfaceSynchronizationTest,

// |child_id1| now should have a temporary reference.
EXPECT_TRUE(HasTemporaryReference(child_id1));
EXPECT_FALSE(HasPersistentReference(child_id1));
EXPECT_TRUE(surface_manager()
->GetSurfacesThatReferenceChildForTesting(child_id1)
.empty());

// Activate |child_id2|.
child_support1().SubmitCompositorFrame(child_id2.local_surface_id(),
MakeDefaultCompositorFrame());

// |child_id2| now should have a temporary reference.
EXPECT_TRUE(HasTemporaryReference(child_id2));
EXPECT_FALSE(HasPersistentReference(child_id2));
EXPECT_TRUE(surface_manager()
->GetSurfacesThatReferenceChildForTesting(child_id2)
.empty());

// Create a reference from |parent_id| to |child_id2|.
parent_support().SubmitCompositorFrame(
Expand All @@ -2328,11 +2330,15 @@ TEST_F(SurfaceSynchronizationTest,

// |child_id1| have no references and can be garbage collected.
EXPECT_FALSE(HasTemporaryReference(child_id1));
EXPECT_FALSE(HasPersistentReference(child_id1));
EXPECT_TRUE(surface_manager()
->GetSurfacesThatReferenceChildForTesting(child_id1)
.empty());

// |child_id2| has a persistent references now.
EXPECT_FALSE(HasTemporaryReference(child_id2));
EXPECT_TRUE(HasPersistentReference(child_id2));
EXPECT_FALSE(surface_manager()
->GetSurfacesThatReferenceChildForTesting(child_id2)
.empty());

// Verify that GetLatestInFlightSurface returns |child_id2|.
EXPECT_EQ(GetSurfaceForId(child_id2),
Expand Down
78 changes: 78 additions & 0 deletions components/viz/service/surfaces/surface_allocation_group.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,82 @@ void SurfaceAllocationGroup::UnregisterSurface(Surface* surface) {
surfaces_.erase(it);
}

Surface* SurfaceAllocationGroup::FindLatestActiveSurfaceInRange(
const SurfaceRange& range) const {
// If the embed token of the end of the SurfaceRange matches that of this
// group, find the latest active surface that is older than or equal to the
// end, then check that it's not older than start.
if (range.end().local_surface_id().embed_token() == embed_token_) {
DCHECK_EQ(submitter_, range.end().frame_sink_id());
Surface* result = FindOlderOrEqual(range.end());
if (result &&
(!range.start() || !range.start()->IsNewerThan(result->surface_id()))) {
return result;
} else {
return nullptr;
}
}

// If we are here, the embed token of the end of the range doesn't match this
// group's embed token. In this case, the range must have a start and its
// embed token must match this group. Simply find the last active surface, and
// check whether it's newer than the range's start.
DCHECK(range.start());
DCHECK_EQ(embed_token_, range.start()->local_surface_id().embed_token());
DCHECK_NE(embed_token_, range.end().local_surface_id().embed_token());
DCHECK_EQ(submitter_, range.start()->frame_sink_id());

Surface* result = nullptr;
// Normally there is at most one pending surface, so this for loop shouldn't
// take more than two iterations.
for (int i = surfaces_.size() - 1; i >= 0; i--) {
if (surfaces_[i]->HasActiveFrame()) {
result = surfaces_[i];
break;
}
}
if (result && range.start()->IsNewerThan(result->surface_id()))
return nullptr;
return result;
}

Surface* SurfaceAllocationGroup::FindOlderOrEqual(
const SurfaceId& surface_id) const {
DCHECK_EQ(submitter_, surface_id.frame_sink_id());
DCHECK_EQ(embed_token_, surface_id.local_surface_id().embed_token());

// Return early if there are no surfaces in this group.
if (surfaces_.empty())
return nullptr;

// If even the first surface is newer than |surface_id|, we can't find a
// surface that is older than or equal to |surface_id|.
if (surfaces_[0]->surface_id().IsNewerThan(surface_id))
return nullptr;

// Perform a binary search the find the latest active surface that is older
// than or equal to |surface_id|.
int begin = 0;
int end = surfaces_.size();
while (end - begin > 1) {
int avg = (begin + end) / 2;
if (surfaces_[avg]->surface_id().IsNewerThan(surface_id))
end = avg;
else
begin = avg;
}

// We have found the latest surface. Now keep iterating back until we find an
// active surface. Normally, there is only one pending surface at a time, so
// this shouldn't take more than two iterations.
for (; begin >= 0; --begin) {
if (surfaces_[begin]->HasActiveFrame())
return surfaces_[begin];
}

// No active surface was found, so return null.
DCHECK_EQ(-1, begin);
return nullptr;
}

} // namespace viz
12 changes: 11 additions & 1 deletion components/viz/service/surfaces/surface_allocation_group.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

#include "base/unguessable_token.h"
#include "components/viz/common/surfaces/frame_sink_id.h"
#include "components/viz/common/surfaces/surface_range.h"
#include "components/viz/service/viz_service_export.h"

namespace viz {
Expand Down Expand Up @@ -41,12 +42,21 @@ class VIZ_SERVICE_EXPORT SurfaceAllocationGroup {
// allocation group.
void UnregisterSurface(Surface* surface);

// Returns the latest active surface in the given range that is a part of this
// allocation group. The embed token of at least one end of the range must
// match the embed token of this group.
Surface* FindLatestActiveSurfaceInRange(const SurfaceRange& range) const;

// Returns the last surface created in this allocation group.
Surface* last_created_surface() {
Surface* last_created_surface() const {
return surfaces_.empty() ? nullptr : surfaces_.back();
}

private:
// Helper method for FindLatestActiveSurfaceInRange. Returns the latest active
// surface whose SurfaceId is older than or equal to |surface_id|.
Surface* FindOlderOrEqual(const SurfaceId& surface_id) const;

// The ID of the FrameSink that is submitting to the surfaces in this
// allocation group.
const FrameSinkId submitter_;
Expand Down
119 changes: 31 additions & 88 deletions components/viz/service/surfaces/surface_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ SurfaceManager::~SurfaceManager() {
// destroyed.
temporary_references_.clear();
temporary_reference_ranges_.clear();
persistent_references_by_frame_sink_id_.clear();
// Create a copy of the children set as RemoveSurfaceReferenceImpl below will
// mutate that set.
base::flat_set<SurfaceId> children(
Expand Down Expand Up @@ -249,43 +248,6 @@ SurfaceManager::GetSurfacesThatReferenceChildForTesting(
return parents;
}

Surface* SurfaceManager::GetLatestInFlightSurfaceForFrameSinkId(
const SurfaceRange& surface_range,
const FrameSinkId& sink_id) {
std::vector<LocalSurfaceId> valid_local_surfaces;
// Get all valid temporary references.
auto temporary_it = temporary_reference_ranges_.find(sink_id);
if (temporary_it != temporary_reference_ranges_.end()) {
for (const LocalSurfaceId& local_id : temporary_it->second) {
if (surface_range.IsInRangeInclusive(SurfaceId(sink_id, local_id)))
valid_local_surfaces.push_back(local_id);
}
}

// Get all valid persistent references.
auto persistent_it = persistent_references_by_frame_sink_id_.find(sink_id);
if (persistent_it != persistent_references_by_frame_sink_id_.end()) {
for (const LocalSurfaceId& local_id : persistent_it->second) {
if (surface_range.IsInRangeInclusive(SurfaceId(sink_id, local_id)))
valid_local_surfaces.push_back(local_id);
}
}

// Sort all possible surfaces from newest to oldest, then return the first
// surface that has an active frame.
std::sort(valid_local_surfaces.begin(), valid_local_surfaces.end(),
[](const LocalSurfaceId& first, const LocalSurfaceId& second) {
return first > second;
});

for (const LocalSurfaceId& local_surface_id : valid_local_surfaces) {
Surface* surface = GetSurfaceForId(SurfaceId(sink_id, local_surface_id));
if (surface && surface->HasActiveFrame())
return surface;
}
return nullptr;
}

SurfaceManager::SurfaceIdSet SurfaceManager::GetLiveSurfacesForReferences() {
SurfaceIdSet reachable_surfaces;

Expand Down Expand Up @@ -343,10 +305,6 @@ void SurfaceManager::AddSurfaceReferenceImpl(

references_[parent_id].insert(child_id);

// Add a real reference to child_id.
persistent_references_by_frame_sink_id_[child_id.frame_sink_id()].insert(
child_id.local_surface_id());

for (auto& observer : observer_list_)
observer.OnAddedSurfaceReference(parent_id, child_id);

Expand All @@ -373,36 +331,12 @@ void SurfaceManager::RemoveSurfaceReferenceImpl(
iter_parent->second.erase(child_iter);
if (iter_parent->second.empty())
references_.erase(iter_parent);

// Remove the presistent reference.
const FrameSinkId& sink_id = child_id.frame_sink_id();
const LocalSurfaceId& local_id = child_id.local_surface_id();

auto sink_it = persistent_references_by_frame_sink_id_.find(sink_id);
if (sink_it == persistent_references_by_frame_sink_id_.end())
return;

auto local_surface_it = sink_it->second.find(local_id);
if (local_surface_it == sink_it->second.end())
return;

sink_it->second.erase(local_surface_it);
if (sink_it->second.empty())
persistent_references_by_frame_sink_id_.erase(sink_it);
}

bool SurfaceManager::HasTemporaryReference(const SurfaceId& surface_id) const {
return temporary_references_.count(surface_id) != 0;
}

bool SurfaceManager::HasPersistentReference(const SurfaceId& surface_id) const {
auto it =
persistent_references_by_frame_sink_id_.find(surface_id.frame_sink_id());
if (it == persistent_references_by_frame_sink_id_.end())
return false;
return it->second.count(surface_id.local_surface_id()) != 0;
}

void SurfaceManager::AddTemporaryReference(const SurfaceId& surface_id) {
DCHECK(!HasTemporaryReference(surface_id));

Expand Down Expand Up @@ -462,29 +396,24 @@ void SurfaceManager::RemoveTemporaryReferenceImpl(const SurfaceId& surface_id,

Surface* SurfaceManager::GetLatestInFlightSurface(
const SurfaceRange& surface_range) {
// If primary exists, we return it.
Surface* primary_surface = GetSurfaceForId(surface_range.end());
if (primary_surface && primary_surface->HasActiveFrame())
return primary_surface;

// If both end of the range exists, we try the primary's FrameSinkId first.
Surface* latest_surface = GetLatestInFlightSurfaceForFrameSinkId(
surface_range, surface_range.end().frame_sink_id());

// If the fallback has a different FrameSinkId, then try that also.
if (!latest_surface && surface_range.HasDifferentFrameSinkIds()) {
latest_surface = GetLatestInFlightSurfaceForFrameSinkId(
surface_range, surface_range.start()->frame_sink_id());
SurfaceAllocationGroup* end_allocation_group =
GetAllocationGroupForSurfaceId(surface_range.end());
if (end_allocation_group) {
Surface* result =
end_allocation_group->FindLatestActiveSurfaceInRange(surface_range);
if (result)
return result;
}

// Fallback might have neither temporary or presistent references, so we
// consider it separately.
if (!latest_surface && surface_range.start())
latest_surface = GetSurfaceForId(*surface_range.start());

if (latest_surface && latest_surface->HasActiveFrame())
return latest_surface;
return nullptr;
if (!surface_range.start() ||
surface_range.start()->local_surface_id().embed_token() ==
surface_range.end().local_surface_id().embed_token()) {
return nullptr;
}
SurfaceAllocationGroup* start_allocation_group =
GetAllocationGroupForSurfaceId(*surface_range.start());
if (!start_allocation_group)
return nullptr;
return start_allocation_group->FindLatestActiveSurfaceInRange(surface_range);
}

void SurfaceManager::ExpireOldTemporaryReferences() {
Expand Down Expand Up @@ -687,4 +616,18 @@ SurfaceAllocationGroup* SurfaceManager::GetOrCreateAllocationGroupForSurfaceId(
return allocation_group.get();
}

SurfaceAllocationGroup* SurfaceManager::GetAllocationGroupForSurfaceId(
const SurfaceId& surface_id) {
auto it = embed_token_to_allocation_group_.find(
surface_id.local_surface_id().embed_token());
if (it == embed_token_to_allocation_group_.end())
return nullptr;
DCHECK(it->second);
if (it->second->submitter_frame_sink_id() != surface_id.frame_sink_id()) {
DLOG(ERROR) << "Cannot reuse embed token across frame sinks";
return nullptr;
}
return it->second.get();
}

} // namespace viz
Loading

0 comments on commit edfa68c

Please sign in to comment.