[cairo-commit] 4 commits - doc/public src/cairo-xlib-surface.c test/Makefile.am test/show-glyphs-many.c
Chris Wilson
ickle at kemper.freedesktop.org
Tue Sep 30 07:39:34 PDT 2008
doc/public/language-bindings.xml | 2
src/cairo-xlib-surface.c | 123 ++++++++++++++++++++++++---------------
test/Makefile.am | 15 ----
test/show-glyphs-many.c | 92 +++++++++++++++++++++++------
4 files changed, 153 insertions(+), 79 deletions(-)
New commits:
commit 0eb0c26474a19477554bfd580aa5f8ae77c29779
Author: Chris Wilson <chris at chris-wilson.co.uk>
Date: Tue Sep 30 13:33:25 2008 +0100
[xlib] Correct calculation of XRenderComposite* request size.
show-glyphs-many is triggering an assertion failure within xlib. The cause
appears to be that we are submitting an overlong request.
Reviewing the code and comparing with libXrender/src/Glyph.c I found two
points to address.
1. When encountering the first 2-byte, or 4-byte glyph and thus triggering
the recalculation of the current request_size, we did not check that there
was enough room for the expanded request. In case there is not, we need to
emit the current request (before expansion) and reset.
2. Subtleties in how XRenderCompositeText* constructs the binary protocol
buffer require that xGlyphElts are aligned to 32-bit boundaries and that
it inserts an additional xGlyphElt every 252 glyphs when width==1 or
every 254 glyphs for width==2 || width==4. Thus we need to explicitly
compute how many bytes would be required to add this glyph in accordance
with the above.
Considering the complexity (and apparent fragility since we require tight
coupling to XRender) of the code, I'm sure there are more bugs to be
found.
diff --git a/src/cairo-xlib-surface.c b/src/cairo-xlib-surface.c
index 3e126b7..9ce4a7c 100644
--- a/src/cairo-xlib-surface.c
+++ b/src/cairo-xlib-surface.c
@@ -3563,11 +3563,10 @@ _cairo_xlib_surface_emit_glyphs_chunk (cairo_xlib_surface_t *dst,
{
/* Which XRenderCompositeText function to use */
cairo_xrender_composite_text_func_t composite_text_func;
- int size;
/* Element buffer stuff */
- XGlyphElt8 *elts;
XGlyphElt8 stack_elts[CAIRO_STACK_ARRAY_LENGTH (XGlyphElt8)];
+ XGlyphElt8 *elts;
/* Reuse the input glyph array for output char generation */
char *char8 = (char *) glyphs;
@@ -3583,22 +3582,18 @@ _cairo_xlib_surface_emit_glyphs_chunk (cairo_xlib_surface_t *dst,
case 1:
/* don't cast the 8-variant, to catch possible mismatches */
composite_text_func = XRenderCompositeText8;
- size = sizeof (char);
break;
case 2:
composite_text_func = (cairo_xrender_composite_text_func_t) XRenderCompositeText16;
- size = sizeof (unsigned short);
break;
default:
case 4:
composite_text_func = (cairo_xrender_composite_text_func_t) XRenderCompositeText32;
- size = sizeof (unsigned int);
}
/* Allocate element array */
- if (num_elts <= ARRAY_LENGTH (stack_elts)) {
- elts = stack_elts;
- } else {
+ elts = stack_elts;
+ if (num_elts > ARRAY_LENGTH (stack_elts)) {
elts = _cairo_malloc_ab (num_elts, sizeof (XGlyphElt8));
if (elts == NULL)
return _cairo_error (CAIRO_STATUS_NO_MEMORY);
@@ -3609,16 +3604,14 @@ _cairo_xlib_surface_emit_glyphs_chunk (cairo_xlib_surface_t *dst,
n = 0;
j = 0;
for (i = 0; i < num_glyphs; i++) {
-
/* Start a new element for first output glyph, and for glyphs with
* unexpected position */
if (!j || glyphs[i].i.x || glyphs[i].i.y) {
if (j) {
- elts[nelt].nchars = n;
- nelt++;
+ elts[nelt++].nchars = n;
n = 0;
}
- elts[nelt].chars = char8 + size * j;
+ elts[nelt].chars = char8 + width * j;
elts[nelt].glyphset = glyphset_info->glyphset;
elts[nelt].xOff = glyphs[i].i.x;
elts[nelt].yOff = glyphs[i].i.y;
@@ -3635,11 +3628,9 @@ _cairo_xlib_surface_emit_glyphs_chunk (cairo_xlib_surface_t *dst,
j++;
}
- if (n) {
- elts[nelt].nchars = n;
- nelt++;
- n = 0;
- }
+ if (n)
+ elts[nelt++].nchars = n;
+ assert (nelt == num_elts);
composite_text_func (dst->dpy,
_render_operator (op),
@@ -3657,6 +3648,7 @@ _cairo_xlib_surface_emit_glyphs_chunk (cairo_xlib_surface_t *dst,
return CAIRO_STATUS_SUCCESS;
}
+#define PAD(sz, A) ((A)-((sz)&((A)-1)))
static cairo_status_t
_cairo_xlib_surface_emit_glyphs (cairo_xlib_surface_t *dst,
cairo_xlib_glyph_t *glyphs,
@@ -3676,7 +3668,7 @@ _cairo_xlib_surface_emit_glyphs (cairo_xlib_surface_t *dst,
unsigned long max_index = 0;
int width = 1;
int num_elts = 0;
- int num_out_glyphs = 0;
+ int num_elt_glyphs = 0;
int max_request_size = XMaxRequestSize (dst->dpy) * 4
- MAX (sz_xRenderCompositeGlyphs8Req,
@@ -3689,7 +3681,7 @@ _cairo_xlib_surface_emit_glyphs (cairo_xlib_surface_t *dst,
for (i = 0; i < num_glyphs; i++) {
int this_x, this_y;
- int old_width;
+ int old_width, this_rq_size;
status = _cairo_scaled_glyph_lookup (scaled_font,
glyphs[i].index,
@@ -3751,13 +3743,48 @@ _cairo_xlib_surface_emit_glyphs (cairo_xlib_surface_t *dst,
width = 4;
else if (max_index >= 256)
width = 2;
- if (width != old_width)
- request_size += (width - old_width) * num_out_glyphs;
+ if (width != old_width) {
+ int expand = (width - old_width) * i;
+
+ /* Check to see if we have enough room within this request
+ * to convert all the current glyphs to the new width.
+ */
+ if (request_size + expand > max_request_size) {
+ status = _cairo_xlib_surface_emit_glyphs_chunk (dst, glyphs, i,
+ scaled_font, op, src, attributes,
+ num_elts, old_width, glyphset_info);
+ if (status)
+ return status;
+
+ glyphs += i;
+ num_glyphs -= i;
+ i = 0;
+
+ request_size = sz_xGlyphElt + width;
+ num_elts = 1;
+ num_elt_glyphs = 1;
+
+ x = this_x + scaled_glyph->x_advance;
+ y = this_y + scaled_glyph->y_advance;
+ glyphset_info = this_glyphset_info;
+
+ glyphs[i].i.x = 0;
+ glyphs[i].i.y = 0;
+ continue;
+ } else
+ request_size += expand;
+ }
}
+ this_rq_size = width;
+ /* If the glyph is in an unexpected position, we need a new GlyphElt.
+ * And libXrender inserts a new GlyphElt (at most) every 252 glyphs.
+ */
+ if ((num_elt_glyphs % 252) == 0 || this_x - x || this_y - y)
+ this_rq_size += PAD (request_size, 4) + sz_xGlyphElt;
+
/* If we will pass the max request size by adding this glyph,
- * flush current glyphs. Note that we account for a
- * possible element being added below.
+ * flush current glyphs.
*
* Also flush if changing glyphsets, as Xrender limits one mask
* format per request, so we can either break up, or use a
@@ -3766,44 +3793,50 @@ _cairo_xlib_surface_emit_glyphs (cairo_xlib_surface_t *dst,
* to the mask first, and then composes that to final surface,
* though it's not a big deal.
*/
- if (request_size + width > max_request_size - sz_xGlyphElt ||
- (this_glyphset_info != glyphset_info)) {
+ if (request_size + this_rq_size > max_request_size ||
+ this_glyphset_info != glyphset_info)
+ {
status = _cairo_xlib_surface_emit_glyphs_chunk (dst, glyphs, i,
scaled_font, op, src, attributes,
num_elts, old_width, glyphset_info);
- if (status != CAIRO_STATUS_SUCCESS)
+ if (status)
return status;
glyphs += i;
num_glyphs -= i;
i = 0;
+
max_index = glyphs[i].index;
width = max_index < 256 ? 1 : max_index < 65536 ? 2 : 4;
- request_size = 0;
- num_elts = 0;
- num_out_glyphs = 0;
- x = y = 0;
- glyphset_info = this_glyphset_info;
- }
- /* Convert absolute glyph position to relative-to-current-point
- * position */
- glyphs[i].i.x = this_x - x;
- glyphs[i].i.y = this_y - y;
+ request_size = width + sz_xGlyphElt;
+ num_elts = 1;
+ num_elt_glyphs = 1;
- /* Start a new element for the first glyph, or for any glyph that
- * has unexpected position */
- if (!num_out_glyphs || glyphs[i].i.x || glyphs[i].i.y) {
- num_elts++;
- request_size += sz_xGlyphElt;
+ x = this_x;
+ y = this_y;
+ glyphset_info = this_glyphset_info;
+
+ glyphs[i].i.x = 0;
+ glyphs[i].i.y = 0;
+ } else {
+ request_size += this_rq_size;
+
+ /* Convert absolute glyph position to relative-to-current-point. */
+ glyphs[i].i.x = this_x - x;
+ glyphs[i].i.y = this_y - y;
+
+ /* Start a new element for any glyph in an unexpected position. */
+ if (num_elt_glyphs == 0 || glyphs[i].i.x || glyphs[i].i.y) {
+ num_elts++;
+ num_elt_glyphs = 1;
+ } else
+ num_elt_glyphs++;
}
- /* adjust current-position */
+ /* Adjust current-point. */
x = this_x + scaled_glyph->x_advance;
y = this_y + scaled_glyph->y_advance;
-
- num_out_glyphs++;
- request_size += width;
}
if (num_elts)
commit 02a56a4c84cd07a2c33134974680bad7f17f733d
Author: Chris Wilson <chris at chris-wilson.co.uk>
Date: Tue Sep 30 15:08:54 2008 +0100
[test/show-glyphs-many] Exercise xlib boundary conditions.
Within _cairo_xlib_surface_emit_glyphs() there are a number of
complications to do with packing as many glyphs as possible into a
single XRenderCompositeGlyph*() call. Essentially these consist of
choosing the right function and packing for the current glyphs, describing
runs of glyphs and ensuring that we do not exceed the maximum request size
within a single call. So we add to the test case we an attempt to show 64k
2-byte glyphs and an attempt to mix 64k 1-byte and 2-byte glyphs, with the
change-over point chosen to overflow the maximum request size, should
_cairo_xlib_surface_emit_glyphs() naively resize the current request.
diff --git a/test/show-glyphs-many.c b/test/show-glyphs-many.c
index 3af73f3..2231517 100644
--- a/test/show-glyphs-many.c
+++ b/test/show-glyphs-many.c
@@ -79,45 +79,90 @@
#define TEXT_SIZE 12
#define NUM_GLYPHS 65535
-/* This is the index into the font for what glyph we'll draw. Since we
- * don't guarantee we'll get any particular font, we can't relibably
- * get any particular glyph. But we don't care what we draw anyway,
- * (see discussion of white-on-white drawing above). For what it's
- * worth, this appears to be giving me 'M' with Bitstream Vera
- * Sans Mono. */
-#define GLYPH_INDEX 48
-
static cairo_test_draw_function_t draw;
-cairo_test_t test = {
+static const cairo_test_t test = {
"show-glyphs-many",
- "Test that cairo_show_glyps works when handed 'many' glyphs",
+ "Test that cairo_show_glyphs works when handed 'many' glyphs",
9, 11,
draw
};
static cairo_test_status_t
-draw (cairo_t *cr, int width, int height)
+get_glyph (cairo_t *cr, const char *utf8, cairo_glyph_t *glyph)
{
- cairo_glyph_t glyphs[NUM_GLYPHS];
+ cairo_glyph_t *text_to_glyphs;
+ cairo_status_t status;
int i;
- /* Initialize our giant array of glyphs. */
- for (i=0; i < NUM_GLYPHS; i++) {
- glyphs[i].index = GLYPH_INDEX;
- glyphs[i].x = 1.0;
- glyphs[i].y = height - 1;
+ text_to_glyphs = glyph;
+ i = 1;
+ status = cairo_scaled_font_text_to_glyphs (cairo_get_scaled_font (cr),
+ 0, 0,
+ utf8, -1,
+ &text_to_glyphs, &i,
+ NULL, NULL,
+ 0);
+ if (status != CAIRO_STATUS_SUCCESS)
+ return status;
+
+ if (text_to_glyphs != glyph) {
+ *glyph = text_to_glyphs[0];
+ cairo_glyph_free (text_to_glyphs);
}
+ return CAIRO_STATUS_SUCCESS;
+}
+
+static cairo_test_status_t
+draw (cairo_t *cr, int width, int height)
+{
+ cairo_glyph_t glyphs[NUM_GLYPHS];
+ const char *characters[] = { /* try to exercise different widths of index */
+ "m", /* Latin letter m, index=0x50 */
+ "μ", /* Greek letter mu, index=0x349 */
+ NULL,
+ }, **utf8;
+ int i, j;
+ cairo_status_t status;
+
/* Paint white background. */
- cairo_set_source_rgb (cr, 1.0, 1.0, 1.0); /* white */
+ cairo_set_source_rgb (cr, 1, 1, 1);
cairo_paint (cr);
- cairo_select_font_face (cr, "Bitstream Vera Sans Mono",
+ cairo_select_font_face (cr, "Sans",
CAIRO_FONT_SLANT_NORMAL,
CAIRO_FONT_WEIGHT_NORMAL);
cairo_set_font_size (cr, TEXT_SIZE);
+ for (utf8 = characters; *utf8 != NULL; utf8++) {
+ status = get_glyph (cr, *utf8, &glyphs[0]);
+ if (status)
+ return CAIRO_TEST_FAILURE;
+
+ if (glyphs[0].index) {
+ glyphs[0].x = 1.0;
+ glyphs[0].y = height - 1;
+ for (i=1; i < NUM_GLYPHS; i++)
+ glyphs[i] = glyphs[0];
+
+ cairo_show_glyphs (cr, glyphs, NUM_GLYPHS);
+ }
+ }
+
+ /* we can pack ~21k 1-byte glyphs into a single XRenderCompositeGlyphs8 */
+ status = get_glyph (cr, "m", &glyphs[0]);
+ if (status)
+ return CAIRO_TEST_FAILURE;
+ for (i=1; i < 21500; i++)
+ glyphs[i] = glyphs[0];
+ /* so check expanding the current 1-byte request for 2-byte glyphs */
+ status = get_glyph (cr, "μ", &glyphs[i]);
+ if (status)
+ return CAIRO_TEST_FAILURE;
+ for (j=i+1; j < NUM_GLYPHS; j++)
+ glyphs[j] = glyphs[i];
+
cairo_show_glyphs (cr, glyphs, NUM_GLYPHS);
return CAIRO_TEST_SUCCESS;
commit 2a347a92b0a27a42840f9538cb98f792be12b277
Author: Chris Wilson <chris at chris-wilson.co.uk>
Date: Tue Sep 30 13:29:35 2008 +0100
[test/show-glyphs-many] Re-enable test to trigger crash in xlib.
Moral of this story is bugs cluster. If we made a mistake, especially in a
complicated bit of code that is interfacing with another library, then we
are likely to make a similar mistake in future. Disabling this test hid a
regression between 1.4 and 1.6.
diff --git a/test/Makefile.am b/test/Makefile.am
index f8db937..d5c0f43 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -141,6 +141,7 @@ self-copy$(EXEEXT) \
self-copy-overlap$(EXEEXT) \
self-intersecting$(EXEEXT) \
set-source$(EXEEXT) \
+show-glyphs-many$(EXEEXT) \
show-text-current-point$(EXEEXT) \
skew-extreme$(EXEEXT) \
smask$(EXEEXT) \
@@ -197,19 +198,6 @@ zero-alpha$(EXEEXT)
# extend-reflect - Triggers an infinite (or near-infinite) bug in some
# X servers in some circumstances. Details and cause unknown.
#
-# show-glyphs-many - This stress test was exercising a particular bug
-# in the xlib surface code (exceeding the X11 protocol request
-# limit) when rendering several thousand glyphs at once. The
-# original xlib-surface bug is fixed now, but the test continues
-# to stress some other aspects of the test suite. For example,
-# when doing text as paths, the resuilting PostScript file is one
-# giant path that ghostscript has a particularly hard time
-# with. I'm disabling this test for now, since I don't care about
-# that performance problem in ghostscript. (But, there is a
-# similar performance problem when using cairo to rasterize the
-# equivalen giant path---from an SVG files say---so this might be
-# a useful kind of test to bring back again for performance (not
-# correctness) testing.
# text-glyph-range - This test triggers the following assertion in cairo:
#
# lt-text-glyph-range: cairo-scaled-font-subsets.c:350:
@@ -221,7 +209,6 @@ zero-alpha$(EXEEXT)
# that's just a bug in the test rig that should just consider
# the abort an XFAIL like any other.
DISABLED_TESTS = \
-show-glyphs-many$(EXEEXT) \
text-glyph-range$(EXEEXT)
# Then we have a collection of tests that are only run if certain
diff --git a/test/show-glyphs-many.c b/test/show-glyphs-many.c
index 9931a25..3af73f3 100644
--- a/test/show-glyphs-many.c
+++ b/test/show-glyphs-many.c
@@ -65,6 +65,15 @@
*
* Status: I replicated bug. The largest value of NUM_GLYPHS for
* which I saw success is 21842.
+ *
+ * 2008-30-08 Chris Wilson <chris at chris-wilson.co.uk>
+ * This is also a valid test case for:
+ *
+ * Bug 5913 crash on overlong string
+ * https://bugs.freedesktop.org/show_bug.cgi?id=5913
+ *
+ * which is still causing a crash in the Xlib backend - presumably, just
+ * a miscalculation of the length of the available request.
*/
#define TEXT_SIZE 12
commit fade54e71a48919cc3e8311e202960c66eab74bf
Author: Torsten Schönfeld <kaffeetisch at gmx.de>
Date: Tue Sep 30 11:47:53 2008 +0100
[doc] Add links to solid pattern constructors.
Add links for the solid pattern constructors to the language bindings
guidelines in line with the other pattern types.
diff --git a/doc/public/language-bindings.xml b/doc/public/language-bindings.xml
index 323fb45..8d98b56 100644
--- a/doc/public/language-bindings.xml
+++ b/doc/public/language-bindings.xml
@@ -513,7 +513,7 @@ CAIRO_STATUS_WRITE_ERROR
cairo_pattern_t
<link linkend="cairo-pattern-set-matrix"><function>cairo_pattern_set_matrix()</function></link>
<link linkend="cairo-pattern-get-matrix"><function>cairo_pattern_get_matrix()</function></link>
- cairo_solid_pattern_t
+ cairo_solid_pattern_t (<link linkend="cairo-pattern-create-rgb"><function>cairo_pattern_create_rgb()</function></link> and <link linkend="cairo-pattern-create-rgba"><function>cairo_pattern_create_rgba()</function></link>)
cairo_surface_pattern_t (<link linkend="cairo-pattern-create-for-surface"><function>cairo_pattern_create_for_surface()</function></link>)
<link linkend="cairo-pattern-set-extend"><function>cairo_pattern_set_extend()</function></link>
<link linkend="cairo-pattern-get-extend"><function>cairo_pattern_get_extend()</function></link>
More information about the cairo-commit
mailing list