[Paint Preview] Defer subframe transforms

This CL defers the subframe transform until serializing so as to not
affect the position of the sub-picture in the paint preview if
embedding into a single SkPicture.

Change-Id: I613e713e08ea453dd7999f06810f7481501b0658
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2364171Reviewed-by: Mehran Mahmoudi <mahmoudi@chromium.org>
Commit-Queue: Calder Kitagawa <ckitagawa@chromium.org>
Cr-Commit-Position: refs/heads/master@{#801007}

components/paint_preview/common/paint_preview_tracker.cc

@@ -99,8 +99,9 @@ uint32_t PaintPreviewTracker::CreateContentForRemoteFrame(
       SkRect::MakeXYWH(rect.x() + scroll_.width(), rect.y() + scroll_.height(),
                        rect.width(), rect.height()));
   const uint32_t content_id = pic->uniqueID();
-  DCHECK(!base::Contains(content_id_to_embedding_token_, content_id));
-  content_id_to_embedding_token_[content_id] = embedding_token;
+  DCHECK(!base::Contains(picture_context_.content_id_to_embedding_token,
+                         content_id));
+  picture_context_.content_id_to_embedding_token[content_id] = embedding_token;
   subframe_pics_[content_id] = pic;
   return content_id;
 }
@@ -147,37 +148,25 @@ void PaintPreviewTracker::AnnotateLink(const GURL& url, const SkRect& rect) {
                      out_rect.height())));
 }
 
