Reftest failures on GTK3 due to differing rendering paths

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: acomminos, Assigned: acomminos)

Tracking

unspecified
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(3 attachments, 13 obsolete attachments)

3.72 KB, patch
acomminos
: review+
Details | Diff | Splinter Review
9.49 KB, patch
acomminos
: review+
Details | Diff | Splinter Review
1.42 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
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
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.
Summary: Reftest failures on GTK3 with XRender enabled and image offscreen surfaces disabled → Reftest failures on GTK3 due to differing rendering paths
Added clipping to LockBits path.
Attachment #8614885 - Attachment is obsolete: true
Assignee: nobody → acomminos
Account for draw target translation.
Attachment #8614898 - Attachment is obsolete: true
Part 2 - Use system cairo transform when dealing with clip rectangles.
Attachment #8614939 - Attachment is obsolete: true
Attachment #8615299 - Flags: review?(karlt)
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.
Attachment #8614884 - Flags: review?(bas)
(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 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.
Attachment #8615299 - Flags: review?(karlt) → review?(lsalzman)
Updated with Lee's suggestions, only require translation and ignore aDrawSize clip.
Attachment #8615299 - Attachment is obsolete: true
Attachment #8615299 - Flags: review?(lsalzman)
Attachment #8616021 - Flags: review?(lsalzman)
Attachment #8616021 - Flags: review?(lsalzman) → review+
(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.
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?
(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.
This patch by Lee and I adds a ClipExporter that can be used to replay clips into the system cairo.
Attachment #8614884 - Attachment is obsolete: true
Attachment #8614884 - Flags: review?(bas)
Attachment #8616870 - Flags: review?(jmuizelaar)
Implements replaying thebes clips into the system cairo on GTK3.
Attachment #8616021 - Attachment is obsolete: true
Attachment #8616873 - Flags: review?(lsalzman)
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.
Attachment #8616870 - Flags: review?(jmuizelaar) → review+
Allocate SystemCairoClipper on the heap to make RefCounted base class happy.
Attachment #8616873 - Attachment is obsolete: true
Attachment #8616873 - Flags: review?(lsalzman)
Attachment #8616922 - Flags: review?(lsalzman)
Fix refcounting and device offsets.
Attachment #8616922 - Attachment is obsolete: true
Attachment #8616922 - Flags: review?(lsalzman)
Attachment #8616933 - Flags: review?(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.
Attachment #8616933 - Flags: review?(lsalzman) → review+
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.
Attachment #8616933 - Flags: review+ → review-
Thanks for the heads up! Carrying the r+ assuming no other issues.
Attachment #8616933 - Attachment is obsolete: true
Attachment #8616997 - Flags: review+
(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.
Status: NEW → ASSIGNED
Keywords: checkin-needed
can we get a try run here ?
Flags: needinfo?(acomminos)
Keywords: checkin-needed
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
Keywords: checkin-needed
Not sure what branch you rebased these on top of, but they still don't apply to inbound without conflicts. Both patches.
Keywords: checkin-needed
Sorry, something appears to be amiss with my git workflow. Tested, applies cleanly to inbound for me.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ff75c7e0b5d4
https://hg.mozilla.org/mozilla-central/rev/85ca98c22bdc
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
This patch break the build. I'm on Fedora 22 with gcc 5.
This changeset.

changeset:   248350:85ca98c22bdc
user:        Andrew Comminos <acomminos@mozilla.com>
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);
Break the build. see comment 34. Will provide a patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8621644 - Attachment description: Proposed fix. → Variable was re-declared causing a bustage on gcc 5.
Attachment #8621644 - Flags: review?(lsalzman) → review+
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.