[LCP] Add animated image support

This CL adds support for better handling of animated images in LCP: * A new attribute is exposing the first animated frame's paint time (behind a flag). * `startTime` is not changed. * The PageLoadMetrics reported for LCP are set to that first frame paint time for animated images (behind another flag). * Entries are not emitted until the image is loaded. Relevant spec issue: w3c/largest-contentful-paint#83 Change-Id: I6bb01eacb4f200f9c032ffcfcd9a1a41126a7773 Bug: 1260953 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3226157 Commit-Queue: Yoav Weiss <[email protected]> Reviewed-by: Nicolás Peña Moreno <[email protected]> Cr-Commit-Position: refs/heads/main@{#935133}
blueboxd · Oct 26, 2021 · b7d510c · b7d510c
1 parent 6bc0a89
commit b7d510c
Show file tree

Hide file tree

Showing 28 changed files with 511 additions and 57 deletions.
diff --git a/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc b/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
@@ -12,6 +12,7 @@
 
 #include "base/bind.h"
 #include "base/check_op.h"
+#include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
@@ -76,6 +77,8 @@
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/common/content_features.h"
+#include "content/public/common/content_paths.h"
+#include "content/public/common/content_switches.h"
 #include "content/public/common/referrer.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/browser_test_utils.h"
@@ -376,6 +379,134 @@ class PageLoadMetricsBrowserTest : public InProcessBrowserTest {
   std::unique_ptr<ukm::TestAutoSetUkmRecorder> test_ukm_recorder_;
 };
 
+class PageLoadMetricsBrowserTestAnimatedLCP
+    : public PageLoadMetricsBrowserTest {
+ protected:
+  void test_animated_image_lcp(bool smaller, bool animated) {
+    // Waiter to ensure main content is loaded.
+    auto waiter = CreatePageLoadMetricsTestWaiter();
+    waiter->AddPageExpectation(TimingField::kLoadEvent);
+    waiter->AddPageExpectation(TimingField::kFirstContentfulPaint);
+    waiter->AddPageExpectation(TimingField::kLargestContentfulPaint);
+
+    const char kHtmlHttpResponseHeader[] =
+        "HTTP/1.1 200 OK\r\n"
+        "Content-Type: text/html; charset=utf-8\r\n"
+        "\r\n";
+    const char kImgHttpResponseHeader[] =
+        "HTTP/1.1 200 OK\r\n"
+        "Content-Type: image/png\r\n"
+        "\r\n";
+    auto main_html_response =
+        std::make_unique<net::test_server::ControllableHttpResponse>(
+            embedded_test_server(), "/mock_page.html",
+            false /*relative_url_is_prefix*/);
+    auto img_response =
+        std::make_unique<net::test_server::ControllableHttpResponse>(
+            embedded_test_server(),
+            animated ? "/images/animated-delayed.png" : "/images/delayed.jpg",
+            false /*relative_url_is_prefix*/);
+
+    ASSERT_TRUE(embedded_test_server()->Start());
+
+    // File is under content/test/data/
+    const std::string file_name_string =
+        animated ? "animated.png" : "single_face.jpg";
+    std::string file_contents;
+    // The first_frame_size number for the animated case (262), represents the
+    // first frame of the animated PNG + an extra chunk enabling the decoder to
+    // understand the first frame is done and decode it.
+    // For the non-animated case (5000), it's an arbitrary number that
+    // represents a part of the JPEG's frame.
+    const unsigned first_frame_size = animated ? 262 : 5000;
+
+    // Read the animated image into two frames.
+    {
+      base::ScopedAllowBlockingForTesting allow_io;
+      base::FilePath test_dir;
+      ASSERT_TRUE(base::PathService::Get(content::DIR_TEST_DATA, &test_dir));
+      base::FilePath file_name = test_dir.AppendASCII(file_name_string);
+      ASSERT_TRUE(base::ReadFileToString(file_name, &file_contents));
+    }
+    // Split the contents into 2 frames
+    std::string first_frame = file_contents.substr(0, first_frame_size);
+    std::string second_frame = file_contents.substr(first_frame_size);
+
+    browser()->OpenURL(content::OpenURLParams(
+        embedded_test_server()->GetURL("/mock_page.html"), content::Referrer(),
+        WindowOpenDisposition::CURRENT_TAB, ui::PAGE_TRANSITION_TYPED, false));
+
+    main_html_response->WaitForRequest();
+    main_html_response->Send(kHtmlHttpResponseHeader);
+    main_html_response->Send(
+        animated ? "<html><body></body><img "
+                   "src=\"/images/animated-delayed.png\"></script></html>"
+                 : "<html><body></body><img "
+                   "src=\"/images/delayed.jpg\"></script></html>");
+    main_html_response->Done();
+
+    img_response->WaitForRequest();
+    img_response->Send(kImgHttpResponseHeader);
+    img_response->Send(first_frame);
+
+    // Trigger a double rAF and take a timestamp afterwards.
+    content::EvalJsResult result =
+        EvalJs(browser()->tab_strip_model()->GetActiveWebContents(),
+               "(async () => {"
+               "const double_raf = () => {"
+               "return new Promise(r => {"
+               "requestAnimationFrame(()=>requestAnimationFrame(r))})};"
+               "await double_raf();})()");
+    EXPECT_EQ("", result.error);
+    content::EvalJsResult result2 =
+        EvalJs(browser()->tab_strip_model()->GetActiveWebContents(),
+               "performance.now()");
+    EXPECT_EQ("", result2.error);
+    double timestamp = result2.ExtractDouble();
+
+    img_response->Send(second_frame);
+    img_response->Done();
+
+    waiter->Wait();
+
+    // LCP is collected only at the end of the page lifecycle. Navigate to
+    // flush.
+    NavigateToUntrackedUrl();
+
+    histogram_tester_->ExpectTotalCount(
+        internal::kHistogramLargestContentfulPaint, 1);
+    auto value =
+        histogram_tester_
+            ->GetAllSamples(internal::kHistogramLargestContentfulPaint)[0]
+            .min;
+
+    if (smaller) {
+      ASSERT_LT(value, timestamp);
+    } else {
+      ASSERT_GT(value, timestamp);
+    }
+  }
+};
+
+class PageLoadMetricsBrowserTestWithAnimatedLCPFlag
+    : public PageLoadMetricsBrowserTestAnimatedLCP {
+ public:
+  PageLoadMetricsBrowserTestWithAnimatedLCPFlag() {
+    scoped_feature_list_.Reset();
+    scoped_feature_list_.InitWithFeatures(
+        {blink::features::kLCPAnimatedImagesReporting}, {});
+  }
+};
+
+class PageLoadMetricsBrowserTestWithRuntimeAnimatedLCPFlag
+    : public PageLoadMetricsBrowserTestAnimatedLCP {
+ public:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitchASCII(switches::kEnableBlinkFeatures,
+                                    "LCPAnimatedImagesWebExposed");
+  }
+};
+
 IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, NoNavigation) {
   ASSERT_TRUE(embedded_test_server()->Start());
   EXPECT_TRUE(NoPageLoadMetricsRecorded())
@@ -2989,6 +3120,27 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, PageLCPStopsUponInput) {
   ASSERT_EQ(all_frames_value, main_frame_value);
 }
 
