[cairo] Comments on cairo-scaled-glyph-7.diff

[Owen Taylor]

On Wed, 2005-08-31 at 10:53 -0700, Keith Packard wrote:
> > 
> > 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/ 
> 
> Can't use cairo_ft_options_t as the new enum is already called that...
> So, we'll use cairo_ft_extra_flags_t unless we can think of a better
> name than that.

I was suggesting renaming both the new enum and the structure; 
repurposing the cairo_ft_options_t name for the structure.

> > 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.
> 
> The FreeType metrics for bitmaps do come right from the images, so they
> will match in fact, but as you say, there's no guarantee in the FreeType
> API about this.
>
> I think the right solution here is to create the surface based on the
> metrics and copy the image into that.

Sounds reasonable. If it doesn't land in the initial version, definitely
should be in bugzilla so we don't forget.

> > =====
> > +#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 :-)
> 
> Yeah, I'd like to figure out exactly which transformations are
> desired here; I can imagine that it would be nice to scale bitmaps
> when the desired size is far from the actual size.

That might be nice, but the current behavior was already useful and
well defined, and we hadn't gotten any complaints yet...

> > =====
> > -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...? 
> 
> I can recode these to use structure pointers if you'd like, but I do
> pass these around by value in several other places.

Not really advocating things one way or the other, just pointing
out the difference from what we do elsewhere. I've never examined
the generated code for passing structures by value. Might
be better if cairo_ft_flags_t was called something
that didn't sound like an enum, so it was at least obvious that
passing structures by value is going on.

> > =====
> > +    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".
> 
> Right, I've changed that to only set NO_BITMAP when INFO_PATH and !
> INFO_SURFACE. When INFO_PATH && INFO_SURFACE, the code already has to
> reload the glyph.

Is there an assumption here that you'll never have INFO_PATH and
INFO_METRICS at the same time?

> Updated patch (attached) available at
> 
> http://freedesktop.org/~keithp/cairo-scaled-glyph-8.diff
> 
> I've restored the -7.diff version (thanks fcrozat) and also generated a
> diff from the -7 version (which is attached below).
> 
> http://freedesktop.org/~keithp/cairo-scaled-glyph-7-8.diff

Generally looks good. A few whitespace things were discussed on
IRC.

Regards,
					Owen

-------------- 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/20050831/73d9afdd/attachment.pgp