1128225 - Decide whether to sync decode content images by remembering whether we've previously drawn them successfully

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

Right now we have a problem that occurs in many different places in image-related display list code. I'll pick on nsImageFrame. Here's the code in question:

https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsImageFrame.cpp?from=nsImageFrame.cpp&case=true#1415

Basically what's happening here is that we will add the entire <img> frame's area to the invalidation region if we're supposed to be sync decoding images and the <img>'s imgIContainer isn't fully decoded. The idea is that if the imgIContainer isn't fully decoded, we haven't been able to draw the entire thing yet, but sync decoding will allow us to do so.

Unfortunately, there are a lot of ways this can go wrong. Most seriously:

- For all images, we now always report them as being decoded once they've been decoded at least once. This was a deliberate change because the concept of "being decoded" has become increasingly nebulous, but it means that we won't sync decode images that have been discarded and are getting redecoded.

- Decode-on-draw images are reported as decoded as soon as their load event fires, because some code depends on this or it won't even try to draw them. That unfortunately means that decode-on-draw images also never get sync decoded.

- Downscale-during-decode images, to make things worse, may be "decoded" in the sense that a decode version of the image at *some* size is available, but it may not be the one we want. Even worse, because we haven't done pixel snapping and don't have full knowledge about all applied transforms when we're asking this question, we don't know which size *is* the one we want, so adding a size parameter to IsDecoded() still wouldn't reliably work.

In this bug, this entire model is going to get replaced by a different one. imgIContainer::Draw will return a value from a new DrawResult enumeration, letting calling code know whether it encountered an error, drew a partial or wrongly sized version of the image, or successfully drew a complete version of the image at the correct resolution. Callers will then record this information, and we'll use it in the display list ComputeInvalidationRegion code to decide whether sync decoding could improve things.

This isn't conceptually that complex, but there are a lot of places in the code that have to be touched. To keep things manageable, we'll only deal with content images in this bug - that is, nsImageFrame, nsBulletFrame, and nsImageBoxFrame. We'll leave CSS backgrounds and the like for a followup.

Comment 1

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

Attachment: (Part 1) - Return a result enumeration from imgIContainer::Draw

In part 1 imgIContainer::Draw starts returning a new DrawResult enumeration,
defined in imgIContainer.idl.

Although in later parts we are mostly going to be concerned with whether the
result was DrawResult::SUCCESS or not, I added a bunch of different result types
to the enumeration because I thought it'd be easiest to add them in now, since I
have to touch all of the old code anyway. The different values are also useful
during debugging.

Most of this patch is just rote conversion of Draw() calls to the new signature.
The interesting stuff is in imgIContainer.idl and in RasterImage.cpp, where we
decide whether to return DrawResult::SUCCESS or not in
DrawWithPreDownscaleIfNeeded().

Comment 2

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

Attachment: (Part 2) - Propagate the imgIContainer::Draw result through the nsLayoutUtils::Draw*Image functions

This part propagates the DrawResult values from imgIContainer::Draw through the
nsLayoutUtils image drawing code.

Comment 3

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

Attachment: (Part 3) - Record the last draw result in nsDisplayImage and use it to decide whether to sync decode

This part implements the strategy described in comment 0 for nsDisplayImage. We
store the DrawResult that we got the last time we painted on nsImageFrame, and
remember it on nsDisplayImageGeometry. We then check this value in
nsDisplayImage::ComputeInvalidationRegion() when we're deciding whether to
invalidate for sync decoding.

One subtle point: we also need to treat it as a successful draw if the image
gets layerized. We can't do that in nsDisplayImage::GetContainer(), because just
calling that doesn't mean that the ImageContainer is *used*.
nsDisplayImage::ConfigureLayer() looks like it only gets called if we do use the
ImageContainer, so that's where I added the code to record a successful draw.

An additional note is that I'm actually not 100% sure we need
nsDisplayImageGeometry at all, but it seems to be the conservative choice. My
concern with just checking |mFrame->LastDrawResult()| is that there may be weird
edge cases where this bites us. For example, presumably pagination may lead us
to generate multiple nsDisplayImage's for the same nsImageFrame, which could
lead to bugs if we don't remember the last DrawResult for each display item
individually.

