1168527 - Reftest failures on GTK3 due to differing rendering paths

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Andrew Comminos [:acomminos]]

Some reftests fail on GTK3 try with gfx.xrender.enabled=true and layers.use-image-offscreen-surfaces=false. Some pixels around the edge of the invalidated button are slightly lighter- I suspect this has something to do with the fact that our GTK3 backend renders into image surfaces instead of Xlib surfaces.

REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/bugs/513153-1a.html | image comparison (==), max difference: 2, number of differing pixels: 26
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/bugs/513153-1b.html | image comparison (==), max difference: 2, number of differing pixels: 26
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/bugs/513153-2a.html | image comparison (==), max difference: 3, number of differing pixels: 22
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/bugs/513153-2b.html | image comparison (==), max difference: 3, number of differing pixels: 22

Comment 1

[Andrew Comminos [:acomminos]]

The issue here is that we render onto a Cairo image surface for complex clips (such as those caused by invalidations), and use an Xlib surface for simple clips. We can work around this by adding support for complex rectangular clips to our Xlib path.

Comment 2

[Andrew Comminos [:acomminos]]

Attachment: Part 1 - Add support for retrieving a list of clip rectangles from a Cairo DrawTarget.

Comment 3

[Andrew Comminos [:acomminos]]

Attachment: Part 2 - Support multiple rectangular clips for Xlib widget rendering on GTK3.

Comment 4

[Andrew Comminos [:acomminos]]

Attachment: Part 2 -Support multiple rectangular clips for Xlib widget rendering on GTK3.

Added clipping to LockBits path.

Comment 5

[Andrew Comminos [:acomminos]]

Attachment: Part 2 - Support multiple rectangular clips for Xlib widget rendering on GTK3.

Account for draw target translation.

Comment 6

[Andrew Comminos [:acomminos]]

Attachment: Support multiple rectangular clips for Xlib widget rendering on GTK3.

Part 2 - Use system cairo transform when dealing with clip rectangles.

Comment 7

[Andrew Comminos [:acomminos]]

Comment on attachment 8614884 [details] [diff] [review]
Part 1 - Add support for retrieving a list of clip rectangles from a Cairo DrawTarget.

Bas, do you mind taking a look at this? The main purpose of adding this is to provide a faster and more consistent path for applying multiple rectangular clips from a system Cairo.

Comment 8

[Bas Schouten (:bas.schouten)]

