To enable downscale-during-decode, we need to update ImageFactory, Decoder, and RasterImage to support the feature. In this bug we'll do that, adding basic support like prefs and new interfaces that the actual downscale-during-decode patches will build on.

Here's the most fundamental stuff: a pref to control whether downscale-during-decode is enabled, and ImageFactory code to determine whether we should enable it for a given Image. As we start landing downscale-during-decode patches for various image formats, ImageFactory::ShouldDownscaleDuringDecode will get updated to include their content types.

This patch adds imgIContainer::RequestDecodeForSize, which behaves like RequestDecode except that the caller can specify a size and decode flags. It will also differ in another way from RequestDecode once bug 1118655 lands: unlike RequestDecode and StartDecoding, RequestDecodeForSize will be respected even for decode-on-draw images. It enables callers to use heuristics to predict the size that we'll need to display an image at, without waiting for the image to be painted. This should arrive with downscale-during-decode so we can put together heuristics to solve any performance problems that may appear with the switch to decode-on-draw for some images. (Although in a worst case scenario, we can also just flip the pref off.) Note that I'm going to update bug 1118655 to use this method (and take advantage of the other things added in this bug).

This patch adds Decoder::SetTargetSize, which allows the caller to request a downscale during decode. For now it's a virtual method so individual decoders can opt in to supporting it; when all the decoders have support, it can stop being virtual. We also need to get that size information to RasterImage::CreateDecoder, which means several other methods in RasterImage need to change their signatures.

Finally, this patch adds downscale-during-decode support to RasterImage::LookupFrame. One key point here is that for downscale-during-decode images on platforms where we won't do decode-on-draw (in other words, platforms without APZ support), downscale-during-decode is going to end up working more like HQ scaling. We'll almost certainly decode the image to its intrinsic size thanks to a call to RequestDecode to StartDecoding. Then, once we actually draw, we'll find out the ideal size and redecode to that size. That's why we try to look up a frame with the image's intrinsic size as a fallback - that's the frame that RequestDecode or StartDecoding would have produced. It's also why we disable HQ scaling for downscale-during-decode images, since downscale-during-decode produces the exact same results that HQ scaling would have. One nice thing is that we can actually throw away the original frame and keep only the HQ scaled frame for downscale-during-decode images!

Minor tweak for part 1: don't bother checking if the content type supports downscale-during-decode if we've already decided not to use it.

I found a couple of bugs in this when testing later patches. - We need to pass aSize and not mSize to LookupFrame in Draw now. - We should not wait for mHasSourceData before doing a downscale-during-decode. It's streaming, after all!

We turn out to need one more part here: imgFrame objects know the size of the image they're associated with, and this needs to be the *target size* that we're decoding to. (This allows the imgFrame objects to take care of padding automatically when imgFrame::Draw gets called.) This patch adds an additional parameter to Decoder::AllocateFrame, allowing callers to pass in the target size they're decoding to.

Comment on attachment 8546590 [details] [diff] [review] (Part 2) - Add RequestDecodeForSize to imgIContainer >+RasterImage::RequestDecodeForSize(const nsIntSize& aSize, uint32_t aFlags) >+ // Sync decode small images if requested. >+ bool shouldSyncDecodeSmallImages = aFlags & FLAG_SYNC_DECODE; This feels like abusing the FLAG_SYNC_DECODE flag. When we pass FLAG_SYNC_DECODE to Draw it means sync decode the whole image, but here we take it to mean sync decode only small images? I think we should use a different flag for this (or use the function name like before).

