[cairo] Comments on cairo-scaled-glyph-7.diff
Owen Taylor
otaylor at redhat.com
Mon Aug 29 15:30:40 PDT 2005
OK, I won't claim to have caught everything that could be commented
on (in a 6000+ line patch), but here's what I noticed going through it
pretty carefully. In general, it looks in pretty good shape; I don't
see anything in the overall structure that should cause problems
for the Win32 backend.
Regards,
Owen
====
+ * cairo_cache_entry_t:
+ *
+ * A #cairo_cache_entry_t contains both a key and a value for
+ * cairo_cache_t. User-derived types for cairo_cache_entry_t must
+ * be type-compatible with this structure (eg. they must have an
+ * unsigned long as the first parameter. The easiest way to get this
+ * is to use:
+ *
+ * typedef _my_entry {
+ * cairo_cache_entry_t base;
+ * ... Remainder of key and value fields here ..
+ * } my_entry_t;
====
This looks like a carryover from the cairo_hash_entry_t docs, since
we have two fields in cache_entry_t. I wouldn't make it optional, just
always require it.
====
+ /* We have to manually remove all entries from the cache ourselves
+ * rather than relying on _cairo_hash_table_destroy to do that
+ * since otherwise the cache->entry_destroy callback would not get
+ * called on each entry. */
+
+ while (1) {
+ entry = _cairo_hash_table_random_entry (cache->hash_table, NULL);
+ if (entry == NULL)
+ break;
+ _cairo_cache_remove (cache, entry);
+ }
====
Would it make sense to just push the entry_destroy function down into
the hash table? It's generally been pretty useful to have the equivalent
functionality in GHashTable.
====
+/**
+ * _cairo_cache_create:
+ * @keys_equal: a function to return TRUE if two keys are equal
====
gtk-doc style issue ... should be %TRUE to get proper markup.
Also applies to enums like %CAIRO_STATUS_SUCCESS.
====
+ * The units for max_size can be chosen by the caller, but should be
+ * consistent with the units of the size field of cache entries. When
+ * adding an entry with _cairo_cache_insert if the total size of
====
gtk-doc style issue, should be _cairo_cache_insert() for consistency
and to get a cross-reference (if we were actually generating docs
for the internal API. I'll skip pointing out other instances of these
two issues.
====
+/**
+ * _cairo_cache_preserve:
+ * @cache: a cache with some precious entries in it (or about to be
+ * added)
+ *
+ * Disable the automatic ejection of entries from the cache. Future
+ * calls to _cairo_cache_insert will add new entries to the cache
+ * regardless of how large the cache grows. See
+ * _cairo_cache_release().
+ **/
====
Generally, having an API like this without reference counting is an
invitation for subtle bugs where a function that preserve()'s calls
another function that does preserve() then release().
The names preserve() and release() aren't obviously paired to me.
The conventional name for this functionality in gtk-land is
freeze()/thaw(), or perhaps something explicit like
_cairo_cache_disable/enable_pruning()?
====
+/**
+ * _cairo_cache_release:
+ * @cache: a cache, just after the entries in it have become less
+ * previous
+ *
+ * Cancel the effects of _cairo_cache_preserve(). That is, allow the
+ * cache to resume ejecting entries when it is larger than max_size as
+ * passed to cairo_cache_create. If the cache is already larger than
+ * max_size, no entries will be immediately removed, but the cache
+ * will be brought down to size at the time of the next call to
+ * _cairo_cache_insert.
+ **/
====
Any particular reason why it doesn't free memory immediately? Just
simplicity of implementation?
====
+/**
+ * _cairo_cache_remove:
+ * @cache: a cache
+ * @entry: key of entry to be removed
+ *
+ * Remove an entry from the cache which has a key that matches @key,
====
@entry vs. @key
====
+ * if any (as determined by the keys_equal() function passed to
+ * _cairo_cache_create).
+ **/
+cairo_private void
+_cairo_cache_remove (cairo_cache_t *cache,
+ cairo_cache_entry_t *entry)
+{
+ cache->size -= entry->size;
+
+ _cairo_hash_table_remove (cache->hash_table,
+ (cairo_hash_entry_t *) entry);
+
+ if (cache->entry_destroy)
+ cache->entry_destroy (entry);
+}
=====
This doesn't behave as documented. As implemented entry has to actually
be in the cache and be an entry not a key.
====
+typedef enum _cairo_ft_options {
+ CAIRO_FT_OPTIONS_HINT_METRICS = (1 << 0),
+ CAIRO_FT_OPTIONS_EMBOLDEN = (1 << 1)
+} cairo_ft_options_t;
+
+typedef struct _cairo_ft_flags {
+ int load_flags; /* flags for FT_Load_Glyph */
+ cairo_ft_options_t options; /* other flags that affect results */
+} cairo_ft_flags_t;
====
Hmmm, maybe it's just me, but it seems strange to me to have a structure
called _flags that contains a bitfield called _flags, and another
bitfield
called _options.
s/cairo_ft_options_t/cairo_ft_extra_flags_t/
s/cairo_ft_flags_t/cairo_ft_options_t/
?
====
+_get_bitmap_surface (cairo_image_surface_t **surface,
+ FT_Bitmap *bitmap,
+ cairo_bool_t own_buffer,
+ cairo_font_options_t *font_options)
====
Pretty much everywhere in your patch, out parameters are last. Here
the out parameter is first.
=====
+ switch (font_options->antialias) {
+ case CAIRO_ANTIALIAS_NONE:
+ format = CAIRO_FORMAT_A1;
+ break;
+ case CAIRO_ANTIALIAS_SUBPIXEL:
format= CAIRO_FORMAT_ARGB32;
- else
+ break;
+ case CAIRO_ANTIALIAS_DEFAULT:
+ case CAIRO_ANTIALIAS_GRAY:
+ default:
format = CAIRO_FORMAT_A8;
+ break;
+ }
====
This feels like it should be split off into a utility function. There
is another copy of the same logic above embedded into a switch with
other
things.
====
+ case CAIRO_ANTIALIAS_DEFAULT:
+ case CAIRO_ANTIALIAS_GRAY:
+ bitmap.pixel_mode = FT_PIXEL_MODE_GRAY;
+ bitmap.num_grays = 256;
+ stride = (width + 3) & -4;
+ format = CAIRO_FORMAT_A8;
=====
Doesn't look like this setting of 'format' is used.
====
+ format = CAIRO_FORMAT_ARGB32;
+ FT_Outline_Transform (outline, &matrix);
====
Or this one.
====
- val->size.x = (short) (cbox.xMin >> 6);
- val->size.y = - (short) (cbox.yMax >> 6);
+ (*surface)->base.device_x_offset = (double) cbox.xMin / 64.0;
+ (*surface)->base.device_y_offset = -(double) cbox.yMax / 64.0;
=====
I believe this quantity is inherently integral. It's the position
of the upper left rendered pixel when the origin is at (0,0). As
such, I think it should be rounded here, not in callers, which
would obscure the logic.
I think there may be a problem with prerendered bitmaps
- The size of the image surface created comes from the size of the
FreeType bitmap.
- The bounding box that we use when computing the bounding box
for a set of glyphs comes from metrics that FreeType repots.
I don't think there is anything guaranteeing that these two
match, and a mismatch will cause misrendering with the unbounded
operators.
====
- _get_bitmap_surface (val, &glyphslot->bitmap, FALSE, FC_RGBA_NONE);
-
- val->size.x = glyphslot->bitmap_left;
- val->size.y = - glyphslot->bitmap_top;
+ _get_bitmap_surface (surface, &glyphslot->bitmap, FALSE,
font_options);
=====
The disappearance of references to glyphslot->bitmap_left,
glyphslot->bitmap_top
looks wrong to me.
=====
+#if 0
+/* XXX */
static cairo_status_t
_transform_glyph_bitmap (cairo_image_glyph_cache_entry_t *val)
{
======
Humph, and I was going to leave that function out until I got pressured
to
write it by certain poeple :-)
=====
-static int
-_get_pattern_load_flags (FcPattern *pattern)
+static cairo_ft_flags_t
+_get_pattern_ft_flags (FcPattern *pattern)
{
=====
We've uniformly avoided returning structures by value so far. Does
that mean that it's a bad thing...?
=====
+ if ((info & CAIRO_SCALED_GLYPH_INFO_SURFACE) == 0)
+ load_flags |= FT_LOAD_NO_BITMAP;
+
+ error = FT_Load_Glyph (scaled_font->unscaled->face,
+ _cairo_scaled_glyph_index(scaled_glyph),
+ load_flags);
=====
I don't think the logic of adding NO_BITMAP here is right. What this
is presumably trying to reproduce is the use of NO_BITMAP when
generating glyph paths in the old code. But the meaning of NO_BITMAP
isn't "don't generate a bitmap", but rather "avoid embedded bitmaps".
Now, from the discussion above about embedded bitmaps, there are
two possibilities for their effect on metrics:
- No effect. Then adding NO_BITMAP when computing just metrics is
pointless. (And the bounding box logic is wrong.)
- An effect. Then adding NO_BITMAP when computing just metrics is
harmful.
Maybe just call FT_Load_Glyph() again always for the outline case?
====
+ /*
+ * embolden glyhps if requested
+ */
====
Typo. (Was already in the old code)
=====
+ if (glyph_info->font->status)
+ return glyph_info->font->status;
====
Not sure what this is doing here deep inside cairo-gstate.c. It looks
fairly random. We should generally only ever be checking the status:
- In a setter (and if so, avoid setting)
- After a call that might set the status to something else
In general, we want to preserve the idea that only public API calls
that mutate state should ever set a status. Since scaled fonts are
inmutable, we should never have to deal with a scaled font already
set on the gstate ever getting a status.
====
+static cairo_bool_t
+_cairo_scaled_glyph_keys_equal (void *abstract_key_a, void
*abstract_key_b)
+{
+ cairo_scaled_glyph_t *key_a = abstract_key_a;
+ cairo_scaled_glyph_t *key_b = abstract_key_b;
+
+ return (_cairo_scaled_glyph_index(key_a) ==
+ _cairo_scaled_glyph_index(key_b));
====
Some missing spaces.
====
+void
+_cairo_scaled_font_set_error (cairo_scaled_font_t *scaled_font,
+ cairo_status_t status)
+{
+ scaled_font->status = status;
+
+ _cairo_error (status);
+}
====
Looks like a patch applied to cairo-font.c after you began your patch
got lost here. (It shouldn't set the status if it was already set)
====
+ scaled_font->glyphs = _cairo_cache_create
(_cairo_scaled_glyph_keys_equal,
+ _cairo_scaled_glyph_destroy,
+ 256);
====
It seems that using a fixed size here (and a fixed per-glyph size of 1)
is wrong... we likely want cache more glyphs from smaller fonts
=====
+cairo_scaled_font_t *
+cairo_scaled_font_reference (cairo_scaled_font_t *scaled_font)
+{
+ if (scaled_font == NULL)
+ return NULL;
+
+ if (scaled_font->ref_count == (unsigned int)-1)
+ return scaled_font;
=====
Another patch from cairo-font.c lost here (should have a comment about
refcounts.) Probably good to go through recent changes to cairo-font.c
and check that they are all in.
=====
+ if (scaled_font == NULL)
+ return;
+
+ if (scaled_font->ref_count == (unsigned int)-1)
+ return;
+
+ if (--(scaled_font->ref_count) > 0)
+ return;
=======
An assert in cairo-font.c is missing here. (Might be another after-fork
change.)
====
+/**
+ * cairo_scaled_font_extents:
+ * @scaled_font: a #cairo_scaled_font_t
+ * @extents: a #cairo_font_extents_t which to store the retrieved
extents.
+ *
+ * Gets the metrics for a #cairo_scaled_font_t.
+ **/
+void
+cairo_scaled_font_extents (cairo_scaled_font_t *scaled_font,
+ cairo_font_extents_t *extents)
+{
[...]
+ _cairo_matrix_compute_scale_factors (&scaled_font->font_matrix,
+ &font_scale_x, &font_scale_y,
+ /* XXX */ 1);
+
+ /*
+ * The font responded in unscaled units, scale by the font
+ * matrix scale factors to get to user space
+ */
+
+ extents->ascent *= font_scale_y;
+ extents->descent *= font_scale_y;
+ extents->height *= font_scale_y;
+ extents->max_x_advance *= font_scale_x;
+ extents->max_y_advance *= font_scale_y;
+}
Since scaled_font->extents is only ever used here, as far as I can see,
should scaled_font->extents just be in user space, rather than
recomputing?
====
+ left = x + _cairo_fixed_integer_floor(scaled_glyph->bbox.p1.x);
+ top = y + _cairo_fixed_integer_floor (scaled_glyph->bbox.p1.y);
+ right = x + _cairo_fixed_integer_ceil(scaled_glyph->bbox.p2.x);
+ bottom = y + _cairo_fixed_integer_ceil (scaled_glyph->bbox.p2.y);
====
Is bbox ever used for anything that isn't pixel aligned? If not, maybe
it
should be integer not fixed?
====
+ cairo_status_t status;
+ cairo_surface_t *mask = 0;
====
NULL, not 0.
====
+ int i;
+
+ if (scaled_font->status)
+ return scaled_font->status;
+
+ status = (*scaled_font->backend->
+ show_glyphs) (scaled_font, operator, pattern,
+ surface,
+ source_x, source_y,
+ dest_x, dest_y,
+ width, height,
+ glyphs, num_glyphs);
===
Probably should allow NULL here to mean "UNSUPPORTED". (I'm not seeeing
immediately how this doesn't segfault for the FT backend, but I'm sure
it doesn't.)
Cairo style for virtual functions is
status = scaled_font->backend->show_glyphs (scaled_font, operator,
pattern,
surface,
source_x, source_y,
dest_x, dest_y,
width, height,
glyphs, num_glyphs);
====
+ for (i = 0; i < num_glyphs; i++)
+ {
+ int x, y;
+ cairo_surface_pattern_t glyph_pattern;
+ cairo_image_surface_t *glyph_surface;
+ cairo_scaled_glyph_t *scaled_glyph;
+
+ status = _cairo_scaled_glyph_create (scaled_font,
+ glyphs[i].index,
+ &scaled_glyph,
+ CAIRO_SCALED_GLYPH_INFO_SURFACE);
=====
Don't you need to actually use _cairo_cache_preserve() here?
====
+ /* Create the mask using the format from the first glyph */
+ if (!mask) {
+ mask = cairo_image_surface_create (glyph_surface->format,
+ width, height);
+ if (!mask)
+ return CAIRO_STATUS_NO_MEMORY;
====
Check should be on mask->status.
===
+ status = _cairo_surface_fill_rectangle (mask,
+ CAIRO_OPERATOR_SOURCE,
+ CAIRO_COLOR_TRANSPARENT,
+ 0, 0,
+ width, height);
====
I recently went through cairo and changed everything to uniformly use
CLEAR/TRANSPARENT
rather than SOURCE/TRANSPARENT. (The TRANSPARENT in CLEAR/TRANSPARENT
doesn't
matter, of course)
=====
+ /* XXX surely this rounding is wrong */
+ x = (int) floor (glyphs[i].x +
+ glyph_surface->base.device_x_offset +
+ 0.5);
+ y = (int) floor (glyphs[i].y +
+ glyph_surface->base.device_y_offset +
+ 0.5);
====
Not so obvious to me, so if you are convinced that it is wrong, the XXX
should
go into detail.
====
+cairo_status_t
+_cairo_scaled_font_font_extents (cairo_scaled_font_t *scaled_font,
+ cairo_font_extents_t *extents)
+{
+ if (scaled_font->status)
+ return scaled_font->status;
+
+ *extents = scaled_font->extents;
+ return CAIRO_STATUS_SUCCESS;
+}
===
Don't think this one needs to be exported any more and don't see much of
a reason to have it at all.
===
+void
+_cairo_scaled_glyph_set_metrics (cairo_scaled_glyph_t *scaled_glyph,
+ cairo_scaled_font_t *scaled_font,
+ cairo_text_extents_t *fs_metrics)
===
This one definitely could use a doc comment. I've been trying to add
doc-comments for all intra-file functions recently, but this one is
less obvious than many.
===
+{
+ int first = 1;
====
Should be:
cairo_bool_t first = TRUE;
===
+ first = 0;
====
And first = FALSE.
====
+cairo_status_t
+_cairo_scaled_glyph_create (cairo_scaled_font_t *scaled_font,
+ unsigned long index,
+ cairo_scaled_glyph_t **scaled_glyph_ret,
+ cairo_scaled_glyph_info_t info)
======
This one also is in particular need of a doc comment. The name also
seems less than perfect to me - it certainly is unexpected for a
function that doesn't return ownership to the caller.
I might suggest _cairo_scaled_font_lookup_glyph().
Also, here the out-parameter ended up penultimate rather than last.
=====
+ /* ask backend to initialize metrics and shape fields */
+ status = (*scaled_font->backend->
+ scaled_glyph_init) (scaled_font, scaled_glyph, info);
+ if (status)
+ return status;
+ status = _cairo_cache_insert (scaled_font->glyphs,
+ &scaled_glyph->cache_entry);
+ if (status)
+ return status;
+ }
=====
Both of these returns leak, right?
====
+ /*
+ * Check and see if the glyph, as provided,
+ * already has the requested data and ammend it if not
+ */
+ need_info = CAIRO_SCALED_GLYPH_INFO_NONE;
+ if ((info & CAIRO_SCALED_GLYPH_INFO_SURFACE) &&
===
Is the cairo style (info & CAIRO_SCALED_GLYPH_INFO_SURFACE) != 0?
(Actually, the Cairo style is != NULL for pointers as well, but I'm not
very good at catching those, so I haven't pointed out instances
of that in this patch.)
===
+ scaled_glyph->surface == NULL)
+ need_info |= CAIRO_SCALED_GLYPH_INFO_SURFACE;
+ if (((info & CAIRO_SCALED_GLYPH_INFO_PATH) &&
+ scaled_glyph->path == NULL))
+ need_info |= CAIRO_SCALED_GLYPH_INFO_PATH;
+ if (need_info) {
====
Seems a little odd to use CAIRO_SCALED_GLYPH_INFO_NONE to mean
0, but then use (need_info) to mean
(need_info != CAIRO_SCALED_GLYPH_INFO_NONE).
====
+ if (font_private && scaled_glyph->surface_private != NULL) {
+ unsigned long glyph_index = _cairo_scaled_glyph_index(scaled_glyph);
====
OK, we aren't that strict about (foo != NULL) vs. (foo), but both
on the same line? :-)
====
+ glyph_info.x = -(int) glyph_surface->base.device_x_offset;
+ glyph_info.y = -(int) glyph_surface->base.device_y_offset;
====
See comments where this is set, but if we don't do the floor()
there, I think you need it here a cast, since it could be
positive or negative.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.freedesktop.org/archives/cairo/attachments/20050829/56ac3e85/attachment.pgp
More information about the cairo
mailing list