Comment 4

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

Attachment: (Part 4) - Record the last draw result in nsDisplayBullet and use it to decide whether to sync decode

Part 4 makes changes similar to the changes made in part 3, but for nsDisplayBullet and nsBulletFrame.

Comment 5

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

Attachment: (Part 5) - Record the last draw result in nsDisplayXULImage and use it to decide whether to sync decode

Part 5 makes similar changes to those in part 3, but for nsDisplayXULImage and nsImageBoxFrame.

Comment 6

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

Here's a try job. The only related failures are for the tests marked fuzzy in bug 1128229.

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

Comment 7

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8557564 [details] [diff] [review]
(Part 1) - Return a result enumeration from imgIContainer::Draw

>+ * INCOMPLETE: We successfully drew a frame that was either partially decoded or
>+ * had the wrong size. (Note that successfully drawing a partially decoded frame
>+ * may not actually draw any pixels!) Drawing again with FLAG_SYNC_DECODE would
>+ * improve the result.

Hmm, do we want to lump these two cases together? If we drew using the intrinsic sized decoded version with a high quality scaling filter we likely don't care too much if imagelib can give us another high quality scaled decoded version at a later time. The canvas drawImage seems like it would want to report success in this condition. Whereas if only half the image is actually decoded that seems like quite a different situation that might be treated differently.

Comment 8

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8557564 [details] [diff] [review]
(Part 1) - Return a result enumeration from imgIContainer::Draw

>@@ -1699,65 +1699,79 @@ RasterImage::DrawWithPreDownscaleIfNeede
>     }
>     if (frameRef && !frameRef->IsImageComplete()) {
>       frameRef.reset();  // We're still scaling, so we can't use this yet.
>     }
>   }
> 
>   gfxContextMatrixAutoSaveRestore saveMatrix(aContext);
>   ImageRegion region(aRegion);
>+  bool frameIsComplete = true;  // We already checked HQ scaled frames.
>   if (!frameRef) {
>+    // There's no HQ scaled frame available, so we'll have to use the frame
>+    // provided by the caller.
>     frameRef = Move(aFrameRef);
>+    frameIsComplete = frameRef->IsImageComplete();
>   }
> 
>   // By now we may have a frame with the requested size. If not, we need to
>   // adjust the drawing parameters accordingly.
>   IntSize finalSize = frameRef->GetImageSize();
>+  bool couldRedecodeForBetterFrame = false;
>   if (ThebesIntSize(finalSize) != aSize) {
>     gfx::Size scale(double(aSize.width) / finalSize.width,
>                     double(aSize.height) / finalSize.height);
>     aContext->Multiply(gfxMatrix::Scaling(scale.width, scale.height));
>     region.Scale(1.0 / scale.width, 1.0 / scale.height);
>+
>+    couldRedecodeForBetterFrame = mDownscaleDuringDecode &&
>+                                  CanDownscaleDuringDecode(aSize, aFlags);
>   }

Shouldn't this take into account if the is a high quality scale (after decode) running? Shouldn't it also consider if the format allows downscale during decode?

Comment 9

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

