946475 - [OMTC] rendering Issues on Twitter timeline background

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[(mostly gone) XtC4UaLL [:xtc4uall]]

STR:

* layers.offmainthreadcomposition.enabled;true
* https://twitter.com/
* scroll down/up

notice whitish rectangles flashing on the background DIV
(i.e. <div id="page-container" class="wrapper wrapper-list white">)

Seems to be triggered by the

background: url('../img/wash-white-30.png') repeat scroll 0% 0% transparent;}

rule set.

Alice, since you did a local bisect in Bug 946186 Comment 4, can you check if this is the same regression (if you have a twitter account of course)?

Comment 1

[Alice0775 White]

(In reply to XtC4UaLL [:xtc4uall] from comment #0)

> Alice, since you did a local bisect in Bug 946186 Comment 4, can you check
> if this is the same regression (if you have a twitter account of course)?

In local build,
Last Good: c99d15a060e2
First Bad: f65cc31c9745

Triggered by:
f65cc31c9745	Matt Woodrow — Bug 923434 - Don't use operator source for D2D in ThebesLayerBuffer since it's slower. r=Bas

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Moz2D Unit test for this bug

The import parts here are that the color is partially transparent (such that drawing it twice with OVER gives different results), and that there are parts of the surface that outside both the ClearRect() area and the clip.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Fix Part 1

This function is meant to work by first making a copy of the entire DrawTarget into a temporary texture. We then remove the clipping from the draw target, and clear the aRect area. Finally we copy back (using OVER) the pixels that were meant to be clipped out and shouldn't have been changed.

The problem here is that if aRect doesn't cover all the clipped pixels, then the ones that weren't modified by the clear still get copied back and end up increasing in opacity.


There's a second bug here too, the PushAxisAlignedClip is only correct if the transform is axis aligned, which this doesn't check (PushClipRect does this correctly).

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Fix Part 2

If all the cumulative clip can be represented by a single rect in device space, then d2d can handle this, and we don't need to bother with the copy to the temporary.

This should be the common case, and a performance win.

Comment 5

[Bas Schouten (:bas.schouten)]

Comment on attachment 8342889 [details] [diff] [review]
Fix Part 1

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

I suppose this'll work. It will however, cause a considerable performance regression when calling this function with no clips pushed to the surface (pretty common), since we'll now always be going through the AutoSaveRestore dance. We should add some code to avoid this (i.e. if !mPushedClips.size() && mTransform.IsAxisAligned() then don't go through this trouble)

Comment 6

[Bas Schouten (:bas.schouten)]

(In reply to Bas Schouten (:bas.schouten) from comment #5)
> Comment on attachment 8342889 [details] [diff] [review]
> Fix Part 1
> 
> Review of attachment 8342889 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I suppose this'll work. It will however, cause a considerable performance
> regression when calling this function with no clips pushed to the surface
> (pretty common), since we'll now always be going through the AutoSaveRestore
> dance. We should add some code to avoid this (i.e. if !mPushedClips.size()
> && mTransform.IsAxisAligned() then don't go through this trouble)

I should note on this subject, can we avoid ever calling this with clips pushed? :( It's sad. If we do call it with clips pushed regularly, we should optimize the case where there's only axis-aligned rectangular clips pushed, we can do that easily.

Comment 7

[Bas Schouten (:bas.schouten)]

Comment on attachment 8342889 [details] [diff] [review]
Fix Part 1

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

Next time I'll read the bug comments before doing the review :P. With the follow-up this is fine.

Comment 8

[Andreas Gal :gal]

Would it be a possible approach here to land the patch but require a follow-up bug that fixes the performance regression?

Comment 9

[Matt Woodrow (:mattwoodrow)]

Part 2 is that follow-up :)

Landed on moz2d:

https://hg.mozilla.org/users/bschouten_mozilla.com/moz2d/rev/713331ea40e0
https://hg.mozilla.org/users/bschouten_mozilla.com/moz2d/rev/d1117636f906
https://hg.mozilla.org/users/bschouten_mozilla.com/moz2d/rev/3f5dab9a2b37

Will land on inbound once it opens.

Comment 10

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/213221252ca3
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a62ab693929

Didn't land the unit test, since it appears we're way out of date there and the patch wouldn't apply.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/213221252ca3
https://hg.mozilla.org/mozilla-central/rev/5a62ab693929

Comment 14

[Jim Mathies [:jimm]]

Matt, the metro browser is rolling out with 28, can this uplift?

Comment 15

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8342889 [details] [diff] [review]
Fix Part 1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 923434, though it really just exposed a d2d bug (which already affected <canvas>)
User impact if declined: Broken text for metro in some cases.
Testing completed (on m-c, etc.): Been on m-c for a while, manually confirmed it fixes the issue. Added moz2d unit test.
Risk to taking this patch (and alternatives if risky): Low risk. Can't really back out the regressing patch since it fixed another bug.
String or IDL/UUID changes made by this patch: None.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8342889 [details] [diff] [review]
Fix Part 1

28 is on Aurora, is there a reason to take this to 27 (Beta)?

Comment 17

[Matt Woodrow (:mattwoodrow)]

These patches will fix bug 946317, which affects 27.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cc5c82e3f109
https://hg.mozilla.org/releases/mozilla-aurora/rev/320a46ff0674

Comment 19

[u279076]

Andrei, please coordinate some extra regression testing around OMTC this Aurora cycle and mark this bug verified fixed if you don't find any further regressions. Thank you

Comment 21

[Andrei Vaida [:avaida]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #19)
> Andrei, please coordinate some extra regression testing around OMTC this
> Aurora cycle and mark this bug verified fixed if you don't find any further
> regressions. Thank you

Anthony, I'm assigning this issue to Bogdan for further investigations.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6c0aa15ef904
https://hg.mozilla.org/releases/mozilla-beta/rev/0192ade0e746

Comment 24

[Bogdan Maris, Desktop QA]

Verified as fixed on Firefox 27beta4, latest Aurora and latest Nightly using STR from comment 0. Also did some exploratory around OMTC on different websites (https://etherpad.mozilla.org/firefox28-0a2-OMTC).