Closed Bug 1118655 Opened 7 years ago Closed 7 years ago
Use decode-on-draw only, and ignore Request
Decode and the like, when APZ and downscale-during-decode are enabled
For downscale-during-decode to do its job correctly, we need to wait until we know what size we're going to draw images at before decoding them. That's an inherent requirement. It's likely that there are many circumstances where we would be able to make a good guess at that size after we do an initial reflow. However, writing heuristics that work in all cases is going to be tricky, and heuristics that are wrong frequently will do more harm than good, as we will end up having to decode images twice - once at the wrong size, and then again at the right size. I think the best bet is instead to rely on decode-on-draw. When we're drawing we are guaranteed to get the correct size, and that size is calculated (obviously) using the same code we use any time we draw, so we don't have to duplicate the complicated calculations we do to determine at which size we should draw an image. There is a cost to relying on decode-on-draw, though. That cost comes in two forms. First, there's the fact that, since we don't start decoding until we try to paint, there will be a visible delay between the time that the rest of the page is drawn and the time that images become decoded and are filled in. This actually isn't noticeable (in my testing so far at least) on initial page load, since images and other content are loading off of the network anyway. However, it's quite noticeable when scrolling. The good news is that because APZ draws predictively - we paint a larger displayport (renderport?) instead of just the viewport - the delay from decode-on-draw is totally hidden. In my tests, I literally can't tell when it's on. And fortunately, downscale-during-decode (that's why we're doing this!) is most important on mobile platforms, which all have APZ enabled. So as long as we only turn on decode-on-draw where we have APZ, we should avoid any issues with flashing during scrolling. The second cost is that we reduce parallelism during page load, because image decoding is delayed until we can do our initial paint. That's not ideal, and APZ doesn't help there. But I think if we move to this model, we will be in a position to gradually eliminate this cost over time. We may be able to get a one-time improvement just by reducing the length of the paint suppression timer. And as we write heuristics that let us accurately predict the size we'll be drawing at in common cases, we'll gain that parallelism back. The nice thing about decode-on-draw being the default is that our heuristics don't have to work in all cases - if we aren't able to predict the size, we just wait until the initial paint to start decoding, and everything works fine. So in conclusion: on platforms where we use APZ, let's switch to relying on decode-on-draw to trigger image decoding. We'll then be able to gradually add heuristics that make decoding start earlier, starting from a solid foundation.
By the way, a lot of chrome code relies on images decoding themselves automatically, and I'm not sure we gain anything from downscale-during-decode for chrome code. So my plan is that "chrome:" URIs, "resource:" URIs, and multipart images (which also don't benefit from downscale-during-decode) will continue to not rely on decode-on-draw.
Here's the patch. (This is the final thing that blocks downscale-during-decode, woohoo!) In the long term what I'd like to do is never ignore RequestDecode or StartDecoding, but simply add a size parameter and remove callsites where we don't have good heuristics for the size. However, in the short term we need to support the old behavior for non-APZ platforms, so we don't want to remove callsites right now. So the existing code in ImageFactory::ComputeImageFlags for decode-on-draw almost perfectly matches the requirements of this bug. (It's disabled for "chrome:" URIs, "resource:" URIs, and multipart images.) Only one little modification is need: we now check that APZ is enabled, as well. In RasterImage, mDecodeOnDraw really doesn't trigger any special behavior anymore. We now give it the following new behavior: - When decode-on-draw images get their first data from the network, they fire the same notifications that decoders fire when they start decoding. - When decode-on-draw images get their last data from the network, they fire the same notifications that decoders fire when they finish decoding. - Decode-on-draw images always return true from IsDecoded() if they have all their source data. - Decode-on-draw images ignore calls to RequestDecode() and StartDecoding(). Those are the main changes, but there are also some minor related changes included in the patch: I removed an NS_ENSURE_SUCCESS call in nsImageFrame and a corresponding one in nsImageBoxFrame. They were useless anyway (since nsRefPtr::forget() does exactly what they did), and we hit that case a little more often with decode-on-draw. I also replaced calls to RequestDecode in nsAlertsIconListenr and nsMenuItemIcon with calls to GetFrame, which will implicitly request async decoding for the image. These two callsites are the only two places in the codebase that look like they need to be updated to support decode-on-draw, because they wait for their images to be decoded before trying to draw them and the images don't necessarily come from "chrome:" or "resource:" URIs. In the long term, once we don't have to support two behaviors for RequestDecode, we should switch these two callsites to use RequestDecode again.
Attachment #8545143 - Flags: review?(tnikkel)
Comment on attachment 8545143 [details] [diff] [review] Use decode-on-draw only, and ignore RequestDecode and the like, when APZ is enabled Calling the variables doDecodeOnDraw and mDecodeOnDraw is confusing I think. Because we can always decode on draw. The unique thing about this is that we try to avoid decoding on things other than draw. Decode on draw only? Ignore decode requests?
Attachment #8545143 - Flags: review?(tnikkel) → review+
Thanks for the review, Timothy! (In reply to Timothy Nikkel (:tn) from comment #3) > Calling the variables doDecodeOnDraw and mDecodeOnDraw is confusing I think. > Because we can always decode on draw. The unique thing about this is that we > try to avoid decoding on things other than draw. Decode on draw only? Ignore > decode requests? I agree. I'll try to think of a better name that is relatively short. (Decode on draw only would definitely work, maybe that's the best bet.)
This updated version of the patch is rebased on top of bug 1119774. The only changes of substance are: - We explicitly only enable decode-on-draw if downscale-on-decode is enabled. - Instead of using GetFrame to force decoding in the two places where that had to be done, we use the new imgIContainer::RequestDecodeForSize method added in bug 1119774.
Attachment #8545143 - Attachment is obsolete: true
Summary: Use decode-on-draw only, and ignore RequestDecode and the like, when APZ is enabled → Use decode-on-draw only, and ignore RequestDecode and the like, when APZ and downscale-during-decode are enabled
Pushed: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2eefd792306d Based on this try job, which is looking pretty green: https://tbpl.mozilla.org/?tree=Try&rev=f3f4d04d1fbe
sorry had to back this out in: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5c20be43873b because it seems backing out alone Bug 1120050 caused a bustage there and so this changeset had to go out as well. Sorry
This needed a rebase. I also fixed the commit message.
Attachment #8546596 - Attachment is obsolete: true
A very small tweak: we allow decode-on-draw without downscale-during-decode if ShouldDownscaleDuringDecode(aMimeType) returns true. This will enable us to evaluate the performance impact of decode-on-draw in isolation, which is something I want to do.
Attachment #8550070 - Attachment is obsolete: true
Just noticed that we also need to flip the default pref value for decode-on-draw on both B2G and Android.
Attachment #8550680 - Attachment is obsolete: true
New try job here: https://tbpl.mozilla.org/?tree=Try&rev=fab5a4b7b888
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment on attachment 8550846 [details] [diff] [review] Use decode-on-draw only, and ignore RequestDecode and the like, when APZ and downscale-during-decode are enabled Approval Request Comment [Feature/regressing bug #]: Pulling in downscale-during-decode refactoring to ensure that patch compatibility between 37 and 38+. [User impact if declined]: Already approved. [Describe test coverage new/current, TreeHerder]: In 38 for a long time. [Risks and why]: Low risk; in 38 for a long time. [String/UUID change made/needed]: None.
Attachment #8550846 - Flags: approval-mozilla-aurora?
Pushed to Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/d55d3ca3e685
Comment on attachment 8550846 [details] [diff] [review] Use decode-on-draw only, and ignore RequestDecode and the like, when APZ and downscale-during-decode are enabled Aurora approval previously granted for a collection of ImageLib related bugs that Seth worked on and QE verified before uplift.
Attachment #8550846 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.