Closed Bug 583115 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Graphics, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b3
Tracking Status
blocking2.0 --- beta3+

People

(Reporter: karlt, Assigned: karlt)

References

()

Details

Attachments

(1 file)

+++ 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.
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.
Attachment #461380 - Flags: review?(jmuizelaar)
Summary: [QAC generated] scrolling causes some pages to smear/repeat → scrolling causes some pages to smear/repeat
Summary: scrolling causes some pages to smear/repeat → scrolling causes some pages (e.g. with fixed backgrounds) to smear/repeat
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.
(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.
Blocks: 583621
Jeff and Joe are away today, making his review here impossible. If this is to make beta3 code freeze, we need another reviewer.
I'm actually working today, and am starting the review now.
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 <karlt+@karlt.net>
>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,
Attachment #461380 - Flags: review?(jmuizelaar) → review+
(Thanks, Jeff!) Karl, can we get a fixed up patch landed ASAP, thanks!
No longer blocks: 583621
Is this going to land before the code freeze at 11pm tonight?
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.
http://hg.mozilla.org/mozilla-central/rev/1d2eda34572a
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b3
Depends on: 646611
You need to log in before you can comment on or make changes to this bug.