583115 - scrolling causes some pages (e.g. with fixed backgrounds) to smear/repeat

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Karl Tomlinson (:karlt)]

+++ This bug was initially created as a clone of Bug #579736 +++

Bug 579736 covers this issue for CONTENT_COLOR xlib surfaces.
This bug covers CONTENT_COLOR_ALPHA surfaces.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: +use XCopyArea for operator SOURCE with compatible COLOR_ALPHA surfaces

This is the change from https://bugs.freedesktop.org/show_bug.cgi?id=29250
and removal of an unused _operator_needs_alpha_composite() than is no longer in cairo git.

Comment 2

[Karsten Düsterloh]

Comment on attachment 461380 [details] [diff] [review]
+use XCopyArea for operator SOURCE with compatible COLOR_ALPHA surfaces

This patch fixes the smearing issue of bug 582147 for me, which attachment 458928 [details] [diff] [review] in bug 579736 did not.

Comment 3

[Hartmut Figge]

(In reply to comment #2)

> This patch fixes the smearing issue of bug 582147 for me, which attachment
> 458928 [details] in bug 579736 did not.

Whereas attachment 458928 [details] [diff] [review] helped for Bug 582147 and this one not.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Jeff and Joe are away today, making his review here impossible. If this is to make beta3 code freeze, we need another reviewer.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

I'm actually working today, and am starting the review now.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 461380 [details] [diff] [review]
+use XCopyArea for operator SOURCE with compatible COLOR_ALPHA surfaces

># HG changeset patch
># Parent 529bcadb24cc13cdd380b018ec08c58e0e87ff5c
># User Karl Tomlinson <[e-mail]>
>use XCopyArea for operator SOURCE with compatible COLOR_ALPHA surfaces
>
>diff --git a/gfx/cairo/README b/gfx/cairo/README
>--- a/gfx/cairo/README
>+++ b/gfx/cairo/README
>@@ -129,16 +129,18 @@ win32-transparent-surface.patch: add API
> cairo_qt_glyphs.patch: Drop X surface from Qt surface, add support for new qt glyphs api
> 
> empty-clip-rectangles.patch: f2fa15680ec3ac95cb68d4957557f06561a7dc55
> 
> empty-clip-extents.patch: b79ea8a6cab8bd28aebecf6e1e8229d5ac017264
> 
> clip-rects-surface-extents.patch: 108b1c7825116ed3f93aa57384bbd3290cdc9181
> 
>+copyarea-with-alpha.patch: support simple overlapping self copies in (some) color_alpha xlib surfaces. https://bugs.freedesktop.org/show_bug.cgi?id=29250
>+
> ==== pixman patches ====
> 
> pixman-android-cpu-detect.patch: Add CPU detection support for Android, where we can't reliably access /proc/self/auxv.
> 
> pixman-rename-and-endian.patch: include cairo-platform.h for renaming of external symbols and endian macros
> 
> NOTE: we previously supported ARM assembler on MSVC, this has been removed because of the maintenance burden
> 
>diff --git a/gfx/cairo/cairo/src/cairo-xlib-surface.c b/gfx/cairo/cairo/src/cairo-xlib-surface.c
>--- a/gfx/cairo/cairo/src/cairo-xlib-surface.c
>+++ b/gfx/cairo/cairo/src/cairo-xlib-surface.c
>@@ -1722,30 +1722,31 @@ _surface_has_alpha (cairo_xlib_surface_t
> 	else
> 	    return FALSE;
>     } else {
> 	/* In the no-render case, we never have alpha */
> 	return FALSE;
>     }
> }
> 
>-/* Returns true if the given operator and source-alpha combination
>- * requires alpha compositing to complete.
>+/* Returns true if the given operator and alpha combination requires alpha
>+ * compositing to complete on source and destination surfaces with the same
>+ * format.  i.e. if a simple bitwise copy is not appropriate.
>  */
> static cairo_bool_t
> _operator_needs_alpha_composite (cairo_operator_t op,
>-				 cairo_bool_t     destination_has_alpha,
>-				 cairo_bool_t     source_has_alpha)
>+				 cairo_bool_t     surfaces_have_alpha)
> {
>-    if (op == CAIRO_OPERATOR_SOURCE ||
>-	(! source_has_alpha &&
>-	 (op == CAIRO_OPERATOR_OVER ||
>-	  op == CAIRO_OPERATOR_ATOP ||
>-	  op == CAIRO_OPERATOR_IN)))
>-	return destination_has_alpha;
>+    if (op == CAIRO_OPERATOR_SOURCE)
>+	return FALSE;
>+
>+    if (op == CAIRO_OPERATOR_OVER ||
>+	op == CAIRO_OPERATOR_IN ||
>+	op == CAIRO_OPERATOR_ATOP)
>+	return surfaces_have_alpha;
> 
>     return TRUE;
> }
> 
> /* There is a bug in most older X servers with compositing using a
>  * untransformed repeating source pattern when the source is in off-screen
>  * video memory, and another with repeated transformed images using a
>  * general transform matrix. When these bugs could be triggered, we need a
>@@ -1846,21 +1847,19 @@ _recategorize_composite_operation (cairo
> 				   cairo_operator_t	       op,
> 				   cairo_xlib_surface_t	      *src,
> 				   cairo_surface_attributes_t *src_attr,
> 				   cairo_bool_t		       have_mask)
> {
>     /* Can we use the core protocol? */
>     if (! have_mask &&
>         src->owns_pixmap &&
>-	src->depth == dst->depth &&
>+	_surfaces_compatible (src, dst) &&
> 	_cairo_matrix_is_integer_translation (&src_attr->matrix, NULL, NULL) &&
>-	! _operator_needs_alpha_composite (op,
>-					   _surface_has_alpha (dst),
>-					   _surface_has_alpha (src)))
>+	! _operator_needs_alpha_composite (op, _surface_has_alpha (dst)))