Comment on attachment 8546592 [details] [diff] [review] (Part 3) - Make it possible to propagate a target size to the decoder >@@ -514,21 +513,28 @@ RasterImage::LookupFrame(uint32_t aFrame >+ if (!ref && !mHasSize) { >+ // We can't request a decode without knowing our intrinsic size. Give up. >+ return DrawableFrameRef(); >+ } Shouldn't we make sure a size decode is queued and also make sure a full decode for the the requested size is requested when that finishes?

(In reply to Timothy Nikkel (:tn) from comment #8) > This feels like abusing the FLAG_SYNC_DECODE flag. When we pass > FLAG_SYNC_DECODE to Draw it means sync decode the whole image, but here we > take it to mean sync decode only small images? I think we should use a > different flag for this (or use the function name like before). I complete agree. The ideal solution would be a different flag; it would simplify things in a lot of places. It's on my to-do list.

(In reply to Timothy Nikkel (:tn) from comment #9) > Shouldn't we make sure a size decode is queued and also make sure a full > decode for the the requested size is requested when that finishes? A size decode is queued in RasterImage::Init, so that's always taken care of. I'm on the fence about whether we should request a full decode or not here in the decode-on-draw case (the other case is covered elsewhere), because it may be that the size that's being used to draw here is different than the size that'd be used to drawn once our intrinsic size is available - it might cause reflow, for example. I'll think on it more.

I went ahead and pushed this: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c65de66fe0aa remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0257e0be8c42 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9e9c62f86ca3 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/02d04afc91b5 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/560042809ee7 Since it seems like this try job 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

also this one caused https://treeherder.mozilla.org/logviewer.html#?job_id=5346550&repo=mozilla-inbound

https://hg.mozilla.org/projects/cypress/rev/e56e7cffe0f4 https://hg.mozilla.org/projects/cypress/rev/2aeba12466eb https://hg.mozilla.org/projects/cypress/rev/4a7177514754 https://hg.mozilla.org/projects/cypress/rev/ce93d29e6e92

Argh, some seemingly minor tweaks in patches below this caused some fairly major rebasing. Just to be sure I've kept Bugzilla in sync, I'm reuploading all the patches.

This patch is not only getting rebased, but also a bug fix: we need to correctly distinguish between the intrinsic size and the target size in Decoder::PostInvalidation. I've fixed this by adding an optional second argument which gives the invalidation in terms of the downscaled image's coordinate space. Eventually this will become mandatory once all of the decoders are updated for downscale-during-decode.

OK, I'm going to insert a couple of new parts in here. Here's the new part 4. This part just adds an optional alternate flags parameter to SurfaceCache::Lookup, to support the optimization in RasterImage where we sometimes will accepted a different set of decode flags (premultiplied alpha vs. not) if the image is opaque. This is more efficient than just calling SurfaceCache::Lookup twice, as the comment says, because we only have to take the lock once. We could make things a bit more efficient still by propagating the alternate flags parameter deeper into the SurfaceCache code, but it's a very small gain and not relevant to what I'm trying to do in this bug. The reason I'm doing this here is to make things consistent with SurfaceCache::LookupBestMatch, which is getting added in the next part. In the case of LookupBestMatch the efficiency gains are more significant, so it's important to support this there, and letting SurfaceCache::Lookup support the same arguments as SurfaceCache::LookupBestMatch allows much simpler code in the callers.

This patch adds the function SurfaceCache::LookupBestMatch. This function tries to retrieve a surface that will be usable by the caller, even if it's not the exact surface they requested. The returned surface may have a different size, and the caller may optionally specify a different set of decode flags that would also be acceptable. To evaluate the best surface to return, we perform a fold over the surfaces held by an image, and conclude that a new surface is better than our previous best surface if: - It matches all the non-optional criteria (the animation time must match exactly, for example). - It's larger than the old best surface, if the old best surface was smaller than our ideal surface. - It's smaller than the old best surface, but at least as large as the ideal surface, if the old best surface was larger than our ideal surface. To evaluate "smaller" and "larger" we use area, which is not the best possible measurement here, since in theory we could scale images to extreme aspect ratios. For example, if we scaled a 10x10 image to a 1x10 surface and a 10x1 surface, then tried to find the best match for a 5x1 surface, we should clearly prefer returning the 10x1 surface. However, calculating this accurately makes the matching process significantly more expensive (since we need to start dealing with vector norms) and I expect that it is extremely unlikely that we will encounter this situation in practice. (And if we did, we'd redecode at the exact size we need and redraw anyway in a very short amount of time.) My take is: there's no need to do better than area unless we actually encounter a situation where it's causing a problem. We also short circuit the matching process for the case where we *do* have a matching surface (which will be the common case) by just trying a normal hash lookup for the ideal surface before doing anything else. There is also an outer loop which retries if the OS has thrown away the best surface we found so far. This is guaranteed to terminate since we are removing one of the surfaces every time, and no more can be added because we hold the lock. Although this is O(N^2), both the fact that virtually all images will have only 1 or 2 surfaces and the fact that the OS throwing out a surface's memory is pretty rare (and impossible for visible images since we lock them), I don't think there's anything to be concerned about here in practice.

This is the old part 4, but renumbered to part 6. It's also been rebased, and it now uses SurfaceCache::LookupBestMatch and the new alternate flags argument to SurfaceCache::Lookup, so the implementation is simpler than the old part 4, where everything was done directly in RasterImage::LookupFrame.

This is the old part 5, just rebased and renumbered.

Comment on attachment 8550061 [details] [diff] [review] (Part 4) - Add an optional alternate flags parameter to SurfaceCache::Lookup >diff --git a/image/src/SurfaceCache.h b/image/src/SurfaceCache.h >--- a/image/src/SurfaceCache.h >+++ b/image/src/SurfaceCache.h >+ SurfaceKey WithFlags(uint32_t aFlags) const >+ { >+ return SurfaceKey(mSize, mSVGContext, mAnimationTime, aFlags); >+ } >+ I think this method wants to be named something like "WithDifferentFlags". The current "WithFlags" name gave me the initial impression that this is supposed to *merge* aFlags into our existing mFlags, when creating the new SurfaceKey, and I was surprised to see that the code does not do that. I now understand *why* it doesn't merge them in, after reading your new Lookup() documentation. But without that context, I think readers are likely to expect different behavior (as I did), based on the current function-name. Alternately, if you really don't want to change the name: a brief line of documentation for this method would clear this up, too. So, r=me with this addressed via a rename or documentation.

Comment on attachment 8550066 [details] [diff] [review] (Part 5) - Add SurfaceCache::LookupBestMatch Review of attachment 8550066 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/src/SurfaceCache.cpp @@ +283,5 @@ > + { } > + > + const SurfaceKey& mIdealKey; > + nsRefPtr<CachedSurface> mBestMatch; > + Maybe<uint32_t> mAlternateFlags; I think this last member-variable should have type "const Maybe<uint32_t>&". (No need to create our own copy of the passed-in Maybe<> struct -- and I don't think you're modifying it, so we should be ok making it const.) Also, extreme-nit: mBestMatch should perhaps be listed last here, instead of between the other two vars, since it's the output & the other two member-vars are inputs. (Unless there's a packing-efficiency reason for it to be where you've got it -- but that doesn't seem likely.) @@ +554,5 @@ > + DrawableFrameRef ref; > + do { > + surface = cache->LookupBestMatch(aSurfaceKey, aAlternateFlags); > + if (!surface) > + return DrawableFrameRef(); // Lookup in the per-image cache missed. optional nit: this probably should have braces. (I think we tend to allow unbraced single-line clauses for simple early-returns at the beginning of a function, but this is inside a second level of bracing (the "do {}" statement), which takes it out of the "trivial early-return" category IMHO) @@ +562,5 @@ > + // The surface was released by the operating system. Remove the cache > + // entry as well. > + Remove(surface); > + } > + } while (!ref); optional nit: The "while (!ref)" here seems a bit redundant/inefficient, since we *just* null-checked ref when we get to that line. Seems a bit more efficient to make this a "while (true)" loop, and add an "else { break; }" clause after your "if (!ref) " block. Your formulation might be a bit more readable, though (and the inefficiency is pretty small), so it's also OK as-is if you prefer it. ::: image/src/SurfaceCache.h @@ +193,5 @@ > const Maybe<uint32_t>& aAlternateFlags > = Nothing()); > > /** > + * Look up the best matching surface in the cache and returns a drawable Grammar nit: s/Look/Looks/ (to match "and returns..." later in the sentence, and to match the other documentation nearby -- "Computes...", "Evicts...", "Removes...", etc.)

Comment on attachment 8550066 [details] [diff] [review] (Part 5) - Add SurfaceCache::LookupBestMatch Review of attachment 8550066 [details] [diff] [review]: ----------------------------------------------------------------- One more note: ::: image/src/SurfaceCache.cpp @@ +548,5 @@ > + return DrawableFrameRef(); // No cached surfaces for this image. > + > + // Repeatedly look up the best match, trying again if the resulting surface > + // has been freed by the operating system, until we can either lock a > + // surface for drawing or there are no matching surfaces left. As you mentioned in IRC, it might be worth noting here that this is theoretically O(n^2), but n (the number of cached surfaces for a given image) is expected to be small, so it's unlikely to be slow in practice. (but could be redesigned to remove surfaces on-the-fly if necessary)

Thanks for the review! I'll make those changes (with the small tweak noted on IRC).

Addressed review comments. (I went with 'WithNewFlags', since it's shorter - what can I say, I'm a sucker for brevity, in function names at least. =)

Addressed review comments.

Here's a new try job: https://tbpl.mozilla.org/?tree=Try&rev=c3eee30acc9f

Looks OK. Pushed: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d77230596cb5 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3ce28cb70ece remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/08965492c770 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/dce1e228d32f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e525b3c81bc4 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/435df926eb10 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5bf670f092eb

Re: in-testsuite-, any test where we ever display images downscaled is a pretty good test for downscale-during-decode. =) (It is landing pref'ed off at first, though. I expect to pref it on later this week on Nightly platforms with APZ enabled.)

https://hg.mozilla.org/mozilla-central/rev/d77230596cb5 https://hg.mozilla.org/mozilla-central/rev/3ce28cb70ece https://hg.mozilla.org/mozilla-central/rev/08965492c770 https://hg.mozilla.org/mozilla-central/rev/dce1e228d32f https://hg.mozilla.org/mozilla-central/rev/e525b3c81bc4 https://hg.mozilla.org/mozilla-central/rev/435df926eb10 https://hg.mozilla.org/mozilla-central/rev/5bf670f092eb

(In reply to Seth Fowler [:seth] from comment #31) > Re: in-testsuite-, any test where we ever display images downscaled is a > pretty good test for downscale-during-decode. =) Totally. "in-testsuite-" doesn't mean "this code is un-tested" -- rather, it asserts "this bug doesn't need its own specific test", which is appropriate for architectural work like this. ("in-testsuite:+" means "this has a test to make sure it stays fixed". That wouldn't make sense here -- if we backed this out or disabled it, nothing would start failing to indicate that it was no longer fixed, AFAIK.) This is semi-documented at https://wiki.mozilla.org/Bugzilla:MozillaFlagsAndSuch

Thanks for clarifying the meaning of that flag, Daniel! I have misunderstood it in the past as meaning "this bug does not have a test, but should". Will read through that document to make sure there aren't any other flags I've misunderstood. =)

(In reply to Seth Fowler [:seth] from comment #34) > Thanks for clarifying the meaning of that flag, Daniel! No problem! > I have misunderstood > it in the past as meaning "this bug does not have a test, but should". (FWIW, that is basically what "in-testsuite:?" implies.)

Comment on attachment 8550052 [details] [diff] [review] (Part 1) - Add a pref and Image init flag for downscale-during-decode 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.

Pushed to Aurora: remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/3bec811ec5b3 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/ef05e787c579 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/debf99339d3c remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/5a8f7b74bf55 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/fb6a943ca2a5 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/d6163bef24fa remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/86dd3d6b0ecc

Comment on attachment 8550052 [details] [diff] [review] (Part 1) - Add a pref and Image init flag for downscale-during-decode Aurora approval previously granted for a collection of ImageLib related bugs that Seth worked on and QE verified before uplift.

Comment on attachment 8550620 [details] [diff] [review] (Part 5) - Add SurfaceCache::LookupBestMatch Review of attachment 8550620 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/src/SurfaceCache.cpp @@ +322,5 @@ > + // for realistic sizes. > + int64_t idealArea = idealKey.Size().width * idealKey.Size().height; > + int64_t surfaceArea = aSurfaceKey.Size().width * aSurfaceKey.Size().height; > + int64_t bestMatchArea = > + bestMatchKey.Size().width * bestMatchKey.Size().height; Coverity Scan is reporting a defect here (actually three defects) that the value potentially overflows before the type gets widen. The "width" and "height" here are int32_t, so the result of them is temporary stored in 32bit form before being assigned to an int64_t variable. This is probably worth a fix, by either making the variables int32_t instead if we don't think it would ever overflow, or casting the first factor to int64_t before applying the multiplication.

Probably worth filing a new bug for comment 39 (and linking to that new bug from here). We shouldn't add any new patches here, 10 months after the original patches landed.

OK, filed bug 1228704.