1119774 - Add basic framework for downscale-during-decode

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

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.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add a pref and Image init flag for downscale-during-decode

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.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add RequestDecodeForSize to imgIContainer

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

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Make it possible to propagate a target size to the decoder

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.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add downscale-during-decode support to RasterImage::LookupFrame

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!

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add a pref and Image init flag for downscale-during-decode

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.

Comment 6

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add downscale-during-decode support to RasterImage::LookupFrame

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!

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 5) - Make it possible to pass a target size to Decoder::AllocateFrame

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 8

[Timothy Nikkel (:tnikkel)]

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 9

[Timothy Nikkel (:tnikkel)]

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?

Comment 10

[Seth Fowler [:seth] [:s2h]]

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

Comment 11

[Seth Fowler [:seth] [:s2h]]

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

Comment 12

[Seth Fowler [:seth] [:s2h]]

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

Comment 13

[Carsten Book [:Tomcat]]

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

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[[:fabrice] Fabrice Desré]

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

Comment 16

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add a pref and Image init flag for downscale-during-decode

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.

Comment 17

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add RequestDecodeForSize to imgIContainer

Comment 18

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Make it possible to propagate a target size to the decoder

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.

Comment 19

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add an optional alternate flags parameter to SurfaceCache::Lookup

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.

Comment 20

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 5) - Add SurfaceCache::LookupBestMatch

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.

Comment 21

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 6) - Add downscale-during-decode support to RasterImage::LookupFrame

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.

Comment 22

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 7) - Make it possible to pass a target size to Decoder::AllocateFrame

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

Comment 23

[Daniel Holbert [:dholbert]]

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 24

[Daniel Holbert [:dholbert]]

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 25

[Daniel Holbert [:dholbert]]

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)

Comment 26

[Seth Fowler [:seth] [:s2h]]

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

Comment 27

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add an optional alternate flags parameter to SurfaceCache::Lookup

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. =)

Comment 28

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 5) - Add SurfaceCache::LookupBestMatch

Addressed review comments.

Comment 29

[Seth Fowler [:seth] [:s2h]]

Here's a new try job:

https://tbpl.mozilla.org/?tree=Try&rev=c3eee30acc9f

Comment 30

[Seth Fowler [:seth] [:s2h]]

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

Comment 31

[Seth Fowler [:seth] [:s2h]]

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

Comment 32

[Carsten Book [:Tomcat]]

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

Comment 33

[Daniel Holbert [:dholbert]]

(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

Comment 34

[Seth Fowler [:seth] [:s2h]]

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. =)

Comment 35

[Daniel Holbert [:dholbert]]

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

[Seth Fowler [:seth] [:s2h]]

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.

Comment 37

[Seth Fowler [:seth] [:s2h]]

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 38

[Lawrence Mandel [:lmandel] (use needinfo)]

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 39

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 40

[Daniel Holbert [:dholbert]]

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.

Comment 41

[Xidorn Quan [:xidorn] UTC+11]

OK, filed bug 1228704.