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

RESOLVED FIXED in Firefox 37

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox37 fixed, firefox38 fixed)

Details

Attachments

(7 attachments, 1 obsolete attachment)

22.06 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
8.02 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
5.20 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
18.90 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
10.05 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
55.18 KB, patch
Details | Diff | Splinter Review
50.41 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
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.
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.
Attachment #8558232 - Flags: review?(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?
Attachment #8558232 - Flags: review?(tnikkel) → review+
(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 '=='.
Whoops, didn't read that closely enough.
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.
Attachment #8559674 - Flags: review?(tnikkel)
OK, that's enough setup. Now we can start moving display items and geometry
items over to the new approach. First up, nsDisplayBackground.
Attachment #8559687 - Flags: review?(tnikkel)
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.
Attachment #8560152 - Flags: review?(tnikkel)
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.
Attachment #8560167 - Flags: review?(tnikkel)
Finally, we can now remove imgIContainer::IsDecoded() and all of the things
built on top of it, since they're all totally unused now.
Attachment #8560172 - Flags: review?(tnikkel)
Attachment #8559674 - Flags: review?(tnikkel) → review+
Attachment #8559687 - Flags: review?(tnikkel) → review+
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?
Attachment #8560172 - Flags: review?(tnikkel) → review+
Attachment #8560167 - Flags: review?(tnikkel) → review+
(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.
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.
Attachment #8560249 - Flags: review?(tnikkel)
Attachment #8560152 - Attachment is obsolete: true
Attachment #8560152 - Flags: review?(tnikkel)
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?
I updated the patch on top of bug 1130817 and fixed the issue I pointed out in comment 13.
Attachment #8560249 - Attachment is obsolete: true
Attachment #8560249 - Flags: review?(tnikkel)
Attachment #8561205 - Flags: review?(tnikkel)
(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.
Attachment #8561205 - Flags: review?(tnikkel) → review+
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.
Attachment #8560249 - Attachment is obsolete: false
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.
Attachment #8558232 - Flags: approval-mozilla-aurora?
Attachment #8559674 - Flags: approval-mozilla-aurora?
Attachment #8559687 - Flags: approval-mozilla-aurora?
Attachment #8560167 - Flags: approval-mozilla-aurora?
Attachment #8560172 - Flags: approval-mozilla-aurora?
Attachment #8560249 - Flags: approval-mozilla-aurora?
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.
Attachment #8558232 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8559674 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8559687 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8560249 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8560167 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8560172 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Flags: needinfo?(seth)
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 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.
Attachment #8558232 - Flags: approval-mozilla-beta?
Attachment #8559674 - Flags: approval-mozilla-beta?
Attachment #8559687 - Flags: approval-mozilla-beta?
Attachment #8560167 - Flags: approval-mozilla-beta?
Attachment #8560172 - Flags: approval-mozilla-beta?
Attachment #8560249 - Flags: approval-mozilla-beta?
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
Do we need to land a follow-up fix on trunk/aurora as well?
Flags: needinfo?(seth)
(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.
Flags: needinfo?(seth)
(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?
Flags: needinfo?(seth)
(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.
Flags: needinfo?(seth)
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+
Attachment #8558232 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8559674 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8559687 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8560249 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8560167 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8560172 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 850197
Blocks: 1147195
You need to log in before you can comment on or make changes to this bug.