[feat] Expose public API #1346

ebraminio · 2018-11-01T10:03:41Z

Closes #1304

ebraminio · 2018-11-01T12:27:16Z

PTAL

src/hb-aat-layout-feat-table.hh

drott

Thanks for working on this. Some feedback on naming and documentation.

src/hb-aat-layout.h

src/hb-aat-layout.cc

src/hb-aat-layout-feat-table.hh

src/hb-aat-layout.h

behdad

Here's an initial review. Didn't validate the API itself. Am heading to read 'feat' table now and be back with commets on that.

ebraminio · 2018-11-01T16:26:31Z

Applied just no strong preference yet whether we need to return default index or default value (currently).

test/api/test-aat-layout.c

behdad · 2018-11-01T16:43:49Z

A couple side-issues:

I currently merge multiple feature settings for the same setting when applying features. Looks like some features are non-exclusive and hence should not be merged? Or I can just remove the merge logic...
This API is not quite useful without mapping from AAT features to OpenType tags. Do we expose that?

ebraminio · 2018-11-01T16:46:56Z

This API is not quite useful without mapping from AAT features to OpenType tags. Do we expose that?

Maybe a separate issue, you said Chrome doesn't need that right now.

behdad · 2018-11-01T17:02:24Z

This API is not quite useful without mapping from AAT features to OpenType tags. Do we expose that?

Maybe a separate issue, you said Chrome doesn't need that right now.

Right. But I want to have the full picture laid out before adding anything. I don't want to find later that we should have done this differently.

behdad · 2018-11-01T17:19:32Z

It occurs to me that it's also possible to get user set AAT features using feature codes directly, by, eg, passing HB_TAG(" 1") to mean feature type 1, and the value will be the selector. I think we should hook these up in addition to the current OpenType to AAT feature mapping.

behdad · 2018-11-01T17:20:00Z

HB_TAG(" 1")

Of just (hb_tag_t) 1.

behdad · 2018-11-01T17:20:10Z

Of just (hb_tag_t) 1.

Yeah I like this one better.

behdad · 2018-11-01T17:47:28Z

Read 'feat' table. So, the proposed API completely misses the part about enumerating features themselves. I don't like that. I think there should be a first layer API to list all features and get their name_id's and look up feature's index, then the second layer takes a feature_index and lists the settings.

ebraminio · 2018-11-01T19:01:35Z

I think there should be a first layer API to list all features and get their name_id's and look up feature's index, then the second layer takes a feature_index and lists the settings.

Done. But I liked the first layer just was fetching features and fetching name_id on second layer.

It occurs to me that it's also possible to get user set AAT features using feature codes directly, by, eg, passing HB_TAG(" 1") to mean feature type 1, and the value will be the selector.

So reverting enum thing?

src/hb.hh

ebraminio · 2018-11-17T12:37:16Z

src/hb-aat.h

+  HB_AAT_LAYOUT_SELECTOR_PROPORTIONAL_CJK_ROMAN		= 1,
+  HB_AAT_LAYOUT_SELECTOR_DEFAULT_CJK_ROMAN		= 2,
+  HB_AAT_LAYOUT_SELECTOR_FULL_WIDTH_CJK_ROMAN		= 3
+} hb_aat_layout_feature_setting_t;


I guess "setting" or "selector" should only be used

Right. Pick one and only use that.

Either "feature_setting" or "feature_selector". Selector sounds less confusing.

ebraminio · 2018-11-17T15:00:59Z

src/hb-aat-layout-feat-table.hh

+    const SettingName* setting = (SettingName*) hb_bsearch (&key, feat+settingTableZ,
+							    nSettings,
+							    SettingName::static_size,
+							    SettingName::cmp);


Probably good idea to have an UnsizedSortedArrayOf (or SortedUnsizedArrayOf?) and move the logic there. COLR/CPAL needed that IIRC.

Yes please.

behdad · 2018-11-19T01:25:05Z

I like us to map out other API that interacts with this, before merging it. So far, things that have been discussed:

If feat feature is missing, dig into the morx table and extracted list of features?
API to convert AAT-style features into OT-style. And back?
If GSUB is missing, convert back feat table features to OT style and return from hb-ot-layout API.
Allow user specifying AAT features directly (without going through OT equivalents) in hb_feature_t, by using tag names like ' 12' for feature number 12.

ebraminio · 2018-11-19T04:17:13Z

There is also some sort of OT-AAT mapping on Zapf AAT table, I am not sure if they are useful for dynamic mapping or extract and having them somewhere (and complete ours) is enough.

…

On Monday, November 19, 2018, Behdad Esfahbod ***@***.***> wrote: I like us to map out other API that interacts with this, before merging it. So far, things that have been discussed: - If feat feature is missing, dig into the morx table and extracted list of features? - API to convert AAT-style features into OT-style. And back? - If GSUB is missing, convert back feat table features to OT style and return from hb-ot-layout API. - Allow user specifying AAT features directly (without going through OT equivalents) in hb_feature_t, by using tag names like ' 12' for feature number 12. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1346 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAy3wfRq4C5kiYwz61C01eC2RW3WzcDnks5uwghygaJpZM4YGHHq> .

drott · 2018-11-22T11:23:38Z

Coming back here. behdad@, I think I got confused: I asked you whether I can retrieve the CTFont from hb_coretext_font, but IIUC, after moving to HarfBuzz AAT we won't have a CTFont or hb_coretext_font anymore, so I won't have a good way of accessing the CTFont we have up in SkTypeface in the case-mapping/small-caps processing code except for changing a bunch of plumbing above, so some exposing feat HarfBuzz in HarfBuzz becomes more essential.

ebraminio · 2018-11-22T12:46:15Z

You can have access to CTFont even after HB AAT, you should just not pass coretext-aat after using hb_shape_full but better to not go there, Behdad concerns are understandable, maybe we can have this behind some compiler flag enabled only for Chromium and only on macOS for now (so no gentoo/chromeos packaging issue I guess) if just merging it is not a good idea.

drott · 2018-11-22T14:22:36Z

You can have access to CTFont even after HB AAT, you should just not pass coretext-aat after using hb_shape_full but better to not go there [...]

I will not be passing a CTFont based hb_font to HarfBuzz anymore after switching to HarfBuzz AAT, so I don't think there is an underlying CTFont anymore.

behdad · 2018-11-22T16:17:05Z

maybe we can have this behind some compiler flag enabled only for Chromium and only on macOS for now

Why not finish it?

ebraminio · 2018-11-22T19:34:02Z

I don't know what to do :)

…

On Thursday, November 22, 2018, Behdad Esfahbod ***@***.***> wrote: maybe we can have this behind some compiler flag enabled only for Chromium and only on macOS for now Why not finish it? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1346 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAy3wRx8b3yCn9qGaxz4ExDUr7pkO-8Tks5uxs4CgaJpZM4YGHHq> .

behdad · 2018-11-22T19:49:35Z

I don't know what to do :)

I listed:

* If `feat` feature is missing, dig into the `morx` table and extracted list of features?

This is easy. But if you don't want to get into it, that's fine for the first round, since doesn't have API implications.

* API to convert AAT-style features into OT-style.  And back?

Propose API for this. It's close to trivial. We have an internal API that converts one to another. Model two APIs based on that (just type here, DON'T send a patch) and we discuss it from there.

* If `GSUB` is missing, convert back `feat` table features to OT style and return from hb-ot-layout API.

If you do the conversion API, this part is also easy, but also not required initially since no API implications.

* Allow user specifying AAT features directly (without going through OT equivalents) in `hb_feature_t`, by using tag names like `'  12'` for feature number 12.

Also easy. Also not required initially since no API implications.

behdad · 2018-11-23T00:42:33Z

* API to convert AAT-style features into OT-style.  And back?
Propose API for this. It's close to trivial. We have an internal API that converts one to another. Model two APIs based on that (just type here, DON'T send a patch) and we discuss it from there.

Even this doesn't have to be implemented before merging this work. I just want to know we have a plan and discuss the API, to avoid any surprises and blind spots later.

behdad · 2018-11-23T03:49:13Z

One more item to add to TODO list: enum for OpenType features.

behdad · 2018-11-23T05:07:33Z

I studied this more and I'm comfortable merging it. We can expose something along the lines of hb_aat_layout_find_feature_mapping() and inverse later.

behdad · 2018-11-23T05:09:54Z

src/hb-aat-layout-feat-table.hh

+    unsigned int settings_count = nSettings;
+    if (count && *count)
+    {
+      unsigned int len = MIN (settings_count - start_offset, *count);


This should be ported to use hb_array_t and sub_array().

behdad · 2018-11-23T05:11:33Z

src/hb-aat-layout-feat-table.hh

+    unsigned int feature_count = featureNameCount;
+    if (count && *count)
+    {
+      unsigned int len = MIN (feature_count - start_offset, *count);


Same re using hb_array_t and .sub_array().

Thought about this before, no sub_array on UnsizedArrayOf, and doesn't worth to IMO.

UnsizedArrayOf should not be used directly. Maybe I remove operator[] from it. Idea is you bind it with a length to get a hb_array_t and use that, which has a sub_array().

UnsizedArrayOf::as_array().

behdad · 2018-11-23T05:18:51Z

src/hb-aat.h

+} hb_aat_layout_feature_setting_t;
+
+HB_EXTERN unsigned int
+hb_aat_layout_get_features (hb_face_t                    *face,


Let's fix namespacing. Again, you have all under "hb_aat_layout" namespace, ie. they all start with "hb_aat_layout_get". We want more fine-grained namspacing.

This one is fine, get_features.

src/hb-aat.h

behdad · 2018-11-23T05:19:53Z

src/hb-aat.h

+				    hb_aat_layout_feature_setting_t *settings         /* OUT.     May be NULL. */);
+
+HB_EXTERN hb_ot_name_id_t
+hb_aat_layout_get_feature_setting_name_id (hb_face_t                       *face,


hb_aat_layout_feature_setting_get_name_id or, feature_selector

The way we did this in ot-layout, and in general do these things, is to have find_feature or something, take feature type and return a feature index. The subsequent API entrypoints then take that index, to avoid having to look up the feature entry again and again.

Done.

Understandable as did once for hb_ot_layout_feature_get_characters

behdad · 2018-11-23T05:20:53Z

src/hb-aat-layout-feat-table.hh

+
+  inline hb_ot_name_id_t get_feature_name_id () const { return nameIndex; }
+
+  inline hb_ot_name_id_t get_feature_setting_name_id (const feat                      *feat,


These functions taking feat table look weird. I think things can be done differently, but I need to pull down the code and take a deeper look than just reading the patch.

behdad · 2018-11-23T05:43:14Z

This part of the spec:

"""For feature types that don't have exclusive settings, there will always be a pair of values. One value turns a selector on and a second value turns the selector off. The on setting must be even and the off setting must be one greater than the corresponding on setting. The off setting is therefore always odd. As a result, only the on setting should have an entry in the setting name array."""

needs at least to go into documentation...

codecov · 2018-11-23T08:27:39Z

Codecov Report

❗ No coverage uploaded for pull request base (master@e3a1a83). Click here to learn what that means.
The diff coverage is 94.36%.

@@            Coverage Diff            @@
##             master    #1346   +/-   ##
=========================================
  Coverage          ?   76.36%           
=========================================
  Files             ?      134           
  Lines             ?    16372           
  Branches          ?        0           
=========================================
  Hits              ?    12503           
  Misses            ?     3869           
  Partials          ?        0

Impacted Files	Coverage Δ
src/hb-aat-map.hh	`58.82% <ø> (ø)`
src/hb-aat-layout.hh	`100% <ø> (ø)`
src/hb-aat-map.cc	`38.09% <0%> (ø)`
src/hb-aat-layout-feat-table.hh	`100% <100%> (ø)`
src/hb-aat-layout.cc	`85.71% <100%> (ø)`
src/hb-aat-layout-morx-table.hh	`90.25% <71.42%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3a1a83...2243f42. Read the comment docs.

behdad · 2018-11-23T16:23:53Z

docs/harfbuzz-sections.txt

-hb_aat_layout_feature_setting_t
+hb_aat_layout_feature_get_selectors
+hb_aat_layout_feature_selector_get_name_id
+hb_aat_layout_feature_selector_t


Type name typically goes before functions using it.

ebraminio mentioned this pull request Nov 1, 2018

[aat] Expose 'feat' table #1304

Closed

ebraminio force-pushed the feat branch 4 times, most recently from 42a50fc to ea64bc0 Compare November 1, 2018 12:05

jfkthame reviewed Nov 1, 2018

View reviewed changes

src/hb-aat-layout-feat-table.hh Outdated Show resolved Hide resolved

jfkthame reviewed Nov 1, 2018

View reviewed changes

src/hb-aat-layout-feat-table.hh Outdated Show resolved Hide resolved

drott reviewed Nov 1, 2018

View reviewed changes

src/hb-aat-layout.h Outdated Show resolved Hide resolved

src/hb-aat-layout.cc Outdated Show resolved Hide resolved

src/hb-aat-layout.cc Outdated Show resolved Hide resolved

src/hb-aat-layout.cc Outdated Show resolved Hide resolved

drott reviewed Nov 1, 2018

View reviewed changes

src/hb-aat-layout.cc Outdated Show resolved Hide resolved

drott reviewed Nov 1, 2018

View reviewed changes

src/hb-aat-layout.cc Outdated Show resolved Hide resolved

behdad reviewed Nov 1, 2018

View reviewed changes

src/hb-aat-layout-feat-table.hh Outdated Show resolved Hide resolved

behdad reviewed Nov 1, 2018

View reviewed changes

src/hb-aat-layout.h Outdated Show resolved Hide resolved

behdad reviewed Nov 1, 2018

View reviewed changes

src/hb-aat-layout.h Outdated Show resolved Hide resolved

behdad reviewed Nov 1, 2018

View reviewed changes

src/hb-aat-layout.h Outdated Show resolved Hide resolved

behdad reviewed Nov 1, 2018

View reviewed changes

src/hb-aat-layout.h Outdated Show resolved Hide resolved

behdad reviewed Nov 1, 2018

View reviewed changes

test/api/test-aat-layout.c Show resolved Hide resolved

ebraminio force-pushed the feat branch 3 times, most recently from 31b5054 to 29694b6 Compare November 6, 2018 17:08

ebraminio commented Nov 17, 2018

View reviewed changes

src/hb.hh Show resolved Hide resolved

ebraminio commented Nov 17, 2018

View reviewed changes

ebraminio force-pushed the feat branch from d1e2743 to b2ba616 Compare November 17, 2018 13:40

ebraminio commented Nov 17, 2018

View reviewed changes

[feat] Use bsearch

ad82482

ebraminio force-pushed the feat branch from b2ba616 to ad82482 Compare November 18, 2018 12:25

behdad reviewed Nov 23, 2018

View reviewed changes

src/hb-aat.h Outdated Show resolved Hide resolved

behdad reviewed Nov 23, 2018

View reviewed changes

src/hb-aat.h Outdated Show resolved Hide resolved

behdad reviewed Nov 23, 2018

View reviewed changes

[feat] Apply renamings and add documentation

8a2e7cc

[feat] Rename API uses of setting to selector

2243f42

behdad reviewed Nov 23, 2018

View reviewed changes

behdad closed this in 926f512 Nov 25, 2018


		inline hb_ot_name_id_t get_feature_name_id () const { return nameIndex; }

		inline hb_ot_name_id_t get_feature_setting_name_id (const feat *feat,

[feat] Expose public API #1346

[feat] Expose public API #1346

Uh oh!

Conversation

ebraminio commented Nov 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ebraminio commented Nov 1, 2018

Uh oh!

Uh oh!

Uh oh!

drott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

behdad left a comment

Choose a reason for hiding this comment

Uh oh!

ebraminio commented Nov 1, 2018

Uh oh!

Uh oh!

behdad commented Nov 1, 2018

Uh oh!

ebraminio commented Nov 1, 2018

Uh oh!

behdad commented Nov 1, 2018

Uh oh!

behdad commented Nov 1, 2018

Uh oh!

behdad commented Nov 1, 2018

Uh oh!

behdad commented Nov 1, 2018

Uh oh!

behdad commented Nov 1, 2018

Uh oh!

ebraminio commented Nov 1, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

behdad commented Nov 19, 2018

Uh oh!

ebraminio commented Nov 19, 2018 via email

Uh oh!

drott commented Nov 22, 2018

Uh oh!

ebraminio commented Nov 22, 2018

Uh oh!

drott commented Nov 22, 2018

Uh oh!

behdad commented Nov 22, 2018

Uh oh!

ebraminio commented Nov 22, 2018 via email

Uh oh!

behdad commented Nov 22, 2018

Uh oh!

behdad commented Nov 23, 2018

Uh oh!

behdad commented Nov 23, 2018

Uh oh!

behdad commented Nov 23, 2018

Uh oh!

ebraminio commented Nov 1, 2018 •

edited

Loading

behdad commented Nov 23, 2018 •

edited

Loading

codecov bot commented Nov 23, 2018 •

edited

Loading