(In reply to Andrew Comminos [:acomminos] from comment #7)
> Comment on attachment 8614884 [details] [diff] [review]
> Part 1 - Add support for retrieving a list of clip rectangles from a Cairo
> DrawTarget.
> 
> Bas, do you mind taking a look at this? The main purpose of adding this is
> to provide a faster and more consistent path for applying multiple
> rectangular clips from a system Cairo.

This doesn't particularly excite me.. Retrieving state from a DrawTarget like that really isn't something we want to support. I feel the DT user should really be keeping track of this if they need it.

This sort of relying on drawing devices as state storage devices is what caused a lot of our trouble with Cairo in the past. In this particular situation I might be convinced this is worth it but I'm very hesitant.

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8615299 [details] [diff] [review]
Support multiple rectangular clips for Xlib widget rendering on GTK3.

(In reply to Andrew Comminos [:acomminos] from comment #7)
> Bas, do you mind taking a look at this? The main purpose of adding this is
> to provide a faster and more consistent path for applying multiple
> rectangular clips from a system Cairo.

I expect this could be important for speed.  I'm not concerned about
consistency as different rendering targets use different tessellation
algorithms so we already have these inconsistencies with masks.

cairo_copy_clip_rectangle_list() was added precisely for this in
gfxXlibNativeRenderer and we have needed this for performance in the past.

However, I don't know why we don't have a cairo_t in the DrawTarget to use
here.  Is this a common enough situation likely to matter for performance?

(In reply to Bas Schouten (:bas.schouten) from comment #8)
> This doesn't particularly excite me.. Retrieving state from a DrawTarget
> like that really isn't something we want to support. I feel the DT user
> should really be keeping track of this if they need it.

Here, what we are trying to do is take the DrawTarget state and translate it
to a cairo state so that we can use cairo for drawing.

If we shouldn't be doing this, then the same logic would seem to imply that we
shouldn't be passing DrawTargets around.

Lee would know more about this code than I now, and so would be a better
reviewer.  I just had a quick look and would suggest trying to avoid the code
duplication where practical.

Comment 10

[Andrew Comminos [:acomminos]]

Attachment: Part 2 - Support multiple rectangular clips for Xlib widget rendering on GTK3.

Updated with Lee's suggestions, only require translation and ignore aDrawSize clip.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

(In reply to Bas Schouten (:bas.schouten) from comment #8)
> (In reply to Andrew Comminos [:acomminos] from comment #7)
> > Comment on attachment 8614884 [details] [diff] [review]
> > Part 1 - Add support for retrieving a list of clip rectangles from a Cairo
> > DrawTarget.
> > 
> > Bas, do you mind taking a look at this? The main purpose of adding this is
> > to provide a faster and more consistent path for applying multiple
> > rectangular clips from a system Cairo.
> 
> This doesn't particularly excite me.. Retrieving state from a DrawTarget
> like that really isn't something we want to support. I feel the DT user
> should really be keeping track of this if they need it.
> 
> This sort of relying on drawing devices as state storage devices is what
> caused a lot of our trouble with Cairo in the past. In this particular
> situation I might be convinced this is worth it but I'm very hesitant.

Your hesitancy is fair. However, having a Push/PopClip style API already encourages a state storage. Having to duplicate tracking of the clip state in the caller seems more error prone and isn't really practical in our current code base. Further, because the API is opt in for DrawTargets, I don't think we have to worry about callers relying on it for state storage.

I.e. the cost of adding this is low and shouldn't cause any long term damage, on the other hand the cost of not adding this is quite a bit of work and potential pain.

Comment 12

[Karl Tomlinson (:karlt)]

Can someone explain please why adding DrawTargetCairo::GetClipRects() is helpful if "A direct Cairo draw target is not available"?

Is the cairo in the DrawTargetCairo not a system cairo?

Comment 13

[Lee Salzman [:lsalzman]]

(In reply to Karl Tomlinson (ni?:karlt) from comment #12)
> Can someone explain please why adding DrawTargetCairo::GetClipRects() is
> helpful if "A direct Cairo draw target is not available"?
> 
> Is the cairo in the DrawTargetCairo not a system cairo?

The DrawTargetCairo code uses our tree Cairo, but the GTK widget code must use system Cairo for interaction with GTK3. Because we have our own patches and differing functionality in our tree Cairo, this situation is preferable to using system Cairo everywhere, which would have buggy and inadequate behavior with respect to many of our tests.

The system Cairo symbols and headers can't be used to directly access our tree Cairo contexts/surfaces (because their layouts differ). Thus, in the widget code which uses those system symbols and headers, we have to do the appropriate dance, hiding stuff carefully behind DrawTargetCairo, to either take the underlying X drawable (or in some cases punt to creating image surfaces) and build an appropriate system Cairo surface/context from it that is safe to pass in to GTK3.

Comment 14

[Andrew Comminos [:acomminos]]

Attachment: Part 1 - Add support to gfxContext for clip exporting.

This patch by Lee and I adds a ClipExporter that can be used to replay clips into the system cairo.

Comment 15

[Andrew Comminos [:acomminos]]

Attachment: Part 2 - Replay clips into the system cairo on GTK3.

Implements replaying thebes clips into the system cairo on GTK3.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8616870 [details] [diff] [review]
Part 1 - Add support to gfxContext for clip exporting.

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

This seems ok.

Comment 17

[Andrew Comminos [:acomminos]]

Attachment: Part 2 - Replay clips into the system cairo on GTK3.

Allocate SystemCairoClipper on the heap to make RefCounted base class happy.

Comment 18

[Andrew Comminos [:acomminos]]

Attachment: Part 2 - Replay clips into the system cairo on GTK3.

Fix refcounting and device offsets.

Comment 19

[Lee Salzman [:lsalzman]]

Comment on attachment 8616933 [details] [diff] [review]
Part 2 - Replay clips into the system cairo on GTK3.

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

Looks fine. This should allow us to inter-operate better now with Skia if we ever choose to use it as a content backend.

Comment 20

[Lee Salzman [:lsalzman]]

Comment on attachment 8616933 [details] [diff] [review]
Part 2 - Replay clips into the system cairo on GTK3.

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

Oops, just noticed a small bug. In the LockBits case, it should be using size.width/size.height instead of aDrawSize.width/aDrawSize.height, as when using the fully general transform from the context it may map to anywhere on the draw target's surface, not just a small drawing rectangle anymore.

Comment 21

[Andrew Comminos [:acomminos]]

Attachment: Part 2 - Replay clips into the system cairo on GTK3. Carry r=lsalzman

Thanks for the heads up! Carrying the r+ assuming no other issues.

Comment 22

[Lee Salzman [:lsalzman]]

(In reply to Andrew Comminos [:acomminos] from comment #21)
> Created attachment 8616997 [details] [diff] [review]
> Part 2 - Replay clips into the system cairo on GTK3. Carry r=lsalzman
> 
> Thanks for the heads up! Carrying the r+ assuming no other issues.

Looks fine now.

Comment 23

[Carsten Book [:Tomcat]]

can we get a try run here ?

Comment 24

[Andrew Comminos [:acomminos]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=24d5d8835c02

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 26

[Andrew Comminos [:acomminos]]

Attachment: Part 1 - Add support to gfxContext for clip exporting. Carry r=jrmuizel

Comment 27

[Andrew Comminos [:acomminos]]

Attachment: Part 2 - Replay clips into the system cairo on GTK3. Carry r=lsalzman

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Not sure what branch you rebased these on top of, but they still don't apply to inbound without conflicts. Both patches.

Comment 29

[Andrew Comminos [:acomminos]]

Attachment: Add support to gfxContext for clip exporting. Carry r=jrmuizel

Comment 30

[Andrew Comminos [:acomminos]]

Attachment: Replay clips into the system cairo on GTK3. Carry r=lsalzman

Comment 31

[Andrew Comminos [:acomminos]]

Sorry, something appears to be amiss with my git workflow. Tested, applies cleanly to inbound for me.

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff75c7e0b5d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/85ca98c22bdc

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ff75c7e0b5d4
https://hg.mozilla.org/mozilla-central/rev/85ca98c22bdc

Comment 34

[Hubert Figuiere [:hub]]

This patch break the build. I'm on Fedora 22 with gcc 5.

Comment 35

[Hubert Figuiere [:hub]]

This changeset.

changeset:   248350:85ca98c22bdc
user:        Andrew Comminos <[e-mail]>
date:        Tue Jun 09 13:46:09 2015 -0400
summary:     Bug 1168527 - Replay clips into the system cairo on GTK3. r=lsalzman

This is caused by "borrow" clobbering a previously defined "borrow". Before the patch they were in a different scope and would cause a -Wshadow. But now, it causes a failure with gcc 5.

A quick fix is something like:

# HG changeset patch
# Parent  d947d66c49dc96c2d25ee7ece0429c85484bdd4f

diff --git a/widget/gtk/nsNativeThemeGTK.cpp b/widget/gtk/nsNativeThemeGTK.cpp
--- a/widget/gtk/nsNativeThemeGTK.cpp
+++ b/widget/gtk/nsNativeThemeGTK.cpp
@@ -817,23 +817,23 @@ DrawThemeWithCairo(gfxContext* aContext,
   if (aScaleFactor != 1)
     transform.PreScale(aScaleFactor, aScaleFactor);
 
   cairo_matrix_t mat;
   GfxMatrixToCairoMatrix(transform, mat);
 
 #ifndef MOZ_TREE_CAIRO
   // Directly use the Cairo draw target to render the widget if using system Cairo everywhere.
-  BorrowedCairoContext borrow(aDrawTarget);
-  if (borrow.mCairo) {
-    cairo_set_matrix(borrow.mCairo, &mat);
+  BorrowedCairoContext borrowC(aDrawTarget);
+  if (borrowC.mCairo) {
+    cairo_set_matrix(borrowC.mCairo, &mat);
 
-    moz_gtk_widget_paint(aGTKWidgetType, borrow.mCairo, &aGDKRect, &aState, aFlags, aDirection);
+    moz_gtk_widget_paint(aGTKWidgetType, borrowC.mCairo, &aGDKRect, &aState, aFlags, aDirection);
 
-    borrow.Finish();
+    borrowC.Finish();
     return;
   }
 #endif
 
   // A direct Cairo draw target is not available, so we need to create a temporary one.
 #if defined(MOZ_X11) && defined(CAIRO_HAS_XLIB_SURFACE)
   // If using a Cairo xlib surface, then try to reuse it.
   BorrowedXlibDrawable borrow(aDrawTarget);

Comment 36

[Hubert Figuiere [:hub]]

Break the build. see comment 34. Will provide a patch.

Comment 37

[Hubert Figuiere [:hub]]

Attachment: Variable was re-declared causing a bustage on gcc 5.

Proposed fix.

Comment 38

[Hubert Figuiere [:hub]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=46d57484f663

Comment 39

[Hubert Figuiere [:hub]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/717dd4a0bb6f

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/717dd4a0bb6f