Bug 203557 - Determine viewport distances for lazy image loading
Summary: Determine viewport distances for lazy image loading
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords:
Depends on:
Blocks: 196698 200764
  Show dependency treegraph
 
Reported: 2019-10-29 02:25 PDT by Rob Buis
Modified: 2020-09-15 08:09 PDT (History)
13 users (show)

See Also:


Attachments
Patch (2.07 KB, patch)
2019-10-29 02:28 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (2.08 KB, patch)
2020-02-24 02:11 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (6.93 KB, patch)
2020-03-17 10:11 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (1.35 KB, patch)
2020-04-01 00:41 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (1.45 KB, patch)
2020-04-01 01:40 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (7.25 KB, patch)
2020-04-01 06:58 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (7.82 KB, patch)
2020-04-01 08:54 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (7.98 KB, patch)
2020-04-03 13:12 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (8.02 KB, patch)
2020-04-06 12:06 PDT, Rob Buis
rbuis: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2019-10-29 02:25:30 PDT
Determine viewport distances for lazy image loading for desktop, iOS etc.
Comment 1 Rob Buis 2019-10-29 02:28:58 PDT
Created attachment 382169 [details]
Patch
Comment 2 Maciej Stachowiak 2020-02-22 12:26:23 PST
Why is the viewport distance different for iOS than everything else? I think the same behavior is reasonably appropriate for all platforms. If anything it should be a function of how fast it's possible to scroll on that platform.
Comment 3 Maciej Stachowiak 2020-02-22 12:29:54 PST
Also curious, where does 100px come from? Is that the threshold Chrome uses?
Comment 4 Maciej Stachowiak 2020-02-22 12:37:57 PST
According to this article, Chromium uses significantly higher distances <https://www.ctrl.blog/entry/lazy-loading-viewports.html>


> Chromium/Blink uses a margin of 3000px on low-latency network connections, and up to 8000px on high-latency connections.

This seems like a reasonable starting point, though it might need some kind of input on expected network latency from the network stack.

I could also imagine wanting a different threshold in some sort of "save data" mode, but I'm not sure WebKit has that as a concept yet.

