Add font instance keys which associate size and other options with a key #1602

lsalzman · 2017-08-23T15:12:51Z

This adds FontInstanceKey to the API, which alleviates the need to pass both
a FontKey and other options parameterizing a font to any functions using that
font. FontInstanceKeys are managed via the new add_font_instance and
delete_font_instance API functions. This enables validation of these parameters
to be done up front when add_font_instance is called, rather than every time
text is submitted. It also future-proofs FontInstances by allowing more
parameters than might otherwise be tenable if one had to supply them all when
submitting text.

At a minimum, one needs to specify a size, much like a ScaledFont in Gecko.
One can also optionally supply FontInstanceOptions for parameters common across
all platforms, and FontInstancePlatformOptions for parameters that are specific
to a given platform (which will be developed further in future patches).

This change is

glennw · 2017-08-24T06:35:03Z

Reviewed 13 of 13 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

webrender/src/frame_builder.rs, line 902 at r1 (raw file):

        // whether this item should be rendered with
        // subpixel AA!
        let mut default_render_mode = cmp::min(self.config.default_font_render_mode,

I'd prefer we don't use cmp() here - it's not clear to me on reading it what the semantics are (even though it works numerically).

webrender/src/frame_builder.rs, line 974 at r1 (raw file):

                let mut text_prim = prim.clone();
                text_prim.font.color = shadow_prim.shadow.color.into();
                text_prim.color = shadow_prim.shadow.color;

Why do we need to store the color in two places here?

webrender/src/prim_store.rs, line 532 at r1 (raw file):

        let mut font = self.font.clone();
        font.size = font.size.scale_by(device_pixel_ratio);
        if run_mode == TextRunMode::Shadow {

Let's stick with the match that was here - that means we'll get a compile error here should anyone ever add a field to the TextRunMode enum.

Comments from Reviewable

glennw · 2017-08-24T06:36:02Z

This generally looks fine to me - just a few comments, and CI seems unhappy too (examples need updating, I think).

bors-servo · 2017-08-24T12:35:35Z

☔ The latest upstream changes (presumably #1597) made this pull request unmergeable. Please resolve the merge conflicts.

lsalzman · 2017-08-24T17:25:55Z

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

webrender/src/frame_builder.rs, line 902 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

I'd prefer we don't use cmp() here - it's not clear to me on reading it what the semantics are (even though it works numerically).

I added a slightly more explicit function for combining the render modes that doesn't use cmp::min in my upcoming review patch. The semantics are meant to agree with Gecko, in that when an individual text run comes in, with its own AA override, that AA override is meant to disable some AA functionality that was otherwise enabled by default. So Gecko's AA default is permissive, and sometimes we need to disallow it.

webrender/src/frame_builder.rs, line 974 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Why do we need to store the color in two places here?

Color is quantized in FontInstance via ColorU to reduce the amount of hash keys and rasterized glyphs we generate, and turning that back into a ColorF will not yield the same fidelity when we access the color to pass it in to the shader or blending. In the interests of preserving both constraints, I had to leave the ColorF field around.

Comments from Reviewable

lsalzman · 2017-08-24T17:26:21Z

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

webrender/src/prim_store.rs, line 532 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Let's stick with the match that was here - that means we'll get a compile error here should anyone ever add a field to the TextRunMode enum.

Okay, can do.

Comments from Reviewable

glennw

Added a few more suggestions - they can either be done as a follow up, or as part of the rebase.

The main thing is to get the public API change in and updated in callers, we can fix up those other issues later if you prefer.

glennw · 2017-08-25T05:33:28Z

webrender/src/frame.rs

@@ -557,12 +559,12 @@ impl Frame {
                                              info.image_rendering);
            }
            SpecificDisplayItem::Text(ref text_info) => {
+                let instance = context.resource_cache.get_font_instance(text_info.font_key).unwrap();


It might make sense to store the font instance key in the text primitive here. Then, we wouldn't need the resource cache at all at this part of the code (it could be fetched later), and the text run primitive structure could be made significantly smaller. We could do this as a follow up though...

glennw · 2017-08-25T05:33:28Z

webrender/src/frame_builder.rs

@@ -913,30 +927,32 @@ impl FrameBuilder {

        // Shadows never use subpixel AA, but need to respect the alpha/mono flag
        // for reftests.
-        let (shadow_render_mode, subpx_dir) = match self.config.default_font_render_mode {
+        let (shadow_render_mode, subpx_dir) = match default_render_mode {
            FontRenderMode::Subpixel | FontRenderMode::Alpha => {
                // TODO(gw): Expose subpixel direction in API once WR supports


This comment can be removed now, I think?

glennw · 2017-08-25T05:33:28Z

webrender/src/prim_store.rs

@@ -504,21 +504,17 @@ pub struct TextShadowPrimitiveCpu {

 #[derive(Debug, Clone)]
 pub struct TextRunPrimitiveCpu {
-    pub font_key: FontKey,
+    pub font: FontInstance,


This is the bit I was mentioning above that could just store the FontInstanceKey here.

glennw · 2017-08-25T05:33:28Z

webrender_api/src/font.rs

+
+    // Combine two font render modes such that the lesser amount of AA limits the AA of the result.
+    pub fn limit_by(self, other: FontRenderMode) -> FontRenderMode {
+        match self {


Might be tidier if written as match (self, other) but can be left as is if you prefer.


        Add font instance keys which associate size and other options with a key

This adds FontInstanceKey to the API, which alleviates the need to pass both a FontKey and other options parameterizing a font to any functions using that font. FontInstanceKeys are managed via the new add_font_instance and delete_font_instance API functions. This enables validation of these parameters to be done up front when add_font_instance is called, rather than every time text is submitted. It also future-proofs FontInstances by allowing more parameters than might otherwise be tenable if one had to supply them all when submitting text. At a minimum, one needs to specify a size, much like a ScaledFont in Gecko. One can also optionally supply FontInstanceOptions for parameters common across all platforms, and FontInstancePlatformOptions for parameters that are specific to a given platform (which will be developed further in future patches).

lsalzman · 2017-08-30T15:13:15Z

Review status: 4 of 22 files reviewed at latest revision, 7 unresolved discussions.

webrender/src/frame.rs, line 562 at r2 (raw file):

Previously, glennw (Glenn Watson) wrote…

It might make sense to store the font instance key in the text primitive here. Then, we wouldn't need the resource cache at all at this part of the code (it could be fetched later), and the text run primitive structure could be made significantly smaller. We could do this as a follow up though...

There's some nastiness with respect to needing to access the size and render mode when building the primitive, which in the simplest form would require multiple lookups into the font instance hashmap to deal with. It might be possible to restructure the code to avoid that, but I think that is best for a followup.

webrender/src/frame_builder.rs, line 932 at r2 (raw file):

Previously, glennw (Glenn Watson) wrote…

This comment can be removed now, I think?

Not quite, as still haven't really exposed subpixel direction in the api. I just merely routed some plumbing there from the FontInstance to not just hardcode the subpixel direction, but there's still no particular way of manipulating that for the FontInstance just yet.

webrender_api/src/font.rs, line 121 at r2 (raw file):

Previously, glennw (Glenn Watson) wrote…

Might be tidier if written as match (self, other) but can be left as is if you prefer.

Noted.

Comments from Reviewable

glennw · 2017-08-31T01:27:13Z

@bors-servo r+

bors-servo · 2017-08-31T01:27:13Z

📌 Commit 5d3c7c3 has been approved by glennw

bors-servo · 2017-08-31T01:27:16Z

⌛ Testing commit 5d3c7c3 with merge adba8e6...


        Auto merge of #1602 - lsalzman:font-instance-key, r=glennw

Add font instance keys which associate size and other options with a key This adds FontInstanceKey to the API, which alleviates the need to pass both a FontKey and other options parameterizing a font to any functions using that font. FontInstanceKeys are managed via the new add_font_instance and delete_font_instance API functions. This enables validation of these parameters to be done up front when add_font_instance is called, rather than every time text is submitted. It also future-proofs FontInstances by allowing more parameters than might otherwise be tenable if one had to supply them all when submitting text. At a minimum, one needs to specify a size, much like a ScaledFont in Gecko. One can also optionally supply FontInstanceOptions for parameters common across all platforms, and FontInstancePlatformOptions for parameters that are specific to a given platform (which will be developed further in future patches).  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1602)

glennw · 2017-08-31T01:28:07Z

Servo patch for the API update is here - glennw/servo@26b5809

I'm assuming @lsalzman will have something similar ready for Gecko as a patch :)

bors-servo · 2017-08-31T02:25:27Z

☀️ Test successful - status-travis
Approved by: glennw
Pushing adba8e6 to master...

glennw approved these changes Aug 25, 2017

View changes

lsalzman force-pushed the lsalzman:font-instance-key branch from c04d0a4 to 5d3c7c3 Aug 30, 2017

This was referenced Sep 1, 2017

Implement Synthetic Bold Support #1653

Closed

Implement Synthetic Italic Support #1654

Closed

glennw mentioned this pull request Sep 5, 2017

Glitchy text-shadow when blurred. #1658

Closed

servo / webrender

Add font instance keys which associate size and other options with a key #1602

Add font instance keys which associate size and other options with a key #1602

lsalzman commented Aug 23, 2017 •

edited by larsbergstrom

glennw commented Aug 24, 2017

glennw commented Aug 24, 2017

bors-servo commented Aug 24, 2017

lsalzman commented Aug 24, 2017

lsalzman commented Aug 24, 2017

glennw left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

lsalzman commented Aug 30, 2017

glennw commented Aug 31, 2017

bors-servo commented Aug 31, 2017

bors-servo commented Aug 31, 2017

glennw commented Aug 31, 2017

bors-servo commented Aug 31, 2017

servo / webrender

Join GitHub today

Add font instance keys which associate size and other options with a key #1602

Add font instance keys which associate size and other options with a key #1602

Conversation

lsalzman commented Aug 23, 2017 • edited by larsbergstrom

glennw commented Aug 24, 2017

glennw commented Aug 24, 2017

bors-servo commented Aug 24, 2017

lsalzman commented Aug 24, 2017

lsalzman commented Aug 24, 2017

glennw left a comment

This comment has been minimized.

glennw Aug 25, 2017

This comment has been minimized.

glennw Aug 25, 2017

This comment has been minimized.

glennw Aug 25, 2017

This comment has been minimized.

glennw Aug 25, 2017

lsalzman commented Aug 30, 2017

glennw commented Aug 31, 2017

bors-servo commented Aug 31, 2017

bors-servo commented Aug 31, 2017

glennw commented Aug 31, 2017

bors-servo commented Aug 31, 2017

lsalzman commented Aug 23, 2017 •

edited by larsbergstrom