Synchronous image decoding doesn't work with retained layers

RESOLVED FIXED in mozilla25

Status

()

Core
Layout
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: khuey, Assigned: tnikkel)

Tracking

unspecified
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 6 obsolete attachments)

3.59 KB, patch
Details | Diff | Splinter Review
3.23 KB, patch
roc
: review+
Details | Diff | Splinter Review
11.27 KB, patch
roc
: review+
Details | Diff | Splinter Review
11.10 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
3.95 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.79 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.37 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
All of the logic to synchronously decode images is in the painting code, which retained layers can cause us to skip.
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.
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.
> 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.
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.
Created attachment 726484 [details] [diff] [review]
(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?
Assignee: nobody → seth
Flags: needinfo?(justin.lebar+bug)
> 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
Flags: needinfo?(justin.lebar+bug)
s/also may/may/
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.
Blocks: 846177
No longer blocks: 685516
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?
No, I think I am. =) Got too focused on bug 846177. That does sound like it's worth a try.
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
Here's your patch by itself:

https://tbpl.mozilla.org/?tree=Try&rev=e95406d69b56
Here's the composition of the two:

https://tbpl.mozilla.org/?tree=Try&rev=9dea2532df55
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...
Now I can reproduce the problem locally. My patch currently does not fix the problem; I'm investigating why.
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.)
Attachment #726484 - Attachment is obsolete: true
> I've got a working patch that makes DBLI work correctly with sync decoding. It does indeed fix your 
> patch.

!!
Created attachment 728821 [details] [diff] [review]
(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.
Attachment #728821 - Flags: superreview?(bzbarsky)
Attachment #728821 - Flags: review?(joe)
Blocks: 845147
Created attachment 728822 [details] [diff] [review]
(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.
Attachment #728822 - Flags: review?(matt.woodrow)
Try job here:

https://tbpl.mozilla.org/?tree=Try&rev=9b35783627e9
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.
(Note that that try job includes Justin's patch. I'll post another try job shortly without it.)
(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.
Here's another try job without Justin's patch:

https://tbpl.mozilla.org/?tree=Try&rev=bab560eab291
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
(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 :)
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.
(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

4 years ago
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.
Attachment #728821 - Flags: superreview?(bzbarsky) → superreview+

Comment 30

4 years ago
> in the WebIDL 

I _meant_ the XPIDL, of course.  ;)
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.
Attachment #728821 - Flags: review?(joe) → review+
(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.
(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.
Heh wow, a few oranges in those try jobs. =) Investigating.
Created attachment 729134 [details] [diff] [review]
(Part 1) - Add imgIContainer::IsDecoded.

Address review comments.
Created attachment 729136 [details] [diff] [review]
(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
Attachment #729136 - Flags: review?(matt.woodrow)
Attachment #728822 - Attachment is obsolete: true
Attachment #728822 - Flags: review?(matt.woodrow)
Attachment #728821 - Attachment is obsolete: true
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).
Attachment #729136 - Flags: review?(matt.woodrow) → review+
(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.
Depends on: 826093
Alright, that's fine with me.
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
Created attachment 730317 [details] [diff] [review]
(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".
Attachment #729136 - Attachment is obsolete: true
New try job here:

https://tbpl.mozilla.org/?tree=Try&rev=e85a660c7c3e
Blocks: 850197
Blocks: 855050
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
(Assignee)

Updated

4 years ago
Depends on: 877458
(Assignee)

Comment 44

4 years ago
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.
(Assignee)

Comment 45

4 years ago
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.
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.
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 47

4 years ago
(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.
(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.
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.
Flags: needinfo?(matt.woodrow)
Thanks Matt, that clarifies things. Resolving.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WORKSFORME
(Assignee)

Comment 51

4 years ago
Bug 845147, comment 69 explains why we actually need this.
Assignee: seth → tnikkel
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(Assignee)

Comment 52

4 years ago
Created attachment 766015 [details] [diff] [review]
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).
Attachment #730317 - Attachment is obsolete: true
Attachment #766015 - Flags: review?(matt.woodrow)
(Assignee)

Updated

4 years ago
Attachment #766015 - Flags: review?(roc)
Attachment #766015 - Flags: review?(roc)
Attachment #766015 - Flags: review?(matt.woodrow)
Attachment #766015 - Flags: review+
(Assignee)

Comment 53

4 years ago
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.
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/aa05520db4ca
https://hg.mozilla.org/mozilla-central/rev/a87304141182
(Assignee)

Comment 55

4 years ago
Created attachment 766459 [details] [diff] [review]
Part 3. Handle a few misc background drawing frame types.
Attachment #766459 - Flags: review?(matt.woodrow)
(Assignee)

Comment 56

4 years ago
Created attachment 766460 [details] [diff] [review]
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.
Attachment #766460 - Flags: review?(roc)
(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.
Attachment #766460 - Flags: review?(roc) → review+
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.
Attachment #766459 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 59

4 years ago
(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.
(Assignee)

Comment 60

4 years ago
Created attachment 767468 [details] [diff] [review]
Part 3. Handle a few misc background drawing frame types.

Factored out the common code to nsDisplayItem.
Attachment #766459 - Attachment is obsolete: true
Attachment #767468 - Flags: review?(matt.woodrow)
(Assignee)

Comment 61

4 years ago
Created attachment 767471 [details] [diff] [review]
Part 5. Handle regular background images.
Attachment #767471 - Flags: review?(matt.woodrow)
(Assignee)

Comment 62

4 years ago
Created attachment 767473 [details] [diff] [review]
Part 6. Handle XUL images.
Attachment #767473 - Flags: review?(matt.woodrow)
(Assignee)

Comment 63

4 years ago
Created attachment 767474 [details] [diff] [review]
Part 7. Handle image frames.
Attachment #767474 - Flags: review?(matt.woodrow)
Attachment #767468 - Flags: review?(matt.woodrow) → review+
Attachment #767471 - Flags: review?(matt.woodrow) → review+
Attachment #767473 - Flags: review?(matt.woodrow) → review+
Attachment #767474 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 64

4 years ago
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
Whiteboard: [leave open]
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
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

Comment 66

3 years ago
How to do Synchronous image decoding?
I need a simple demo for 
decoding a image URL to  nsCOMPtr<imgIContainer>  synchronously
You need to log in before you can comment on or make changes to this bug.