Revert "Reland "[LCP] Add animated image support""

This reverts commit 2ebdf0d26ea55271942e6b5ed585a08016a3e80b. Reason for revert: Still appears to be causing flakiness on PageLoadMetricsBrowserTestWithAnimatedLCPFlag.PageLCPNonAnimatedImage Flakiness stopped while this was reverted and resumed when it relanded. https://screenshot.googleplex.com/85Ucaejfj42M5ov.png https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=PageLoadMetricsBrowserTestWithAnimatedLCPFlag.PageLCPNonAnimatedImage&testType=browser_tests%20on%20Mac-11 https://ci.chromium.org/ui/p/chromium/builders/ci/mac11-arm64-rel-tests/1167/overview Original change's description: > Reland "[LCP] Add animated image support" > > This is a reland of b7d510c06e0436cfb4bd7260175cd460b949225c > > Original change's description: > > [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} > > Bug: 1260953 > Change-Id: I5eaaf0cfd1daa7fb905e68aed4994cb931dc7750 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3247449 > Commit-Queue: Yoav Weiss <[email protected]> > Reviewed-by: Nicolás Peña Moreno <[email protected]> > Cr-Commit-Position: refs/heads/main@{#935635} Bug: 1260953 Change-Id: I07f60d8c423a3458cf7230843a8258682be134ad No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3249376 Commit-Queue: Glen Robertson <[email protected]> Auto-Submit: Glen Robertson <[email protected]> Owners-Override: Glen Robertson <[email protected]> Reviewed-by: Alan Cutter <[email protected]> Cr-Commit-Position: refs/heads/main@{#935767} NOKEYCHECK=True GitOrigin-RevId: 2fb2daf6c7fddaa3f6c8b8e0ac5787ef1e0f44a1
mjfroman · Oct 28, 2021 · 03f715d · 03f715d
1 parent fd282b8
commit 03f715d
Show file tree

Hide file tree

Showing 26 changed files with 57 additions and 359 deletions.
diff --git a/blink/common/features.cc b/blink/common/features.cc
@@ -1101,10 +1101,5 @@ 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/blink/public/common/features.h b/blink/public/common/features.h
@@ -518,8 +518,6 @@ 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/blink/renderer/core/loader/resource/image_resource_content.cc b/blink/renderer/core/loader/resource/image_resource_content.cc
@@ -626,12 +626,7 @@ ResourceStatus ImageResourceContent::GetContentStatus() const {
   return content_status_;
 }
 
