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.

Comment on attachment 8604905 [details] [diff] [review] (Part 1) - Add an IsImageContainerAvailable method to imgIContainer Need an IID bump on imgIContainer I believe.

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.

(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.

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?

(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.

As promised, here's the updated patch.

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.

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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/66d87284ab5c https://hg.mozilla.org/integration/mozilla-inbound/rev/c0654f3b986d

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?)

(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.

https://hg.mozilla.org/mozilla-central/rev/66d87284ab5c https://hg.mozilla.org/mozilla-central/rev/c0654f3b986d