[OMTC] rendering Issues on Twitter timeline background

VERIFIED FIXED in Firefox 27

Status

()

Core
Graphics: Layers
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: xtc4uall, Assigned: mattwoodrow)

Tracking

({regression})

Trunk
mozilla27
x86_64
Windows 7
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox27 verified, firefox28 verified, firefox29 verified)

Details

(Whiteboard: [beta28][gfx], URL)

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
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

4 years ago
(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
Blocks: 923434
(Assignee)

Comment 2

4 years ago
Created attachment 8342887 [details] [diff] [review]
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.
Attachment #8342887 - Flags: review?(jmuizelaar)
(Assignee)

Comment 3

4 years ago
Created attachment 8342889 [details] [diff] [review]
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).
Attachment #8342889 - Flags: review?(jmuizelaar)
(Assignee)

Comment 4

4 years ago
Created attachment 8342890 [details] [diff] [review]
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.
Attachment #8342890 - Flags: review?(jmuizelaar)
(Assignee)

Updated

4 years ago
Assignee: nobody → matt.woodrow
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)
Attachment #8342889 - Flags: review?(jmuizelaar) → review-
(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 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.
Attachment #8342889 - Flags: review- → review+
Attachment #8342890 - Flags: review?(jmuizelaar) → review+
Attachment #8342887 - Flags: review?(jmuizelaar) → review+

Comment 8

4 years ago
Would it be a possible approach here to land the patch but require a follow-up bug that fixes the performance regression?
(Assignee)

Comment 10

4 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/213221252ca3
https://hg.mozilla.org/mozilla-central/rev/5a62ab693929
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Updated

4 years ago
Duplicate of this bug: 936792

Updated

4 years ago
Duplicate of this bug: 936609

Comment 14

4 years ago
Matt, the metro browser is rolling out with 28, can this uplift?
Flags: needinfo?(matt.woodrow)

Updated

4 years ago
Whiteboard: [beta28][gfx]

Updated

4 years ago
tracking-firefox28: --- → ?
(Assignee)

Comment 15

4 years ago
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.
Attachment #8342889 - Flags: approval-mozilla-beta?
Attachment #8342889 - Flags: approval-mozilla-aurora?
Flags: needinfo?(matt.woodrow)
Comment on attachment 8342889 [details] [diff] [review]
Fix Part 1

28 is on Aurora, is there a reason to take this to 27 (Beta)?
Attachment #8342889 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

4 years ago
Blocks: 946317
(Assignee)

Comment 17

4 years ago
These patches will fix bug 946317, which affects 27.
https://hg.mozilla.org/releases/mozilla-aurora/rev/cc5c82e3f109
https://hg.mozilla.org/releases/mozilla-aurora/rev/320a46ff0674
status-firefox27: --- → affected
status-firefox28: --- → fixed
status-firefox29: --- → fixed
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
Flags: needinfo?(andrei.vaida)
Keywords: verifyme
(Assignee)

Updated

4 years ago
Duplicate of this bug: 946186
(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.
Flags: needinfo?(andrei.vaida)
QA Contact: bogdan.maris
Duplicate of this bug: 949883
Attachment #8342889 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
tracking-firefox28: ? → ---

Updated

4 years ago
Target Milestone: mozilla29 → mozilla27
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).
Status: RESOLVED → VERIFIED
status-firefox27: fixed → verified
status-firefox28: fixed → verified
status-firefox29: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.