Closed Bug 946475 Opened 6 years ago Closed 6 years ago

[OMTC] rendering Issues on Twitter timeline background

Categories

(Core :: Graphics: Layers, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla27
Tracking Status
firefox27 --- verified
firefox28 --- verified
firefox29 --- verified

People

(Reporter: xtc4uall, Assigned: mattwoodrow)

References

()

Details

(Keywords: regression, Whiteboard: [beta28][gfx])

Attachments

(3 files)

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)?
(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
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)
Attached patch Fix Part 1Splinter Review
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)
Attached patch Fix Part 2Splinter Review
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: 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+
Would it be a possible approach here to land the patch but require a follow-up bug that fixes the performance regression?
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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Duplicate of this bug: 936792
Duplicate of this bug: 936609
Matt, the metro browser is rolling out with 28, can this uplift?
Flags: needinfo?(matt.woodrow)
Whiteboard: [beta28][gfx]
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+
Blocks: 946317
These patches will fix bug 946317, which affects 27.
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
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+
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).
You need to log in before you can comment on or make changes to this bug.