Bug 203557

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?

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

https://github.com/whatwg/html/pull/5917 has now been merged. See https://github.com/whatwg/html/issues/5408#issuecomment-643505639 for what Chromium is currently doing. The relevant distances are in https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/frame/settings.json5 (e.g. "lazyImageLoadingDistanceThresholdPx4G").

Is there a new approach I can experiment with? Latest change Mozilla made for this feature seems to be https://bugzilla.mozilla.org/show_bug.cgi?id=1673785.

300px seems way too small. Chrome's 1250px seems more reasonable for macOS/iOS scrolling speeds. I think we should implement something in the 1000px range (or some multiple of viewport size) and play with it.

Created attachment 423186 [details] Patch

Comment on attachment 423186 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423186&action=review > Source/WebCore/html/LazyLoadImageObserver.cpp:90 > + if (auto renderView = document.renderView()) { > + StringBuilder builder; > + builder.appendNumber(renderView->viewportSizeForCSSViewportUnits().height()); > + builder.append("px 0px"); > + return builder.toString(); > + } This should use makeString instead of StringBuilder. It’s more efficient and reads better too: if (auto renderView = document.renderView()) return makeString(renderView->viewportSizeForCSSViewportUnits().height(), "px 0px");

Created attachment 423199 [details] Patch

Comment on attachment 423199 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423199&action=review > Source/WebCore/html/LazyLoadImageObserver.cpp:88 > + if (auto renderView = document.renderView()) > + return makeString(renderView->viewportSizeForCSSViewportUnits().height(), "px 0px"); > + static NeverDestroyed<const String> lazyLoadingRootMarginFallback(MAKE_STATIC_STRING_IMPL("1250px 0px")); > + return lazyLoadingRootMarginFallback; Had three other follow-up thoughts: 1) Just realized that this accesses the RenderView without doing any form of "updateLayout" first. Not sure that’s OK. 2) Should probably write it like this: auto renderView = document.renderView(); return makeString(renderView ? renderView->viewportSizeForCSSViewportUnits().height() : 1250, "px 0px"); Why go to the trouble of optimizing the very unusual case where there is no RenderView? Might want the 1250 to be a named constant, also, so we can comment and explain how we chose the value. 3) Side note about MAKE_STATIC_STRING_IMPL: If we do keep around a string constant, we should figure out if MAKE_STATIC_STRING_IMPL is optimizing for the right thing in this case. I’d be tempted to just write this: return "1250px 0px"_s; That allocates a String every time this is called, but points at the characters in the binary rather than copying them into the WTF::StringImpl object; it’s pretty efficient and the smallest, I think, although the thing above in (2) is even smaller. The next level of optimization is to eliminate the cost of creating and destroying a string every time by doing this (not sure I have the syntax right): static NeverDestroyed<String> fallback = "1250px 0px"_s return fallback; That uses a single global string so it’s never destroyed. The next level would be to use ConstructFromLiteral, slightly more efficient to initialize because it doesn't compute the length at runtime, but uglier source code and generates a slightly bigger binary since it compiles the length in. MAKE_STATIC_STRING_IMPL is a further optimization that we devised, specifically because we wanted a thread safe way to do it I think. It’s definitely uglier in its own way (ALL CAPS) and also seems to trade off an even bigger binary for slightly better performance. I don’t think we want to use MAKE_STATIC_STRING_IMPL any time we want to return a constant string value. I’d love to find a rule of thumb.

Sharing some insight from Chrome's experiments into LazyLoad thresholds in case it may be helpful for WebKit: - We experimented with a number of popular JavaScript library solutions for lazy-loading and determined that the optimal threshold (for specifically aligning with developer expectations) is in the range of 750px - 1250px for the root-margin. When testing values in this range manually, they also appeared to meet user expectations of a good experience during scroll. - We studied (at scale) how developers would manually override threshold values when using a library like LazySizes. The most common override value was ~1000px. Some would set it higher and others slightly lower, but not by a large amount. - I agree with Simon that 300px as a threshold is too small. I would recommend 1250px as the feedback from developers has been this met most expectations. I could also see the argument for a value in the 1000-1250px range (given the above note). I do think it's worth experimenting. - With respect to https://github.com/w3c/IntersectionObserver/issues/431, Google properties are a bit behind in terms of switching to native LazyLoad for their carousels and I unfortunately haven't received any notes regarding shortcomings from those who have updated to use loading=lazy. That said, I view 431 as a nice incremental addition to LazyLoad that could come later. I hope something in here is helpful to the discussion... :)

(In reply to Darin Adler from comment #36) > Might want the 1250 to be a named constant, also, so we can > comment and explain how we chose the value. I do wonder if it makes sense to make it a setting, rather than hardcoding the value into WebCore. This would also make experimenting easier, and non-web applications might have their own opinions here.

I've been advocating that expressing these thresholds in vh/vw is superior to static px values. Hasn't gotten much traction on chromium yet. https://bugs.chromium.org/p/chromium/issues/detail?id=1011182

Hi folks, This issue was discussed on Twitter last week: https://twitter.com/zcorpan/status/1427878342531948545 While implementing the non-normative suggestions in the spec in a good way will require some testing and research, I suggest that a simple approach like a fixed pixel value is good enough for a first version.

Twitter also noted that Chrome had a bug with printing. We need a story for printing and lazy image loads.

It seems like Firefox handles this correctly if I understand Emilio's comment [1] correctly. A good start would be for someone to spec what should happen here. That would certainly go towards unblocking our Chromium bug around this as well, and would probably up the priority of it. (Unfortunately we're not staffing any effort around that bug and other lingering lazyload issues at the moment). [1]: https://github.com/whatwg/html/issues/6581#issuecomment-821910632 [2]: https://bugs.chromium.org/p/chromium/issues/detail?id=875403

