695763 - Synchronous image decoding doesn't work with retained layers

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

All of the logic to synchronously decode images is in the painting code, which retained layers can cause us to skip.

Comment 1

[Matt Woodrow (:mattwoodrow)]

With DLBI we now store information about all the items contained within a retained layer, and some data describing how that item was rendered (usually it's size/position).

When we do the next paint, we do a comparison between the current set of items, and the saved data from the previous paint. This lets us generate an invalidation region if the item has changed (e.g. the item moved or resized).

Each display item type may implement it's own set of data to store, and logic for comparing data and computing which pixels to redraw.

If we store the 'decode state' of the image on any display items that paint image, we can use this to check if the image needs to be redrawn.

The only hard problem that I can see is that we compute and store the data during layer building, but the decoding state might change during painting (I think?). We might need to retrieve the stored data during painting and modifying it in this case.

Relevant code is:

nsDisplayItem subclasses can override AllocateGeometry and ComputeInvalidationRegion (nsDisplayList.h/cpp) to implement their own invalidation logic, and can create a new subclass of nsDisplayItemGeometry (nsDisplayItemGeometry.h) if they need to store custom data.

These should all be called from within FrameLayerBuilder.cpp.

Comment 2

[Justin Lebar (not reading bugmail)]

This might be a really elegant way to fix the test failures in bug 845147, but I don't want to block on this approach unless you guys think you can get it done pretty quickly, since we may have other approaches and we really should fix bug 845147 before the next branch.

Comment 3

[Justin Lebar (not reading bugmail)]

> we really should fix bug 845147 before the next branch.

seth pinged me on IRC to ask which branch I was referring to here.  I mean the next time we branch m-c to Aurora, which is scheduled for April 1.

Comment 4

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

Thanks for getting back to me, Justin. Currently my plan is to take a stab at this before the end of the week and see how bad it looks. I'll update this bug when I've done that. If it looks like a task that can be completed quickly I'm willing to take it on. It sounds like you have a backup plan ready to go in case this doesn't work out, which is good.

Comment 5

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

Attachment: (Part X) - Always invalidate if we are sync decoding an undecoded image.

Rough patch. This needs some work before it's ready to be reviewed, but passes reftests locally. Justin, I'm wondering if this is enough already that you could try this out and see if it fixes your problem? Even better, do you have a test case (reftest/mochitest) that I could use to verify when things are working correctly?

Comment 6

[Justin Lebar (not reading bugmail)]

> Even better, do you have a test case (reftest/mochitest) that I could use to verify when 
> things are working correctly?

That's even better for me, too.  :)

I'm not sure exactly what this bug is trying to fix, but if reftests patch with this bug and also with attachment 719161 [details] [diff] [review], that would be beyond fantastic.  The reftest which was failing was mostly 816948-1.html, although there were some random assertions in 364066-1.html which also may or may not matter.

This link will probably go dead soon, but until it does: https://tbpl.mozilla.org/?tree=Try&rev=af9e2f293ae6

Comment 7

[Justin Lebar (not reading bugmail)]

s/also may/may/

Comment 8

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

Hmm, I'm not sure this patch is going to have much of an interaction with attachment 719161 [details] [diff] [review]. This is intended to enable you to take a snapshot of a page using CanvasRenderingContextD::DrawWindow. That method is supposed to synchronously decode images unless you explicitly request that it doesn't (by passing RENDER_ASYNC_DECODE_IMAGES), but the premise of this bug is that the support for this in the layers subsystem is incomplete at the moment. In other words, this should fix bug 846177.

I'll ask about a reftest over there.

Comment 9

[Justin Lebar (not reading bugmail)]

Well, my thought was that reftest uses DrawWindow, so fixing this would force all images to be decoded in the reftest results.  Am I missing something there?

Comment 10

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

No, I think I am. =) Got too focused on bug 846177. That does sound like it's worth a try.

Comment 11

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

The try reset was unfortunate, as I can't see what platforms that patches was giving you failures on. I can't reproduce any issues locally with or without my patch. Let's see if try does a better job of flushing out issues.

Here's the new patch by itself:

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

Comment 12

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