+// Tests that an animated image's reported LCP values are smaller than its load
+// times, when the feature flag for animated image reporting is enabled.
+IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTestWithAnimatedLCPFlag,
+                       PageLCPAnimatedImage) {
+  test_animated_image_lcp(/*smaller=*/true, /*animated=*/true);
+}
+
+// Tests that an animated image's reported LCP values are larger than its load
+// times, when only the feature flag for animated image web exposure is enabled.
+IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTestWithRuntimeAnimatedLCPFlag,
+                       PageLCPAnimatedImageOnlyRuntimeFlag) {
+  test_animated_image_lcp(/*smaller=*/false, /*animated=*/true);
+}
+
+// Tests that a non-animated image's reported LCP values are larger than its
+// load times, when the feature flag for animated image reporting is enabled.
+IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTestWithAnimatedLCPFlag,
+                       PageLCPNonAnimatedImage) {
+  test_animated_image_lcp(/*smaller=*/false, /*animated=*/false);
+}
+
 IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, FirstInputDelayFromClick) {
   ASSERT_TRUE(embedded_test_server()->Start());
 

diff --git a/content/test/data/animated.png b/content/test/data/animated.png
diff --git a/third_party/blink/common/features.cc b/third_party/blink/common/features.cc
@@ -1106,5 +1106,10 @@ const base::Feature kDeprecationWillLogToConsole{
 const base::Feature kDeprecationWillLogToDevToolsIssue{
     "DeprecationWillLogToDevToolsIssue", base::FEATURE_DISABLED_BY_DEFAULT};
 
+// Enables reporting and web-exposure (respectively) of the time the first frame
+// of an animated image was painted.
+const base::Feature kLCPAnimatedImagesReporting{
+    "LCPAnimatedImagesReporting", base::FEATURE_DISABLED_BY_DEFAULT};
+
 }  // namespace features
 }  // namespace blink
diff --git a/third_party/blink/public/common/features.h b/third_party/blink/public/common/features.h
@@ -522,6 +522,8 @@ BLINK_COMMON_EXPORT extern const base::Feature kDeprecationWillLogToConsole;
 BLINK_COMMON_EXPORT extern const base::Feature
     kDeprecationWillLogToDevToolsIssue;
 
