Closed Bug 1163878 Opened 4 years ago Closed 4 years ago

Stop creating ImageContainers for images we aren't going to layerize

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Creating an ImageContainer for an image is potentially hugely expensive in a downscale-during-decode world, because it currently requires us to decode the image at native size.

On the B2G Gallery app, this is currently the most serious issue holding back downscale-during-decode. Each image ends up with a downscaled surface at a size like 360x480 and a much larger surface at the intrinsic size of, say, 1944x2592. (These are actual numbers from about:memory.) The only reason that intrinsic size surface is created is because we call GetImageContainer() via FrameLayerBuilder::CanOptimizeImageLayer(). What's really unfortunate is that we generally do not actually layerize the image - we do all that additional decoding work and use all that memory for nothing.

In this bug, I'll change how FrameLayerBuilder and the various image-related display items interact with ImageLib to ensure that we don't create ImageContainers for images that we aren't going to layerize.
Part 1 adds imgIContainer::IsImageContainerAvailable(). Right now it performs
two checks that we previously performed by calling
imgIContainer::GetImageContainer(). One check verifies that we could possibly
produce an ImageContainer for this image - for VectorImage, for example, the
answer is always "no". The other check verifies that the image is not larger
than the LayerManager's maximum ImageContainer size.

We don't actually check that the image is currently decoded right now, and I
don't think there's a real reason to, but just in case we want to start doing
that in the future I went ahead and made IsImageContainerAvailable() take a
flags parameter.
Attachment #8604905 - Flags: review?(tnikkel)
Comment on attachment 8604905 [details] [diff] [review]
(Part 1) - Add an IsImageContainerAvailable method to imgIContainer

Need an IID bump on imgIContainer I believe.
Attachment #8604905 - Flags: review?(tnikkel) → review+
Part 2 does the real work. In this part, all the code paths that could call
imgIContainer::GetImageContainer() when we weren't certain we wanted to layerize
the image are changed to call imgIContainer::IsImageContainerAvailable()
instead.

The important code paths that had to be changed are:

- PaintedLayerData::CanOptimizeImageLayer(). This got renamed to
  CanOptimizeToImageLayer() for consistency, because it now calls a new method,
  nsDisplayImageContainer::CanOptimizeToImageLayer(). All display items that
  implement nsDisplayImageContainer have been updated to support this method,
  which is implemented by performing whatever checks they formerly performed in
  nsDisplayImageContainer::GetContainer(), but substituting
  imgIContainer::IsImageContainerAvailable() for
  imgIContainer::GetImageContainer().

- nsDisplayItem::GetLayerState(). This simply got updated to call
  imgIContainer::IsImageContainerAvailable() where it formerly called
  imgIContainer::GetImageContainer().

Before the changes in this part, imgIContainer::GetImageContainer() was called
for every image on my test site. After this change, the only image for which
imgIContainer::GetImageContainer() was called was the loading indicator.
Attachment #8604907 - Flags: review?(tnikkel)
(In reply to Timothy Nikkel (:tn) from comment #2)
> Comment on attachment 8604905 [details] [diff] [review]
> (Part 1) - Add an IsImageContainerAvailable method to imgIContainer
> 
> Need an IID bump on imgIContainer I believe.

Thanks for the quick review! I'll upload a revised version in just a sec.
Added an IID bump for imgIContainer.
Attachment #8604905 - Attachment is obsolete: true
Comment on attachment 8604907 [details] [diff] [review]
(Part 2) - Use IsImageContainerAvailable() when making layerization decisions and only call GetImageContainer() if we layerize

>@@ -2809,44 +2825,52 @@ void ContainerState::FinishPaintedLayerD

If I'm reading this patch right we change

if (imagelayer or color layer) {
  if (imageContainer) {
    //image layer stuff
  } else {
    //do color layer stuff
  }
  // do common image or color layer stuff
} else {
 //do painted layer stuff
}

to

if (imagelayer or color layer) {
  if (canOptimizeImageLayer) {
    imageContainer = GetContainer();
    if (imageContainer) {
      //image layer stuff
    } else {
      //do painted layer stuff
    }
  } else {
    //do color layer stuff
  }
  // do common image or color layer stuff
} else {
 //do painted layer stuff
}

This seems wrong because we will do the "painted layer stuff" and the "common image or color layer stuff" if canOptimizeImageLayer and !imageContainer. Or am I missing something?
Attachment #8604907 - Flags: review?(tnikkel)
(In reply to Timothy Nikkel (:tn) from comment #7)
> This seems wrong because we will do the "painted layer stuff" and the
> "common image or color layer stuff" if canOptimizeImageLayer and
> !imageContainer. Or am I missing something?

You are not missing anything; I misread the control flow there. Whoops! I'll upload a fixed version in just a second.

It's a little scary that that didn't obviously cause any test failures. There *was* a test failure on that try job, though, where we crash in nsDisplayBackgroundImage due to code assuming that mImageContainer is non-null. Now we fill it in lazily in nsDisplayBackgroundImage::GetContainer(), so I reworked all the code that touches mImageContainer to either not depend on mImageContainer at all, or to call GetContainer(). Those fixes will be in the new version of the patch as well.
Attachment #8604907 - Attachment is obsolete: true
Comment on attachment 8604981 [details] [diff] [review]
(Part 2) - Use IsImageContainerAvailable() when making layerization decisions and only call GetImageContainer() if we layerize

Sweet, and the result is much more readable code in FrameLayerBuilder.
Attachment #8604981 - Flags: review?(tnikkel) → review+
Yeah, an improvement all around. =) Thanks for the quick review!

Try looks good too - all the failures are existing intermittents. So I think this one is ready to land.
Blocks: 1162535
Blocks: 1164372
Comment on attachment 8604981 [details] [diff] [review]
(Part 2) - Use IsImageContainerAvailable() when making layerization decisions and only call GetImageContainer() if we layerize

New clang build warning (using clang 3.7, probably 3.6 too), treated as an error:
{
FrameLayerBuilder.cpp:2378:12: error: implicit conversion of nullptr constant to 'bool' [-Werror,-Wnull-conversion]
}

This comes from this chunk of the patch here (note 'bool' vs. 'return nullptr'):

>+++ b/layout/base/FrameLayerBuilder.cpp
[...]
>+bool
>+PaintedLayerData::CanOptimizeToImageLayer(nsDisplayListBuilder* aBuilder)
>+{
>+  if (!mImage) {
>+    return nullptr;
>+  }
>+
>+  return mImage->CanOptimizeToImageLayer(mLayer->Manager(), aBuilder);
>+}

Seth, should we just be returning false here? (If so, could you push a followup to do that?)
Flags: needinfo?(seth)
(In reply to Daniel Holbert [:dholbert] from comment #14)
> Seth, should we just be returning false here? (If so, could you push a
> followup to do that?)

Yeah, we should. Classic copy/paste error. I'll push a followup within the next couple of hours.
Flags: needinfo?(seth)
https://hg.mozilla.org/mozilla-central/rev/66d87284ab5c
https://hg.mozilla.org/mozilla-central/rev/c0654f3b986d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Blocks: 1166134
No longer depends on: 1165686
You need to log in before you can comment on or make changes to this bug.