Bug 208501 – Background images should be taken into account in the visually non-empty heuristic

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Noam Rosenthal]

Currently, we block the initial render on our "visually non-empty" heuristic. This heuristic depends on images and other things, but currently doesn't take background images into account.

See https://w3c.github.io/paint-timing/#first-contentful-paint.

[Noam Rosenthal]

Created attachment 392267 [details]
Patch

[Noam Rosenthal]

I didn't see any tests that cover the visually-non-empty heuristic. Maybe FCP behind a flag would also help us test these?

[Noam Rosenthal]

Created attachment 392268 [details]
Patch

[alan]

Comment on attachment 392268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392268&action=review

> Source/WebCore/rendering/RenderBox.cpp:1717
> +    incrementVisuallyNonEmptyPixelCountIfNeeded(flooredIntSize(size()));

While this indeed increments the VNE pixel count, we don't really act on it until after the next tree building/layout (e.g this would only trigger a repaint on a fixed sized block level box). This line also implies we've got computed geometry which is not necessarily the case and also rather irrelevant for border images.

> Source/WebCore/rendering/RenderElement.h:277
> +    void incrementVisuallyNonEmptyPixelCountIfNeeded(const IntSize&);

Any reason why this pixel counting logic ended up in the RenderElenent?

[alan]

This makes me wonder if we should rethink the way we collect information on VNE pixels in general and come up with a better model.

[Noam Rosenthal]

(In reply to zalan from comment #4)
> Comment on attachment 392268 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392268&action=review
> 
> > Source/WebCore/rendering/RenderBox.cpp:1717
> > +    incrementVisuallyNonEmptyPixelCountIfNeeded(flooredIntSize(size()));
> 
> While this indeed increments the VNE pixel count, we don't really act on it
> until after the next tree building/layout (e.g this would only trigger a
> repaint on a fixed sized block level box). This line also implies we've got
> computed geometry which is not necessarily the case and also rather
> irrelevant for border images.
OK, maybe an additional line is missing telling FrameView it might need to report paint milestones 

> 
> > Source/WebCore/rendering/RenderElement.h:277
> > +    void incrementVisuallyNonEmptyPixelCountIfNeeded(const IntSize&);
> 
> Any reason why this pixel counting logic ended up in the RenderElenent?
Because RenderElement.h seemed like the place where we put this kinds of flags, and it's common for RenderImage/RenderBox, but it could be in RenderBox just the same (as long as the flag memory is in RenderElement).

[Noam Rosenthal]

(In reply to zalan from comment #5)
> This makes me wonder if we should rethink the way we collect information on
> VNE pixels in general and come up with a better model.

Yes I was thinking the same... I think the pixel model is bound to have a lot of bugs/quirks/inaccuracies. For example in the current model image pixels are computed according to the source image, but there could be 32x32 images that are CSS-shrunk to 1x1 or vice-versa.

Would be happy to chat about this on Slack/IRC

[Noam Rosenthal]

Created attachment 392289 [details]
Patch

[alan]

I am not sure if you've followed the VNE first-paint work on trunk. It has changed a lot recently.
The idea is very simple though:
We try to delay layout and paint until after certain amount of content (mixture of character count/pixel amount) is available.

1. These values are mostly collected through the render tree building phase in response to style changes (ignore replaced content for now)
2. We don't run layout unless it is forced (JS or other means), but the general idea is that we should be able tell whether the content is VNE without computing geometry information.
Since we don't run layout/fake paint to verify if the produced content is VNE, the VNE layout (and the subsequent meaningful paint (incorrect name, really)) milestones could be fired on a VE content. The plan is to recover from this incorrect state through frequent, subsequent paints.
3. The meaningful paint milestone is dispatched as the result of the VNE _layout_ (unless something somewhere somehow forces paint on VE content)

Something along these lines:
- content keeps coming in
- renderer tree gains new nodes through style resolve
- meet the threshold
- unfreeze the layer tree
- schedule layer flush
- initiate VNE layout
- fire VNE layout milestone
- paint to layers
- fire meaningful paint milestone

I've got 2 observations:
-It seems that our logic is considerable different from what https://w3c.github.io/paint-timing/#first-contentful-paint has and while our threshold value has changed over time, it will probably never be like "any text". It also means that the first paint won't happen until after the VNE threshold is met (and in case of highly different thresholds it could result in a very late timestamp for FCP). I guess I should have read the emails on webkit-dev first, I am sure you've already worked this out.

-The current code is very much in flux. It is also a very old and neglected. Not a great combination. I was planning to hook up SVG and other replaced content this/next week and make sure we trigger VNE check for example on incoming image data.
In the long run, I think this feature needs some serious refactoring. I have some ideas e.g move the logic from the c'tors to the RenderTreeBuilder, but it needs some more thinking. I am not sure when I will get to it though.

[Noam Rosenthal]

Got it. In terms of style images - I suggest then to go with the size of the image as the pixel indicator. I will revise the patch for this.

In terms of VNE vs. FCP: I think we should thrive for "closer as possible", and have FCP conform to the spec/WPT, leaving room both in the spec, WPT and implementation for delaying paint on heuristics.

[Noam Rosenthal]

Created attachment 392403 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 392403 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392403&action=review

> Source/WebCore/rendering/RenderBox.cpp:1722
> +        return static_cast<FloatSize>(size());

This assumes that layout is up-to-date, of which there is no guarantee here.

We need to be really careful about when it's valid to query renderer geometry (and thus to do VNE accounting).

> Source/WebCore/rendering/RenderBox.cpp:1725
> +    incrementVisuallyNonEmptyPixelCountIfNeeded(flooredIntSize(imageSize()));

What if imageChanged is called multiple times for the same image? Or for multiple background images on the same renderer? Or for both border-image and background-image on the same renderer?

> Source/WebCore/rendering/RenderElement.h:356
> +    unsigned m_didIncrementVisuallyNonEmptyPixelCount : 1;

I would call this m_didContributeToVisuallyNonEmpty or something.

[Noam Rosenthal]

Comment on attachment 392403 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392403&action=review

>> Source/WebCore/rendering/RenderBox.cpp:1722
>> +        return static_cast<FloatSize>(size());
> 
> This assumes that layout is up-to-date, of which there is no guarantee here.
> 
> We need to be really careful about when it's valid to query renderer geometry (and thus to do VNE accounting).

Gotcha. I will send an empty size in this case.

>> Source/WebCore/rendering/RenderBox.cpp:1725
>> +    incrementVisuallyNonEmptyPixelCountIfNeeded(flooredIntSize(imageSize()));
> 
> What if imageChanged is called multiple times for the same image? Or for multiple background images on the same renderer? Or for both border-image and background-image on the same renderer?

Only the first one will contribute to the VNE heuristic (didIncrementVisuallyNonEmptyPixelCount will return true). They're most likely to affect the same pixels.
It's true that we can't tell their actual pixel contribution because layout has not been performed yet - but this is a general problem with VNE and is also true for regular RenderImages (their content is maybe X pixels, but CSS might make them much larger/smaller).

>> Source/WebCore/rendering/RenderElement.h:356
>> +    unsigned m_didIncrementVisuallyNonEmptyPixelCount : 1;
> 
> I would call this m_didContributeToVisuallyNonEmpty or something.

It was just moved from a different place (RenderImage.h), but sure :)

[alan]

Comment on attachment 392403 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392403&action=review

> Source/WebCore/page/FrameView.h:948
> +    if (m_visuallyNonEmptyPixelCount > visualPixelThreshold && !needsLayout())
> +        fireLayoutRelatedMilestonesIfNeeded();

Firing a layout milestone without actually running layout looks really hackish (I know we do it at load complete as a last resort, but that's not the pattern we should follow).
The issue here is that the current architecture can't deal with paint only changes. I am not sure if trying to hack it like this is the right direction, but other may disagree.

[Noam Rosenthal]

Comment on attachment 392403 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392403&action=review

>> Source/WebCore/page/FrameView.h:948
>> +        fireLayoutRelatedMilestonesIfNeeded();
> 
> Firing a layout milestone without actually running layout looks really hackish (I know we do it at load complete as a last resort, but that's not the pattern we should follow).
> The issue here is that the current architecture can't deal with paint only changes. I am not sure if trying to hack it like this is the right direction, but other may disagree.

Yea, I don't think this is ideal. It's also an option not to fire here at all - at least at the next layout there will be enough pixels to count as VNE.
Open to suggestions!

[Noam Rosenthal]

Created attachment 392451 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 392403 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392403&action=review

> Source/WebCore/rendering/RenderBox.cpp:1718
> +        if (style().borderImage().image() && style().borderImage().image()->data() == image)
> +            return style().borderImage().image()->imageSize(this, style().effectiveZoom());
> +        if (style().maskBoxImage().image() && style().maskBoxImage().image()->data() == image)
> +            return style().maskBoxImage().image()->imageSize(this, style().effectiveZoom());
> +        auto& layers = style().backgroundLayers();
> +        for (auto* layer = &layers; layer; layer = layer->next()) {
> +            if (layer->image() && layer->image()->data() == image)

This stuff is also wrong for tiled background images and border-image. For a tiled 2x2 background image, you want to report the size of the background area, not 4px, right? The spec needs to clarify this too (please file an issue).

[Simon Fraser (smfr)]

(In reply to zalan from comment #14)
> Comment on attachment 392403 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392403&action=review
> 
> > Source/WebCore/page/FrameView.h:948
> > +    if (m_visuallyNonEmptyPixelCount > visualPixelThreshold && !needsLayout())
> > +        fireLayoutRelatedMilestonesIfNeeded();
> 
> Firing a layout milestone without actually running layout looks really
> hackish (I know we do it at load complete as a last resort, but that's not
> the pattern we should follow).
> The issue here is that the current architecture can't deal with paint only
> changes. I am not sure if trying to hack it like this is the right
> direction, but other may disagree.

I think we should also fire milestones at some well-known time, directly from the runloop (on a timer or something). Otherwise clients may call back into WebKit and trigger weird re-entrancy issues).

[Noam Rosenthal]

Comment on attachment 392403 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392403&action=review

>> Source/WebCore/rendering/RenderBox.cpp:1718
>> +            if (layer->image() && layer->image()->data() == image)
> 
> This stuff is also wrong for tiled background images and border-image. For a tiled 2x2 background image, you want to report the size of the background area, not 4px, right? The spec needs to clarify this too (please file an issue).

The paint API spec doesn't care about number of pixels..
As far as VNE in WebKit, I actually think it's debatable - is a background tile of 2x2 in a 100x100 DIV considered 4px of content or 10000px of content.
I personally believe that pixel count heuristics are bound to be buggy.

[Noam Rosenthal]

View in context: https://bugs.webkit.org/attachment.cgi?id=392403&action=review

>> Source/WebCore/rendering/RenderBox.cpp:1718
>> +            if (layer->image() && layer->image()->data() == image)
> 
> This stuff is also wrong for tiled background images and border-image. For a tiled 2x2 background image, you want to report the size of the background area, not 4px, right? The spec needs to clarify this too (please file an issue).

The paint API spec doesn't care about number of pixels..
As far as VNE in WebKit, I actually think it's debatable - is a background tile of 2x2 in a 100x100 DIV considered 4px of content or 10000px of content.
I personally believe that pixel count heuristics are bound to be buggy.

[Noam Rosenthal]

(In reply to Simon Fraser (smfr) from comment #18)

> I think we should also fire milestones at some well-known time, directly
> from the runloop (on a timer or something). Otherwise clients may call back
> into WebKit and trigger weird re-entrancy issues).

This is also true for when we fire this from load-progress, am I right?

[Noam Rosenthal]

Created attachment 395151 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 395151 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395151&action=review

> Source/WebCore/rendering/RenderBox.cpp:1715
> +            // Since at this point we don't know the layout size of the box, use the background image size for the pixel-count heuristic.
> +            incrementVisuallyNonEmptyPixelCountIfNeeded(flooredIntSize(layer->image()->imageSize(this, style().effectiveZoom())));

This seems pretty bizarre. What if I have a 2x2px tiled image covering a large area?

[Noam Rosenthal]

Comment on attachment 395151 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395151&action=review

>> Source/WebCore/rendering/RenderBox.cpp:1715
>> +            incrementVisuallyNonEmptyPixelCountIfNeeded(flooredIntSize(layer->image()->imageSize(this, style().effectiveZoom())));
> 
> This seems pretty bizarre. What if I have a 2x2px tiled image covering a large area?

Right. I pointed it out in the changelog and in the above comment. At this point we simply don't know what area this image covers! This is consistent with RenderImage - the image intrinsic size is used for the pixel rather than the layout size.

[Simon Fraser (smfr)]

(In reply to Noam Rosenthal from comment #24)
> Comment on attachment 395151 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395151&action=review
> 
> >> Source/WebCore/rendering/RenderBox.cpp:1715
> >> +            incrementVisuallyNonEmptyPixelCountIfNeeded(flooredIntSize(layer->image()->imageSize(this, style().effectiveZoom())));
> > 
> > This seems pretty bizarre. What if I have a 2x2px tiled image covering a large area?
> 
> Right. I pointed it out in the changelog and in the above comment. At this
> point we simply don't know what area this image covers! This is consistent
> with RenderImage - the image intrinsic size is used for the pixel rather
> than the layout size.

Does the spec say anything about this?

[Noam Rosenthal]

(In reply to Simon Fraser (smfr) from comment #25)
> (In reply to Noam Rosenthal from comment #24)
> > Comment on attachment 395151 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=395151&action=review
> > 
> > >> Source/WebCore/rendering/RenderBox.cpp:1715
> > >> +            incrementVisuallyNonEmptyPixelCountIfNeeded(flooredIntSize(layer->image()->imageSize(this, style().effectiveZoom())));
> > > 
> > > This seems pretty bizarre. What if I have a 2x2px tiled image covering a large area?
> > 
> > Right. I pointed it out in the changelog and in the above comment. At this
> > point we simply don't know what area this image covers! This is consistent
> > with RenderImage - the image intrinsic size is used for the pixel rather
> > than the layout size.
> 
> Does the spec say anything about this?

The spec doesn't care about number of pixels... The pixel count is purely a WebKit VNE heuristic.

[alan]

Comment on attachment 395151 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395151&action=review

> Source/WebCore/page/FrameView.h:960
> +    if (m_visuallyNonEmptyPixelCount > visualPixelThreshold && !needsLayout() && !layoutContext().isInLayout())
> +        checkAndDispatchDidReachVisuallyNonEmptyState();

I am not sure if it's the right place for this. It'd be great if we could do something similar to what we do with the style/content mutations (RenderTreeBuilder::reportVisuallyNonEmptyContent gets called when we are done with all the style/content mutations and ready to check for the first paint threshold.

> Source/WebCore/rendering/RenderBox.cpp:1753
> +void RenderBox::incrementVisuallyNonEmptyPixelCountIfNeeded(const IntSize& size)
> +{
> +    if (didMarkVisuallyNonEmptyPixels())
> +        return;
> +
> +    view().frameView().incrementVisuallyNonEmptyPixelCount(size);
> +    setDidMarkVisuallyNonEmptyPixels();
> +}

In the long run, we should have some kind of first paint class where we could track all these reportings and not pollute RenderBox with first paint.

[Noam Rosenthal]

Comment on attachment 395151 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395151&action=review

>> Source/WebCore/page/FrameView.h:960
>> +        checkAndDispatchDidReachVisuallyNonEmptyState();
> 
> I am not sure if it's the right place for this. It'd be great if we could do something similar to what we do with the style/content mutations (RenderTreeBuilder::reportVisuallyNonEmptyContent gets called when we are done with all the style/content mutations and ready to check for the first paint threshold.

Maybe for now avoid doing this at all and let VNE take place in the next layout? it will be consistent with how RenderImage works today - it can also report the threshold in a network callback code path and VNE will be reported only by the next layout/progress.

[alan]

(In reply to Noam Rosenthal from comment #28)
> Comment on attachment 395151 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395151&action=review
> 
> >> Source/WebCore/page/FrameView.h:960
> >> +        checkAndDispatchDidReachVisuallyNonEmptyState();
> > 
> > I am not sure if it's the right place for this. It'd be great if we could do something similar to what we do with the style/content mutations (RenderTreeBuilder::reportVisuallyNonEmptyContent gets called when we are done with all the style/content mutations and ready to check for the first paint threshold.
> 
> Maybe for now avoid doing this at all and let VNE take place in the next
> layout? it will be consistent with how RenderImage works today - it can also
Yeah we could do that. (but eventually we have to find a way to explicitly trigger the VNE check because optional layouts are now suspended on visually empty content)

[alan]

Comment on attachment 395151 [details]
Patch

^^^ as discussed above.

[Noam Rosenthal]

Created attachment 396325 [details]
Patch for landing

[Simon Fraser (smfr)]

Comment on attachment 396325 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=396325&action=review

> Source/WebCore/rendering/RenderBox.cpp:1712
> +    auto& layers = style().backgroundLayers();
> +    for (auto* layer = &layers; layer; layer = layer->next()) {
> +        if (layer->image() && layer->image()->data() == image) {
> +            // Since at this point we don't know the layout size of the box, use the background image size for the pixel-count heuristic.
> +            incrementVisuallyNonEmptyPixelCountIfNeeded(flooredIntSize(layer->image()->imageSize(this, style().effectiveZoom())));
> +            break;
> +        }
> +    }

This would be much nicer using a helper that find the image in the background fill layers (if we don't already have such a thing) and returns true if found.

> Source/WebCore/rendering/RenderElement.h:189
> +    bool didMarkVisuallyNonEmptyPixels() const { return m_didMarkVisuallyNonEmptyPixels; }
> +    void setDidMarkVisuallyNonEmptyPixels() { m_didMarkVisuallyNonEmptyPixels = true; }

The 'mark' here is a bit vague. Maybe didContributeTo? Once set, does this stick forever for this renderer?

[Noam Rosenthal]

Comment on attachment 396325 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=396325&action=review

>> Source/WebCore/rendering/RenderBox.cpp:1712
>> +    }
> 
> This would be much nicer using a helper that find the image in the background fill layers (if we don't already have such a thing) and returns true if found.

Sure. No current helper, will add one.

>> Source/WebCore/rendering/RenderElement.h:189
>> +    void setDidMarkVisuallyNonEmptyPixels() { m_didMarkVisuallyNonEmptyPixels = true; }
> 
> The 'mark' here is a bit vague. Maybe didContributeTo? Once set, does this stick forever for this renderer?

Yes, it sticks forever... same as it did so far for RenderImage.

[Noam Rosenthal]

Created attachment 396331 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 396331 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396331&action=review

> Source/WebCore/rendering/RenderBox.cpp:1727
> +
> +    

Extra blank line.

> Source/WebCore/rendering/RenderBox.cpp:1734
> +
> +

Extra blank line.

[Noam Rosenthal]

Created attachment 396334 [details]
Patch

[EWS]

Committed r260045: <https://trac.webkit.org/changeset/260045>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396334 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61740814>