ElementTiming: Proxy ImageElementTiming calls through PaintTimingDetector [chromium/src : main]
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
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 added 1 comment![]( "Open in Gerrit")
- NotifyBackgroundImageFinished was ElementTiming-only. This now flows
through PaintTimingDetector, and as a followup, I'll move the
background image load time tracking into ImagePaintTimingDetector.
- NotifyImageFinished (from LayoutImage) was ElementTiming-only.
This is now also routed through PaintTimingDetector. This will
lead to potentially more image load timestamps being recorded
(for pseudo elements), but that's fine since we'll also need to fix
that case for LCP and make the corresponding experimental flag work
for ElementTiming via ImagePaintTimingDetector.just checking-- this means that LCP will benefit from improvements that only ET had before?
I think is true for the second point here, but I think we have an alternative mechanism for the first?
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 1 comment![]( "Open in Gerrit")
- NotifyBackgroundImageFinished was ElementTiming-only. This now flows
through PaintTimingDetector, and as a followup, I'll move the
background image load time tracking into ImagePaintTimingDetector.
- NotifyImageFinished (from LayoutImage) was ElementTiming-only.
This is now also routed through PaintTimingDetector. This will
lead to potentially more image load timestamps being recorded
(for pseudo elements), but that's fine since we'll also need to fix
that case for LCP and make the corresponding experimental flag work
for ElementTiming via ImagePaintTimingDetector.just checking-- this means that LCP will benefit from improvements that only ET had before?
I think is true for the second point here, but I think we have an alternative mechanism for the first?
Jose added some ElementTiming-only handling for image `content:` pseudo elements, so there's an extra call for NotifyImageFinished for ET that doesn't exist for LCP. The main one [1] applies to both, but this new one [2] only applies to ET -- and only when there's no associated Node (pseudo elements). This was added in https://chromium-review.googlesource.com/c/chromium/src/+/6656960.
We'll have to do more work to make this work for LCP and ICP, specifically change IPTD to not ignore LayoutObjects with a null Node and make the SoftNavigationPaintAttributionTracker aware of this. This change just makes it so the "loaded" timestamp gets recorded.
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. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 6 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
6 comments
Needs a rebase-- but this change is long overdue! Makes it much clearer what the diffs are, thanks!
Added a few questions but they are not blocking.
- NotifyBackgroundImageFinished was ElementTiming-only. This now flows
through PaintTimingDetector, and as a followup, I'll move the
background image load time tracking into ImagePaintTimingDetector.
- NotifyImageFinished (from LayoutImage) was ElementTiming-only.
This is now also routed through PaintTimingDetector. This will
lead to potentially more image load timestamps being recorded
(for pseudo elements), but that's fine since we'll also need to fix
that case for LCP and make the corresponding experimental flag work
for ElementTiming via ImagePaintTimingDetector.Scott Haseleyjust checking-- this means that LCP will benefit from improvements that only ET had before?
I think is true for the second point here, but I think we have an alternative mechanism for the first?
Jose added some ElementTiming-only handling for image `content:` pseudo elements, so there's an extra call for NotifyImageFinished for ET that doesn't exist for LCP. The main one [1] applies to both, but this new one [2] only applies to ET -- and only when there's no associated Node (pseudo elements). This was added in https://chromium-review.googlesource.com/c/chromium/src/+/6656960.
We'll have to do more work to make this work for LCP and ICP, specifically change IPTD to not ignore LayoutObjects with a null Node and make the SoftNavigationPaintAttributionTracker aware of this. This change just makes it so the "loaded" timestamp gets recorded.
Acknowledged
// First video frame is not supported for Element Timing.Should we have an issue for this? AFAIK video is underspecified, but per spec the exact same records should be eligible for LCP and Element timing -- so this is not desired it is a historical oversight because of the fracturing of these apis?
if (!cached_image->IsLoaded()) {Should this be "sufficiently loaded", eventually?
Again, this is the existing divergent behaviour between these but per spec the behaviour has to be the same, I think.
bool is_image_or_video_element = IsA<HTMLImageElement>(node) ||I'm confused by support for video here, when on line 115-119 we skip video frames?
Poster images on video elements, I guess?
// added. See also https://crbug.com/1027948.(musing)
Do you think we are closer to being able to report an element timing entry, even if a late `elementtiming` attribute was added to an already loaded and painted element, now that we have this re-architecture?
---
I figure that:
- Paint Timing is moving toward measuring all contentful elements, always (changing recorded-set to a sufficiently-loaded-and-already-painted-set).
- Paint Timing might be moving toward observing all size changes and all "contentfulness changes" directly (and updating caches).
- For long-term memory, potentially we could replace "recorded-set" with a "recorded-map" to frame index/PaintTimingMixin.
Even if we don't keep anything but recoded set, we could just report the Now() time after the attribute is added, as long as we know the content already loaded+rendered.
(For things like `loadTime`, I think we already are considering removing this entirely...)
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 4 comments![]( "Open in Gerrit")
// First video frame is not supported for Element Timing.Should we have an issue for this? AFAIK video is underspecified, but per spec the exact same records should be eligible for LCP and Element timing -- so this is not desired it is a historical oversight because of the fracturing of these apis?
Yeah good idea. Created a bug and added a TODO.
if (!cached_image->IsLoaded()) {Should this be "sufficiently loaded", eventually?
Again, this is the existing divergent behaviour between these but per spec the behaviour has to be the same, I think.
I was going to say yes, but `IsSufficientContentLoadedForPaint()` is actually just an alias for `IsLoaded()` 😞. Not sure if this was always the case or not. I think we mean two different things by "sufficiently loaded" and "fully loaded", but the code doesn't back it up. (Note that sufficiently loaded doesn't take into account first animated frame, that's different).
```
bool ImageResourceContent::IsSufficientContentLoadedForPaint() const {
return IsLoaded();
}
```
bool is_image_or_video_element = IsA<HTMLImageElement>(node) ||I'm confused by support for video here, when on line 115-119 we skip video frames?
Poster images on video elements, I guess?
Yeah poster images. Note that I added SVGImageElement here because the original code path wasn't used for SVGs.
// added. See also https://crbug.com/1027948.(musing)
Do you think we are closer to being able to report an element timing entry, even if a late `elementtiming` attribute was added to an already loaded and painted element, now that we have this re-architecture?---
I figure that:
- Paint Timing is moving toward measuring all contentful elements, always (changing recorded-set to a sufficiently-loaded-and-already-painted-set).
- Paint Timing might be moving toward observing all size changes and all "contentfulness changes" directly (and updating caches).
- For long-term memory, potentially we could replace "recorded-set" with a "recorded-map" to frame index/PaintTimingMixin.
Even if we don't keep anything but recoded set, we could just report the Now() time after the attribute is added, as long as we know the content already loaded+rendered.
(For things like `loadTime`, I think we already are considering removing this entirely...)
I think we're moving closer to being able to experiment with that. I've been keeping that in mind w/ the refactoring. I think we'll have a better idea once the repaint work is done, but I think the pieces will all be in place then :).
---
(For things like `loadTime`, I think we already are considering removing this entirely...)
- 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. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 2 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
2 comments
if (!cached_image->IsLoaded()) {Scott HaseleyShould this be "sufficiently loaded", eventually?
Again, this is the existing divergent behaviour between these but per spec the behaviour has to be the same, I think.
I was going to say yes, but `IsSufficientContentLoadedForPaint()` is actually just an alias for `IsLoaded()` 😞. Not sure if this was always the case or not. I think we mean two different things by "sufficiently loaded" and "fully loaded", but the code doesn't back it up. (Note that sufficiently loaded doesn't take into account first animated frame, that's different).
```
bool ImageResourceContent::IsSufficientContentLoadedForPaint() const {
return IsLoaded();
}
```
WTF. I was convinced we shipped this change for images years ago.
I guess we could swap for `IsSufficientContentLoadedForPaint` for consistency then without changing anything :P but also could leave this as is and wait for "full" unification.
bool is_image_or_video_element = IsA<HTMLImageElement>(node) ||Scott HaseleyI'm confused by support for video here, when on line 115-119 we skip video frames?
Poster images on video elements, I guess?
Yeah poster images. Note that I added SVGImageElement here because the original code path wasn't used for SVGs.
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 2 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Commit-Queue | +2 |
2 comments
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![]( "Open in Gerrit")
Change information
ElementTiming: Proxy ImageElementTiming calls through PaintTimingDetector
This is a step towards using making ImageElementTiming a client of
PaintTiming instead of having its own image tracking. This CL moves all
the ImageElementTiming API calls to occur from within the corresponding
PaintTimingDetector method. The differences were:
- NotifyImagePainted for first video frame doesn't apply to element
timing currently, so this is filtered out by the type of MediaTiming
- NotifyBackgroundImageFinished was ElementTiming-only. This now flows
through PaintTimingDetector, and as a followup, I'll move the
background image load time tracking into ImagePaintTimingDetector.
- NotifyImageFinished (from LayoutImage) was ElementTiming-only.
This is now also routed through PaintTimingDetector. This will
lead to potentially more image load timestamps being recorded
(for pseudo elements), but that's fine since we'll also need to fix
that case for LCP and make the corresponding experimental flag work
for ElementTiming via ImagePaintTimingDetector.
- There were several extra checks (e.g. IsLoaded(), feature flag,
UseCounter) before NotifyImagePainted() was called, which are moved
into ImageElementTiming, which is a better place for them anyway.
- M third_party/blink/renderer/core/css/css_image_value.cc
- M third_party/blink/renderer/core/css/css_image_value.h
- M third_party/blink/renderer/core/layout/layout_image.cc
- M third_party/blink/renderer/core/layout/layout_object.cc
- M third_party/blink/renderer/core/loader/resource/image_resource_content.h
- M third_party/blink/renderer/core/loader/resource/video_timing.h
- M third_party/blink/renderer/core/paint/box_painter_base.cc
- M third_party/blink/renderer/core/paint/image_painter.cc
- M third_party/blink/renderer/core/paint/svg_image_painter.cc
- M third_party/blink/renderer/core/paint/timing/image_element_timing.cc
- M third_party/blink/renderer/core/paint/timing/image_element_timing.h
- M third_party/blink/renderer/core/paint/timing/paint_timing_detector.cc
- M third_party/blink/renderer/core/paint/timing/paint_timing_detector.h
- M third_party/blink/renderer/core/style/style_fetched_image.cc
- M third_party/blink/renderer/core/style/style_fetched_image.h
- M third_party/blink/renderer/platform/loader/fetch/media_timing.h
Code-Review: +1 by Michal Mocny
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |