Skip to content

Commit

Permalink
Revert "view-transitions: Don't damage full surface during transitions."
Browse files Browse the repository at this point in the history
This reverts commit 05671df.

Reason for revert: There are multiple issues pertaining damage that have likely been masked by the logic being removed here. Fixes for those should happen first.

Bug: 1427721
Original change's description:
> view-transitions: Don't damage full surface during transitions.
>
> If the frame for a Surface was replaced by SurfaceAnimationManager, the
> code assumes the whole surface needs to be damaged. This was needed when
> the animation was viz driven, that code didn't compute the damage rect.
> But now that its all driven in the renderer, CC should be setting up
> the damage rects correctly.
>
> R=​magchen@chromium.org, vmpstr@chromium.org
>
> Change-Id: I4e5053237c9533faef16b78874fa66e06f7542df
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4367538
> Commit-Queue: Maggie Chen <magchen@chromium.org>
> Auto-Submit: Khushal Sagar <khushalsagar@chromium.org>
> Reviewed-by: Maggie Chen <magchen@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1121404}

Change-Id: I4c2143bfe4e2b2a45d093923d8f03c189acbf168
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4368788
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Khushal Sagar <khushalsagar@chromium.org>
Reviewed-by: Maggie Chen <magchen@chromium.org>
Reviewed-by: Khushal Sagar <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1122048}
  • Loading branch information
khushalsagar authored and Chromium LUCI CQ committed Mar 25, 2023
1 parent 3a48a1d commit 5f3104b
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 1 deletion.
10 changes: 10 additions & 0 deletions components/viz/service/display/resolved_frame_data.cc
Expand Up @@ -269,6 +269,16 @@ bool ResolvedFrameData::IsNextFrameSinceLastAggregation() const {
gfx::Rect ResolvedFrameData::GetSurfaceDamage() const {
DCHECK(valid_);

// The |damage_rect| set in |SurfaceAnimationManager| is the |output_rect|.
// However, we dont use |damage_rect| because when we transition from
// interpolated frame we would end up using the |damage_rect| from the
// original non interpolated frame.
// TODO(vmpstr): This damage may be too large, but I think it's hard to figure
// out a small bounds on the damage given an animation that happens in
// SurfaceAnimationManager.
if (surface_->HasSurfaceAnimationDamage())
return GetOutputRect();

if (IsSameFrameAsLastAggregation()) {
return gfx::Rect();
} else if (IsNextFrameSinceLastAggregation()) {
Expand Down
10 changes: 9 additions & 1 deletion components/viz/service/display/surface_aggregator.cc
Expand Up @@ -407,6 +407,12 @@ const DrawQuad* SurfaceAggregator::FindQuadWithOverlayDamage(
const gfx::Transform& parent_target_transform,
const Surface* surface,
size_t* overlay_damage_index) {
// If we have damage from a surface animation, then we shouldn't have an
// overlay candidate from the root render pass, since that's an interpolated
// pass with "artificial" damage.
if (surface->HasSurfaceAnimationDamage())
return nullptr;

// Only process the damage rect at the root render pass, once per surface.
const CompositorFrame& frame = surface->GetActiveFrame();
bool is_last_pass_on_src_surface =
Expand Down Expand Up @@ -546,7 +552,8 @@ ResolvedFrameData* SurfaceAggregator::GetResolvedFrame(
// If there is a new CompositorFrame for `surface` compute resolved frame
// data for the new resolved CompositorFrame.
if (resolved_frame.previous_frame_index() !=
surface->GetActiveFrameIndex()) {
surface->GetActiveFrameIndex() ||
surface->HasSurfaceAnimationDamage()) {
base::ElapsedTimer timer;
ProcessResolvedFrame(resolved_frame);
stats_->declare_resources_time += timer.Elapsed();
Expand Down Expand Up @@ -905,6 +912,7 @@ void SurfaceAggregator::EmitSurfaceContent(
}

referenced_surfaces_.erase(surface_id);
surface->DidAggregate();
}

void SurfaceAggregator::EmitDefaultBackgroundColorQuad(
Expand Down
13 changes: 13 additions & 0 deletions components/viz/service/surfaces/surface.cc
Expand Up @@ -732,10 +732,23 @@ const CompositorFrameMetadata& Surface::GetActiveFrameMetadata() const {
return active_frame_data_->frame.metadata;
}

void Surface::ResetInterpolatedFrame() {
interpolated_frame_.reset();
has_damage_from_interpolated_frame_ = true;
}

void Surface::SetInterpolatedFrame(CompositorFrame frame) {
interpolated_frame_.emplace(std::move(frame));
}

bool Surface::HasSurfaceAnimationDamage() const {
return interpolated_frame_.has_value() || has_damage_from_interpolated_frame_;
}

void Surface::DidAggregate() {
has_damage_from_interpolated_frame_ = false;
}

const CompositorFrame& Surface::GetPendingFrame() {
DCHECK(pending_frame_data_);
return pending_frame_data_->frame;
Expand Down
5 changes: 5 additions & 0 deletions components/viz/service/surfaces/surface.h
Expand Up @@ -202,6 +202,7 @@ class VIZ_SERVICE_EXPORT Surface final {
const CompositorFrame& GetActiveFrame() const;
const CompositorFrameMetadata& GetActiveFrameMetadata() const;

void ResetInterpolatedFrame();
void SetInterpolatedFrame(CompositorFrame frame);
const CompositorFrame& GetActiveOrInterpolatedFrame() const;
bool HasInterpolatedFrame() const;
Expand Down Expand Up @@ -310,6 +311,8 @@ class VIZ_SERVICE_EXPORT Surface final {
std::unique_ptr<CopyOutputRequest> copy_request,
CompositorRenderPassId render_pass_id);

void DidAggregate();

// Returns frame id of the oldest uncommitted frame if any,
absl::optional<BeginFrameId> GetFirstUncommitedFrameId();

Expand Down Expand Up @@ -437,6 +440,8 @@ class VIZ_SERVICE_EXPORT Surface final {

const raw_ptr<SurfaceAllocationGroup> allocation_group_;

bool has_damage_from_interpolated_frame_ = false;

const size_t max_uncommitted_frames_;

base::WeakPtrFactory<Surface> weak_factory_{this};
Expand Down

0 comments on commit 5f3104b

Please sign in to comment.