Skip to content

Conversation

khaledhosny
Copy link
Collaborator

@khaledhosny khaledhosny commented May 1, 2018

WIP, the tests do not pass (table in cpal-v1.ttf fails sanitization). Here is a LibreOffice using it (WIP as well):
image

{
const OT::CPAL& cpal = _get_cpal(face);
if (unlikely (palette >= cpal.numPalettes))
Copy link
Collaborator

@ebraminio ebraminio May 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This work is great, thanks!

I believe you can move the logic of this and hb_ot_color_get_color_layers to their tables themselves and use this make this a proxy for them.

I guess we can remove the non-used helpers on that tables also afterward.

@ebraminio
Copy link
Collaborator

Closes #849

@ebraminio ebraminio closed this May 1, 2018
@ebraminio ebraminio reopened this May 1, 2018
*
* Since: REPLACEME
*/
typedef uint32_t hb_ot_color_t;
Copy link
Collaborator

@ebraminio ebraminio May 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a bikeshedding, hb_ot_color_t or hb_color_t Behdad? Also suggested your idea on having helper macros for color channels.

@khaledhosny
Copy link
Collaborator Author

Closing, not sure about this any more. Can be done from scratch if we ever want it.

@behdad
Copy link
Member

behdad commented Sep 11, 2018

Why?!! I definitely want something like this. Leave open, it's fine.

@behdad behdad reopened this Sep 11, 2018
@khaledhosny
Copy link
Collaborator Author

I thought we had another PR from @ebraminio for that, but looks like we don’t.

@khaledhosny khaledhosny changed the title WIP: Minimal API for COLR/CPAL [WIP] Minimal API for COLR/CPAL Oct 11, 2018
@ebraminio
Copy link
Collaborator

Khaled, do you remember why this was considered WIP? Can we revive it maybe?

@khaledhosny
Copy link
Collaborator Author

I wanted other opinions about the API. I tried to rebase it on master in September 11 but there were so many changes and I could manage to get it compile but it was crashing and I got lost finding out what was going on.

@ebraminio
Copy link
Collaborator

No problem, I will also look it to see if I can progress it maybe :)

@ebraminio
Copy link
Collaborator

ebraminio commented Oct 19, 2018

Khaled and Behdad any chance you can see if is this good enough?

My questions, is hb_ot_color_has_{cpal,colr}_data as public API needed? May hb_ot_color_t be renamed to hb_color_t? Is hb_ot_color_get_{red,green,blue,alpha} (added by myself) correct name for public macros on harfbuzz?

@khaledhosny
Copy link
Collaborator Author

The macros should be all caps I think.

@khaledhosny
Copy link
Collaborator Author

has_*_datawould be used by clients to see if the font can decompose color glyphs or not, and it can check that once instead of calling the color API on each glyph and checking the result.

@ebraminio
Copy link
Collaborator

ebraminio commented Oct 19, 2018 via email

@behdad
Copy link
Member

behdad commented Oct 19, 2018

Don't merge any API in please until we get the 2.0.0 post-releases out first.

@ebraminio
Copy link
Collaborator

Don't merge any API in please until we get the 2.0.0 post-releases out first.

Sure.

How about have it as a branch on the repo itself?

Asking from Khaled, any personal reason marking it as WIP? Please rename the macros if you like to

int r = hb_ot_color_get_red (color);
int g = hb_ot_color_get_green (color);
int b = hb_ot_color_get_blue (color);
cairo_set_source_rgba (cr, r / 255., g / 255., b / 255., alpha);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My fault probably, this needs / 255. also I guess

@khaledhosny
Copy link
Collaborator Author

@ebraminio feel free to close this PR, move the branch to harfbuzz/harfbuzz repo and open a new one. I don’t seem to remember much of what I was doing here, so feel free to fix as you see fit.

@ebraminio ebraminio changed the base branch from master to ot-color October 20, 2018 13:48
@ebraminio ebraminio changed the title [WIP] Minimal API for COLR/CPAL Minimal API for COLR/CPAL Oct 20, 2018
@ebraminio ebraminio merged commit 3eef17e into harfbuzz:ot-color Oct 20, 2018
@ebraminio
Copy link
Collaborator

ebraminio commented Oct 20, 2018

Merged on ot-color as Khaled agreement (not master, don't get confused)

@khaledhosny khaledhosny deleted the colr-cpal-api branch October 21, 2018 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants