Bug 208482 - Canvas content should factor into visually-non-empty heuristic
Summary: Canvas content should factor into visually-non-empty heuristic
Status: RESOLVED WORKSFORME
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks: 208500
  Show dependency treegraph
 
Reported: 2020-03-02 15:48 PST by Ben Nham
Modified: 2020-04-13 13:56 PDT (History)
19 users (show)

See Also:


Attachments
Patch (1.52 KB, patch)
2020-03-02 21:38 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (5.77 KB, patch)
2020-04-01 00:21 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (5.71 KB, patch)
2020-04-02 00:05 PDT, Noam Rosenthal
zalan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Nham 2020-03-02 15:48:54 PST
Currently, we block the initial render on our "visually non-empty" heuristic. This heuristic depends on (among other things) how much text is in the document, how many image pixels are in the document, and whether or not the main resource is fully parsed. However, SVG and canvas content is not considered by the heuristic.

This causes a delayed first contentful paint for certain websites. For instance, YouTube's first contentful paint is often delayed until far after the main HTML resource is parsed:

1. They don't ship down any text initially, so we won't trip over the visually non-empty character threshold
2. They don't ship down any actual images initially, so we won't trip over the filled-image-pixel threshold (everything that looks like an image is actually an SVG)
3. On an uncached load, we won't paint after parsing the initial HTML either, since there's usually an outstanding web font loading, and our heuristic delays the first paint until the web fonts load if there isn't enough character or image content

We could improve our FCP for YT and other similar sites by including an estimate of SVG and canvas content sizes in the visually-non-empty heuristic. This will also improve FCP as reported by https://w3c.github.io/paint-timing/#first-contentful-paint.
Comment 1 Radar WebKit Bug Importer 2020-03-02 15:49:26 PST
<rdar://problem/59971943>
Comment 2 Ben Nham 2020-03-02 15:49:55 PST
Assigning to Alan since he said he could take a look at this.
Comment 3 zalan 2020-03-02 21:38:16 PST
Created attachment 392248 [details]
Patch
Comment 4 Simon Fraser (smfr) 2020-03-03 11:02:24 PST
Comment on attachment 392248 [details]
Patch

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

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:62
> +    if (this->style().width().isFixed() && this->style().height().isFixed())
> +        view().frameView().incrementVisuallyNonEmptyPixelCount({ this->style().width().intValue(), this->style().height().intValue() });

Don't you need to convert non-pixel fixed types to pixels?
Comment 5 zalan 2020-03-05 13:25:42 PST
SVG content is tracked over here bug 208663
Comment 6 Noam Rosenthal 2020-04-01 00:21:49 PDT
Created attachment 395137 [details]
Patch
Comment 7 Noam Rosenthal 2020-04-01 00:25:14 PDT
Alan, I hope it's OK that I took a crack at this!
Trying to match canvas VNE behavior to FCP, as requested in https://bugs.webkit.org/show_bug.cgi?id=78011#c7

I know that this still has the same problem as https://bugs.webkit.org/show_bug.cgi?id=208501#c14, thinking we can fix those in one go.
Comment 8 Simon Fraser (smfr) 2020-04-01 12:01:04 PDT
Comment on attachment 395137 [details]
Patch

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

Why no test?

> Source/WebCore/ChangeLog:12
> +        Reviewed by NOBODY (OOPS!).

This goes above the description.

> Source/WebCore/html/HTMLCanvasElement.cpp:465
> +        didAttachContext();
>          // Need to make sure a RenderLayer and compositing layer get created for the Canvas.
>          invalidateStyleAndLayerComposition();

Why is this in a different order to the previous one?

> Source/WebCore/rendering/RenderHTMLCanvas.cpp:113
> +    view().frameView().incrementVisuallyNonEmptyPixelCount(ceiledIntSize(intrinsicSize()));

When called from the constructor, is intrinsicSize set yet?
Comment 9 Noam Rosenthal 2020-04-01 12:05:17 PDT
Comment on attachment 395137 [details]
Patch

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

>> Source/WebCore/ChangeLog:12
>> +        Reviewed by NOBODY (OOPS!).
> 
> This goes above the description.

Gotcha

>> Source/WebCore/html/HTMLCanvasElement.cpp:465
>>          invalidateStyleAndLayerComposition();
> 
> Why is this in a different order to the previous one?

No reason, will make more consistent.

>> Source/WebCore/rendering/RenderHTMLCanvas.cpp:113
>> +    view().frameView().incrementVisuallyNonEmptyPixelCount(ceiledIntSize(intrinsicSize()));
> 
> When called from the constructor, is intrinsicSize set yet?

I believe so. Will make sure.
Comment 10 Noam Rosenthal 2020-04-01 12:05:55 PDT
(In reply to Simon Fraser (smfr) from comment #8)
> Comment on attachment 395137 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395137&action=review
> 
> Why no test?
Can I get some pointers to how this kind of feature is tested? https://trac.webkit.org/changeset/257952/webkit, for example, adds similar functionality and did not add tests.
Comment 11 Noam Rosenthal 2020-04-01 13:05:48 PDT
Comment on attachment 395137 [details]
Patch

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

>>> Source/WebCore/rendering/RenderHTMLCanvas.cpp:113
>>> +    view().frameView().incrementVisuallyNonEmptyPixelCount(ceiledIntSize(intrinsicSize()));
>> 
>> When called from the constructor, is intrinsicSize set yet?
> 
> I believe so. Will make sure.

Actually, it has to be available, since the rendering context is available.
Comment 12 Darin Adler 2020-04-01 13:32:35 PDT
Comment on attachment 395137 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:927
> +    auto& canvasRenderer = downcast<RenderHTMLCanvas>(*renderer);
> +    canvasRenderer.didAttachContext();

Better as a one-liner:

    downcast<RenderHTMLCanvas>(*renderer).didAttachContext();
Comment 13 Noam Rosenthal 2020-04-02 00:05:22 PDT
Created attachment 395245 [details]
Patch
Comment 14 Noam Rosenthal 2020-04-13 10:30:26 PDT
Seems like this was fixed in https://trac.webkit.org/changeset/257899/webkit
Comment 15 zalan 2020-04-13 12:48:02 PDT
Comment on attachment 395245 [details]
Patch

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

> Source/WebCore/rendering/RenderHTMLCanvas.cpp:115
> +void RenderHTMLCanvas::incrementVisuallyNonEmptyPixelCountIfNeeded()
> +{
> +    if (m_didIncrementVisuallyNonEmptyPixelCount)
> +        return;
> +
> +    view().frameView().incrementVisuallyNonEmptyPixelCount(ceiledIntSize(intrinsicSize()));
> +    m_didIncrementVisuallyNonEmptyPixelCount = true;    
> +}

This is another example where this logic would be much better in a dedicated first paint class with all the tracking.
Comment 16 Noam Rosenthal 2020-04-13 13:09:47 PDT
(In reply to zalan from comment #15)
> Comment on attachment 395245 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395245&action=review
> 
> > Source/WebCore/rendering/RenderHTMLCanvas.cpp:115
> > +void RenderHTMLCanvas::incrementVisuallyNonEmptyPixelCountIfNeeded()
> > +{
> > +    if (m_didIncrementVisuallyNonEmptyPixelCount)
> > +        return;
> > +
> > +    view().frameView().incrementVisuallyNonEmptyPixelCount(ceiledIntSize(intrinsicSize()));
> > +    m_didIncrementVisuallyNonEmptyPixelCount = true;    
> > +}
> 
> This is another example where this logic would be much better in a dedicated
> first paint class with all the tracking.

I saw that you already made canvas increment the pixel count in https://trac.webkit.org/changeset/257899/webkit, isn't this patch superfluous now?
Comment 17 Simon Fraser (smfr) 2020-04-13 13:47:17 PDT
Comment on attachment 395245 [details]
Patch

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

> Source/WebCore/rendering/RenderHTMLCanvas.h:54
> +    bool m_didIncrementVisuallyNonEmptyPixelCount { false };

Why doesn't this use RenderElement's didMarkFor...?
Comment 18 Noam Rosenthal 2020-04-13 13:54:38 PDT
Comment on attachment 395245 [details]
Patch

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

>> Source/WebCore/rendering/RenderHTMLCanvas.h:54
>> +    bool m_didIncrementVisuallyNonEmptyPixelCount { false };
> 
> Why doesn't this use RenderElement's didMarkFor...?

Oh damn! It was added after this patch. Will fix. But I want to know first if this is even needed, given https://trac.webkit.org/changeset/257899/webkit
Comment 19 Noam Rosenthal 2020-04-13 13:56:13 PDT
(In reply to Noam Rosenthal from comment #18)
> Comment on attachment 395245 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395245&action=review
> 
> >> Source/WebCore/rendering/RenderHTMLCanvas.h:54
> >> +    bool m_didIncrementVisuallyNonEmptyPixelCount { false };
> > 
> > Why doesn't this use RenderElement's didMarkFor...?
> 
> Oh damn! It was added after this patch. Will fix. But I want to know first
> if this is even needed, given https://trac.webkit.org/changeset/257899/webkit

Ah it was in my other patch... sure. I will revise this after the bg-image patch goes in, once we figure out if this is needed...