-uint32_t PaintPreviewTracker::TransformContentForRemoteFrame(uint32_t old_id) {
-  if (matrix_.isIdentity())
-    return old_id;
-
-  auto pic_it = subframe_pics_.find(old_id);
+void PaintPreviewTracker::TransformClipForFrame(uint32_t id) {
+  auto pic_it = subframe_pics_.find(id);
   if (pic_it == subframe_pics_.end())
-    return old_id;
+    return;
 
   SkRect out_rect;
   matrix_.mapRect(&out_rect, pic_it->second->cullRect());
-
-  base::UnguessableToken embedding_token =
-      content_id_to_embedding_token_[old_id];
-  uint32_t new_id = CreateContentForRemoteFrame(
-      gfx::Rect(out_rect.x(), out_rect.y(), out_rect.width(),
-                out_rect.height()),
-      embedding_token);
-  subframe_pics_.erase(old_id);
-  content_id_to_embedding_token_.erase(old_id);
-  return new_id;
+  picture_context_.content_id_to_transformed_clip.emplace(id, out_rect);
 }
 
 void PaintPreviewTracker::CustomDataToSkPictureCallback(SkCanvas* canvas,
                                                         uint32_t content_id) {
-  auto map_it = content_id_to_embedding_token_.find(content_id);
-  if (map_it == content_id_to_embedding_token_.end())
+  auto map_it = picture_context_.content_id_to_embedding_token.find(content_id);
+  if (map_it == picture_context_.content_id_to_embedding_token.end())
     return;
 
   auto it = subframe_pics_.find(content_id);
   // DCHECK is sufficient as |subframe_pics_| has same entries as
-  // |content_id_to_proxy_id_|.
+  // |content_id_to_proxy_id|.
   DCHECK(it != subframe_pics_.end());
 
   SkRect rect = it->second->cullRect();

components/paint_preview/common/paint_preview_tracker.h

@@ -72,9 +72,13 @@ class PaintPreviewTracker {
   // Adds |link| with bounding box |rect| to the list of links.
   void AnnotateLink(const GURL& link, const SkRect& rect);
 
-  // Transforms the cull rect for |id| to be in the correct place and return a
-  // non-zero replacement ID if the cull rect needed to be updated.
-  uint32_t TransformContentForRemoteFrame(uint32_t id);
+  // Used when walking the PaintOpBuffer to transforms the clip rect associated
+  // with the SkPicture with |id| to be relative to the top-left corner of
+  // the root space of the current frame associated with this tracker using
+  // |matrix_|. Originally the clip rect is relative to the current matrix stack
+  // of the canvas at time of drawing. The result in stored and accessible via
+  // the PictureSerializationContext during serialization.
+  void TransformClipForFrame(uint32_t id);
 
   // Data Serialization -------------------------------------------------------
   // NOTE: once any of these methods are called the PaintPreviewTracker should
@@ -87,7 +91,7 @@ class PaintPreviewTracker {
   // Expose internal maps for use in MakeSerialProcs().
   // NOTE: Cannot be const due to how SkPicture procs work.
   PictureSerializationContext* GetPictureSerializationContext() {
-    return &content_id_to_embedding_token_;
+    return &picture_context_;
   }
   TypefaceUsageMap* GetTypefaceUsageMap() { return &typeface_glyph_usage_; }
 
@@ -112,7 +116,7 @@ class PaintPreviewTracker {
   std::vector<SkMatrix> states_;
 
   std::vector<mojom::LinkDataPtr> links_;
-  PictureSerializationContext content_id_to_embedding_token_;
+  PictureSerializationContext picture_context_;
   TypefaceUsageMap typeface_glyph_usage_;
   base::flat_map<uint32_t, sk_sp<SkPicture>> subframe_pics_;
 

components/paint_preview/common/paint_preview_tracker_unittest.cc

@@ -48,8 +48,9 @@ TEST(PaintPreviewTrackerTest, TestRemoteFramePlaceholderPicture) {
       tracker.CreateContentForRemoteFrame(rect, kEmbeddingTokenChild);
   PictureSerializationContext* context =
       tracker.GetPictureSerializationContext();
-  EXPECT_TRUE(context->count(content_id));
-  EXPECT_EQ((*context)[content_id], kEmbeddingTokenChild);
+  EXPECT_TRUE(context->content_id_to_embedding_token.count(content_id));
+  EXPECT_EQ(context->content_id_to_embedding_token[content_id],
+            kEmbeddingTokenChild);
 
   SkPictureRecorder recorder;
   SkCanvas* canvas = recorder.beginRecording(100, 100);
@@ -75,8 +76,9 @@ TEST(PaintPreviewTrackerTest, TestRemoteFramePlaceholderPictureWithScroll) {
       tracker.CreateContentForRemoteFrame(rect, kEmbeddingTokenChild);
   PictureSerializationContext* context =
       tracker.GetPictureSerializationContext();
-  EXPECT_TRUE(context->count(content_id));
-  EXPECT_EQ((*context)[content_id], kEmbeddingTokenChild);
+  EXPECT_TRUE(context->content_id_to_embedding_token.count(content_id));
+  EXPECT_EQ(context->content_id_to_embedding_token[content_id],
+            kEmbeddingTokenChild);
 
   SkPictureRecorder recorder;
   SkCanvas* canvas = recorder.beginRecording(100, 100);

components/paint_preview/common/serial_utils.cc

@@ -22,6 +22,10 @@ struct SerializedRectData {
   int64_t y;
   int64_t width;
   int64_t height;
+  int64_t transformed_x;
+  int64_t transformed_y;
+  int64_t transformed_width;
+  int64_t transformed_height;
 };
 #pragma pack(pop)
 
@@ -29,14 +33,20 @@ struct SerializedRectData {
 sk_sp<SkData> SerializePictureAsRectData(SkPicture* picture, void* ctx) {
   const PictureSerializationContext* context =
       reinterpret_cast<PictureSerializationContext*>(ctx);
-  SerializedRectData rect_data = {
-      picture->uniqueID(), picture->cullRect().x(), picture->cullRect().y(),
-      picture->cullRect().width(), picture->cullRect().height()};
 
-  if (context->count(picture->uniqueID()))
-    return SkData::MakeWithCopy(&rect_data, sizeof(rect_data));
+  auto it = context->content_id_to_transformed_clip.find(picture->uniqueID());
   // Defers picture serialization behavior to Skia.
+  if (it == context->content_id_to_transformed_clip.end())
     return nullptr;
+
+  const SkRect& transformed_cull_rect = it->second;
+  SerializedRectData rect_data = {
+      picture->uniqueID(),           picture->cullRect().x(),
+      picture->cullRect().y(),       picture->cullRect().width(),
+      picture->cullRect().height(),  transformed_cull_rect.x(),
+      transformed_cull_rect.y(),     transformed_cull_rect.width(),
+      transformed_cull_rect.height()};
+  return SkData::MakeWithCopy(&rect_data, sizeof(rect_data));
 }
 
 // De-duplicates and subsets used typefaces and discards any unused typefaces.
@@ -76,7 +86,8 @@ sk_sp<SkPicture> DeserializePictureAsRectData(const void* data,
   auto* context = reinterpret_cast<DeserializationContext*>(ctx);
   context->insert(
       {rect_data.content_id,
-       gfx::Rect(rect_data.x, rect_data.y, rect_data.width, rect_data.height)});
+       gfx::Rect(rect_data.transformed_x, rect_data.transformed_y,
+                 rect_data.transformed_width, rect_data.transformed_height)});
   return MakeEmptyPicture();
 }
 
@@ -116,6 +127,14 @@ sk_sp<SkPicture> GetPictureFromDeserialContext(const void* data,
 
 }  // namespace
 
+PictureSerializationContext::PictureSerializationContext() = default;
+PictureSerializationContext::~PictureSerializationContext() = default;
+
+PictureSerializationContext::PictureSerializationContext(
+    PictureSerializationContext&&) = default;
+PictureSerializationContext& PictureSerializationContext::operator=(
+    PictureSerializationContext&&) = default;
+
 TypefaceSerializationContext::TypefaceSerializationContext(
     TypefaceUsageMap* usage)
     : usage(usage) {}

components/paint_preview/common/serial_utils.h

@@ -20,9 +20,25 @@
 
 namespace paint_preview {
 
-// Maps a content ID to an embedding token.
-using PictureSerializationContext =
-    base::flat_map<uint32_t, base::UnguessableToken>;
+struct PictureSerializationContext {
+  PictureSerializationContext();
+  ~PictureSerializationContext();
+
+  PictureSerializationContext(const PictureSerializationContext&) = delete;
+  PictureSerializationContext& operator=(const PictureSerializationContext&) =
+      delete;
+
+  PictureSerializationContext(PictureSerializationContext&&) noexcept;
+  PictureSerializationContext& operator=(
+      PictureSerializationContext&&) noexcept;
+
+  // Maps a content ID to a transformed clip rect.
+  base::flat_map<uint32_t, SkRect> content_id_to_transformed_clip;
+
+  // Maps a content ID to an embedding token.
+  base::flat_map<uint32_t, base::UnguessableToken>
+      content_id_to_embedding_token;
+};
 
 // Maps a typeface ID to a glyph usage tracker.
 using TypefaceUsageMap = base::flat_map<SkFontID, std::unique_ptr<GlyphUsage>>;

components/paint_preview/common/serial_utils_unittest.cc

@@ -20,13 +20,18 @@ TEST(PaintPreviewSerialUtils, TestMakeEmptyPicture) {
   EXPECT_GE(data->size(), 0U);
 }
 
-TEST(PaintPreviewSerialUtils, TestPictureProcs) {
+TEST(PaintPreviewSerialUtils, TestTransformedPictureProcs) {
   auto pic = MakeEmptyPicture();
   uint32_t content_id = pic->uniqueID();
   const base::UnguessableToken kFrameGuid = base::UnguessableToken::Create();
   PictureSerializationContext picture_ctx;
-  EXPECT_TRUE(
-      picture_ctx.insert(std::make_pair(content_id, kFrameGuid)).second);
+  EXPECT_TRUE(picture_ctx.content_id_to_embedding_token
+                  .insert(std::make_pair(content_id, kFrameGuid))
+                  .second);
+  auto new_clip = SkRect::MakeXYWH(10, 20, 30, 40);
+  EXPECT_TRUE(picture_ctx.content_id_to_transformed_clip
+                  .insert(std::make_pair(content_id, new_clip))
+                  .second);
 
   TypefaceUsageMap usage_map;
   TypefaceSerializationContext typeface_ctx(&usage_map);
@@ -47,10 +52,10 @@ TEST(PaintPreviewSerialUtils, TestPictureProcs) {
       serial_pic_data->data(), serial_pic_data->size(),
       deserial_procs.fPictureCtx);
   EXPECT_TRUE(deserial_ctx.count(content_id));
-  EXPECT_EQ(deserial_ctx[content_id].x(), pic->cullRect().x());
-  EXPECT_EQ(deserial_ctx[content_id].y(), pic->cullRect().y());
-  EXPECT_EQ(deserial_ctx[content_id].width(), pic->cullRect().width());
-  EXPECT_EQ(deserial_ctx[content_id].height(), pic->cullRect().height());
+  EXPECT_EQ(deserial_ctx[content_id].x(), new_clip.x());
+  EXPECT_EQ(deserial_ctx[content_id].y(), new_clip.y());
+  EXPECT_EQ(deserial_ctx[content_id].width(), new_clip.width());
+  EXPECT_EQ(deserial_ctx[content_id].height(), new_clip.height());
 }
 
 TEST(PaintPreviewSerialUtils, TestSerialPictureNotInMap) {

components/paint_preview/common/serialized_recording_unittest.cc

@@ -50,10 +50,13 @@ sk_sp<const SkPicture> PaintGrayPictureWithSubframes(
   for (const auto& subframe : subframes) {
     const base::UnguessableToken& subframe_id = subframe.first;
     gfx::Rect clip_rect = subframe.second;
-    sk_sp<SkPicture> temp = SkPicture::MakePlaceholder(SkRect::MakeXYWH(
-        clip_rect.x(), clip_rect.y(), clip_rect.width(), clip_rect.height()));
+    SkRect rect = SkRect::MakeXYWH(clip_rect.x(), clip_rect.y(),
+                                   clip_rect.width(), clip_rect.height());
+    sk_sp<SkPicture> temp = SkPicture::MakePlaceholder(rect);
     expected_deserialization_context->insert({temp->uniqueID(), clip_rect});
-    context->insert({temp->uniqueID(), subframe_id});
+    context->content_id_to_embedding_token.insert(
+        {temp->uniqueID(), subframe_id});
+    context->content_id_to_transformed_clip.insert({temp->uniqueID(), rect});
     canvas->drawPicture(temp.get());
   }
 

components/paint_preview/player/android/javatests/paint_preview_test_service.cc

@@ -178,8 +178,8 @@ PaintPreviewTestService::CreateSingleSkp(
     const int y = child_rects[i * 4 + 1];
     const int width = child_rects[i * 4 + 2];
     const int height = child_rects[i * 4 + 3];
-    auto sub_pic =
-        SkPicture::MakePlaceholder(SkRect::MakeXYWH(x, y, width, height));
+    auto rect = SkRect::MakeXYWH(x, y, width, height);
+    auto sub_pic = SkPicture::MakePlaceholder(rect);
     SkMatrix matrix = SkMatrix::Translate(x, y);
     uint32_t sub_id = sub_pic->uniqueID();
     canvas->drawPicture(sub_pic, &matrix, nullptr);
@@ -198,7 +198,10 @@ PaintPreviewTestService::CreateSingleSkp(
     // Re-find |data| as it may have moved when emplacing the new frame.
     it = frames_.find(id);
     data = &it->second;
-    data->ctx.insert(std::make_pair(sub_id, token));
+    data->ctx.content_id_to_embedding_token.insert(
+        std::make_pair(sub_id, token));
+    data->ctx.content_id_to_transformed_clip.insert(
+        std::make_pair(sub_id, rect));
   }
 
   data->skp = recorder.finishRecordingAsPicture();

components/paint_preview/renderer/paint_preview_recorder_utils.cc

@@ -46,7 +46,7 @@ void ParseGlyphsAndLinks(const cc::PaintOpBuffer* buffer,
       }
       case cc::PaintOpType::CustomData: {
         auto* custom_op = static_cast<cc::CustomDataOp*>(*it);
-        custom_op->id = tracker->TransformContentForRemoteFrame(custom_op->id);
+        tracker->TransformClipForFrame(custom_op->id);
         break;
       }
       case cc::PaintOpType::Save: {
@@ -125,7 +125,7 @@ void BuildResponse(PaintPreviewTracker* tracker,
   PictureSerializationContext* picture_context =
       tracker->GetPictureSerializationContext();
   if (picture_context) {
-    for (const auto& id_pair : *picture_context) {
+    for (const auto& id_pair : picture_context->content_id_to_embedding_token) {
       response->content_id_to_embedding_token.insert(
           {id_pair.first, id_pair.second});
     }

components/paint_preview/renderer/paint_preview_recorder_utils_unittest.cc

@@ -184,16 +184,16 @@ TEST(PaintPreviewRecorderUtilsTest, TestTransformSubframeRects) {
 
   ParseGlyphsAndLinks(record.get(), &tracker);
 
-  map = tracker.GetSubframePicsForTesting();
-  ASSERT_EQ(map.size(), 1U);
-  // Iterate over the one element since we don't know the key.
-  for (const auto& pair : map) {
-    auto old_cull_rect = pair.second->cullRect();
-    EXPECT_EQ(rect.x() + 10, old_cull_rect.x());
-    EXPECT_EQ(rect.y() + 20, old_cull_rect.y());
-    EXPECT_EQ(rect.width(), old_cull_rect.width());
-    EXPECT_EQ(rect.height(), old_cull_rect.height());
-  }
+  auto* picture_ctx = tracker.GetPictureSerializationContext();
+  ASSERT_EQ(picture_ctx->content_id_to_transformed_clip.size(), 1U);
+  auto clip_it = picture_ctx->content_id_to_transformed_clip.find(old_id);
+  ASSERT_NE(clip_it, picture_ctx->content_id_to_transformed_clip.end());
+
+  SkRect new_cull_rect = clip_it->second;
+  EXPECT_EQ(rect.x() + 10, new_cull_rect.x());
+  EXPECT_EQ(rect.y() + 20, new_cull_rect.y());
+  EXPECT_EQ(rect.width(), new_cull_rect.width());
+  EXPECT_EQ(rect.height(), new_cull_rect.height());
 }
 
 class PaintPreviewRecorderUtilsSerializeAsSkPictureTest
@@ -270,10 +270,13 @@ TEST_P(PaintPreviewRecorderUtilsSerializeAsSkPictureTest, Roundtrip) {
   uint32_t content_id = tracker.CreateContentForRemoteFrame(
       gfx::Rect(10, 10), base::UnguessableToken::Create());
   canvas->recordCustomData(content_id);
+  tracker.TransformClipForFrame(content_id);
   ctx.insert(content_id);
+
   content_id = tracker.CreateContentForRemoteFrame(
       gfx::Rect(20, 20), base::UnguessableToken::Create());
   canvas->recordCustomData(content_id);
+  tracker.TransformClipForFrame(content_id);
   ctx.insert(content_id);
 
   size_t out_size = 0;
@@ -324,10 +327,10 @@ TEST(PaintPreviewRecorderUtilsTest, TestBuildResponse) {
   EXPECT_THAT(response->links[1]->url, GURL("www.chromium.org"));
   EXPECT_THAT(response->links[1]->rect, gfx::Rect(10, 20, 10, 20));
 
-  auto* content_map = tracker.GetPictureSerializationContext();
+  auto* picture_ctx = tracker.GetPictureSerializationContext();
   for (const auto& id_pair : response->content_id_to_embedding_token) {
-    auto it = content_map->find(id_pair.first);
-    EXPECT_NE(it, content_map->end());
+    auto it = picture_ctx->content_id_to_embedding_token.find(id_pair.first);
+    EXPECT_NE(it, picture_ctx->content_id_to_embedding_token.end());
     EXPECT_EQ(id_pair.first, it->first);
     EXPECT_EQ(id_pair.second, it->second);
   }

components/services/paint_preview_compositor/paint_preview_compositor_impl_unittest.cc

@@ -232,9 +232,9 @@ void PopulateFrameProto(
         subframe_id.GetLowForSerialization());
     content_id_embedding_token_pair->set_embedding_token_high(
         subframe_id.GetHighForSerialization());
-
     expected_frame_data->subframes.push_back(
         mojom::SubframeClipRect::New(subframe_id, clip_rect));
+    tracker.TransformClipForFrame(content_id);
   }
 
   sk_sp<SkPicture> pic = recorder.finishRecordingAsPicture();