If the threshold is lower for smaller screens, it should probably be based on viewport height, not OS. iPads should probably have a threshold similar to desktop devices at the same viewport height, for instance.
Comment 5 Rob Buis 2020-02-24 00:19:35 PST
(In reply to Maciej Stachowiak from comment #3)
> Also curious, where does 100px come from? Is that the threshold Chrome uses?

I can't find any reference to that except the patch attached to this bug.
Comment 6 Rob Buis 2020-02-24 02:11:31 PST
Created attachment 391519 [details]
Patch
Comment 7 Rob Buis 2020-03-09 07:35:40 PDT
(In reply to Maciej Stachowiak from comment #2)
> Why is the viewport distance different for iOS than everything else? I think
> the same behavior is reasonably appropriate for all platforms. If anything
> it should be a function of how fast it's possible to scroll on that platform.

Using scrolling speed does sound reasonable to me.
Comment 8 Rob Buis 2020-03-09 07:39:10 PDT
(In reply to Maciej Stachowiak from comment #3)
> Also curious, where does 100px come from? Is that the threshold Chrome uses?

For the record, right now we should match Firefox in that we simply do not use a threshold value. From my tests on desktop and with my reasonable internet connection this works well, but things may be different on mobile and so-so connections.
Comment 9 Simon Fraser (smfr) 2020-03-09 10:08:04 PDT
(In reply to Rob Buis from comment #7)
> (In reply to Maciej Stachowiak from comment #2)
> > Why is the viewport distance different for iOS than everything else? I think
> > the same behavior is reasonably appropriate for all platforms. If anything
> > it should be a function of how fast it's possible to scroll on that platform.
> 
> Using scrolling speed does sound reasonable to me.

We already have scrolling speed computations for tile coverage (see GraphicsLayerCA::adjustCoverageRect()). This stuff is hard to get right, and I'd be reluctant to see another implementation added. I'd prefer to leverage existing IntersectionObserver or tile coverage code.
Comment 10 Rob Buis 2020-03-10 09:22:21 PDT
(In reply to Simon Fraser (smfr) from comment #9)
> (In reply to Rob Buis from comment #7)
> > (In reply to Maciej Stachowiak from comment #2)
> > > Why is the viewport distance different for iOS than everything else? I think
> > > the same behavior is reasonably appropriate for all platforms. If anything
> > > it should be a function of how fast it's possible to scroll on that platform.
> > 
> > Using scrolling speed does sound reasonable to me.
> 
> We already have scrolling speed computations for tile coverage (see
> GraphicsLayerCA::adjustCoverageRect()). This stuff is hard to get right, and
> I'd be reluctant to see another implementation added. I'd prefer to leverage
> existing IntersectionObserver or tile coverage code.

I played around with it today. On my 15" mbp I get values for roughly the window height (I use mini browser) when I add logging. However it seems at LazyLoadImageObserver construction time the information is not reliable yet, I guess because compositing has not been done/started yet? Exposing some GraphicsLayer API is easy for me but it is harder for me to reason about compositing which I am not familiar with. Is an alternative to use heuristics approximating it?
Comment 11 Rob Buis 2020-03-13 09:05:43 PDT
I looked at this a bit more, I think there is a problem with using GraphicsLayerCA::adjustCoverageRect().

In RemoteLayerTreeDrawingArea::updateRendering(), m_webPage.updateRendering() is called first, which will run the intersection observer algorithm.

However GraphicsLayerCA::adjustCoverageRect() is only called a bit later using m_webPage.mainFrameView()>flushCompositingStateIncludingSubframes().

So AFAICS the intersection observer algorithm could use the coverage rect information from the previous time RemoteLayerTreeDrawingArea::updateRendering was called, but not the current time.
Comment 12 Simon Fraser (smfr) 2020-03-16 10:43:42 PDT
(In reply to Rob Buis from comment #11)
> I looked at this a bit more, I think there is a problem with using
> GraphicsLayerCA::adjustCoverageRect().
> 
> In RemoteLayerTreeDrawingArea::updateRendering(),
> m_webPage.updateRendering() is called first, which will run the intersection
> observer algorithm.
> 
> However GraphicsLayerCA::adjustCoverageRect() is only called a bit later
> using m_webPage.mainFrameView()>flushCompositingStateIncludingSubframes().
> 
> So AFAICS the intersection observer algorithm could use the coverage rect
> information from the previous time
> RemoteLayerTreeDrawingArea::updateRendering was called, but not the current
> time.

That's true.
Comment 13 Rob Buis 2020-03-17 10:11:33 PDT
Created attachment 393764 [details]
Patch
Comment 14 Rob Buis 2020-03-18 14:38:34 PDT
(In reply to Simon Fraser (smfr) from comment #12)
> (In reply to Rob Buis from comment #11)
> > So AFAICS the intersection observer algorithm could use the coverage rect
> > information from the previous time
> > RemoteLayerTreeDrawingArea::updateRendering was called, but not the current
> > time.
> 
> That's true.

Simon, is the m_coverageRect in document coordinates? And if not how can it be converted?
Comment 15 Rob Buis 2020-04-01 00:41:33 PDT
Created attachment 395139 [details]
Patch
Comment 16 Rob Buis 2020-04-01 01:40:53 PDT
Created attachment 395148 [details]
Patch
Comment 17 Rob Buis 2020-04-01 06:58:15 PDT
Created attachment 395161 [details]
Patch
Comment 18 Rob Buis 2020-04-01 08:54:37 PDT
Created attachment 395171 [details]
Patch
Comment 19 Simon Fraser (smfr) 2020-04-01 12:03:41 PDT
Comment on attachment 395171 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:7570
> +        if (observer.determineDynamicRootMargin()) {

"dynamic" is a bit vague. Maybe paintCoverageRootMargin?

> Source/WebCore/page/FrameView.cpp:5453
> +FloatRect FrameView::coverageRect() const

Is it OK that this will lag one frame behind?

Does something trigger a rendering update if coverage rect changes in a frame, but there are no other changes that trigger one?
Comment 20 Rob Buis 2020-04-03 13:12:49 PDT
Created attachment 395407 [details]
Patch
Comment 21 Rob Buis 2020-04-03 13:17:04 PDT
Comment on attachment 395171 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:7570
>> +        if (observer.determineDynamicRootMargin()) {
> 
> "dynamic" is a bit vague. Maybe paintCoverageRootMargin?

Sure, I used that name now.

>> Source/WebCore/page/FrameView.cpp:5453
>> +FloatRect FrameView::coverageRect() const
> 
> Is it OK that this will lag one frame behind?
> 
> Does something trigger a rendering update if coverage rect changes in a frame, but there are no other changes that trigger one?

The first frame there will not be any coverage rect but I think the fallback to viewport height is reasonable. Later frames will indeed lag one frame behind but from my testing on OS X the coverage rect height is pretty constant while scrolling, so I don't think it is a problem.

Can you detail your second question or give an example? I am not familiar with compositing and only a bit with rendering (these days).
Comment 22 Darin Adler 2020-04-03 13:23:05 PDT
Comment on attachment 395407 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:7580
> +            auto intRect = enclosingIntRect(floatRect);

I suggest only putting the height into a local variable since we aren’t using the location or width:

    auto height = enclosingIntRect(floatRect).height();

> Source/WebCore/dom/Document.cpp:7581
> +            fprintf(stderr, "Using rect %d\n", intRect.height());

I suspect this printf is something you do not want to check in?

> Source/WebCore/html/LazyLoadImageObserver.cpp:80
>      auto& observer = document.lazyLoadImageObserver();
> -    ASSERT(observer.isObserved(element));
>      observer.m_lazyLoadIntersectionObserver->unobserve(element);

I suggest making it a one-liner.

> Source/WebCore/page/FrameView.cpp:5481
> +    RenderView* renderView = this->renderView();
> +    if (!renderView)
> +        return FloatRect();
> +
> +    RenderLayerBacking* backing = renderView->layer()->backing();
> +    if (!backing)
> +        return FloatRect();
> +
> +    auto* graphicsLayer = backing->graphicsLayer();
> +    if (!graphicsLayer)
> +        return FloatRect();
> +
> +    return graphicsLayer->coverageRect();

My preference is just "auto" here for all these locals, and one word names. Local variable names like "view", "backing", "layer" don’t need to be two word phrases in such a short function; the type system will make sure we don’t get it wrong. Also like { } better than FloatRect().

    auto view = renderView();
    if (!view)
        return { };

Maybe the word "view" isn’t so good?

An advantage of auto is that if we change to return RefPtr we don’t have to change call sites like these. If we wrote "auto*" then we would. It’s better not to be so specific; we want to null check something and dereference it, but we don’t need to constrain its type.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:326
> +    FloatRect coverageRect() const override { return m_coverageRect; }

Can this be final instead?
Comment 23 Simon Fraser (smfr) 2020-04-03 15:02:51 PDT
Should we land this before https://github.com/whatwg/html/issues/5408 is resolved?
Comment 24 Rob Buis 2020-04-06 12:06:33 PDT
Created attachment 395597 [details]
Patch
Comment 25 Rob Buis 2020-04-06 13:40:36 PDT
Comment on attachment 395407 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:7580
>> +            auto intRect = enclosingIntRect(floatRect);
> 
> I suggest only putting the height into a local variable since we aren’t using the location or width:
> 
>     auto height = enclosingIntRect(floatRect).height();

Sure, done.

>> Source/WebCore/dom/Document.cpp:7581
>> +            fprintf(stderr, "Using rect %d\n", intRect.height());
> 
> I suspect this printf is something you do not want to check in?

Oops! Removed.

>> Source/WebCore/html/LazyLoadImageObserver.cpp:80
>>      observer.m_lazyLoadIntersectionObserver->unobserve(element);
> 
> I suggest making it a one-liner.

Done.

>> Source/WebCore/page/FrameView.cpp:5481
>> +    return graphicsLayer->coverageRect();
> 
> My preference is just "auto" here for all these locals, and one word names. Local variable names like "view", "backing", "layer" don’t need to be two word phrases in such a short function; the type system will make sure we don’t get it wrong. Also like { } better than FloatRect().
> 
>     auto view = renderView();
>     if (!view)
>         return { };
> 
> Maybe the word "view" isn’t so good?
> 
> An advantage of auto is that if we change to return RefPtr we don’t have to change call sites like these. If we wrote "auto*" then we would. It’s better not to be so specific; we want to null check something and dereference it, but we don’t need to constrain its type.

Done. Yes I am starting to see the benefits of auto.

>> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:326
>> +    FloatRect coverageRect() const override { return m_coverageRect; }
> 
> Can this be final instead?

Sure, done.
Comment 26 Rob Buis 2020-04-06 14:24:54 PDT
(In reply to Simon Fraser (smfr) from comment #23)
> Should we land this before https://github.com/whatwg/html/issues/5408 is
> resolved?

The expectation is that nothing stronger than a recommendation/hint will be the result. But it looks like Simon Pieters is actively investigating so indeed we can hold landing this off maybe a bit longer. Related to that, are there any APIs available to me to take into account connection speed/battery life? Any existing code in WebKit that queries this information?
Comment 27 Simon Fraser (smfr) 2020-04-06 15:06:02 PDT
(In reply to Rob Buis from comment #26)

> there any APIs available to me to take into account connection speed/battery
> life? Any existing code in WebKit that queries this information?

No, because we consider those privacy-sensitive.
Comment 28 Rob Buis 2020-04-23 13:37:20 PDT
Comment on attachment 395597 [details]
Patch

I am disabling cq for this patch based on Simon's comment in https://github.com/whatwg/html/issues/5408, to avoid any confusion about the state of the patch. Simon, any suggestions what we could try instead, or should we wait 5408 discussion?
Comment 29 Simon Pieters 2020-09-15 08:09:46 PDT
If you were waiting on https://github.com/whatwg/html/issues/5408 , I've now written a PR at https://github.com/whatwg/html/pull/5917