Closed Bug 1534240 Opened 7 months ago Closed 7 months ago

Increase the priority of images loaded using CSS

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: smaug, Assigned: heycam)

Details

Attachments

(2 files, 2 obsolete files)

By default images are loaded using low priority.
If I read the code correctly, images bound to <img> may then get higher priority
https://searchfox.org/mozilla-central/rev/2f1020dc4176d38dd5f3d0496f3c46849862ee0b/layout/generic/nsImageFrame.cpp#367
https://searchfox.org/mozilla-central/rev/2f1020dc4176d38dd5f3d0496f3c46849862ee0b/layout/generic/nsImageFrame.cpp#2087

But I don't see similar happening to images loading via CSS.
As an example https://www.huffingtonpost.com/ has the main image loaded as css background.

Filter https://img.huffingtonpost.com/asset/5c85d874240000f404a2c6d1.jpeg?cache=i1o3kdw5eu&ops=2160_720,prog,quality_75
in https://perfht.ml/2EPRwpN
That image is loaded with low priority (10).
Many other images are loaded with a tiny bit higher priority 8, and are loaded before the main image.
(-20 is the highest priority)

Assignee: nobody → cam
Status: NEW → ASSIGNED
Priority: -- → P2
Attached patch patch (obsolete) — Splinter Review

smaug, the page has changed and doesn't have a big main content CSS background-image any more. Do you have other similar sites you could use to test this patch? Or would you prefer me to just get it reviewed and landed? I spot checked locally on a couple of pages that the priorities get boosted for CSS background-images.

Flags: needinfo?(bugs)

The main image is still
style="background-image: url(https://img.huffingtonpost.com/asset/5c92f6972300007d006c4276.png?cache=hvzjao0a46&ops=960_720,prog,quality_75);" attribute in a <div> element.

Flags: needinfo?(bugs)

One argument for prioritizing <img> over CSS images (particularly background-image/border-image) is that <img> can affect the layout of the page, so loading can cause things to move around. (This is particularly true for prioritization of images that are offscreen.)

The existing prioritization boost of HTML <img> will be greater than the CSS images with this patch, for exactly that reason. (It doesn't use the CATEGORY_SIZE_QUERY boost category for CSS images.)

(In reply to Olli Pettay [:smaug] from comment #2)

The main image is still
style="background-image: url(https://img.huffingtonpost.com/asset/5c92f6972300007d006c4276.png?cache=hvzjao0a46&ops=960_720,prog,quality_75);" attribute in a <div> element.

Oh, I didn't realize I was being redirected to the Australian edition of the site, which doesn't have the big banner background image.

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it) from comment #3)

One argument for prioritizing <img> over CSS images (particularly background-image/border-image) is that <img> can affect the layout of the page, so loading can cause things to move around. (This is particularly true for prioritization of images that are offscreen.)

But that makes me realize that list-style-image images should use that category.

Attached patch patch v0.2 (obsolete) — Splinter Review
Attachment #9052491 - Attachment is obsolete: true

Olli I don't know how to evaluate the effectiveness of this patch. What do you suggest?

Flags: needinfo?(bugs)

I would at least profile https://www.huffingtonpost.com/ page load. But perhaps the patch won't be enough to help there, since there are other images which are usually loaded before the main image.

Flags: needinfo?(bugs)

I didn't see any significant change in load ordering on that page.

Profile without patch: https://perfht.ml/2URtBNf
Profile with patch: https://perfht.ml/2CBbcND

I wrote a small test to verify that the priority changing does cause a CSS background-image that's about to be painted to start its load before others that are blocked due to HTTP connection limits: https://mcc.id.au/2019/image-load/

That's on my server using HTTP 1.1. I don't really know how HTTP 2 is meant to work, which is what huffpost.com uses.

When I updated my server to use HTTP 2 all the resources in my test loaded simultaneously. That does look more like pattern when loading huffpost.com, where the initial batch of image loads from img.huffingtonpost.com (28 of them, including this banner image) start loading at the same time.

This is what the first batch of loads from img.huffingtonpost.com looks like. The highlighted one is the banner image.

Of the 28 images that are loading at that point, the nsISupportsPriority priorities of them are:

  • 1 with priority 20
  • 7 with priority 18
  • 8 with priority 8
  • 2 with priority -1, one of which is the banner image here.

These get converted into HTTP 2 priority values in the range [1, 40], but it seems like HTTP 2 priorities are intended to be interpreted as proportions, not as groups where more important groups get absolute priority. mt tells me that servers are inconsistent in their implementation of priorities anyway, so there's probably not much we can do here other than select a priority and hope the server does something useful with it.

So with that in mind I'll request review of my patch but don't expect it to make a difference on this site.

Attachment #9052757 - Attachment is obsolete: true
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7f0d03ec917c
Boost loading priority of CSS images similarly to how we do for HTML images. r=tnikkel
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.