+BLINK_COMMON_EXPORT extern const base::Feature kLCPAnimatedImagesReporting;
+
 }  // namespace features
 }  // namespace blink
 

diff --git a/third_party/blink/renderer/core/loader/resource/image_resource_content.cc b/third_party/blink/renderer/core/loader/resource/image_resource_content.cc
@@ -626,7 +626,12 @@ ResourceStatus ImageResourceContent::GetContentStatus() const {
   return content_status_;
 }
 
-// TODO(hiroshige): Consider removing the following methods, or stoping
+bool ImageResourceContent::IsAnimatedImageWithPaintedFirstFrame() const {
+  return (image_ && !image_->IsNull() && image_->MaybeAnimated() &&
+          image_->CurrentFrameIsComplete());
+}
+
+// TODO(hiroshige): Consider removing the following methods, or stopping
 // redirecting to ImageResource.
 const KURL& ImageResourceContent::Url() const {
   return info_->Url();

diff --git a/third_party/blink/renderer/core/loader/resource/image_resource_content.h b/third_party/blink/renderer/core/loader/resource/image_resource_content.h
@@ -109,6 +109,7 @@ class CORE_EXPORT ImageResourceContent final
   bool IsLoading() const;
   bool ErrorOccurred() const;
   bool LoadFailedOrCanceled() const;
+  bool IsAnimatedImageWithPaintedFirstFrame() const;
 
   // Redirecting methods to Resource.
   const KURL& Url() const;

diff --git a/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc b/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
@@ -53,6 +53,11 @@ uint64_t DownScaleIfIntrinsicSizeIsSmaller(
   return visual_size;
 }
 
+bool ShouldReportAnimatedImages() {
+  return (RuntimeEnabledFeatures::LCPAnimatedImagesWebExposedEnabled() ||
+          base::FeatureList::IsEnabled(features::kLCPAnimatedImagesReporting));
+}
+
 }  // namespace
 
 static bool LargeImageFirst(const base::WeakPtr<ImageRecord>& a,
@@ -101,6 +106,7 @@ void ImagePaintTimingDetector::ReportCandidateToTrace(
   DCHECK(!largest_image_record.paint_time.is_null());
   auto value = std::make_unique<TracedValue>();
   PopulateTraceValue(*value, largest_image_record);
+  // TODO(yoav): Report first animated frame times as well.
   TRACE_EVENT_MARK_WITH_TIMESTAMP2("loading", "LargestImagePaint::Candidate",
                                    largest_image_record.paint_time, "data",
                                    std::move(value), "frame",
@@ -123,20 +129,29 @@ void ImagePaintTimingDetector::ReportNoCandidateToTrace() {
 ImageRecord* ImagePaintTimingDetector::UpdateCandidate() {
   ImageRecord* largest_image_record =
       records_manager_.FindLargestPaintCandidate();
-  const base::TimeTicks time = largest_image_record
-                                   ? largest_image_record->paint_time
-                                   : base::TimeTicks();
+  base::TimeTicks time = largest_image_record ? largest_image_record->paint_time
+                                              : base::TimeTicks();
+  // This doesn't use ShouldReportAnimatedImages(), as it should only update the
+  // record when the base::Feature is enabled, regardless of the runtime-enabled
+  // flag.
+  if (base::FeatureList::IsEnabled(features::kLCPAnimatedImagesReporting) &&
+      largest_image_record &&
+      !largest_image_record->first_animated_frame_time.is_null()) {
+    time = largest_image_record->first_animated_frame_time;
+  }
   const uint64_t size =
       largest_image_record ? largest_image_record->first_size : 0;
   PaintTimingDetector& detector = frame_view_->GetPaintTimingDetector();
+  // Calling NotifyIfChangedLargestImagePaint only has an impact on
+  // PageLoadMetrics, and not on the web exposed metrics.
+  //
   // Two different candidates are rare to have the same time and size.
   // So when they are unchanged, the candidate is considered unchanged.
   bool changed = detector.NotifyIfChangedLargestImagePaint(
       time, size, records_manager_.LargestRemovedImagePaintTime(),
       records_manager_.LargestRemovedImageSize());
   if (changed) {
-    if (!time.is_null()) {
-      DCHECK(largest_image_record->loaded);
+    if (!time.is_null() && largest_image_record->loaded) {
       ReportCandidateToTrace(*largest_image_record);
     } else {
       ReportNoCandidateToTrace();
@@ -166,7 +181,7 @@ void ImagePaintTimingDetector::NotifyImageRemoved(
     const LayoutObject& object,
     const ImageResourceContent* cached_image) {
   RecordId record_id = std::make_pair(&object, cached_image);
-  records_manager_.RemoveImageFinishedRecord(record_id);
+  records_manager_.RemoveImageTimeRecords(record_id);
   records_manager_.RemoveInvisibleRecordIfNeeded(record_id);
   if (!records_manager_.IsRecordedVisibleImage(record_id))
     return;
@@ -215,7 +230,13 @@ void ImageRecordsManager::AssignPaintTimeToRegisteredQueuedRecords(
     }
     if (record->frame_index > last_queued_frame_index)
       break;
-    record->paint_time = timestamp;
+    if (record->loaded) {
+      record->paint_time = timestamp;
+    }
+    if (record->queue_animated_paint) {
+      record->first_animated_frame_time = timestamp;
+      record->queue_animated_paint = false;
+    }
     images_queued_for_paint_time_.pop_front();
   }
 }
@@ -228,6 +249,7 @@ void ImagePaintTimingDetector::RecordImage(
     const StyleFetchedImage* style_image,
     const IntRect& image_border) {
   Node* node = object.GetNode();
+
   if (!node)
     return;
 
@@ -269,25 +291,28 @@ void ImagePaintTimingDetector::RecordImage(
     return;
   }
 
-  if (is_recorded_visible_image &&
-      !records_manager_.IsVisibleImageLoaded(record_id) &&
-      cached_image.IsLoaded()) {
-    records_manager_.OnImageLoaded(record_id, frame_index_, style_image);
-    need_update_timing_at_frame_end_ = true;
-    if (absl::optional<PaintTimingVisualizer>& visualizer =
-            frame_view_->GetPaintTimingDetector().Visualizer()) {
-      FloatRect mapped_visual_rect =
-          frame_view_->GetPaintTimingDetector().CalculateVisualRect(
-              image_border, current_paint_chunk_properties);
-      visualizer->DumpImageDebuggingRect(object, mapped_visual_rect,
-                                         cached_image);
+  if (is_recorded_visible_image) {
+    if (ShouldReportAnimatedImages() &&
+        cached_image.IsAnimatedImageWithPaintedFirstFrame()) {
+      need_update_timing_at_frame_end_ |=
+          records_manager_.OnFirstAnimatedFramePainted(record_id, frame_index_);
+    }
+    if (!records_manager_.IsVisibleImageLoaded(record_id) &&
+        cached_image.IsLoaded()) {
+      records_manager_.OnImageLoaded(record_id, frame_index_, style_image);
+      need_update_timing_at_frame_end_ = true;
+      if (absl::optional<PaintTimingVisualizer>& visualizer =
+              frame_view_->GetPaintTimingDetector().Visualizer()) {
+        FloatRect mapped_visual_rect =
+            frame_view_->GetPaintTimingDetector().CalculateVisualRect(
+                image_border, current_paint_chunk_properties);
+        visualizer->DumpImageDebuggingRect(object, mapped_visual_rect,
+                                           cached_image);
+      }
     }
     return;
   }
 
-  if (is_recorded_visible_image)
-    return;
-
   FloatRect mapped_visual_rect =
       frame_view_->GetPaintTimingDetector().CalculateVisualRect(
           image_border, current_paint_chunk_properties);
@@ -299,6 +324,11 @@ void ImagePaintTimingDetector::RecordImage(
   } else {
     records_manager_.RecordVisible(record_id, rect_size, image_border,
                                    mapped_visual_rect);
+    if (ShouldReportAnimatedImages() &&
+        cached_image.IsAnimatedImageWithPaintedFirstFrame()) {
+      need_update_timing_at_frame_end_ |=
+          records_manager_.OnFirstAnimatedFramePainted(record_id, frame_index_);
+    }
     if (cached_image.IsLoaded()) {
       records_manager_.OnImageLoaded(record_id, frame_index_, style_image);
       need_update_timing_at_frame_end_ = true;
@@ -366,6 +396,19 @@ void ImagePaintTimingDetector::ReportLargestIgnoredImage() {
 ImageRecordsManager::ImageRecordsManager(LocalFrameView* frame_view)
     : size_ordered_set_(&LargeImageFirst), frame_view_(frame_view) {}
 
+bool ImageRecordsManager::OnFirstAnimatedFramePainted(
+    const RecordId& record_id,
+    unsigned current_frame_index) {
+  base::WeakPtr<ImageRecord> record = FindVisibleRecord(record_id);
+  DCHECK(record);
+  if (record->first_animated_frame_time.is_null()) {
+    record->queue_animated_paint = true;
+    QueueToMeasurePaintTime(record, current_frame_index);
+    return true;
+  }
+  return false;
+}
+
 void ImageRecordsManager::OnImageLoaded(const RecordId& record_id,
                                         unsigned current_frame_index,
                                         const StyleFetchedImage* style_image) {