It's not obvious that _surface_has_alpha (dst) is equivalent to surfaces_have_alpha.
Perhaps add a comment about why that is true.

>     {
> 	if (src_attr->extend == CAIRO_EXTEND_NONE)
> 	    return DO_XCOPYAREA;
> 
> 	if (dst->buggy_repeat && src_attr->extend == CAIRO_EXTEND_REPEAT)
> 	    return DO_XTILE;
>     }
> 
>@@ -2211,34 +2210,28 @@ _cairo_xlib_surface_composite (cairo_ope
>     cairo_surface_attributes_t	src_attr, mask_attr;
>     cairo_xlib_surface_t	*dst = abstract_dst;
>     cairo_xlib_surface_t	*src;
>     cairo_xlib_surface_t	*mask;
>     cairo_int_status_t		status;
>     composite_operation_t       operation;
>     int				itx, ity;
>     cairo_bool_t		is_integer_translation;
>-    cairo_bool_t		needs_alpha_composite;
>     GC				gc;
> 
>     if (mask_pattern != NULL && ! CAIRO_SURFACE_RENDER_HAS_COMPOSITE (dst))
> 	return UNSUPPORTED ("no support for masks");
> 
>     operation = _categorize_composite_operation (dst, op, src_pattern,
> 						 mask_pattern != NULL);
>     if (operation == DO_UNSUPPORTED)
> 	return UNSUPPORTED ("unsupported operation");
> 
>     X_DEBUG ((dst->dpy, "composite (dst=%x)", (unsigned int) dst->drawable));
> 
>-    needs_alpha_composite =
>-	_operator_needs_alpha_composite (op,
>-					 _surface_has_alpha (dst),
>-					 ! _cairo_pattern_is_opaque (src_pattern));
>-
>     _cairo_xlib_display_notify (dst->display);
> 
>     status =
> 	_cairo_xlib_surface_acquire_pattern_surfaces (dst,
> 						      src_pattern, mask_pattern,
> 						      src_x, src_y,
> 						      mask_x, mask_y,
> 						      width, height,
>diff --git a/gfx/cairo/copyarea-with-alpha.patch b/gfx/cairo/copyarea-with-alpha.patch
>new file mode 100644
>--- /dev/null
>+++ b/gfx/cairo/copyarea-with-alpha.patch
>@@ -0,0 +1,106 @@
>+use XCopyArea for operator SOURCE with compatible COLOR_ALPHA surfaces
>+
>+diff --git a/gfx/cairo/cairo/src/cairo-xlib-surface.c b/gfx/cairo/cairo/src/cairo-xlib-surface.c
>+--- a/gfx/cairo/cairo/src/cairo-xlib-surface.c
>++++ b/gfx/cairo/cairo/src/cairo-xlib-surface.c
>+@@ -1722,30 +1722,31 @@ _surface_has_alpha (cairo_xlib_surface_t
>+ 	else
>+ 	    return FALSE;
>+     } else {
>+ 	/* In the no-render case, we never have alpha */
>+ 	return FALSE;
>+     }
>+ }
>+ 
>+-/* Returns true if the given operator and source-alpha combination
>+- * requires alpha compositing to complete.
>++/* Returns true if the given operator and alpha combination requires alpha
>++ * compositing to complete on source and destination surfaces with the same
>++ * format.  i.e. if a simple bitwise copy is not appropriate.
>+  */
>+ static cairo_bool_t
>+ _operator_needs_alpha_composite (cairo_operator_t op,
>+-				 cairo_bool_t     destination_has_alpha,
>+-				 cairo_bool_t     source_has_alpha)
>++				 cairo_bool_t     surfaces_have_alpha)
>+ {
>+-    if (op == CAIRO_OPERATOR_SOURCE ||
>+-	(! source_has_alpha &&
>+-	 (op == CAIRO_OPERATOR_OVER ||
>+-	  op == CAIRO_OPERATOR_ATOP ||
>+-	  op == CAIRO_OPERATOR_IN)))
>+-	return destination_has_alpha;
>++    if (op == CAIRO_OPERATOR_SOURCE)
>++	return FALSE;
>++
>++    if (op == CAIRO_OPERATOR_OVER ||
>++	op == CAIRO_OPERATOR_IN ||
>++	op == CAIRO_OPERATOR_ATOP)
>++	return surfaces_have_alpha;
>+ 
>+     return TRUE;
>+ }
>+ 
>+ /* There is a bug in most older X servers with compositing using a
>+  * untransformed repeating source pattern when the source is in off-screen
>+  * video memory, and another with repeated transformed images using a
>+  * general transform matrix. When these bugs could be triggered, we need a
>+@@ -1846,21 +1847,19 @@ _recategorize_composite_operation (cairo
>+ 				   cairo_operator_t	       op,
>+ 				   cairo_xlib_surface_t	      *src,
>+ 				   cairo_surface_attributes_t *src_attr,
>+ 				   cairo_bool_t		       have_mask)
>+ {
>+     /* Can we use the core protocol? */
>+     if (! have_mask &&
>+         src->owns_pixmap &&
>+-	src->depth == dst->depth &&
>++	_surfaces_compatible (src, dst) &&
>+ 	_cairo_matrix_is_integer_translation (&src_attr->matrix, NULL, NULL) &&
>+-	! _operator_needs_alpha_composite (op,
>+-					   _surface_has_alpha (dst),
>+-					   _surface_has_alpha (src)))
>++	! _operator_needs_alpha_composite (op, _surface_has_alpha (dst)))
>+     {
>+ 	if (src_attr->extend == CAIRO_EXTEND_NONE)
>+ 	    return DO_XCOPYAREA;
>+ 
>+ 	if (dst->buggy_repeat && src_attr->extend == CAIRO_EXTEND_REPEAT)
>+ 	    return DO_XTILE;
>+     }
>+ 
>+@@ -2211,34 +2210,28 @@ _cairo_xlib_surface_composite (cairo_ope
>+     cairo_surface_attributes_t	src_attr, mask_attr;
>+     cairo_xlib_surface_t	*dst = abstract_dst;
>+     cairo_xlib_surface_t	*src;
>+     cairo_xlib_surface_t	*mask;
>+     cairo_int_status_t		status;
>+     composite_operation_t       operation;
>+     int				itx, ity;
>+     cairo_bool_t		is_integer_translation;
>+-    cairo_bool_t		needs_alpha_composite;
>+     GC				gc;
>+ 
>+     if (mask_pattern != NULL && ! CAIRO_SURFACE_RENDER_HAS_COMPOSITE (dst))
>+ 	return UNSUPPORTED ("no support for masks");
>+ 
>+     operation = _categorize_composite_operation (dst, op, src_pattern,
>+ 						 mask_pattern != NULL);
>+     if (operation == DO_UNSUPPORTED)
>+ 	return UNSUPPORTED ("unsupported operation");
>+ 
>+     X_DEBUG ((dst->dpy, "composite (dst=%x)", (unsigned int) dst->drawable));
>+ 
>+-    needs_alpha_composite =
>+-	_operator_needs_alpha_composite (op,
>+-					 _surface_has_alpha (dst),
>+-					 ! _cairo_pattern_is_opaque (src_pattern));
>+-
>+     _cairo_xlib_display_notify (dst->display);
>+ 
>+     status =
>+ 	_cairo_xlib_surface_acquire_pattern_surfaces (dst,
>+ 						      src_pattern, mask_pattern,
>+ 						      src_x, src_y,
>+ 						      mask_x, mask_y,
>+ 						      width, height,

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

(Thanks, Jeff!) Karl, can we get a fixed up patch landed ASAP, thanks!

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Is this going to land before the code freeze at 11pm tonight?

Comment 9

[Karl Tomlinson (:karlt)]

I don't know how to foretell the state of the tree, even 3 hours into the future, but I have a changeset ready to go.

Comment 10

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/1d2eda34572a