Closed Bug 1118655 Opened 7 years ago Closed 7 years ago

Use decode-on-draw only, and ignore RequestDecode and the like, when APZ and downscale-during-decode are enabled

Categories

(Core :: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

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.)
Depends on: 1119774
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
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
https://hg.mozilla.org/mozilla-central/rev/8b1ba2b6a35f
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?
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+
Depends on: 1148832
You need to log in before you can comment on or make changes to this bug.