Skip to content

Commit

Permalink
Don't update the surface range at every activation
Browse files Browse the repository at this point in the history
Updating the fallback lowerbound is not necessary anymore and will just
waste time by causing an unnecessary commit and draw.

Bug: 870456
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: Icd553c179c5a2eb98c1608524d9c1caf42a27835
Reviewed-on: https://chromium-review.googlesource.com/1226366
Commit-Queue: Saman Sami <samans@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594380}
  • Loading branch information
Saman Sami authored and Commit Bot committed Sep 26, 2018
1 parent e10714d commit 5dea8c2
Show file tree
Hide file tree
Showing 6 changed files with 16 additions and 55 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -132,15 +132,6 @@ static HitTestRegionList CreateHitTestData(const CompositorFrame& frame) {
const SurfaceDrawQuad* surface_quad =
SurfaceDrawQuad::MaterialCast(quad);

// Skip the quad if the FrameSinkId between fallback and primary is not
// the same, because we don't know which FrameSinkId would be used to
// draw this quad.
if (surface_quad->surface_range.start() &&
surface_quad->surface_range.start()->frame_sink_id() !=
surface_quad->surface_range.end().frame_sink_id()) {
continue;
}

// Skip the quad if the transform is not invertible (i.e. it will not
// be able to receive events).
gfx::Transform target_to_quad_transform;
Expand Down
4 changes: 1 addition & 3 deletions content/browser/renderer_host/delegated_frame_host.cc
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ void DelegatedFrameHost::ProcessCopyOutputRequest(
}

bool DelegatedFrameHost::CanCopyFromCompositingSurface() const {
return HasFallbackSurface() && active_device_scale_factor_ != 0.f;
return active_device_scale_factor_ != 0.f;
}

bool DelegatedFrameHost::TransformPointToLocalCoordSpaceLegacy(
Expand Down Expand Up @@ -330,8 +330,6 @@ void DelegatedFrameHost::OnFirstSurfaceActivation(
return;
}

client_->DelegatedFrameHostGetLayer()->SetFallbackSurfaceId(
surface_info.id());
active_local_surface_id_ = surface_info.id().local_surface_id();
active_device_scale_factor_ = surface_info.device_scale_factor();

Expand Down
2 changes: 0 additions & 2 deletions content/browser/renderer_host/render_widget_host_view_aura.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1698,8 +1698,6 @@ bool RenderWidgetHostViewAura::TransformPointToCoordSpaceForView(
// In TransformPointToLocalCoordSpace() there is a Point-to-Pixel conversion,
// but it is not necessary here because the final target view is responsible
// for converting before computing the final transform.
if (!HasFallbackSurface())
return false;
return target_view->TransformPointToLocalCoordSpace(
point, GetCurrentSurfaceId(), transformed_point, source);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3428,8 +3428,7 @@ TEST_F(RenderWidgetHostViewAuraSurfaceSynchronizationTest,
EXPECT_FALSE(view_->HasFallbackSurface());
}

// This test verifies that the primary SurfaceId is populated on resize and
// the fallback SurfaceId is populated in OnFirstSurfaceActivation.
// This test verifies that the primary SurfaceId is populated on resize.
TEST_F(RenderWidgetHostViewAuraSurfaceSynchronizationTest, SurfaceChanges) {
// Early out because DelegatedFrameHost is not used in mash.
if (features::IsMultiProcessMash())
Expand All @@ -3445,25 +3444,8 @@ TEST_F(RenderWidgetHostViewAuraSurfaceSynchronizationTest, SurfaceChanges) {
view_->SetSize(gfx::Size(300, 300));
ASSERT_TRUE(view_->HasPrimarySurface());
EXPECT_EQ(gfx::Size(300, 300), view_->window_->layer()->size());
EXPECT_FALSE(view_->HasFallbackSurface());
EXPECT_EQ(gfx::Size(300, 300),
view_->delegated_frame_host_->CurrentFrameSizeInDipForTesting());

// Resizing should update the primary SurfaceId.
view_->SetSize(gfx::Size(400, 400));
EXPECT_EQ(gfx::Size(400, 400), view_->window_->layer()->size());
EXPECT_EQ(nullptr, view_->window_->layer()->GetFallbackSurfaceId());
EXPECT_EQ(gfx::Size(400, 400),
view_->delegated_frame_host_->CurrentFrameSizeInDipForTesting());

// Fallback SurfaceId should be updated in OnFirstSurfaceActivation.
// Submitting a CompositorFrame should update the fallback SurfaceId
viz::SurfaceId surface_id(view_->GetFrameSinkId(),
view_->GetLocalSurfaceId());
view_->delegated_frame_host_->OnFirstSurfaceActivation(
viz::SurfaceInfo(surface_id, 1.f, gfx::Size(400, 400)));
EXPECT_EQ(gfx::Size(400, 400), view_->window_->layer()->size());
EXPECT_EQ(surface_id, *view_->window_->layer()->GetFallbackSurfaceId());
}

// This test verifies that the primary SurfaceId is updated on device scale
Expand Down Expand Up @@ -3660,7 +3642,8 @@ TEST_F(RenderWidgetHostViewAuraSurfaceSynchronizationTest,

// Make [1] hidden, resize it. It should drop its frame.
views[1]->Hide();
EXPECT_TRUE(views[1]->HasFallbackSurface());
// TODO(samans): Fix this expectation once https://crbug.com/878372 is fixed.
EXPECT_FALSE(views[1]->HasFallbackSurface());
gfx::Size size2(200, 200);
views[1]->SetSize(size2);
EXPECT_FALSE(views[1]->HasFallbackSurface());
Expand Down Expand Up @@ -5933,7 +5916,9 @@ TEST_F(RenderWidgetHostViewAuraSurfaceSynchronizationTest,
viz::LocalSurfaceId id1 = view_->GetLocalSurfaceId();
view_->delegated_frame_host_->OnFirstSurfaceActivation(viz::SurfaceInfo(
viz::SurfaceId(view_->GetFrameSinkId(), id1), 1, gfx::Size(20, 20)));
EXPECT_TRUE(view_->window_->layer()->GetFallbackSurfaceId()->is_valid());
// TODO(samans): Fix these expectations once https://crbug.com/878372 is
// fixed.
EXPECT_FALSE(view_->window_->layer()->GetFallbackSurfaceId());
view_->Hide();
view_->SetSize(gfx::Size(54, 32));
view_->Show();
Expand All @@ -5956,10 +5941,12 @@ TEST_F(RenderWidgetHostViewAuraSurfaceSynchronizationTest,
viz::LocalSurfaceId id1 = view_->GetLocalSurfaceId();
view_->delegated_frame_host_->OnFirstSurfaceActivation(viz::SurfaceInfo(
viz::SurfaceId(view_->GetFrameSinkId(), id1), 1, gfx::Size(20, 20)));
EXPECT_TRUE(view_->window_->layer()->GetFallbackSurfaceId()->is_valid());
// TODO(samans): Fix these expectations once https://crbug.com/878372 is
// fixed.
EXPECT_FALSE(view_->window_->layer()->GetFallbackSurfaceId());
view_->Hide();
view_->Show();
EXPECT_TRUE(view_->window_->layer()->GetFallbackSurfaceId()->is_valid());
EXPECT_FALSE(view_->window_->layer()->GetFallbackSurfaceId());
}

// Check that TakeFallbackContentFrom() copies the fallback SurfaceId and
Expand All @@ -5984,16 +5971,10 @@ TEST_F(RenderWidgetHostViewAuraSurfaceSynchronizationTest,
view2->GetNativeView(), parent_view_->GetNativeView()->GetRootWindow(),
gfx::Rect());

// Set fallback for the first view.
viz::LocalSurfaceId id = view_->GetLocalSurfaceId();
view_->delegated_frame_host_->OnFirstSurfaceActivation(viz::SurfaceInfo(
viz::SurfaceId(view_->GetFrameSinkId(), id), 1, gfx::Size(20, 20)));
EXPECT_TRUE(view_->window_->layer()->GetFallbackSurfaceId()->is_valid());

// Call TakeFallbackContentFrom(). The second view should now have the same
// fallback as the first view.
// Call TakeFallbackContentFrom(). The second view should obtain a fallback
// from the first view.
view2->TakeFallbackContentFrom(view_);
EXPECT_EQ(*view_->window_->layer()->GetFallbackSurfaceId(),
EXPECT_EQ(view_->window_->layer()->GetPrimarySurfaceId()->ToSmallestId(),
*view2->window_->layer()->GetFallbackSurfaceId());

DestroyView(view2);
Expand Down
2 changes: 0 additions & 2 deletions content/browser/renderer_host/render_widget_host_view_mac.mm
Original file line number Diff line number Diff line change
Expand Up @@ -1259,8 +1259,6 @@ void CombineTextNodesAndMakeCallback(SpeechCallback callback,
return true;
}

if (!HasFallbackSurface())
return false;
return target_view->TransformPointToLocalCoordSpace(
point, GetCurrentSurfaceId(), transformed_point, source);
}
Expand Down
9 changes: 2 additions & 7 deletions ui/android/delegated_frame_host_android.cc
Original file line number Diff line number Diff line change
Expand Up @@ -177,9 +177,7 @@ void DelegatedFrameHostAndroid::CopyFromCompositingSurface(
}

bool DelegatedFrameHostAndroid::CanCopyFromCompositingSurface() const {
return content_layer_ && content_layer_->fallback_surface_id() &&
content_layer_->fallback_surface_id()->is_valid() &&
view_->GetWindowAndroid() &&
return pending_local_surface_id_.is_valid() && view_->GetWindowAndroid() &&
view_->GetWindowAndroid()->GetCompositor();
}

Expand Down Expand Up @@ -405,7 +403,6 @@ void DelegatedFrameHostAndroid::OnFirstSurfaceActivation(
return;
}

content_layer_->SetFallbackSurfaceId(surface_info.id());
active_local_surface_id_ = surface_info.id().local_surface_id();
active_device_scale_factor_ = surface_info.device_scale_factor();

Expand Down Expand Up @@ -465,9 +462,7 @@ void DelegatedFrameHostAndroid::ProcessCopyOutputRequest(
}

viz::SurfaceId DelegatedFrameHostAndroid::SurfaceId() const {
return content_layer_ && content_layer_->fallback_surface_id()
? *content_layer_->fallback_surface_id()
: viz::SurfaceId();
return viz::SurfaceId(frame_sink_id_, active_local_surface_id_);
}

bool DelegatedFrameHostAndroid::HasPrimarySurface() const {
Expand Down

0 comments on commit 5dea8c2

Please sign in to comment.