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

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

This is a continuation of bug 1128225 that takes the same approach used in that bug and applies it to CSS images. See the discussion in that bug for the details.

Comment 1

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

Attachment: (Part 1) - Propagate the imgIContainer::Draw result through the nsLayoutUtils::PaintBackground* functions

We'll need access to the DrawResult we get when drawing background images, so we
need to propagate it through nsLayoutUtils::PaintBackground and
nsLayoutUtils::PaintBackgroundWithSC.

Unfortunately, some callers of PaintBackgroundWithSC ask it to paint all the
background layers at once. In this situation, we're forced to return a
non-SUCCESS DrawResult if *any* of the layers fail. On the other hand, such
callers will inevitably repaint all the layers anyway if they decide to repaint
due to a non-SUCCESS DrawResult, so it doesn't really matter.

Comment 2

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8558232 [details] [diff] [review]
(Part 1) - Propagate the imgIContainer::Draw result through the nsLayoutUtils::PaintBackground* functions

>@@ -3000,27 +3005,36 @@ nsCSSRendering::PaintBackgroundWithSC(ns
>+          DrawResult resultForLayer =
>+            state.mImageRenderer.DrawBackground(aPresContext, aRenderingContext,
>+                                                state.mDestArea, state.mFillArea,
>+                                                state.mAnchor + paintBorderArea.TopLeft(),
>+                                                clipState.mDirtyRect);
>+
>+          if (result == DrawResult::SUCCESS) {
>+            result = resultForLayer;
>+          }

You want != here, right?

Comment 3

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

(In reply to Timothy Nikkel (:tn) from comment #2)
> You want != here, right?

Well, the approach I'm taking here is to return a non-SUCCESS value if we fail to successfully draw *any* background layer. So we keep updating |result| until we hit the first non-SUCCESS value, and then we refuse to overwrite it, ensuring that that is what we'll return. That's why I'm using '=='.

Comment 4

[Timothy Nikkel (:tnikkel)]

Whoops, didn't read that closely enough.

Comment 5

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

Attachment: (Part 2) - Check if we invalidated for a sync decode and never painted before invalidating for sync decoding again

There's an additional wrinkle that the CSS background tests trigger that we
didn't encounter for the content tests. Completely occluded display items that
include images that need sync decoding can cause invalidation loops. The details
are described in bug 1128756, so I won't repeat them here.

This patch solves the problem described in bug 1128756 in an alternate manner,
so we can avoid special-casing drawWindow in any way.

The idea is pretty straightforward: if we're going to invalidate to trigger a
sync decode, the geometry item sets a flag indicating that it's waiting for a
paint. UpdateDrawResult() clears that flag. If the flag is still set next time
we're deciding whether to invalidate to trigger sync decoding, we don't
invalidate. Since only the geometry item knows this, display items now defer the
decision of whether to invalidate for sync decoding to
nsImageGeometryMixin::ShouldInvalidateToSyncDecodeImages().

The behavior of this patch isn't as obvious as I'd like because we don't have
access to the new geometry item we just allocated via AllocateGeometry() when we
call ComputeInvalidationRegion(). That means that
ShouldInvalidateToSyncDecodeImages() is always called on the *old* geometry
item, which in turn means that we cannot set mWaitingToPaint there (when we know
for sure that we're going to invalidate) because the old geometry item gets
discarded after ComputeInvalidationRegion() is called and can't influence the
new geometry item anymore. Fortunately, the new geometry item copies its state
from the old geometry item in its constructor, so when it's first created it
will reach the same decision in ShouldInvalidateToSyncDecodeImages(). Thus, we
can check ShouldSyncDecodeImages() and ShouldInvalidateToSyncDecodeImages() in
the new geometry item's constructor, and if they both return true, set the
mWaitingToPaint flag at that point.

Comment 6

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

Attachment: (Part 3) - Record the last draw result when drawing CSS backgrounds and use it to decide whether to sync decode

OK, that's enough setup. Now we can start moving display items and geometry
items over to the new approach. First up, nsDisplayBackground.

Comment 7

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

Attachment: (Part 4) - Record the last draw result when drawing CSS tables and use it to decide whether to sync decode

Here's the patch for tables. This is all the usual
nsDisplayItemGenericImageGeometry boilerplate, except that I had to update
TablePainter to return a DrawResult for its public methods, which ended up
requiring some pretty ugly in-out parameter wiring. Whenever we properly
DLBI-ize tables the ugliness should go away.

BTW, I just removed a couple of early returns that were checking for null being
returned from |new|. That can't happen; the normal version of |new| is
infallible in Gecko.

Comment 8

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

Attachment: (Part 5) - Record the last draw result for various less common frame types and use it to decide whether to sync decode

Alright, all that's left at this point is a few odds and ends. They are all
cases where we can just use the usual nsDisplayItemGenericImageGeometry
boilerplate.

Comment 9

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

Attachment: (Part 6) - Remove imgIContainer::IsDecoded and all remaining callers

Finally, we can now remove imgIContainer::IsDecoded() and all of the things
built on top of it, since they're all totally unused now.

Comment 10

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8560152 [details] [diff] [review]
(Part 4) - Record the last draw result when drawing CSS tables and use it to decide whether to sync decode

> nsTableFrame::PaintTableBorderBackground(nsRenderingContext& aRenderingContext,
>+  DrawResult result =
>+    painter.PaintTable(this, deflate, deflate != nsMargin(0, 0, 0, 0));
>+  if (result == DrawResult::TEMPORARY_ERROR) {
>+    return result;
>+  }

But what if one of the images returns temporary error and it's not as a result of one of the things that made PaintTable return a failure code before? Then shouldn't we continue to paint the borders and not return early?

Comment 11

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

(In reply to Timothy Nikkel (:tn) from comment #10)
> But what if one of the images returns temporary error and it's not as a
> result of one of the things that made PaintTable return a failure code
> before? Then shouldn't we continue to paint the borders and not return early?

You know, looking at the cases where nsTablePainter could've ended up returning a failing nsresult value, I think we can just remove those cases and get rid of the early return here totally. It looks to me like they're all the result of code that checks for |new| failure, but |new| is infallible in Gecko anyway, as I mentioned in comment 7. I'll update the patch to remove those checks from nsTablePainter and drop the early return.

Comment 12

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

Attachment: (Part 4) - Record the last draw result when drawing CSS tables and use it to decide whether to sync decode

OK, this version of part 4 removes the early return and all of the nsresult
return values in nsTablePainter. This enabled me to make the nsTablePainter
methods just return DrawResult values directly, and I think the final result is
a lot cleaner.

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8560249 [details] [diff] [review]
(Part 4) - Record the last draw result when drawing CSS tables and use it to decide whether to sync decode

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

::: layout/tables/nsTablePainter.cpp
@@ +432,5 @@
>      nsRect rgOverflowRect = rgVisualOverflow + rg->GetPosition();
>      nsRect rgNormalRect = rgVisualOverflow + rg->GetNormalPosition();
>  
>      if (rgOverflowRect.Union(rgNormalRect).Intersects(mDirtyRect - mRenderPt)) {
> +      PaintRowGroup(rg, rg->IsPseudoStackingContextFromStyle());

Did you mean to ignore the return value of this call?

Comment 14

[Markus Stange [:mstange]]

Attachment: (Part 4) - Record the last draw result when drawing CSS tables and use it to decide whether to sync decode

I updated the patch on top of bug 1130817 and fixed the issue I pointed out in comment 13.

Comment 15

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

(In reply to Markus Stange [:mstange] from comment #13)
> Did you mean to ignore the return value of this call?

No, I didn't. Good catch! Thanks for updating the patch.

Comment 16

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

Thanks for the review! Pushed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a4be606796ba
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/893dcee72667
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d6c37a5ec39c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a72dc404ea1f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e2239d5fcb1e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f97eece7bdb6

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a4be606796ba
https://hg.mozilla.org/mozilla-central/rev/893dcee72667
https://hg.mozilla.org/mozilla-central/rev/d6c37a5ec39c
https://hg.mozilla.org/mozilla-central/rev/a72dc404ea1f
https://hg.mozilla.org/mozilla-central/rev/e2239d5fcb1e
https://hg.mozilla.org/mozilla-central/rev/f97eece7bdb6

Comment 18

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

Comment on attachment 8560249 [details] [diff] [review]
(Part 4) - Record the last draw result when drawing CSS tables and use it to decide whether to sync decode

This version needs to be the basis for the uplift to 37.

Comment 19

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

Comment on attachment 8558232 [details] [diff] [review]
(Part 1) - Propagate the imgIContainer::Draw result through the nsLayoutUtils::PaintBackground* functions

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 20

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

Pushed to Aurora:

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/907ae07461e4
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/cfc0c8037827
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/c581cb43428d
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/20fe191ffa44
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/25345c8d5771
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/da8d29b51b28

Comment 21

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

Comment on attachment 8558232 [details] [diff] [review]
(Part 1) - Propagate the imgIContainer::Draw result through the nsLayoutUtils::PaintBackground* functions

Part of a collection of ImageLib work that has been tested by QE and verbally approved to land. Adding approval after the fact.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

This caused Linux32 debug bustage (probably non-unified). We're two days from the uplift and tree rules say that you should be watching your pushes, so I expect to take about 30 seconds trying to fix this myself before just backing it all out (and anything you landed on top of it) instead.

https://treeherder.mozilla.org/logviewer.html#?job_id=616120&repo=mozilla-aurora

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Backed out.
https://hg.mozilla.org/releases/mozilla-aurora/rev/0d2ff4249817

Comment 24

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

The failure here does indeed appear to be a non-unified-build issue. It looks like nsTableRowFrame.cpp needed |using namespace mozilla::image;|. I've pushed a try job here with this fix (and also to triple check that there aren't any other issues on any platforms):

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

Comment 25

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

Comment on attachment 8558232 [details] [diff] [review]
(Part 1) - Propagate the imgIContainer::Draw result through the nsLayoutUtils::PaintBackground* functions

Approval Request Comment
[Feature/regressing bug #]: Part of fix for bug 1125272 and bug 1119938.
[User impact if declined]: Problems with synchronous decoding of images, which can affect features like the about:newtab page and the B2G app switcher, as well as our reftest harness.
[Describe test coverage new/current, TreeHerder]: In 38 for a long time.
[Risks and why]: Low risk; in 38 for a long time. Was already approved for uplift to 37 when 37 was Aurora, but was backed out because of a non-unified-build failure.
[String/UUID change made/needed]: None.

Comment 26

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

We needed another "using mozilla::image" instance. Here's a new try job (that's already green):

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Do we need to land a follow-up fix on trunk/aurora as well?

Comment 28

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #27)
> Do we need to land a follow-up fix on trunk/aurora as well?

It's definitely not worth it for Aurora IMO. We don't support non-unified builds anymore, so uplifting something like that doesn't seem justified.

OTOH, it *is* nice to fix such problems when we know about them, so pushing a followup to trunk might be worthwhile.

Comment 29

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

(In reply to Seth Fowler [:seth] from comment #25)
> [Risks and why]: Low risk; in 38 for a long time. Was already approved for
> uplift to 37 when 37 was Aurora, but was backed out because of a
> non-unified-build failure.

Was the failure due to this patch? Is any change needed before relanding on 37?

Comment 30

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

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #29)
> Was the failure due to this patch? Is any change needed before relanding on
> 37?

The failure was due to missing "using namespace image" in a couple of files in this patch, yes. We only saw it on 37 because in 38+ we stopped doing non-unified builds.

I've already made the necessary change, but I'd rather go ahead and land it myself on beta and then land a separate fix on trunk. There are small incompatibilities on the two branches due to some refactoring that happened in the table layout code in 38.

Comment 31

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

Comment on attachment 8558232 [details] [diff] [review]
(Part 1) - Propagate the imgIContainer::Draw result through the nsLayoutUtils::PaintBackground* functions

Thanks for confirming Seth. Let's land this change again for Beta 3. Beta+

Comment 32

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

Thanks for the approval! Pushed to beta:

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/759ad062242f
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/69da299d5e49
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/d80f4050e348
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/782bd163aeed
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/498290d95f1c
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/7b3c7ba30dfe