Use IO thread to do copying for chrome and blob URIs

RESOLVED FIXED in Firefox 58

Status

()

Core
ImageLib
P3
normal
RESOLVED FIXED
11 months ago
6 months ago

People

(Reporter: aosmond, Assigned: aosmond)

Tracking

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

11 months ago
Currently imagelib only allows images received over an HTTP channel to be processed using the IO thread (to do the memcpy into our encoded data cache):

http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/image/imgRequest.cpp#818

However nsIChannel implements GetContentType, not nsIHttpChannel specifically. If we switch to nsIChannel, that means chrome, data and blob URIs will all now be copied off the main thread and on the IO thread.

The content type is important because if it is an SVG, we need to handle it on the main thread. Bug 867755 comment 35 discusses scenarios where the content type is wrong, and its reasoning remains true to this day -- we can only process SVGs images if the given content type is correct. If we need to content sniff to determine it, imgLoader::GetMimeTypeFromContent is used:

http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/image/imgLoader.cpp#2560

And it only supports raster image types. Thus there shouldn't be a new class of marked-as-not-SVG-but-really-SVG bugs to be revealed by the nsIChannel::GetContentType working differently from nsIHttpChannel::GetContentType.
(Assignee)

Updated

11 months ago
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Priority: -- → P3
Whiteboard: [gfx-noted]
(Assignee)

Comment 1

11 months ago
Created attachment 8888481 [details] [diff] [review]
Retarget non-HTTP image URIs (e.g. chrome, data, blob) to the image IO thread if not an SVG., v1

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=40ea77ab838d17f449e5422119ddf52e7072fd76
(Assignee)

Comment 2

11 months ago
(In reply to Andrew Osmond [:aosmond] from comment #1)
> Created attachment 8888481 [details] [diff] [review]
> Retarget non-HTTP image URIs (e.g. chrome, data, blob) to the image IO
> thread if not an SVG., v1
> 
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=40ea77ab838d17f449e5422119ddf52e7072fd76

It appears browser/base/content/test/performance/browser_startup_images.js fails because it doesn't wait for the images to start decoding -- running chrome image decoding on the main thread appears to have caused a happy accident (although there are some suspicious intermittent-but-ignored flags for some images that are probably the same issue).

Changing the patch to ignore SVGs and/or chrome URIs so that only blob and data get handled off main thread passes the test.
(Assignee)

Comment 3

11 months ago
Created attachment 8888758 [details] [diff] [review]
Retarget non-HTTP image URIs (e.g. chrome, data, blob) to the image IO thread if not an SVG., v2

Rather than skip putting chrome images on the image IO thread, let's just make sure the image-loading notifications are dispatched if not on the main thread, and that we only do this for chrome images (because that's all the browser chrome tests should care about). This seems to fix the issue locally for me.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf6d85ef765a29e32f5b5daeabf1aa38896daf5c
Attachment #8888481 - Attachment is obsolete: true
(Assignee)

Comment 4

11 months ago
Created attachment 8888759 [details] [diff] [review]
Retarget non-HTTP image URIs (e.g. chrome, data, blob) to the image IO thread if not an SVG., v3

No functional change, just move more of the ugliness into NotifyChromeImageLoading.
Attachment #8888758 - Attachment is obsolete: true
(Assignee)

Comment 5

11 months ago
mozilla-central just happened to be broken when I pulled / updated...

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd245de046e7ffd9badb905996972f4b239d2e10
status-firefox57: --- → wontfix
(Assignee)

Comment 6

9 months ago
This won't work for data URIs as it will violate the spec. We need to continue processing them on the main thread so it remains synchronous. See bug 1325080.
Summary: Use IO thread to do copying for chrome, blob and data URIs → Use IO thread to do copying for chrome and blob URIs
(Assignee)

Comment 7

9 months ago
Created attachment 8908586 [details] [diff] [review]
Retarget non-HTTP image URIs (chrome, blob) to the image IO thread if not an SVG., v4

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=acc31483971bf9bd5e27347b494b22768783b63e
Attachment #8888759 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Attachment #8908586 - Flags: review?(tnikkel)
Comment on attachment 8908586 [details] [diff] [review]
Retarget non-HTTP image URIs (chrome, blob) to the image IO thread if not an SVG., v4

RasterImage::NotifyDrawingObservers should have chrome in the name if it's only for chrome images.

Are these image-loading observers only needed for chrome images though? I think they are intended to track any images loaded during startup that aren't drawn. That could include at least resource://, maybe more. Talk to Johann who implemented that in bug 1363059?

Or can you just make the test handle these events coming async?

r+ for the main change, just need to figure out how to make the test pass.
Attachment #8908586 - Flags: review?(tnikkel)
(Assignee)

Comment 9

9 months ago
(In reply to Timothy Nikkel (:tnikkel) from comment #8)
> Comment on attachment 8908586 [details] [diff] [review]
> Retarget non-HTTP image URIs (chrome, blob) to the image IO thread if not an
> SVG., v4
> 
> RasterImage::NotifyDrawingObservers should have chrome in the name if it's
> only for chrome images.
> 
> Are these image-loading observers only needed for chrome images though? I
> think they are intended to track any images loaded during startup that
> aren't drawn. That could include at least resource://, maybe more. Talk to
> Johann who implemented that in bug 1363059?
> 
> Or can you just make the test handle these events coming async?
> 
> r+ for the main change, just need to figure out how to make the test pass.

Hm you are right! I looked at those test cases (a while ago) and thought they were just chrome but somehow I missed resource. I wonder if it maps to something else. Ugh. Maybe it is best to notify for all URIs like it does today.

On the other hand, why didn't it fail on Linux on my try run? Only Windows and Mac OS X are "whitelisted" for intermittent failures inside the test itself but I ran *all* of the tests on Linux.
(Assignee)

Comment 10

9 months ago
Created attachment 8909826 [details] [diff] [review]
Retarget non-HTTP image URIs (chrome, blob) to the image IO thread if not an SVG., v5

I discussed this with johannh on IRC. We should only need to supply resource and chrome URIs. The reason the Linux test passes is because we don't check the resource IDs on Linux, and we accepted an intermittent failure on Windows. I've updated the patch to post notifications for both resource and chrome instead of chrome only.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ec6967ba273fa317ebbaa23abf49e2b79894e0b
Attachment #8908586 - Attachment is obsolete: true
(Assignee)

Comment 11

9 months ago
Try looks good, failures do not appear to be related, I will land this once 58 begins.

Comment 12

9 months ago
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db42206dd449
Retarget non-HTTP image URIs (chrome, blob) to the image IO thread if not an SVG. r=tnikkel

Comment 13

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/db42206dd449
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58

Updated

7 months ago
Depends on: 1421176
Depends on: 1424183
You need to log in before you can comment on or make changes to this bug.