Created attachment 439729 [details] Patch

(In reply to Ben Roberts from comment #39) > I've been advocating that expressing these thresholds in vh/vw is superior > to static px values. Hasn't gotten much traction on chromium yet. > https://bugs.chromium.org/p/chromium/issues/detail?id=1011182 I also like that idea since some time. I made a new patch that basically uses 1vh above and below the current page for the areas to start lazy image loads.

Comment on attachment 439729 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439729&action=review > Source/WebCore/html/LazyLoadImageObserver.cpp:89 > + static NeverDestroyed<const String> lazyLoadingRootMarginFallback(MAKE_STATIC_STRING_IMPL("100% 0px")); > + IntersectionObserver::Init options { std::nullopt, lazyLoadingRootMarginFallback, { } }; Doesn't this need to account for horizontally scrollable documents? You need to look at main view scrollability.

Can you use "1 viewport in all directions"?

Created attachment 439891 [details] Patch

(In reply to Simon Pieters (:zcorpan) from comment #46) > Can you use "1 viewport in all directions"? I think so, I tried to implement that in my latest patch.

Is it OK to load images offscreen on an axis that that the user can never scroll to? Should images that can only be revealed by rubber-banding lazy-load?

(In reply to Simon Fraser (smfr) from comment #49) > Is it OK to load images offscreen on an axis that that the user can never > scroll to? You mean load lazily? Seems ok if the web author wants to do that for some reason. Also note that AFAICS the offscreen image can be moved into viewport by repositioning/JS, so I am not sure we can make any assumptions about images being offscreen at a certain moment. > Should images that can only be revealed by rubber-banding lazy-load? I do not see the problem, using a suitable root margin liken the patch they would be revealed like they are eagerly loaded AFAICS.

(In reply to Rob Buis from comment #50) > (In reply to Simon Fraser (smfr) from comment #49) > > Is it OK to load images offscreen on an axis that that the user can never > > scroll to? > > You mean load lazily? Seems ok if the web author wants to do that for some > reason. Also note that AFAICS the offscreen image can be moved into viewport > by repositioning/JS, so I am not sure we can make any assumptions about > images being offscreen at a certain moment. I'm thinking of a case where a page as a bunch of lazy-load images stacked just offscreen (left: -100%) because the author wants to animated them in. A default horizontal root margin of 100vw (on a page that is not horizontally scrollble) will trigger loads on these images when the author may not expect that, because the user can't reveal these images via scrolling. Also, how does lazy loading and animation interact? If an image is brought into the lazy-loading area via a transform animation, does it load? This needs a bunch of WPT testing.

(In reply to Simon Fraser (smfr) from comment #51) > Also, how does lazy loading and animation interact? If an image is brought > into the lazy-loading area via a transform animation, does it load? This relies on intersection observer. Intersection observer seems to trigger the callback but only once the animation ends: https://jsfiddle.net/38v2dots/2/ Firefox triggers as soon as the image enters the viewport. Not sure what the specification says about this but to me Firefox behaviour seems more intuitive.

(In reply to Simon Fraser (smfr) from comment #51) > This needs a bunch of WPT testing. Some of the tests in https://wpt.fyi/results/html/semantics/embedded-content/the-img-element?label=master&label=experimental&product=chrome&product=firefox&product=webkitgtk&aligned&q=lazy seem to be related to such things (image-loading-lazy-negative-margin.html for one).

(In reply to Sam Sneddon [:gsnedders] from comment #53) > (In reply to Simon Fraser (smfr) from comment #51) > > This needs a bunch of WPT testing. > > Some of the tests in > https://wpt.fyi/results/html/semantics/embedded-content/the-img- > element?label=master&label=experimental&product=chrome&product=firefox&produc > t=webkitgtk&aligned&q=lazy seem to be related to such things > (image-loading-lazy-negative-margin.html for one). Well spotted, importing them here: https://bugs.webkit.org/show_bug.cgi?id=231297

What's next here? Do we need to resubmit the patch to check what EWS says?

(In reply to Simon Fraser (smfr) from comment #51) > (In reply to Rob Buis from comment #50) > > (In reply to Simon Fraser (smfr) from comment #49) > > > Is it OK to load images offscreen on an axis that that the user can never > > > scroll to? > > > > You mean load lazily? Seems ok if the web author wants to do that for some > > reason. Also note that AFAICS the offscreen image can be moved into viewport > > by repositioning/JS, so I am not sure we can make any assumptions about > > images being offscreen at a certain moment. > > I'm thinking of a case where a page as a bunch of lazy-load images stacked > just offscreen (left: -100%) because the author wants to animated them in. A > default horizontal root margin of 100vw (on a page that is not horizontally > scrollble) will trigger loads on these images when the author may not expect > that, because the user can't reveal these images via scrolling. Are you willing to create a GitHub issue for this? I mostly get what you are saying but I think you can formulate it better than I could. > Also, how does lazy loading and animation interact? If an image is brought > into the lazy-loading area via a transform animation, does it load? I created https://github.com/w3c/IntersectionObserver/issues/484 for this.

(In reply to Jon Lee from comment #55) > What's next here? Do we need to resubmit the patch to check what EWS says? Simon still has some questions so likely we will have to get clarification on spec level and maybe add some WPT tests.

(In reply to Rob Buis from comment #57) > (In reply to Jon Lee from comment #55) > > What's next here? Do we need to resubmit the patch to check what EWS says? > > Simon still has some questions so likely we will have to get clarification > on spec level and maybe add some WPT tests. Should we review/land this, even if we keep it behind the flag for now? Get it to the point where it's more easily testable.

Committed r284148 (242968@main): <https://commits.webkit.org/242968@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 439891 [details].