203557 – Determine viewport distances for lazy image loading

Bug 203557 - Determine viewport distances for lazy image loading

Determine viewport distances for lazy image loading for desktop, iOS etc.

Created attachment 382169 [details]
Patch

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.

Also curious, where does 100px come from? Is that the threshold Chrome uses?

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.

(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.

Created attachment 391519 [details]
Patch

(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.

(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.

(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.

(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?

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.

(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.

Created attachment 393764 [details]
Patch

(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?

Created attachment 395139 [details]
Patch

Created attachment 395148 [details]
Patch

Created attachment 395161 [details]
Patch

Created attachment 395171 [details]
Patch

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?

Created attachment 395407 [details]
Patch

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 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?

Should we land this before https://github.com/whatwg/html/issues/5408 is resolved?

Created attachment 395597 [details]
Patch

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.

(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?

(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 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?