As mentioned in bug 449292, comment 58, the harfbuzz API's for getting glyphs and glyph metrics, make a callback per-glyph and the current implementations go through all the fixed cost of per-character lookup multiple times. This applies to these functions, which each take a single codepoint instead of an array of codepoints. hb_font_get_glyph hb_font_get_glyph_metrics hb_font_get_kerning Below is the way the current patch on bug 449292 handles glyph metric lookup, the code in hb_position_default runs over characters in the hb_buffer and calls the metrics callback for each character: hb_position_default (hb_font_t *font, hb_face_t *face, hb_buffer_t *buffer, hb_feature_t *features HB_UNUSED, unsigned int num_features HB_UNUSED) { hb_buffer_clear_positions (buffer); unsigned int count = buffer->len; for (unsigned int i = 0; i < count; i++) { hb_glyph_metrics_t metrics; hb_font_get_glyph_metrics (font, face, buffer->info[i].codepoint, &metrics); buffer->pos[i].x_advance = metrics.x_advance; buffer->pos[i].y_advance = metrics.y_advance; } } void gfxHarfBuzzShaper::GetGlyphMetrics(gfxContext *aContext, hb_codepoint_t glyph, hb_glyph_metrics_t *metrics) const { if (mUseHintedWidths) { metrics->x_advance = mFont->GetHintedGlyphWidth(aContext, glyph); return; } // font did not implement GetHintedGlyphWidth, so get an unhinted value // directly from the font tables metrics->x_advance = metrics->y_advance = 0; metrics->x_offset = metrics->y_offset = 0; metrics->width = metrics->height = 0; if (mNumLongMetrics == 0) { mNumLongMetrics = -1; // read this from the hhea table without caching the blob hb_blob_t *hheaTable = mFont->GetFontTable(TRUETYPE_TAG('h','h','e','a')); if (hb_blob_get_length(hheaTable) >= sizeof(HMetricsHeader)) { const HMetricsHeader* hhea = reinterpret_cast<const HMetricsHeader*> (hb_blob_lock(hheaTable)); mNumLongMetrics = hhea->numberOfHMetrics; hb_blob_unlock(hheaTable); } hb_blob_destroy(hheaTable); mHmtxTable = mFont->GetFontTable(TRUETYPE_TAG('h','m','t','x')); } if (mNumLongMetrics < 1) { return; } if (glyph >= mNumLongMetrics) { glyph = mNumLongMetrics - 1; } if ((glyph + 1) * sizeof(HLongMetric) <= hb_blob_get_length(mHmtxTable)) { const HMetrics* hmtx = reinterpret_cast<const HMetrics*>(hb_blob_lock(mHmtxTable)); metrics->x_advance = FloatToFixed(mFont->FUnitsToDevUnitsFactor() * PRUint16(hmtx->metrics[glyph].advanceWidth)); hb_blob_unlock(mHmtxTable); } // TODO: set additional metrics if/when harfbuzz needs them } I would suggest that array-based versions of these routines would eliminate the need to do all the overhead involved with table lookups on a per-glyph basis.

The way it is supposed to work is that you cache whatever you want in the userdata argument of the font callbacks. void hb_font_set_funcs (hb_font_t *font, hb_font_funcs_t *klass, hb_destroy_func_t destroy, void *user_data); typedef void (*hb_font_get_glyph_metrics_func_t) (hb_font_t *font, hb_face_t *face, const void *user_data, hb_codepoint_t glyph, hb_glyph_metrics_t *metrics);

(In reply to comment #1) > The way it is supposed to work is that you cache whatever you want in the > userdata argument of the font callbacks. ...which in our case includes the gfxHarfBuzzShaper object, which caches the font tables it needs in order to look up metrics and glyph IDs. I'm not convinced there's an issue here; marshalling arrays of glyphs to pass to a "get multiple glyph metrics" callback would be just as expensive.

(In reply to comment #2) > (In reply to comment #1) > > The way it is supposed to work is that you cache whatever you want in the > > userdata argument of the font callbacks. > > ...which in our case includes the gfxHarfBuzzShaper object, which caches the > font tables it needs in order to look up metrics and glyph IDs. I'm not > convinced there's an issue here; marshalling arrays of glyphs to pass to a "get > multiple glyph metrics" callback would be just as expensive. Caching decisions are hard to make when there's no hint as to the number of times something will be used. Is the string 5 characters long or 5000? And is the lifetime of what's cached simply the lifetime of the call or the lifetime of the font? And I don't think there's an issue of marshalling arrays since the calling routines are already working with input/output arrays. After analyzing this carefully we may decide that there's not a problem here but until that's been done please leave the status alone.

What you are accessing there is a couple properties of the font, and the lifetime of the userdata is tied to the font, so you can simply cache whatever you want there. All I'm saying is that the harfbuzz API gives you all the flexibility you want already. The only case for adding an array-based callback api would be if the call overhead is an issue, which I don't think it would ever be. You wrote the bug as a harfbuzz feature request and that's why I closed it. Otherwise I typically don't touch status around this bugzilla.

(In reply to comment #3) > Caching decisions are hard to make when there's no hint as to the number of > times something will be used. Is the string 5 characters long or 5000? And is > the lifetime of what's cached simply the lifetime of the call or the lifetime > of the font? The shaper caches cmap and metrics tables for the lifetime of the gfxFont object. > And I don't think there's an issue of marshalling arrays since > the calling routines are already working with input/output arrays. But the natural API for a "GetGlyphIDs" callback would be to pass an array of Unicode characters, and get back an array of glyph IDs. That's not at all the same thing as the internal arrays in the buffer. (And it would be complicated further by variation-selector sequences.) A more likely way forward here would be to build cmap processing right into harfbuzz, so it would just call the GetTable callback once to get the table. That's certainly possible (though it means we'd be duplicating code/functionality within harfbuzz that we already have in thebes), but I don't think we should rush to do this... Similarly, for GetGlyphMetrics, we'd need to return an array of metrics records, from which harfbuzz would then have to extract the information it needs. I don't think having callbacks that operate directly on the arrays within the buffer would be a good design; much too fragile.

It's safe to say this should be closed.