Here's your patch by itself:

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

Comment 13

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

Here's the composition of the two:

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

Comment 14

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

Argh. Apparently qimportbz corrupted my patch queue and my previous tests are invalid, because the imported patch (your patch) didn't contain what I thought it did. Retrying things now...

Comment 15

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

Now I can reproduce the problem locally. My patch currently does not fix the problem; I'm investigating why.

Comment 16

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

OK, Justin, I've got a working patch that makes DBLI work correctly with sync decoding. It does indeed fix your patch. (I'll post a revised version of that patch to the other bug, by the way; it can be made cleaner.)

Comment 17

[Justin Lebar (not reading bugmail)]

> I've got a working patch that makes DBLI work correctly with sync decoding. It does indeed fix your 
> patch.

!!

Comment 18

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

Attachment: (Part 1) - Add imgIContainer::IsDecoded.

We need a way to check if an imgIContainer has been decoded yet. This patch adds a method for that.

Comment 19

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

Attachment: (Part 2) - Make DLBI always invalidate if we are sync decoding an undecoded image.

Now whenever we call ComputeInvalidationRegion on an image-related display item, we just need to check if a sync decode has been requested, and if so, check if the image has not yet been decoded. If both of those things are true, we invalidate the display item's entire region. This causes the image to be decoded and drawn.

Comment 20

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

Try job here:

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

Comment 21

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 728822 [details] [diff] [review]
(Part 2) - Make DLBI always invalidate if we are sync decoding an undecoded image.

Review of attachment 728822 [details] [diff] [review]:
-----------------------------------------------------------------

Isn't this checking the decoded state as of the current paint, instead of what it was when we last painted?

If it wasn't decoded when we last painted, then we need to invalidate and redraw those pixels, regardless of whether decoding has finished since then or not.

Comment 22

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

(Note that that try job includes Justin's patch. I'll post another try job shortly without it.)

Comment 23

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #21)
> Isn't this checking the decoded state as of the current paint, instead of
> what it was when we last painted?

Yeah.

> If it wasn't decoded when we last painted, then we need to invalidate and
> redraw those pixels, regardless of whether decoding has finished since then
> or not.

To the best of my knowledge, that's not necessary. When decoding finishes the display item already gets invalidated through other mechanisms, so an image that has already been decoded should already be taken care of. The only case that isn't handled right now is when decoding isn't finished, but it would be if we forced the image to draw, because we're going to pass the sync decode flag. This patch just adds support for that case.

Comment 24

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

Here's another try job without Justin's patch:

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

Comment 25

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 728822 [details] [diff] [review]
(Part 2) - Make DLBI always invalidate if we are sync decoding an undecoded image.

