LCP: Use largest painted image for web-exposed entry [chromium/src : main]
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
PTAL (not expecting to land this before the holidays, just want to send it out!). Not sure if we should go "experimental" or just enable this.
Related details
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 4 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
4 comments
Huzzah!
<title>Largest Contentful Paint: performance entries are emitted in the expected sequence</title>Just FYI that Firefox will not pass this, and landing this into the LCP repo will make it automatically part of the interop work... you can untag it, but I dont know the full process.
(It could also start tentative until we merge in spec changes? Though I can prob get that in alongside this...)
container.appendChild(createDivWithText('text1', 'AB'));Nit: could you consider adding a third div, which is als smaller, that comes last (after the largest)?
That way, regardless of arbitrary paint order for any implementation (well not guaranteed but higher odds...) you can test that only one (largest) candidate is emitted per paint.
Otherwise you could just be accidentally measuring largest before smaller...
// a LargestContentfulPaint entry for image1.Is this 100% guaranteed? Is the random appended to ensure no cache?
Related details
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 3 comments![]( "Open in Gerrit")
<title>Largest Contentful Paint: performance entries are emitted in the expected sequence</title>Just FYI that Firefox will not pass this, and landing this into the LCP repo will make it automatically part of the interop work... you can untag it, but I dont know the full process.
(It could also start tentative until we merge in spec changes? Though I can prob get that in alongside this...)
Yeah maybe making it .tentative for now makes sense. I thought there was a predefined list of tests for the interop effort that will trip up edits, but I'm not fully sure how that works.
container.appendChild(createDivWithText('text1', 'AB'));Nit: could you consider adding a third div, which is als smaller, that comes last (after the largest)?
That way, regardless of arbitrary paint order for any implementation (well not guaranteed but higher odds...) you can test that only one (largest) candidate is emitted per paint.
Otherwise you could just be accidentally measuring largest before smaller...
Yeah good idea, I'll update this shortly... And yeah this is tricky since paint order _could_ be anything.
// a LargestContentfulPaint entry for image1.Is this 100% guaranteed? Is the random appended to ensure no cache?
It should be. The key here is we don't want the image to load synchronously, otherwise it'll be in this frame. The only time that's _supposed_ to happen is when the image is in the list of "list of available images" [1] (used for de-duping during loading, and I think for reloads), but that's keyed off of absolute URL, so this should prevent reuse there.
Chrome loads data URIs synchronously, which IIUC is a (maybe willful) spec violation, but this doesn't apply here.
Preloads and HTTP cache hits are still supposed to have an async hop since it calls out to Fetch, but I haven't tested this in all browsers (I think I have a test showing this is the case in Chrome) -- but regardless, the random query param should prevent a cached/preloaded resource from being used, so I think this is sound.
[1]https://html.spec.whatwg.org/#list-of-available-images
---
BUT, I'm finding this is flaky locally for another reason: sometimes the feedback for frame N arrives _after_ N+1, so we give everything the timestamp for N+1 :(. So I'm seeing all of the text nodes getting the same timestamp in some cases.
It looks mostly confined to non-threaded compositing (I've seen weirdness with this mode before wrt presentation callbacks), but it timed out at least once with threaded compositing (although I can't get it to fail now..). Looks like it could be related to the jitter we add or something, as these are delayed tasks.
Related details
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
// a LargestContentfulPaint entry for image1.Scott HaseleyIs this 100% guaranteed? Is the random appended to ensure no cache?
It should be. The key here is we don't want the image to load synchronously, otherwise it'll be in this frame. The only time that's _supposed_ to happen is when the image is in the list of "list of available images" [1] (used for de-duping during loading, and I think for reloads), but that's keyed off of absolute URL, so this should prevent reuse there.
Chrome loads data URIs synchronously, which IIUC is a (maybe willful) spec violation, but this doesn't apply here.
Preloads and HTTP cache hits are still supposed to have an async hop since it calls out to Fetch, but I haven't tested this in all browsers (I think I have a test showing this is the case in Chrome) -- but regardless, the random query param should prevent a cached/preloaded resource from being used, so I think this is sound.
[1]https://html.spec.whatwg.org/#list-of-available-images
---
BUT, I'm finding this is flaky locally for another reason: sometimes the feedback for frame N arrives _after_ N+1, so we give everything the timestamp for N+1 :(. So I'm seeing all of the text nodes getting the same timestamp in some cases.
It looks mostly confined to non-threaded compositing (I've seen weirdness with this mode before wrt presentation callbacks), but it timed out at least once with threaded compositing (although I can't get it to fail now..). Looks like it could be related to the jitter we add or something, as these are delayed tasks.
Re: out-of-order presentation time:
- When coarsening, we round up to nearest multiple of 4ms and use that as the presentation time. We then post a delayed task with delay = origin + presentation_time - now.
- But we sometimes get the same presentation timestamp for consecutive frames. This happens a lot more without threaded compositing, but can also happen with threaded compositing, e.g. with dropped frames (which is what I'm seeing)
- The flakiness results from computing the delay: the target_time (origin + pres_time) is the same, but there's some margin for error since we compute now() and then add that delay back to now() in the scheduling layer
The new-ish `PostDelayedTaskAt()` should fix this since that would use the target_time directly `SequenceManager` rather than doing more math, and the tie would be broken based on sequence number (posting order). But, it's guarded by a PassKey, so I'll need to get this blessed.
One alternative is to use a Timer and maintain a list of callbacks, ordered by target_time, and have a single outstanding task (earliest presentation time). But that's basically duplicating the `SequenceManager` logic.
Related details
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 3 comments![]( "Open in Gerrit")
<title>Largest Contentful Paint: performance entries are emitted in the expected sequence</title>Scott HaseleyJust FYI that Firefox will not pass this, and landing this into the LCP repo will make it automatically part of the interop work... you can untag it, but I dont know the full process.
(It could also start tentative until we merge in spec changes? Though I can prob get that in alongside this...)
Yeah maybe making it .tentative for now makes sense. I thought there was a predefined list of tests for the interop effort that will trip up edits, but I'm not fully sure how that works.
Done
container.appendChild(createDivWithText('text1', 'AB'));Scott HaseleyNit: could you consider adding a third div, which is als smaller, that comes last (after the largest)?
That way, regardless of arbitrary paint order for any implementation (well not guaranteed but higher odds...) you can test that only one (largest) candidate is emitted per paint.
Otherwise you could just be accidentally measuring largest before smaller...
Yeah good idea, I'll update this shortly... And yeah this is tricky since paint order _could_ be anything.
Done
// a LargestContentfulPaint entry for image1.Scott HaseleyIs this 100% guaranteed? Is the random appended to ensure no cache?
Scott HaseleyIt should be. The key here is we don't want the image to load synchronously, otherwise it'll be in this frame. The only time that's _supposed_ to happen is when the image is in the list of "list of available images" [1] (used for de-duping during loading, and I think for reloads), but that's keyed off of absolute URL, so this should prevent reuse there.
Chrome loads data URIs synchronously, which IIUC is a (maybe willful) spec violation, but this doesn't apply here.
Preloads and HTTP cache hits are still supposed to have an async hop since it calls out to Fetch, but I haven't tested this in all browsers (I think I have a test showing this is the case in Chrome) -- but regardless, the random query param should prevent a cached/preloaded resource from being used, so I think this is sound.
[1]https://html.spec.whatwg.org/#list-of-available-images
---
BUT, I'm finding this is flaky locally for another reason: sometimes the feedback for frame N arrives _after_ N+1, so we give everything the timestamp for N+1 :(. So I'm seeing all of the text nodes getting the same timestamp in some cases.
It looks mostly confined to non-threaded compositing (I've seen weirdness with this mode before wrt presentation callbacks), but it timed out at least once with threaded compositing (although I can't get it to fail now..). Looks like it could be related to the jitter we add or something, as these are delayed tasks.
Re: out-of-order presentation time:
- When coarsening, we round up to nearest multiple of 4ms and use that as the presentation time. We then post a delayed task with delay = origin + presentation_time - now.
- But we sometimes get the same presentation timestamp for consecutive frames. This happens a lot more without threaded compositing, but can also happen with threaded compositing, e.g. with dropped frames (which is what I'm seeing)
- The flakiness results from computing the delay: the target_time (origin + pres_time) is the same, but there's some margin for error since we compute now() and then add that delay back to now() in the scheduling layerThe new-ish `PostDelayedTaskAt()` should fix this since that would use the target_time directly `SequenceManager` rather than doing more math, and the tie would be broken based on sequence number (posting order). But, it's guarded by a PassKey, so I'll need to get this blessed.
One alternative is to use a Timer and maintain a list of callbacks, ordered by target_time, and have a single outstanding task (earliest presentation time). But that's basically duplicating the `SequenceManager` logic.
Update: I reparented this on a change to use `PostDelayedTaskAt()` which fixes the ordering issue.
Related details
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 2 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
2 comments
painted image instead of largest pending image. This does not affect
UKM/metrics. This matches the behavior agreed upon by the WebPerfIs this meant to be "yet" or "by design"? IIUC because UKM only reports the final largest, and because we don't report if we still have a pending, it might not matter for UKM?
(Though I still would like to see more eager reporting.)
// Frame 1: paint three divs, with one being the largerst . This should producenit: largest
Related details
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 2 comments![]( "Open in Gerrit")
painted image instead of largest pending image. This does not affect
UKM/metrics. This matches the behavior agreed upon by the WebPerfIs this meant to be "yet" or "by design"? IIUC because UKM only reports the final largest, and because we don't report if we still have a pending, it might not matter for UKM?
(Though I still would like to see more eager reporting.)
By design / just noting that this isn't a behavior change for UKM and this won't affect metrics.
Right. Previously we would suppress emission of candidates that were smaller than the largest pending image but larger than the last emitted candidate. UKM knows about the largest pending image, so it's already up-to-date (but ahead of reality), and if the page unloads before getting presentation time for largest pending image, we don't report metrics.
---
Though I still would like to see more eager reporting.
I think we'd need to determine the benefit vs. work involved. There certainly may be some improvements we can make, but being able to clean up the web API without changing UKM was a pleasant surprise in terms of impact vs. work (though it was kind of a slog getting to this point...).
// Frame 1: paint three divs, with one being the largerst . This should produceScott Haseleynit: largest
I dunno, largerst sounds cool :P.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/57210.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The exported PR, https://github.com/web-platform-tests/wpt/pull/57210, has failed the following check(s) on GitHub:
wpt-firefox-nightly-stability (https://github.com/web-platform-tests/wpt/runs/60607234407)
These failures will block the export. They may represent new or existing problems; please take a look at the output and see if it can be fixed. Unresolved failures will be looked at by the Ecosystem-Infra sheriff after this CL has been landed in Chromium; if you need earlier help please contact blin...@chromium.org.
Any suggestions to improve this service are welcome; crbug.com/1027618.
Gerrit CL SHA: 49a3ad1d521773a469b1facd52ef32fb8ac15dc9
Patchset Number: 7
Scott Haseley (Gerrit)
Scott Haseley voted and added 1 comment![]( "Open in Gerrit")
Votes added by Scott Haseley
| Commit-Queue | +2 |
1 comment
I'm going land this now that the spec change landed. I'll send out a PSA next week.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/web_tests/external/wpt/largest-contentful-paint/performance-entry-sequence.tentative.html
Insertions: 1, Deletions: 1.
@@ -26,7 +26,7 @@
window.LargestContentfulPaint, "LargestContentfulPaint is not implemented");
requestAnimationFrame(() => {
- // Frame 1: paint three divs, with one being the largerst . This should produce
+ // Frame 1: paint three divs, with one being the largest. This should produce
// a LargestContentfulPaint entry for text2.
container.appendChild(createDivWithText('text1', 'AB'));
container.appendChild(createDivWithText('text2', 'ABC'));
```
Change information
LCP: Use largest painted image for web-exposed entry
We track both a largest painted image and largest pending image for LCP.
The largest painted image corresponds to the largest image we have
presentation time for. The largest pending image is the largest image
we've encountered but don't yet have presentation time for, i.e. because
it's still loading.
The pending/painted distinction is used to prevent recording metrics for
pages that haven't finished loaded, but it also determines what gets
emitted to performance timeline (we test largest based on pending, not
painted). This causes us to suppress emitting new LCP candidates that
are smaller than the largest pending image, but larger than anything
presented so far, which is not specced behavior.
This CL changes it so that we emit candidates based on the largest
painted image instead of largest pending image. This does not affect
UKM/metrics. This matches the behavior agreed upon by the WebPerf
Working Group at TPAC '25.
Note: more changes are required for soft navs to match this behavior
since candidates can be overwritten during rendering/paint. This isn't
a problem for hard LCP since candidates are updated during presentation
callbacks, which are based on frame index.
Chromestatus: https://chromestatus.com/feature/5167930847395840
- M third_party/blink/renderer/core/paint/timing/largest_contentful_paint_calculator.cc
- M third_party/blink/renderer/core/paint/timing/largest_contentful_paint_calculator_test.cc
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- A third_party/blink/web_tests/external/wpt/largest-contentful-paint/performance-entry-sequence.tentative.html
Code-Review: +1 by Michal Mocny
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
luci-bisection@appspot.gserviceaccount.com (Gerrit)
luci-bi...@appspot.gserviceaccount.com has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |