GNOME.org
 

Re: HarfBuzz API design

On 08/19/2009 02:45 PM, Carl Worth wrote:

Excerpts from Behdad Esfahbod's message of Tue Aug 18 16:23:50 -0700 2009:

[Warning: long email ahead]


My reply might bounce to unsubscribed-from lists. Feel free to forward
if you think there's anything important in here for others to read.


Thanks Carl.


_reference(), _destory(), and _get_reference_count().  At some point we may
want to add _[gs]et_user_data() also which is useful for language bindings.


That should be "destroy" not "destory", of course. I normally wouldn't
point out a typo in an email message, but I did see the same error in
a comment that might be copied from actual code:


Ouch.  Fixed the typo.



/* calls destory() when not needing user_data anymore */
hb_face_t *
hb_face_create_for_tables (hb_get_table_func_t  get_table,
                             hb_destroy_func_t    destroy,
                             void                *user_data);


So don't forget to spell-check your header files. :-)


Yeah, postponed that to when I add actual docs.
Humm, seems like s/writeable/writable/g is also needed. Stupid me. The geek inside me prefers writeable though. 



typedef enum {
    HB_MEMORY_MODE_DUPLICATE,
    HB_MEMORY_MODE_READONLY,
    HB_MEMORY_MODE_WRITEABLE,
    HB_MEMORY_MODE_READONLY_NEVER_DUPLICATE,
    HB_MEMORY_MODE_READONLY_MAY_MAKE_WRITEABLE,
} hb_memory_mode_t;

...

    DUPLICATE: copy data right away and own it.

    READONLY: the data passed in can be kept for later use, but should not be
modified.  If modification is needed, the blob will duplicate the data lazily.

    WRITEABLE: data is writeable, use it freely.

    READONLY_NEVER_DUPLICATE: data is readonly and should never be duplicated.
   This disables operations needing write access to data.

    READONLY_MAY_MAKE_WRITEABLE: data is readonly but may be made writeable
using mprotect() or equivalent win32 calls.  It's up to the user to make sure
calling mprotect() or system-specific equivalents on the data is safe.  In
practice, that's never an issue on Linux and (according to Tor) on win32.


I don't think these names are quite right yet.
That's definitely one of the most arbitrary parts of the code, thanks for catching! I do think there's a slight confusion though, which if I clarify (as the docs will eventually will), the values make more sense. Look at it this way, the mode parameter describes the characteristics of the input data only. It's not a property of the blob itself, and for that reason there is no getter for it. 

Looking at it that way, these values are accurate:

>>     HB_MEMORY_MODE_DUPLICATE,
>>     HB_MEMORY_MODE_READONLY,
>>     HB_MEMORY_MODE_WRITEABLE,
>>     HB_MEMORY_MODE_READONLY_MAY_MAKE_WRITEABLE,

This one, not so much:

>>     HB_MEMORY_MODE_READONLY_NEVER_DUPLICATE,
As it also describe a behavior of the blob itself. That value however is for very corner case uses (when you're just interested in knowing whether the data is sane and you want to avoid the cost of copying). 



I think the thing that strikes me first as wrong is that if you create
a READONLY blob then it's still perfectly valid to write to it, (such
that you have to have a separate READONLY_NEVER_DUPLICATE for
"readonly---and I mean it).


More clear in the above context?
I also did some Hoffman encoding here. I initially had READONLY and READONLY_MAY_DUPLICATE, but figured that READONLY_MAY_DUPLICATE is what I want most users to use, and one should think twice before using READONLY. So I gave the short name to the common one and made the corner case use a longer name. 



One difficulty is that you're capturing separate notions here,
(whether the data buffer is writeable vs. whether the created blob
should be writeable).


Yes and no indeed.



And there's some missing orthogonality too. For example, if I have
data that I don't want the blob to reference, (hence DUPLICATE), might
I not still want to create a blob in which all operations requiring
write access are disabled? That combination is not possible to express
from what I can see.
Initially I had that design in mind, with separate writeable. But I figured the extra control is not needed, no. 



So, here's pseudocode trying to capture the decision-tree for choosing
one of the modes currently:

if (the data cannot be referenced by the blob)
	DUPLICATE
else if (the data buffer can be written to)
	if (writing requires calling mprotect)
		READONLY_MAY_MAKE_WRITEABLE
	else
		WRITEABLE
else if (the resulting blob should be readonly)
	READONLY_NEVER_DUPLICATE
else
	READONLY;


Again, lets forget about READONLY_NEVER_DUPLICATE, my mental model for this was:

if (I don't own the data)
  DUPLICATE;
else if (I malloced it)
  WRITEABLE;
else if (it's mmapped)
  READONLY_MAY_MAKE_WRITEABLE;
else /* ie.  it's shared with someone else, I don't have perms to modify it */
  READONLY;



So perhaps what's actually desired here is a set of flags to express
each of those conditions independently? I don't have a good proposal
for what those flag names might be.
I found the flag approach even more confusing as one would then think about (and the docs have to explain) all different kind of combinations. 



Although, having a name for the entire behavior is helpful, (rather
than just a conjunction of flags). So maybe we just need simpler names
to capture the behaviors of interest:

COPY: Data is immediately copied by the blob. Resulting blob supports
	write operations. [New name for DUPLICATE]

COPY_ON_WRITE: Data is referenced by the blob. Resulting blob support
	write operations which are implemented by lazily copying the
	data, (the original data is never modified). [New name for
	READONLY]

READ_WRITE: Data is referenced by the blob. Resulting blob supports
	write operations which will directly modify original data.
	[New name for WRITEABLE. Of the new names I'm proposing, this
	is the one I like the least.]

READONLY: Data is referenced by the blob. Resulting blob does not
	support write operations. [New name for READONLY_NEVER_DUPLICATE]

MPROTECT: Data is referenced by the blob. Resulting blob supports
	write operations which are implemented by first calling
	mprotect or equivalent. [New name for
	READONLY_MAY_MAKE_WRITEABLE. Obviously this isn't an ideal
	name since it references an operating-system-specific
	feature. Suggestions welcome.]
I wanted to keep MPROTECT out of the name. Again, please think about it again without the mode determining whether the blob will be writeable or not. Just as describing the access mode of the input data only. Do you think the current enum makes sense that way? 



I don't have much to say about the actual font-related parts of the
proposal. But I do think it's perhaps a bad idea to reuse the same
buffer for text input and glyph output. Is memory that tight? Might a
user not want to be able to query the text back out even after
shaping?
Memory is not tight, no. Note that this is only about what the input/output to the hb_shape() function will be. Users already keep the original text in some location anyway, and the cluster mapping is what's used to map the glyphs back. All I'm doing here essentially is to let the user populate a hb-buffer and pass that to hb_shape(), instead of hb_shape() doing it.
The benefit of course is having one hb_shape() API, instead of hb_shape_utf8(), hb_shape_utf16(), and hb_shape_utf32(). 

Makes more sense?

Thanks again,
behdad




-Carl
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]