From 115f38332946734ac034c86c2357cc87cc6e11b4 Mon Sep 17 00:00:00 2001
From: kylechar <kylechar@chromium.org>
Date: Wed, 21 Oct 2026 07:43:36 -0700
Subject: [PATCH] Fix CopyOutputRequest check with OffsetTags

ResolvedFrameData makes a copy of all CompositorRenderPasses to modify
when there is an OffsetTag involved. CopyOutputRequests were not moved
to the copy but SurfaceAggregator checked the copy instead of original.
This resulted in not correctly marking the render pass as containing a
copy output request and not expanding damage. The CopyOutputRequest was
still moved from original CompositorRenderPass to AggregatedRenderPass
correctly.

Bug: 574382050
Change-Id: I872479f72f05ee5d8b05a8ee7b5b1c4241cb6c28
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8613476
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Commit-Queue: Kyle Charbonneau <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1723797}
---
diff --git a/components/viz/service/display/resolved_frame_data.cc b/components/viz/service/display/resolved_frame_data.cc
index 2d917595a..82b3bde 100644
--- a/components/viz/service/display/resolved_frame_data.cc
+++ b/components/viz/service/display/resolved_frame_data.cc
@@ -91,6 +91,10 @@
   return *fixed_.render_pass;
 }
 
+bool ResolvedPassData::HasCopyOutputRequests() const {
+  return !fixed_.original_render_pass->copy_requests.empty();
+}
+
 void ResolvedPassData::CopyAndResetPersistentPassData() {
   previous_persistent_data_ = current_persistent_data_;
   current_persistent_data_ = PersistentPassData();
@@ -100,10 +104,12 @@
   CHECK(pass);
   CHECK_EQ(pass->id, fixed_.render_pass_id);
   fixed_.render_pass = pass;
+  fixed_.original_render_pass = pass;
 }
 
 void ResolvedPassData::ResetCompositorRenderPass() {
   fixed_.render_pass = nullptr;
+  fixed_.original_render_pass = nullptr;
 }
 
 ResolvedFrameData::ResolvedFrameData(DisplayResourceProvider* resource_provider,
@@ -211,6 +217,7 @@
     FixedPassData fixed;
 
     fixed.render_pass = render_pass.get();
+    fixed.original_render_pass = render_pass.get();
 
     AggregatedRenderPassId& remapped_id = aggregated_id_map_[render_pass->id];
     if (remapped_id.is_null()) {
@@ -345,8 +352,8 @@
     // cleared elsewhere they can be reused.
     CHECK_EQ(offset_tag_render_passes_.size(), resolved_passes_.size());
     for (size_t i = 0; i < offset_tag_render_passes_.size(); ++i) {
-      resolved_passes_[i].SetCompositorRenderPass(
-          offset_tag_render_passes_[i].get());
+      resolved_passes_[i].fixed_.render_pass =
+          offset_tag_render_passes_[i].get();
     }
     // Skip running RecomputeOffsetTagDamage() since nothing changed since
     // last frame and there is no added damage.
diff --git a/components/viz/service/display/resolved_frame_data.h b/components/viz/service/display/resolved_frame_data.h
index 250b0e20..eb97be1a 100644
--- a/components/viz/service/display/resolved_frame_data.h
+++ b/components/viz/service/display/resolved_frame_data.h
@@ -55,6 +55,10 @@
   // destroyed during aggregation so the pointer will remain valid for the
   // duration of aggregation (until it's set to null).
   raw_ptr<CompositorRenderPass> render_pass = nullptr;
+  // Points to the original CompositorRenderPass held by the surface, which may
+  // differ from `render_pass` when `RebuildRenderPassesForOffsetTags()` makes a
+  // modified copy.
+  raw_ptr<const CompositorRenderPass> original_render_pass = nullptr;
 
   // How many times this render pass is embedded by another render pass in the
   // same frame.
@@ -175,6 +179,11 @@
     return !fixed_.is_root && fixed_.embed_count == 0;
   }
 
+  // Returns true if the original CompositorRenderPass in `Surface` has any
+  // CopyOutputRequests. Note that `render_pass().copy_requests` may be empty if
+  // `render_pass()` points to an OffsetTag-modified copy.
+  bool HasCopyOutputRequests() const;
+
   AggregationPassData& aggregation() { return aggregation_; }
   const AggregationPassData& aggregation() const { return aggregation_; }
 
diff --git a/components/viz/service/display/surface_aggregator.cc b/components/viz/service/display/surface_aggregator.cc
index 9a90419c..9e25476 100644
--- a/components/viz/service/display/surface_aggregator.cc
+++ b/components/viz/service/display/surface_aggregator.cc
@@ -2101,11 +2101,9 @@
   }
 
   for (auto& resolved_pass : resolved_frame.GetResolvedPasses()) {
-    auto& render_pass = resolved_pass.render_pass();
-
     // Checking for copy requests need to be done after the prewalk because
     // copy requests can get added after damage is computed.
-    if (!render_pass.copy_requests.empty()) {
+    if (resolved_pass.HasCopyOutputRequests()) {
       has_copy_requests_ = true;
       MarkAndPropagateCopyRequestPasses(resolved_pass);
     }
diff --git a/components/viz/service/display/surface_aggregator_unittest.cc b/components/viz/service/display/surface_aggregator_unittest.cc
index 154229c..51205d7 100644
--- a/components/viz/service/display/surface_aggregator_unittest.cc
+++ b/components/viz/service/display/surface_aggregator_unittest.cc
@@ -2408,6 +2408,120 @@
   EXPECT_THAT(original_pass_list[1]->copy_requests, testing::IsEmpty());
 }
 
+// Verify that a render pass with a CopyOutputRequest on a surface that has a
+// non-zero OffsetTag value still marks the frame as having copy requests and
+// forces full damage on the copy request pass.
+TEST_F(SurfaceAggregatorValidSurfaceTest, CopyRequestWithOffsetTag) {
+  TestSurfaceIdAllocator provider_surface_id(child_sink_->frame_sink_id());
+
+  const OffsetTag tag = OffsetTag::CreateRandom();
+  const OffsetTagDefinition tag_def(
+      tag, SurfaceRange(provider_surface_id),
+      OffsetTagConstraints(-10.0f, 10.0f, -10.0f, 10.0f));
+
+  // Submit provider frame with a non-zero OffsetTagValue.
+  {
+    CompositorFrame provider_frame =
+        CompositorFrameBuilder()
+            .AddRenderPass(
+                RenderPassBuilder(kSurfaceSize)
+                    .AddSolidColorQuad(gfx::Rect(kSurfaceSize), SkColors::kRed))
+            .AddOffsetTagValue(OffsetTagValue{tag, gfx::Vector2dF(5.0f, 5.0f)})
+            .Build();
+    child_sink_->SubmitCompositorFrame(provider_surface_id.local_surface_id(),
+                                       std::move(provider_frame));
+  }
+
+  constexpr gfx::Rect child_pass_rect(20, 20);
+
+  // Frame 1: Full damage on root surface.
+  {
+    CompositorFrame root_frame =
+        CompositorFrameBuilder()
+            .AddRenderPass(
+                RenderPassBuilder(CompositorRenderPassId{1}, child_pass_rect)
+                    .AddSolidColorQuad(child_pass_rect, SkColors::kBlue))
+            .AddRenderPass(
+                RenderPassBuilder(CompositorRenderPassId{2}, kSurfaceSize)
+                    .AddRenderPassQuad(child_pass_rect,
+                                       CompositorRenderPassId{1})
+                    .AddSurfaceQuad(gfx::Rect(kSurfaceSize),
+                                    SurfaceRange(provider_surface_id))
+                    .SetQuadOffsetTag(tag))
+            .AddOffsetTagDefinition(tag_def)
+            .Build();
+    root_sink_->SubmitCompositorFrame(root_surface_id_.local_surface_id(),
+                                      std::move(root_frame));
+
+    auto aggregated_frame = AggregateFrame(root_surface_id_);
+    ASSERT_EQ(aggregated_frame.render_pass_list.size(), 2u);
+  }
+
+  // Frame 2: Root damage does not intersect `child_pass_rect` and no
+  // CopyOutputRequest. The child render pass has no damage.
+  {
+    constexpr gfx::Rect non_intersecting_damage(50, 50, 10, 10);
+    CompositorFrame root_frame =
+        CompositorFrameBuilder()
+            .AddRenderPass(
+                RenderPassBuilder(CompositorRenderPassId{1}, child_pass_rect)
+                    .SetDamageRect(gfx::Rect())
+                    .AddSolidColorQuad(child_pass_rect, SkColors::kBlue))
+            .AddRenderPass(
+                RenderPassBuilder(CompositorRenderPassId{2}, kSurfaceSize)
+                    .SetDamageRect(non_intersecting_damage)
+                    .AddRenderPassQuad(child_pass_rect,
+                                       CompositorRenderPassId{1})
+                    .AddSurfaceQuad(gfx::Rect(kSurfaceSize),
+                                    SurfaceRange(provider_surface_id))
+                    .SetQuadOffsetTag(tag))
+            .AddOffsetTagDefinition(tag_def)
+            .Build();
+    root_sink_->SubmitCompositorFrame(root_surface_id_.local_surface_id(),
+                                      std::move(root_frame));
+
+    auto aggregated_frame = AggregateFrame(root_surface_id_);
+    ASSERT_EQ(aggregated_frame.render_pass_list.size(), 2u);
+    EXPECT_THAT(aggregated_frame.render_pass_list[0]->copy_requests,
+                testing::IsEmpty());
+    EXPECT_EQ(aggregated_frame.render_pass_list[0]->damage_rect, gfx::Rect());
+  }
+
+  // Frame 3: Root damage does not intersect `child_pass_rect`, but a
+  // CopyOutputRequest targets the child render pass. The child render pass has
+  // full damage.
+  {
+    constexpr gfx::Rect non_intersecting_damage(50, 50, 10, 10);
+    CompositorFrame root_frame =
+        CompositorFrameBuilder()
+            .AddRenderPass(
+                RenderPassBuilder(CompositorRenderPassId{1}, child_pass_rect)
+                    .SetDamageRect(gfx::Rect())
+                    .AddStubCopyOutputRequest()
+                    .AddSolidColorQuad(child_pass_rect, SkColors::kBlue))
+            .AddRenderPass(
+                RenderPassBuilder(CompositorRenderPassId{2}, kSurfaceSize)
+                    .SetDamageRect(non_intersecting_damage)
+                    .AddRenderPassQuad(child_pass_rect,
+                                       CompositorRenderPassId{1})
+                    .AddSurfaceQuad(gfx::Rect(kSurfaceSize),
+                                    SurfaceRange(provider_surface_id))
+                    .SetQuadOffsetTag(tag))
+            .AddOffsetTagDefinition(tag_def)
+            .Build();
+    root_sink_->SubmitCompositorFrame(root_surface_id_.local_surface_id(),
+                                      std::move(root_frame));
+
+    auto aggregated_frame = AggregateFrame(root_surface_id_);
+    EXPECT_TRUE(aggregated_frame.has_copy_requests);
+    ASSERT_EQ(aggregated_frame.render_pass_list.size(), 2u);
+    EXPECT_THAT(aggregated_frame.render_pass_list[0]->copy_requests,
+                testing::SizeIs(1));
+    EXPECT_EQ(aggregated_frame.render_pass_list[0]->damage_rect,
+              child_pass_rect);
+  }
+}
+
 TEST_F(SurfaceAggregatorValidSurfaceTest,
        ShouldNotTakeRootCopyRequestIfTakeCopyRequestIsFalse) {
   auto embedded_support = std::make_unique<CompositorFrameSinkSupport>(
diff --git a/components/viz/test/compositor_frame_helpers.cc b/components/viz/test/compositor_frame_helpers.cc
index 730188c..8b17960 100644
--- a/components/viz/test/compositor_frame_helpers.cc
+++ b/components/viz/test/compositor_frame_helpers.cc
@@ -543,6 +543,12 @@
   return *this;
 }
 
+CompositorFrameBuilder& CompositorFrameBuilder::AddOffsetTagValue(
+    const OffsetTagValue& value) {
+  frame_->metadata.offset_tag_values.push_back(value);
+  return *this;
+}
+
 CompositorFrameBuilder& CompositorFrameBuilder::AddContentFrameIntervalInfo(
     const ContentFrameIntervalInfo& content_frame_interval_info) {
   frame_->metadata.frame_interval_inputs.content_interval_info.push_back(
diff --git a/components/viz/test/compositor_frame_helpers.h b/components/viz/test/compositor_frame_helpers.h
index dffab79..59b6be2 100644
--- a/components/viz/test/compositor_frame_helpers.h
+++ b/components/viz/test/compositor_frame_helpers.h
@@ -303,6 +303,7 @@
       const gfx::DelegatedInkMetadata& metadata);
   CompositorFrameBuilder& AddOffsetTagDefinition(
       const OffsetTagDefinition& definition);
+  CompositorFrameBuilder& AddOffsetTagValue(const OffsetTagValue& value);
   CompositorFrameBuilder& AddContentFrameIntervalInfo(
       const ContentFrameIntervalInfo& content_frame_interval_info);