-bool ImageResourceContent::IsAnimatedImageWithPaintedFirstFrame() const {
-  return (image_ && !image_->IsNull() && image_->MaybeAnimated() &&
-          image_->CurrentFrameIsComplete());
-}
-
-// TODO(hiroshige): Consider removing the following methods, or stopping
+// TODO(hiroshige): Consider removing the following methods, or stoping
 // redirecting to ImageResource.
 const KURL& ImageResourceContent::Url() const {
   return info_->Url();

diff --git a/blink/renderer/core/loader/resource/image_resource_content.h b/blink/renderer/core/loader/resource/image_resource_content.h
@@ -109,7 +109,6 @@ 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/blink/renderer/core/paint/image_paint_timing_detector.cc b/blink/renderer/core/paint/image_paint_timing_detector.cc
@@ -53,11 +53,6 @@ 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,
@@ -106,7 +101,6 @@ 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",
@@ -129,29 +123,20 @@ void ImagePaintTimingDetector::ReportNoCandidateToTrace() {
 ImageRecord* ImagePaintTimingDetector::UpdateCandidate() {
   ImageRecord* largest_image_record =
       records_manager_.FindLargestPaintCandidate();
-  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 base::TimeTicks time = largest_image_record
+                                   ? largest_image_record->paint_time
+                                   : base::TimeTicks();
   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() && largest_image_record->loaded) {
+    if (!time.is_null()) {
+      DCHECK(largest_image_record->loaded);
       ReportCandidateToTrace(*largest_image_record);
     } else {
       ReportNoCandidateToTrace();
@@ -181,7 +166,7 @@ void ImagePaintTimingDetector::NotifyImageRemoved(
     const LayoutObject& object,
     const ImageResourceContent* cached_image) {
   RecordId record_id = std::make_pair(&object, cached_image);
-  records_manager_.RemoveImageTimeRecords(record_id);
+  records_manager_.RemoveImageFinishedRecord(record_id);
   records_manager_.RemoveInvisibleRecordIfNeeded(record_id);
   if (!records_manager_.IsRecordedVisibleImage(record_id))
     return;
@@ -230,13 +215,7 @@ void ImageRecordsManager::AssignPaintTimeToRegisteredQueuedRecords(
     }
     if (record->frame_index > last_queued_frame_index)
       break;
-    if (record->loaded) {
-      record->paint_time = timestamp;
-    }
-    if (record->queue_animated_paint) {
-      record->first_animated_frame_time = timestamp;
-      record->queue_animated_paint = false;
-    }
+    record->paint_time = timestamp;
     images_queued_for_paint_time_.pop_front();
   }
 }
@@ -249,7 +228,6 @@ void ImagePaintTimingDetector::RecordImage(
     const StyleFetchedImage* style_image,
     const gfx::Rect& image_border) {
   Node* node = object.GetNode();
-
   if (!node)
     return;
 
@@ -291,28 +269,25 @@ void ImagePaintTimingDetector::RecordImage(
     return;
   }
 
-  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()) {
-        gfx::RectF 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 &&
+      !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()) {
+      gfx::RectF 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;
+
   gfx::RectF mapped_visual_rect =
       frame_view_->GetPaintTimingDetector().CalculateVisualRect(
           image_border, current_paint_chunk_properties);
@@ -324,11 +299,6 @@ 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;
@@ -396,19 +366,6 @@ 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) {

diff --git a/blink/renderer/core/paint/image_paint_timing_detector.h b/blink/renderer/core/paint/image_paint_timing_detector.h
@@ -57,10 +57,7 @@ class ImageRecord : public base::SupportsWeakPtr<ImageRecord> {
   // The time of the first paint after fully loaded. 0 means not painted yet.
   base::TimeTicks paint_time = base::TimeTicks();
   base::TimeTicks load_time = base::TimeTicks();
-  base::TimeTicks first_animated_frame_time = base::TimeTicks();
   bool loaded = false;
-  // An animated frame is queued for paint timing.
-  bool queue_animated_paint = false;
   // LCP rect information, only populated when tracing is enabled.
   std::unique_ptr<LCPRectInfo> lcp_rect_info_;
 };
@@ -92,7 +89,7 @@ class CORE_EXPORT ImageRecordsManager {
     invisible_images_.erase(record_id);
   }
 
-  inline void RemoveImageTimeRecords(const RecordId& record_id) {
+  inline void RemoveImageFinishedRecord(const RecordId& record_id) {
     image_finished_times_.erase(record_id);
   }
 
@@ -137,17 +134,14 @@ class CORE_EXPORT ImageRecordsManager {
     // not currently the case. If we plumb some information from
     // ImageResourceContent we may be able to ensure that this call does not
     // require the Contains() check, which would save time.
-    if (!image_finished_times_.Contains(record_id)) {
+    if (!image_finished_times_.Contains(record_id))
       image_finished_times_.insert(record_id, base::TimeTicks::Now());
-    }
   }
 
   inline bool IsVisibleImageLoaded(const RecordId& record_id) const {
     DCHECK(visible_images_.Contains(record_id));
     return visible_images_.at(record_id)->loaded;
   }
-  bool OnFirstAnimatedFramePainted(const RecordId&,
-                                   unsigned current_frame_index);
   void OnImageLoaded(const RecordId&,
                      unsigned current_frame_index,
                      const StyleFetchedImage*);

diff --git a/blink/renderer/core/paint/largest_contentful_paint_calculator.cc b/blink/renderer/core/paint/largest_contentful_paint_calculator.cc
@@ -22,7 +22,7 @@ LargestContentfulPaintCalculator::LargestContentfulPaintCalculator(
     WindowPerformance* window_performance)
     : window_performance_(window_performance) {}
 
-void LargestContentfulPaintCalculator::UpdateLargestContentfulPaintIfNeeded(
+void LargestContentfulPaintCalculator::UpdateLargestContentPaintIfNeeded(
     const TextRecord* largest_text,
     const ImageRecord* largest_image) {
   uint64_t text_size = largest_text ? largest_text->first_size : 0u;
@@ -72,12 +72,9 @@ void LargestContentfulPaintCalculator::UpdateLargestContentfulImage(
       image_element ? image_element->GetIdAttribute() : AtomicString();
   window_performance_->OnLargestContentfulPaintUpdated(
       expose_paint_time_to_api ? largest_image->paint_time : base::TimeTicks(),
-      largest_image->first_size, largest_image->load_time,
-      expose_paint_time_to_api ? largest_image->first_animated_frame_time
-                               : base::TimeTicks(),
-      image_id, image_url, image_element);
+      largest_image->first_size, largest_image->load_time, image_id, image_url,
+      image_element);
 
-  // TODO: update trace value with animated frame data
   if (LocalDOMWindow* window = window_performance_->DomWindow()) {
     TRACE_EVENT_MARK_WITH_TIMESTAMP2(kTraceCategories, kLCPCandidate,
                                      largest_image->paint_time, "data",
@@ -103,7 +100,7 @@ void LargestContentfulPaintCalculator::UpdateLargestContentfulText(
       text_element ? text_element->GetIdAttribute() : AtomicString();
   window_performance_->OnLargestContentfulPaintUpdated(
       largest_text.paint_time, largest_text.first_size, base::TimeTicks(),
-      base::TimeTicks(), text_id, g_empty_string, text_element);
+      text_id, g_empty_string, text_element);
 
   if (LocalDOMWindow* window = window_performance_->DomWindow()) {
     TRACE_EVENT_MARK_WITH_TIMESTAMP2(kTraceCategories, kLCPCandidate,

diff --git a/blink/renderer/core/paint/largest_contentful_paint_calculator.h b/blink/renderer/core/paint/largest_contentful_paint_calculator.h
@@ -23,8 +23,8 @@ class CORE_EXPORT LargestContentfulPaintCalculator final
   LargestContentfulPaintCalculator& operator=(
       const LargestContentfulPaintCalculator&) = delete;
 
-  void UpdateLargestContentfulPaintIfNeeded(const TextRecord* largest_text,
-                                            const ImageRecord* largest_image);
+  void UpdateLargestContentPaintIfNeeded(const TextRecord* largest_text,
+                                         const ImageRecord* largest_image);
 
   void Trace(Visitor* visitor) const;
 

diff --git a/blink/renderer/core/paint/paint_timing_detector.cc b/blink/renderer/core/paint/paint_timing_detector.cc
@@ -33,7 +33,6 @@
 #include "third_party/blink/renderer/platform/graphics/paint/property_tree_state.h"
 #include "third_party/blink/renderer/platform/graphics/paint/scoped_paint_chunk_properties.h"
 #include "third_party/blink/renderer/platform/graphics/static_bitmap_image.h"
-#include "third_party/blink/renderer/platform/runtime_enabled_features.h"
 #include "third_party/blink/renderer/platform/wtf/cross_thread_functional.h"
 #include "third_party/blink/renderer/platform/wtf/functional.h"
 
@@ -123,23 +122,14 @@ void PaintTimingDetector::NotifyBackgroundImagePaint(
   LocalFrameView* frame_view = object->GetFrameView();
   if (!frame_view)
     return;
-
-  ImagePaintTimingDetector* detector =
-      frame_view->GetPaintTimingDetector().GetImagePaintTimingDetector();
-  if (!detector)
+  PaintTimingDetector& detector = frame_view->GetPaintTimingDetector();
+  if (!detector.GetImagePaintTimingDetector())
     return;
-
   if (!IsBackgroundImageContentful(*object, image))
     return;
-
-  ImageResourceContent* cached_image = style_image.CachedImage();
-  DCHECK(cached_image);
-  // TODO(yoav): |image| and |cached_image.GetImage()| are not the same here in
-  // the case of SVGs. Figure out why and if we can remove this footgun.
-
-  detector->RecordImage(*object, ToGfxSize(image.Size()), *cached_image,
-                        current_paint_chunk_properties, &style_image,
-                        image_border);
+  detector.GetImagePaintTimingDetector()->RecordImage(
+      *object, ToGfxSize(image.Size()), *style_image.CachedImage(),
+      current_paint_chunk_properties, &style_image, image_border);
 }
 
 // static
@@ -154,13 +144,12 @@ void PaintTimingDetector::NotifyImagePaint(
   LocalFrameView* frame_view = object.GetFrameView();
   if (!frame_view)
     return;
-  ImagePaintTimingDetector* detector =
-      frame_view->GetPaintTimingDetector().GetImagePaintTimingDetector();
-  if (!detector)
+  PaintTimingDetector& detector = frame_view->GetPaintTimingDetector();
+  if (!detector.GetImagePaintTimingDetector())
     return;
-
-  detector->RecordImage(object, intrinsic_size, cached_image,
-                        current_paint_chunk_properties, nullptr, image_border);
+  detector.GetImagePaintTimingDetector()->RecordImage(
+      object, intrinsic_size, cached_image, current_paint_chunk_properties,
+      nullptr, image_border);
 }
 
 void PaintTimingDetector::NotifyImageFinished(
@@ -396,8 +385,8 @@ void PaintTimingDetector::UpdateLargestContentfulPaintCandidate() {
     largest_image_record = image_timing_detector->UpdateCandidate();
   }
 
-  lcp_calculator->UpdateLargestContentfulPaintIfNeeded(largest_text_record,
-                                                       largest_image_record);
+  lcp_calculator->UpdateLargestContentPaintIfNeeded(largest_text_record,
+                                                    largest_image_record);
 }
 
 void PaintTimingDetector::ReportIgnoredContent() {

diff --git a/blink/renderer/core/timing/largest_contentful_paint.cc b/blink/renderer/core/timing/largest_contentful_paint.cc
@@ -11,20 +11,17 @@
 
 namespace blink {
 
-LargestContentfulPaint::LargestContentfulPaint(
-    double start_time,
-    base::TimeDelta render_time,
-    uint64_t size,
-    base::TimeDelta load_time,
-    base::TimeDelta first_animated_frame_time,
-    const AtomicString& id,
-    const String& url,
-    Element* element)
+LargestContentfulPaint::LargestContentfulPaint(double start_time,
+                                               base::TimeDelta render_time,
+                                               uint64_t size,
+                                               base::TimeDelta load_time,
+                                               const AtomicString& id,
+                                               const String& url,
+                                               Element* element)
     : PerformanceEntry(g_empty_atom, start_time, start_time),
       size_(size),
       render_time_(render_time),
       load_time_(load_time),
-      first_animated_frame_time_(first_animated_frame_time),
       id_(id),
       url_(url),
       element_(element) {}
@@ -56,8 +53,6 @@ void LargestContentfulPaint::BuildJSONValue(V8ObjectBuilder& builder) const {
   builder.Add("size", size_);
   builder.Add("renderTime", render_time_.InMillisecondsF());
   builder.Add("loadTime", load_time_.InMillisecondsF());
-  builder.Add("firstAnimatedFrameTime",
-              first_animated_frame_time_.InMillisecondsF());
   builder.Add("id", id_);
   builder.Add("url", url_);
 }