(In reply to Timothy Nikkel (:tn) from comment #7)
> Hmm, do we want to lump these two cases together? If we drew using the
> intrinsic sized decoded version with a high quality scaling filter we likely
> don't care too much if imagelib can give us another high quality scaled
> decoded version at a later time. The canvas drawImage seems like it would
> want to report success in this condition. Whereas if only half the image is
> actually decoded that seems like quite a different situation that might be
> treated differently.

I can see what you mean, but high-quality scaling and downscale-during-decode are mutually exclusive. We will never apply high-quality scaling to an image that we can downscale-during-decode. (See RasterImage::CanScale.) We would only ever return this for a downscale-during-decode image that was scaled (using non-high-quality scaling, i.e., Cairo or GPU scaling) to a different size than it was intended for. I think we do want to synchronously redecode in such a case, so that we end up with nicely scaled images.

That said, I have no objection to splitting this out into two separate flags.

Comment 10

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

(In reply to Timothy Nikkel (:tn) from comment #8)
> Shouldn't this take into account if the is a high quality scale (after
> decode) running? Shouldn't it also consider if the format allows downscale
> during decode?

Well, high-quality scaling and downscale-during-decode are mutually exclusive, so we don't need to worry about a running high-quality scaling job.

As far as the format allowing downscale-during-decode, ImageFactory already took care of that; it won't pass INIT_FLAG_DOWNSCALE_DURING_DECODE unless the format allows it. (And mDownscaleDuringDecode is set based upon INIT_FLAG_DOWNSCALE_DURING_DECODE.) We have assertions in the decoder code that makes sure we don't mess that up.

Comment 11

[Timothy Nikkel (:tnikkel)]

(In reply to Seth Fowler [:seth] from comment #9)
> I can see what you mean, but high-quality scaling and
> downscale-during-decode are mutually exclusive. We will never apply
> high-quality scaling to an image that we can downscale-during-decode. (See
> RasterImage::CanScale.) We would only ever return this for a
> downscale-during-decode image that was scaled (using non-high-quality
> scaling, i.e., Cairo or GPU scaling) to a different size than it was
> intended for. I think we do want to synchronously redecode in such a case,
> so that we end up with nicely scaled images.
> 
> That said, I have no objection to splitting this out into two separate flags.

But when we draw we need to use a filter to draw the image at a different size. If that filter is high enough quality the user might not care. But specifically I don't think canvas can do anything once it's drawn its image it can't come back later and drawn a better scaled version.

Comment 12

[Timothy Nikkel (:tnikkel)]

(In reply to Seth Fowler [:seth] from comment #10)
> (In reply to Timothy Nikkel (:tn) from comment #8)
> > Shouldn't this take into account if the is a high quality scale (after
> > decode) running? Shouldn't it also consider if the format allows downscale
> > during decode?
> 
> Well, high-quality scaling and downscale-during-decode are mutually
> exclusive, so we don't need to worry about a running high-quality scaling
> job.

But shouldn't we return INCOMPLETE if a high quality scaled frame is coming later?

Comment 13

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

(In reply to Timothy Nikkel (:tn) from comment #12)
> But shouldn't we return INCOMPLETE if a high quality scaled frame is coming
> later?

Conceptually it makes sense, I agree. The reason I'm avoiding it for now is that FLAG_SYNC_DECODE won't change anything on that front. If we call Draw with FLAG_SYNC_DECODE and the scaled frame is still not ready, we'll just have to return INCOMPLETE again, which could cause an invalidation loop.

That won't be the case forever, though - I intend in the Firefox 39 timeframe to support downscale-during-decode for all image formats, and get rid of the high-quality scaling concept totally. (We may still supporting downscaling from an existing decode of the image at its native resolution, but I'd reimplement it as an optimization to the downscale-during-decode stuff, so externally it'd behave like a normal decode.)

Comment 14

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

(In reply to Timothy Nikkel (:tn) from comment #11)
> But when we draw we need to use a filter to draw the image at a different
> size. If that filter is high enough quality the user might not care. But
> specifically I don't think canvas can do anything once it's drawn its image
> it can't come back later and drawn a better scaled version.

The problem is that with downscale-during-decode, we may be drawing from a decode that's at a *very* different size. Imagine if the original image is 100x100 and we want to draw it to a 100x100 dest rect, but the only decoded version we have is a downscaled version that's only 10x10! No filter will help in that case; it's going to be ugly.

In general I'm operating on the principle here that we want to sync decode unless we got to paint either:

- The fully-decoded image at its native size, in the non-downscale-during-decode case.
- The fully-decoded image at the destination size, in the downscale-during-decode case.

Right now I don't worry about the high-quality scaled version, because that's going away soon and it's disabled in reftests anyway. In Firefox 39 what I expect is that we'll use downscale-during-decode for all non-animated images. (Of course, we'll probably disable downscale-during-decode in reftests as well!)

Comment 15

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8557566 [details] [diff] [review]
(Part 3) - Record the last draw result in nsDisplayImage and use it to decide whether to sync decode

I think we need to store the last draw status in the geometry item. The last draw status needs to be associated with the layer that we drew in for two reasons. 1) we may draw to another (non-retained) layer tree. 2) if the image stops being visible (scrolls out of view, is covered, tab switch) we need to drop the last draw status, because if the image is then shown again we will have no drawn copy in the retained layers.

Comment 16

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8557567 [details] [diff] [review]
(Part 4) - Record the last draw result in nsDisplayBullet and use it to decide whether to sync decode

Same comment as part 3.

Comment 17

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8557568 [details] [diff] [review]
(Part 5) - Record the last draw result in nsDisplayXULImage and use it to decide whether to sync decode

Same comment as part 3.

Comment 18

[Timothy Nikkel (:tnikkel)]

(In reply to Seth Fowler [:seth] from comment #13)
> (In reply to Timothy Nikkel (:tn) from comment #12)
> > But shouldn't we return INCOMPLETE if a high quality scaled frame is coming
> > later?
> 
> Conceptually it makes sense, I agree. The reason I'm avoiding it for now is
> that FLAG_SYNC_DECODE won't change anything on that front. If we call Draw
> with FLAG_SYNC_DECODE and the scaled frame is still not ready, we'll just
> have to return INCOMPLETE again, which could cause an invalidation loop.

Ah, okay, that makes sense.

Comment 19

[Timothy Nikkel (:tnikkel)]

(In reply to Seth Fowler [:seth] from comment #14)
> (In reply to Timothy Nikkel (:tn) from comment #11)
> > But when we draw we need to use a filter to draw the image at a different
> > size. If that filter is high enough quality the user might not care. But
> > specifically I don't think canvas can do anything once it's drawn its image
> > it can't come back later and drawn a better scaled version.
> 
> The problem is that with downscale-during-decode, we may be drawing from a
> decode that's at a *very* different size. Imagine if the original image is
> 100x100 and we want to draw it to a 100x100 dest rect, but the only decoded
> version we have is a downscaled version that's only 10x10! No filter will
> help in that case; it's going to be ugly.
> 
> In general I'm operating on the principle here that we want to sync decode
> unless we got to paint either:
> 
> - The fully-decoded image at its native size, in the
> non-downscale-during-decode case.
> - The fully-decoded image at the destination size, in the
> downscale-during-decode case.

Okay, but for the canvas case there is nothing we can do after we returned from drawImage, whatever we drew is there, and we can't change it. So we either want to make it block on getting an acceptable result, or returning success if we drew a "not-exactly-the-right-size" version of the image.

Comment 20

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

Attachment: (Part 1) - Return a result enumeration from imgIContainer::Draw

Updated part one to split DrawResult::INCOMPLETE into DrawResult::INCOMPLETE and
DrawResult::WRONG_SIZE.

Comment 21

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8558837 [details] [diff] [review]
(Part 1) - Return a result enumeration from imgIContainer::Draw

I'm assuming you meant to ask for review.

Comment 22

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

Attachment: (Part 1) - Return a result enumeration from imgIContainer::Draw

I did! Thanks. =)

Fixed a typo in the new version of part 1.

Comment 23

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

Attachment: (Part 3) - Add infrastructure for tracking draw results in geometry items

OK, so I've reimplemented part 3, 4, and 5 (which are now parts 4, 5, and 6)
with a totally new approach based on our conversation last night.

- We now track everything on the geometry items. Frames don't need to be
  modified at all (other than they now need to return a DrawResult from their
  paint methods).

- Geometry items can add the functionality they need to track image drawing
  results using a mixin, nsImageGeometryMixin. The common case is that display
  items are using nsDisplayItemGenericGeometry, so I've added a new generic
  geometry item class that inherits from nsDisplayItemGenericGeometry and
  nsImageGeometryMixin called nsDisplayItemGenericImageGeometry.

- To support nsImageGeometryMixin's functionality, we need to be able to get the
  most recent geometry item for a display item. I've implemented this in
  FrameLayerBuilder::GetMostRecentGeometry(). It uses a similar approach to
  FrameLayerBuilder::IterateRetainedDataFor(), as we discussed.

Comment 24

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

Attachment: (Part 4) - Record the last draw result in nsDisplayImage and use it to decide whether to sync decode

The new part 4 replaces the old part 3, implementing the new draw-result-based
sync decoding approach in nsImageFrame. We're able to use
nsDisplayItemGenericImageGeometry, so the implementation is straightforward.

Comment 25

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

Attachment: (Part 5) - Record the last draw result in nsDisplayBullet and use it to decide whether to sync decode

Here's the updated version of part 4, now part 5. nsBulletFrame already had a
custom geometry item class, so this patch uses nsImageGeometryMixin instead.

Comment 26

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

Attachment: (Part 6) - Record the last draw result in nsDisplayXULImage and use it to decide whether to sync decode

Here's the updated version of part 5, now 6. Like with nsImageFrame, we're able
to use nsDisplayItemGenericImageGeometry here.

Comment 27

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8558993 [details] [diff] [review]
(Part 3) - Add infrastructure for tracking draw results in geometry items

After looking at the next part I think we need to initialize the last draw result in the geometry item as failure. And then update it as usual if there was a previous geometry item or we get a new draw result. That way if we fail to update the result on this (new) paint it will be recorded as a failure in the last draw result.

Comment 28

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

(In reply to Timothy Nikkel (:tn) from comment #27)
> Comment on attachment 8558993 [details] [diff] [review]
> (Part 3) - Add infrastructure for tracking draw results in geometry items
> 
> After looking at the next part I think we need to initialize the last draw
> result in the geometry item as failure. And then update it as usual if there
> was a previous geometry item or we get a new draw result. That way if we
> fail to update the result on this (new) paint it will be recorded as a
> failure in the last draw result.

Ah, good catch! It's supposed to be that way, but I messed up when I split the mixin out. I'll upload a fix.

Comment 29

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

Attachment: (Part 3) - Add infrastructure for tracking draw results in geometry items

OK, in this version we properly initialize nsImageGeometryMixin::mLastDrawResult
to DrawResult::NOT_READY.

Comment 30

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

Thanks for the reviews! The new approach feels much cleaner.

Comment 31

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

The try job here looks ready to go:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75e3724a84af

I want to verify that this lets me flip on decode-on-draw on B2G as well, though, because if not then there's still a bug somewhere. So I'm waiting on the retriggers for R20 on this try job:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9da3e016e9d1

Comment 32

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

OK, everything looks good. Pushed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dfef0d93fb45
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3648fe4dc795
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/628b3df398dd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/21c1ffba4c09
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/56c87c3a0589
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5231ced6c4ec

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dfef0d93fb45
https://hg.mozilla.org/mozilla-central/rev/3648fe4dc795
https://hg.mozilla.org/mozilla-central/rev/628b3df398dd
https://hg.mozilla.org/mozilla-central/rev/21c1ffba4c09
https://hg.mozilla.org/mozilla-central/rev/56c87c3a0589
https://hg.mozilla.org/mozilla-central/rev/5231ced6c4ec

Comment 34

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

Comment on attachment 8558848 [details] [diff] [review]
(Part 1) - Return a result enumeration from imgIContainer::Draw

Approval Request Comment
[Feature/regressing bug #]: Part of fix for bug 1125272 and bug 1119938.
[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 35

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

Comment on attachment 8558994 [details] [diff] [review]
(Part 4) - Record the last draw result in nsDisplayImage and use it to decide whether to sync decode

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Comment 36

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

Pushed to Aurora:

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/46fd30ea6548
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/c1ecd02b0129
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/09b37c406334
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/46473c5ec5b2
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/7ea739139e3c
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/d62ea4de2f1f

Comment 37

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

Comment on attachment 8558848 [details] [diff] [review]
(Part 1) - Return a result enumeration from imgIContainer::Draw

This is part of a collection of ImageLib fixes that have been tested by QE and that Seth had approval to land. Adding approval to the bug after the fact.