Review of attachment 728822 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsDisplayList.cpp
@@ +2191,5 @@
> +
> +  if (aBuilder->ShouldSyncDecodeImages() && SyncDecodeRequiresInvalidation(aBuilder)) {
> +    // A sync decode was requested and this image hasn't been fully decoded
> +    // yet. We need to invalidate to allow the image to be decoded and drawn.
> +    aInvalidRegion->Or(positioningArea, geometry->mBounds);

This should be using 'bounds' rather than 'positioningArea'

::: layout/generic/nsImageFrame.cpp
@@ +1316,5 @@
> +  if (aBuilder->ShouldSyncDecodeImages() && mImage && !mImage->IsDecoded()) {
> +    // A sync decode was requested and this image hasn't been fully decoded
> +    // yet. We need to invalidate to allow the image to be decoded and drawn.
> +    nsRect rect;
> +    mFrame->GetClientRect(rect);

The client rect is relative to the parent frame, and we expect a rect/region relative to the display list reference frame. nsDisplayItem::GetBounds() is probably what you want here.

We also need to union this with the bounds from the previous paint (aGeometry->mBounds), just in case the image got moved as well.

Both of these apply to the nsImageBoxFrame implementation too. I guess you could share code between these with a helper if you wanted

Comment 26

[Matt Woodrow (:mattwoodrow)]

(In reply to Seth Fowler [:seth] from comment #23) 
> To the best of my knowledge, that's not necessary. When decoding finishes
> the display item already gets invalidated through other mechanisms, so an
> image that has already been decoded should already be taken care of. The
> only case that isn't handled right now is when decoding isn't finished, but
> it would be if we forced the image to draw, because we're going to pass the
> sync decode flag. This patch just adds support for that case.

You are completely correct, ignore me :)

Comment 27

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

Thanks for the review, Matt!

(In reply to Matt Woodrow (:mattwoodrow) from comment #25)
> This should be using 'bounds' rather than 'positioningArea'

Before the image has been decoded, bounds is an empty rect. (This is what GetBackgroundLayerRect gives you if the image isn't decoded.) That's why I used positioningArea. However, it occurs to me that I could alter GetBackgroundLayerRect to use the SYNC_DECODE flag, which I believe would make it successfully compute the bounds even if the image isn't decoded yet. (But wouldn't actually _cause_ a sync decode at the point where the layer is constructed.)

> The client rect is relative to the parent frame, and we expect a rect/region
> relative to the display list reference frame. nsDisplayItem::GetBounds() is
> probably what you want here.

This was also the reason why I avoided GetBounds here.

The more I think about it, the more I like the idea of just fixing GetBackgroundLayerRect. I'll post an updated patch tomorrow.

Comment 28

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

(In reply to Seth Fowler [:seth] from comment #27)
> This was also the reason why I avoided GetBounds here.

Well, GetBackgroundLayerRect specifically isn't the reason in this case, of course, but it was to avoid the same kind of problem.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 728821 [details] [diff] [review]
(Part 1) - Add imgIContainer::IsDecoded.

sr=me, but if you add nostdcall to the extended attributes in the WebIDL you can drop the NS_IMETHODIMP stuff too and just do:

 bool
 ImageWrapper::IsDecoded()

etc.  Might be worth it.

Comment 30

[Boris Zbarsky [:bzbarsky]]

> in the WebIDL 

I _meant_ the XPIDL, of course.  ;)

Comment 31

[Joe Drew (not getting mail)]

Comment on attachment 728821 [details] [diff] [review]
(Part 1) - Add imgIContainer::IsDecoded.

Review of attachment 728821 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/public/imgIContainer.idl
@@ +237,2 @@
>     */
>    void requestDecode();

change the UUID too, or you'll end up in endless pain with our IDL compiler not regenerating the typelibs.

Comment 32

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

(In reply to Joe Drew (:JOEDREW! \o/) (Vacation 26 March–11 April) from comment #31)
> change the UUID too, or you'll end up in endless pain with our IDL compiler
> not regenerating the typelibs.

Drat, somehow I always forget that. Thanks for the review.

Comment 33

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

(In reply to Boris Zbarsky (:bz) from comment #29)
> sr=me, but if you add nostdcall to the extended attributes in the WebIDL you
> can drop the NS_IMETHODIMP stuff too and just do:

Cool, didn't know about that. I'll give it a shot. Thanks for the review.

Comment 34

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

Heh wow, a few oranges in those try jobs. =) Investigating.

Comment 35

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

Attachment: (Part 1) - Add imgIContainer::IsDecoded.

Address review comments.

Comment 36

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

Attachment: (Part 2) - Make DLBI always invalidate if we are sync decoding an undecoded image.

Address review comments thus far. Instead of relying on |positioningArea| to decide how to invalidate background images, I've switched to making |GetBackgroundLayerRect| produce a non-empty rect even if the image has not yet been decoded. I checked and in the other cases it should be safe to use |GetBounds| without changes to the implementation, so I switched them over. (In summary: GetBounds is now used in every case.) Also addressed a crash in nsImageBoxFrame caused by assuming that we'd always have an image request.

There's a try job here (it includes Justin's patch, since it's a good way of exercising the code):

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

Comment 37

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 729136 [details] [diff] [review]
(Part 2) - Make DLBI always invalidate if we are sync decoding an undecoded image.

Review of attachment 729136 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSRendering.cpp
@@ +3024,5 @@
>                                         const nsStyleBackground::Layer& aLayer)
>  {
>    nsBackgroundLayerState state =
> +      PrepareBackgroundLayer(aPresContext, aForFrame,
> +                             nsCSSRendering::PAINTBG_SYNC_DECODE_IMAGES,

Won't this force a synchronous decode during this call in some cases?

I see a call to ExtractFrame in nsImageRenderer::PrepareImage which is sometimes called.

I think this should be conditional on aBuilder->ShouldSyncDecodeImages (we can pass aBuilder into GetBoundsInternal and then pass flags into here).

Comment 38

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #37)
> Won't this force a synchronous decode during this call in some cases?
> 
> I see a call to ExtractFrame in nsImageRenderer::PrepareImage which is
> sometimes called.

Heh, that's true, for the -moz-image-rect case that could happen. That's only there until after bug 826093 lands, so I wasn't too concerned. Alas this _can't_ be conditional on ShouldSyncDecodeImages, though, if we want to rely on GetBounds in the way we are currently, because when the display item is originally constructed aBuilder->ShouldSyncDecodeImages is false.

IMO we should stick with the way things are now. What I'll do is just make this bug depend on bug 826093 so we don't have to pay a penalty in any cases for this approach. That bug is close to landing too.

Comment 39

[Matt Woodrow (:mattwoodrow)]

Alright, that's fine with me.

Comment 40

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

Regarding the try job, everything looks good so far except the reftests, which are red (not orange!) because my logging is producing so much output it brings the reftest harness to its knees. Whoops =) I'll keep this try run around for the non-reftest results, but I've pushed a reftest-only run without the logging here:

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

Comment 41

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

Attachment: (Part 2) - Make DLBI always invalidate if we are sync decoding an undecoded image.

Added a check in nsImageRenderer::IsDecoded to make sure we are dealing with an actual image and not something like a gradient. Sync decoding doesn't do anything for gradients, but before we were triggering a sync decode every time with gradients. This not only caused wasteful invalidation but also prevented mozAfterPaint from being delivered. We now consider anything that's not a real image to be "decoded".

Comment 42

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

New try job here:

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

Comment 43

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

I haven't had time to touch this for a while, so I'm pushing a new try job to see to what extent this has bitrotted:

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

Comment 44

[Timothy Nikkel (:tnikkel)]

list style images weren't honouring the sync decode flag, fixing that (bug 877458) fixes the only failure from the try run in comment 43.

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

So these patches should now be green on try server.

Comment 45

[Timothy Nikkel (:tnikkel)]

Turns out that we weren't passing the sync decode flag through correctly for background image drawing, see bug 878612. The patch in bug 878612 is both necessary and sufficient to make bug 845147 be green on try server. So is there a need for the patches in this bug then? Otherwise I'll just land bug 845147 after I land bug 878612.

Comment 46

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

I'm not sure about that. The patch in bug 878612 only affects behavior when the layer is being created. It's not clear to me that that's enough when DrawWindow is called later.

On the flip side, it looks like you fixed in bug 877458 the issue that prevented this patch from being checked in, which is awesome!

Matt, could you weigh in on whether these patches are still necessary? If not we can mark this as WORKSFORME and move on.

Comment 47

[Timothy Nikkel (:tnikkel)]

(In reply to Seth Fowler [:seth] from comment #46)
> I'm not sure about that. The patch in bug 878612 only affects behavior when
> the layer is being created. It's not clear to me that that's enough when
> DrawWindow is called later.

All the other cases already correctly passed through the background draw flags (ie the sync decode images flag), so that was the only case that needed fixing.

Comment 48

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

(In reply to Timothy Nikkel (:tn) from comment #47)
> All the other cases already correctly passed through the background draw
> flags (ie the sync decode images flag), so that was the only case that
> needed fixing.

I'm sure you understand this much better than I, but I confess I just don't get this. It's not obvious to me why passing in the sync decode images flag when the display item (not layer, sorry) is being created is enough to make DrawWindow always cause a sync decode.

I expect that typically the sync decode flag _won't_ be set when the display item is created, but when DrawWindow gets called later, it will be set. In this case, as far as I know, ComputeInvalidationRegion will do the right thing if the patches in these bugs are applied, but not otherwise.

Comment 49

[Matt Woodrow (:mattwoodrow)]

Seth: Display items are short lived objects that are created (and then destroyed) for each paint event.

When we get the DrawWindow, then we will build a new display list (with the sync decode flag set on the builder), which should get applied to all relevant items.

Comment 50

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

Thanks Matt, that clarifies things. Resolving.

Comment 51

[Timothy Nikkel (:tnikkel)]

Bug 845147, comment 69 explains why we actually need this.

Comment 52

[Timothy Nikkel (:tnikkel)]

Attachment: Part 2. Handle bullet frames.

This patch handles bullet frames. I scavenged this from Seth. We need to handle many more frames, but this is the most urgent since it fixes the orange caused by bug 845147. We need to handle every frame that changes it behaviour if the sync decode flag is present. I've got patches for all of them except tables frames (which require a bit more work).

Comment 53

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa05520db4ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/a87304141182

leave open because I have a few more patches to post and land here.

Comment 54

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/aa05520db4ca
https://hg.mozilla.org/mozilla-central/rev/a87304141182

Comment 55

[Timothy Nikkel (:tnikkel)]

Attachment: Part 3. Handle a few misc background drawing frame types.

Comment 56

[Timothy Nikkel (:tnikkel)]

Attachment: Part 4. Handle tables.

My understanding of table display items is that if a nsDisplayTableCellBackground, nsDisplayTableRowGroupBackground, or nsDisplayTableRowBackground is created then a nsDisplayTableBorderBackground will also be present. So we only need the extra invalidation in nsDisplayTableBorderBackground and can get rid of it in the other types. Let me know if this is correct and I'll post a patch without that unnecessary stuff.

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Timothy Nikkel (:tn) from comment #56)
> My understanding of table display items is that if a
> nsDisplayTableCellBackground, nsDisplayTableRowGroupBackground, or
> nsDisplayTableRowBackground is created then a nsDisplayTableBorderBackground
> will also be present. So we only need the extra invalidation in
> nsDisplayTableBorderBackground and can get rid of it in the other types.

That's not correct. The invalidation performed by nsDisplayTableBorderBackground might end up in a different layer to invalidation performed by the other table parts.

Comment 58

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 766459 [details] [diff] [review]
Part 3. Handle a few misc background drawing frame types.

Review of attachment 766459 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/forms/nsButtonFrameRenderer.cpp
@@ +165,5 @@
> +    }
> +  }
> +
> +  nsDisplayItem::ComputeInvalidationRegion(aBuilder, aGeometry, aInvalidRegion);
> +}

Rather than repeating this function 3 times you could make it a helper on nsDisplayItem.

Comment 59

[Timothy Nikkel (:tnikkel)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #57)
> That's not correct. The invalidation performed by
> nsDisplayTableBorderBackground might end up in a different layer to
> invalidation performed by the other table parts.

Oh yes, of course.

Comment 60

[Timothy Nikkel (:tnikkel)]

Attachment: Part 3. Handle a few misc background drawing frame types.

Factored out the common code to nsDisplayItem.

Comment 61

[Timothy Nikkel (:tnikkel)]

Attachment: Part 5. Handle regular background images.

Comment 62

[Timothy Nikkel (:tnikkel)]

Attachment: Part 6. Handle XUL images.

Comment 63

[Timothy Nikkel (:tnikkel)]

Attachment: Part 7. Handle image frames.

Comment 64

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/696b9dc159fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/a13302dbb547
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0ee773e6d35
https://hg.mozilla.org/integration/mozilla-inbound/rev/3727fed22750
https://hg.mozilla.org/integration/mozilla-inbound/rev/53510fcee71c

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/696b9dc159fd
https://hg.mozilla.org/mozilla-central/rev/a13302dbb547
https://hg.mozilla.org/mozilla-central/rev/a0ee773e6d35
https://hg.mozilla.org/mozilla-central/rev/3727fed22750
https://hg.mozilla.org/mozilla-central/rev/53510fcee71c

Comment 66

[Yonggang Luo]

How to do Synchronous image decoding?
I need a simple demo for 
decoding a image URL to  nsCOMPtr<imgIContainer>  synchronously