[cairo] Re: [PATCH] PS/PDF improve efficiency of text output
Behdad Esfahbod
behdad at behdad.org
Mon Oct 23 12:12:35 PDT 2006
On Mon, 2006-10-23 at 22:33 +0930, Adrian Johnson wrote:
> Behdad Esfahbod wrote:
> > Hi Adrian,
> >
> > I was making Firefox print using Pango and when looking at your
> > xshow/yshow/xyshow patch I think I've found a minor bug. Namely, the
> > advance vector passed to xshow/yshow should be the same length as the
> > glyph vector, but your code generates one that is one entry shorter.
> > I'm referring to this patch:
> >
> > http://lists.freedesktop.org/archives/cairo/2006-September/007930.html
> >
> > Of course for positioning purposes n-1 entries are enough to position n
> > glyphs, but the n'th entry is needed to correctly advance the current
> > position and while ghostscript doesn't warn or err about it, the HP
> > Laserjet printer I was testing with refused to print any such
> > PostScript. Adding a final 0 (and two zeros in case of xyshow) should
> > fix it. Or you can be smarter and use this feature to eliminate the
> > moveto when switching between subfonts.
> >
>
> Obviously I didn't test this patch on my LaserJet. The problem with
> trying to save paper by using ghostscript is that, as you found out, it
> tolerates some types of malformed PostScript.
It helps if ghostscript was more verbose about non-standard input.
> The attached patch fixes the bug, includes the moveto optimization you
> suggested, and ensures the status of the word wrap stream is checked
> when destroyed.
Looks good. Minor comments follow, but other than that I think you
should go ahead and push this. Better commit a not-perfect-yet patch
and get testing rather than letting it rot.
> diff --git a/src/cairo-ps-surface.c b/src/cairo-ps-surface.c
> index cc95f2e..df319ac 100644
> --- a/src/cairo-ps-surface.c
> +++ b/src/cairo-ps-surface.c
> @@ -2090,6 +2090,15 @@ _cairo_ps_surface_fill (void *abstract_
> return status;
> }
>
> +/* This size keeps the length of the hex encoded string of glyphs
> + * within 80 columns. */
> +#define MAX_GLYPHS_PER_SHOW 36
I'm not sure I understand why this is needed. Can't a hex encoded
string be wrapped? It works with ghostscript at least.
> +typedef struct _cairo_ps_glyph_id {
> + unsigned int subset_id;
> + unsigned int glyph_id;
> +} cairo_ps_glyph_id_t;
> +
> static cairo_int_status_t
> _cairo_ps_surface_show_glyphs (void *abstract_surface,
> cairo_operator_t op,
> @@ -2101,9 +2110,12 @@ _cairo_ps_surface_show_glyphs (void
> cairo_ps_surface_t *surface = abstract_surface;
> cairo_output_stream_t *stream = surface->stream;
> unsigned int current_subset_id = -1;
> - unsigned int font_id, subset_id, subset_glyph_index;
> + unsigned int font_id;
> + cairo_ps_glyph_id_t *glyph_ids;
> cairo_status_t status;
> - int i;
> + int i, j, last, end;
> + cairo_bool_t vertical, horizontal;
> + cairo_output_stream_t *word_wrap;
>
> if (surface->paginated_mode == CAIRO_PAGINATED_MODE_ANALYZE)
> return _analyze_operation (surface, op, source);
> @@ -2113,37 +2125,113 @@ _cairo_ps_surface_show_glyphs (void
> _cairo_output_stream_printf (stream,
> "%% _cairo_ps_surface_show_glyphs\n");
>
> - if (num_glyphs)
> - emit_pattern (surface, source, 0, 0);
> + if (num_glyphs == 0)
> + return CAIRO_STATUS_SUCCESS;
> +
> + emit_pattern (surface, source, 0, 0);
> + glyph_ids = malloc (num_glyphs*sizeof(cairo_ps_glyph_id_t));
> + if (glyph_ids == NULL)
> + return CAIRO_STATUS_NO_MEMORY;
>
> for (i = 0; i < num_glyphs; i++) {
> status = _cairo_scaled_font_subsets_map_glyph (surface->font_subsets,
> scaled_font, glyphs[i].index,
> - &font_id, &subset_id, &subset_glyph_index);
> + &font_id,
> + &(glyph_ids[i].subset_id),
> + &(glyph_ids[i].glyph_id));
> if (status)
> - return status;
> + goto fail;
> + }
>
> - if (subset_id != current_subset_id) {
> + i = 0;
> + while (i < num_glyphs) {
> + if (glyph_ids[i].subset_id != current_subset_id) {
> _cairo_output_stream_printf (surface->stream,
> "/CairoFont-%d-%d findfont\n"
> "[ %f %f %f %f 0 0 ] makefont\n"
> "setfont\n",
> font_id,
> - subset_id,
> + glyph_ids[i].subset_id,
> scaled_font->scale.xx,
> scaled_font->scale.yx,
> -scaled_font->scale.xy,
> -scaled_font->scale.yy);
> - current_subset_id = subset_id;
> + current_subset_id = glyph_ids[i].subset_id;
> }
>
> - _cairo_output_stream_printf (surface->stream,
> - "%f %f M <%02x> S\n",
> - glyphs[i].x, glyphs[i].y,
> - subset_glyph_index);
> + if (i == 0)
> + _cairo_output_stream_printf (stream,
> + "%f %f M\n",
> + glyphs[i].x,
> + glyphs[i].y);
This should be simply moved out of the loop.
> + horizontal = TRUE;
> + vertical = TRUE;
> + end = num_glyphs;
> + if (end - i > MAX_GLYPHS_PER_SHOW)
> + end = i + MAX_GLYPHS_PER_SHOW;
> + last = end - 1;
> + for (j = i; j < end - 1; j++) {
> + if ((glyphs[j].y != glyphs[j + 1].y))
> + horizontal = FALSE;
> + if ((glyphs[j].x != glyphs[j + 1].x))
> + vertical = FALSE;
> + if (glyph_ids[j].subset_id != glyph_ids[j + 1].subset_id) {
> + last = j;
> + break;
> + }
> + }
> +
> + if (i == last) {
> + _cairo_output_stream_printf (surface->stream, "<%02x> S\n", glyph_ids[i].glyph_id);
> + } else {
> + word_wrap = _word_wrap_stream_create (surface->stream, 79);
> + _cairo_output_stream_printf (word_wrap, "<");
> + for (j = i; j <= last; j++)
> + _cairo_output_stream_printf (word_wrap, "%02x", glyph_ids[j].glyph_id);
> + _cairo_output_stream_printf (word_wrap, ">\n[");
> +
> + for (j = i + 1; j <= last + 1; j++) {
This will be particularly more understandable if written as (j = i; j <=
last; j++).
> + if (j == num_glyphs) {
> + if (horizontal || vertical)
> + _cairo_output_stream_printf (word_wrap, "0 ");
> + else
> + _cairo_output_stream_printf (word_wrap, "0 0 ");
> + } else {
> + if (horizontal) {
> + _cairo_output_stream_printf (word_wrap,
> + "%f ", glyphs[j].x - glyphs[j - 1].x);
> + } else if (vertical) {
> + _cairo_output_stream_printf (word_wrap,
> + "%f ", glyphs[j].y - glyphs[j - 1].y);
> + } else {
> + _cairo_output_stream_printf (word_wrap,
> + "%f %f ",
> + glyphs[j].x - glyphs[j - 1].x,
> + glyphs[j].y - glyphs[j - 1].y);
>
> + }
> + }
> + }
> + if (horizontal)
> + _cairo_output_stream_printf (word_wrap, "] xshow\n");
> + else if (vertical)
> + _cairo_output_stream_printf (word_wrap, "] yshow\n");
> + else
> + _cairo_output_stream_printf (word_wrap, "] xyshow\n");
The above can also be written as a single "if else else" for the three
different cases and loops inside each. That would be slightly faster
and cleaner. Doesn't really matter though.
> + status = _cairo_output_stream_destroy (word_wrap);
> + if (status)
> + goto fail;
> + }
> + i = last + 1;
> }
>
> + free (glyph_ids);
> return _cairo_output_stream_get_status (surface->stream);
Replace these two lines with:
status = _cairo_output_stream_get_status (surface->stream);
> fail:
> + free (glyph_ids);
> + return status;
> }
>
> static void
Thanks!
--
behdad
http://behdad.org/
"Commandment Three says Do Not Kill, Amendment Two says Blood Will Spill"
-- Dan Bern, "New American Language"
More information about the cairo
mailing list