Skip to content

Commit

Permalink
Revert "[LCP] Add animated image support"
Browse files Browse the repository at this point in the history 
This reverts commit b7d510c.

Reason for revert: speculative revert for new flakiness in
PageLoadMetricsBrowserTestWithAnimatedLCPFlag, see first failing
build
https://ci.chromium.org/p/chromium/builders/ci/Linux%20Tests%20%28dbg%29%281%29/99853

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 <yoavweiss@chromium.org>
> Reviewed-by: Nicolás Peña Moreno <npm@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#935133}

Bug: 1260953
Change-Id: I00878689de95de38645195da81c6baf577d868a6
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3247071
Auto-Submit: Mikel Astiz <mastiz@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Owners-Override: Mikel Astiz <mastiz@google.com>
Cr-Commit-Position: refs/heads/main@{#935350}
  • Loading branch information
Mikel Astiz authored and Chromium LUCI CQ committed Oct 27, 2021
1 parent 16be071 commit f764ece
Show file tree
Hide file tree
Showing 28 changed files with 57 additions and 511 deletions.
152 changes: 0 additions & 152 deletions chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@

#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"
Expand Down Expand Up @@ -77,8 +76,6 @@
#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"
Expand Down Expand Up @@ -379,134 +376,6 @@ 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())
Expand Down Expand Up @@ -3120,27 +2989,6 @@ 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());

Expand Down
Binary file removed content/test/data/animated.png
View file
Binary file not shown.
5 changes: 0 additions & 5 deletions third_party/blink/common/features.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1106,10 +1106,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
2 changes: 0 additions & 2 deletions third_party/blink/public/common/features.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -522,8 +522,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

Expand Down
7 changes: 1 addition & 6 deletions third_party/blink/renderer/core/loader/resource/image_resource_content.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
1 change: 0 additions & 1 deletion third_party/blink/renderer/core/loader/resource/image_resource_content.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
87 changes: 22 additions & 65 deletions third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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",
Expand All @@ -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();
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}
}
Expand All @@ -249,7 +228,6 @@ void ImagePaintTimingDetector::RecordImage(
const StyleFetchedImage* style_image,
const IntRect& image_border) {
Node* node = object.GetNode();

if (!node)
return;

Expand Down Expand Up @@ -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()) {
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 &&
!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);
Expand All @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
Loading

0 comments on commit f764ece

Please sign in to comment.