Closed Bug 1382783 Opened 3 years ago Closed 3 years ago
Use IO thread to do copying for chrome and blob URIs
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: nobody → aosmond
Status: NEW → ASSIGNED
Priority: -- → P3
(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.
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
No functional change, just move more of the ugliness into NotifyChromeImageLoading.
Attachment #8888758 - Attachment is obsolete: true
mozilla-central just happened to be broken when I pulled / updated... try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd245de046e7ffd9badb905996972f4b239d2e10
3 years 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
Attachment #8888759 - Attachment is obsolete: true
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.
(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.
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
Try looks good, failures do not appear to be related, I will land this once 58 begins.
Pushed by firstname.lastname@example.org: 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
You need to log in before you can comment on or make changes to this bug.