Render glyphs with Pathfinder if the `pathfinder` feature flag is enabled. #2566

pcwalton · 2018-03-23T21:58:05Z

This is very preliminary. Notable problems:

Intermediate textures are not cached.
Font appearance resembles that of macOS regardless of platform.
Subpixel AA is always used, even if disabled.
The SourceTexture extension is hacky.
Emoji and other color glyphs will not render properly. We should fall
back to the CPU for those.
There is no hinting or pixel snapping yet.

r? @glennw

This change is

glennw · 2018-03-23T22:11:01Z

Awesome! Will review first thing Monday :)

glennw · 2018-03-24T01:46:43Z

Windows CI failure:

error[E0061]: this function takes 8 parameters but 4 parameters were supplied
   --> src\glyph_rasterizer.rs:973:9
    |
624 | /     pub fn resolve_glyphs(
625 | |         &mut self,
626 | |         glyph_cache: &mut GlyphCache,
627 | |         texture_cache: &mut TextureCache,
...   |
720 | |         }
721 | |     }
    | |_____- defined here
...
973 |           &mut glyph_cache,
    |           ^^^^^^^^^^^^^^^^ expected 8 parameters
error: aborting due to previous error
error: Could not compile `webrender`.

fschutt · 2018-03-24T15:09:36Z

Would this change make freetype (possibly) obsolete in the future?

pcwalton · 2018-03-24T18:11:26Z

No, because FreeType is still used to load the outlines and hint the glyphs. Besides, HarfBuzz has to use it during layout, I believe.

…

On Sat, Mar 24, 2018, 8:09 AM fschutt ***@***.***> wrote: Question, would this change make freetype (possibly) obsolete in the future? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2566 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJoydMFOOY4OMBnIEAV8E2ZOEw_oiueks5thmGxgaJpZM4S5bQm> .

glennw · 2018-03-26T03:04:38Z

Reviewed 24 of 24 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

webrender/Cargo.toml, line 13 at r1 (raw file):

freetype-lib = ["freetype/servo-freetype-sys"]
profiler = ["thread_profiler/thread_profiler"]
debugger = ["ws", "serde_json", "serde", "image", "base64"]

Why is image no longer an optional dependency?

webrender/res/ps_text_run.glsl, line 177 at r1 (raw file):

    vec3 tc = vec3(clamp(vUv.xy, vUvBorder.xy, vUvBorder.zw), vUv.z);
    vec4 mask = texture(sColor0, tc);
    if (vMaskSwizzle.z > 0.0)

Is this necessary? For the normal (CPU rendered) alpha mode, we store the channels correctly in the RGBA output during rasterization, so there's no runtime swizzle needed in this case.

webrender/src/device.rs, line 441 at r1 (raw file):

pub struct Texture {
    id: gl::GLuint,
    target: TextureTarget,

Why do we need to store this as TextureTarget and calculate the GL target each time it's used?

webrender/src/glyph_rasterizer.rs, line 530 at r1 (raw file):

                                                                     false)) {
                (Ok(outline), Ok(dimensions)) => {
                    // TODO(pcwalton)

Could you expand on this TODO?

webrender/src/texture_cache.rs, line 174 at r1 (raw file):

// In this case, the cache handle needs to re-upload this item
// to the texture cache (see request() below).
#[derive(Clone, Debug)]

Texture cache handles shouldn't be cloned in normal use - if one is updated then the other won't have the correct location. Is this still relied on?

webrender/src/tiling.rs, line 977 at r1 (raw file):

        let instance = BlurInstance {
            task_address: task_address,
            src_task_address: task_address,

This doesn't seem right. The two addresses are the render task being drawn to (provides target rect etc) and the source render task we are going to blur.

Comments from Reviewable

glennw · 2018-03-26T03:13:39Z

The main bits I don't fully understand yet are:

Why we need a new variant in the SourceTexture enum.
The code that unconditionally creates an extra alpha/color pass for glyphs.

I think the second one is probably because we don't have proper support for multiple top-level render task trees yet, and the pathfinder stuff doesn't really fit into the hierarchical nature of the existing render task tree? I've been planning to support multiple roots in the task tree at some point, which I think will tidy this stuff up.

I'll take another look at it this afternoon regarding the first question above.

We'll also need to do a gecko try (instructions here https://wiki.mozilla.org/Platform/GFX/Quantum_Render#Try_syntax) and it'd be good to add an entry to the TaskCluster config file in the repo that builds the Pathfinder feature, so we at least don't regress on compilation errors there.

bors-servo · 2018-03-26T22:29:34Z

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

pcwalton · 2018-03-27T22:42:14Z

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

webrender/Cargo.toml, line 13 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Why is image no longer an optional dependency?

Because it's needed to load the area LUT, which is stored in PNG format.

webrender/res/ps_text_run.glsl, line 177 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Is this necessary? For the normal (CPU rendered) alpha mode, we store the channels correctly in the RGBA output during rasterization, so there's no runtime swizzle needed in this case.

Yes, it's needed to grab the single R8 texture channel properly from a MODE_ALPHA texture. Here's what it looks like without that line (notice the text on the top left):

webrender/src/device.rs, line 441 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Why do we need to store this as TextureTarget and calculate the GL target each time it's used?

To make the to_external() method I added work. ExternalTexture requires a TextureTarget, not a GLuint.

webrender/src/glyph_rasterizer.rs, line 530 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Could you expand on this TODO?

Oops, that was a comment I forgot to remove. Deleted it.

Comments from Reviewable

glennw · 2018-03-27T23:08:40Z

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

webrender/Cargo.toml, line 13 at r1 (raw file):

Previously, pcwalton (Patrick Walton) wrote…

Because it's needed to load the area LUT, which is stored in PNG format.

Ah, that makes sense. Should we enable it only when pathfinder is enabled, so that Gecko doesn't pull in a new image dependency then? Perhaps in the future we could consider just embedding it in the binary as raw image data.

webrender/res/ps_text_run.glsl, line 177 at r1 (raw file):

Previously, pcwalton (Patrick Walton) wrote…

Yes, it's needed to grab the single R8 texture channel properly from a MODE_ALPHA texture. Here's what it looks like without that line (notice the text on the top left):

Instead of writing to an R8 texture though, what if we stored the output as an RGBA texture? That's what we currently do when rasterizing glyphs, even in alpha mode.

The benefits of this are that we don't need an extra swizzle here (pay the cost during glyph rasterizing once rather than an extra cost during rendering the glyph each frame), and batching is better since glyphs will end up in the same texture atlas as almost everything else.

Comments from Reviewable

bors-servo · 2018-04-02T21:06:24Z

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

pcwalton · 2018-04-02T23:05:56Z

Updated per feedback.

r? @glennw

glennw · 2018-04-02T23:21:48Z

CI failures from servo-tidy:

Checking files for tidiness...
  Progress: 0% (2/708)
./Cargo.lock:1: duplicate versions for package `env_logger`
	The following packages depend on version 0.4.3 from 'crates.io':
		pathfinder_partitioner
	The following packages depend on version 0.5.3 from 'crates.io':
		webrender
		wrench
./Cargo.lock:1: duplicate versions for package `num-traits`
	The following packages depend on version 0.1.43 from 'crates.io':
		app_units
		enum_primitive
		euclid
		image
		lyon_geom
		num
		num-integer
		num-iter
		num-rational
		plane-split
		serde_json
		webrender
	The following packages depend on version 0.2.2 from 'crates.io':
		num-traits

Are those ones easy to fix up? If not, we can possibly whitelist them as allowed duplicates, but that's not ideal.

glennw · 2018-04-03T00:27:39Z

Reviewed 4 of 25 files at r1, 19 of 23 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

webrender/src/device.rs, line 47 at r2 (raw file):

#[derive(Debug, Copy, Clone, PartialEq, Ord, Eq, PartialOrd)]
pub struct TextureId(gl::GLuint);

Is this still needed?

Comments from Reviewable

glennw · 2018-04-03T00:30:19Z

Looks good! Added a short list of the remaining things to get this merged:

Fix up the CI tidy errors above (or whitelist duplicated dependencies if we have to).
Do one more pass over glyph_rasterizer and/or get @nical to take a pass over it.
Do a gecko try run - see https://wiki.mozilla.org/Platform/GFX/Quantum_Render#Try_syntax.
Fix / answer the one remaining reviewable question above.

pcwalton · 2018-04-03T01:24:45Z

I don't know why num-traits 1.4.43 depends on num-traits 0.2.2. That doesn't make sense to me…

I tried upgrading to num-traits 0.2.2 but other packages in the dependency tree depend on num-traits 0.1.

glennw · 2018-04-03T05:20:09Z

That num-traits error is weird - maybe the cargo log is wrong and there are other crates that depend on each of them?

Also CI compile error on Windows (this might be on all platforms via cargo test):

error[E0061]: this function takes 8 parameters but 5 parameters were supplied
   --> src\glyph_rasterizer.rs:983:13
    |
613 | /     pub fn request_glyphs(
614 | |         &mut self,
615 | |         glyph_cache: &mut GlyphCache,
616 | |         font: FontInstance,
...   |
669 | |         self.request_glyphs_from_backend(font, new_glyphs);
670 | |     }
    | |_____- defined here
...
983 |               &mut glyph_cache,
    |               ^^^^^^^^^^^^^^^^ expected 8 parameters
error[E0061]: this function takes 6 parameters but 4 parameters were supplied
   --> src\glyph_rasterizer.rs:994:9
    |
738 | /     pub fn resolve_glyphs(
739 | |         &mut self,
740 | |         glyph_cache: &mut GlyphCache,
741 | |         texture_cache: &mut TextureCache,
...   |
809 | |         self.remove_dead_fonts();
810 | |     }
    | |_____- defined here
...
994 |           &mut glyph_cache,
    |           ^^^^^^^^^^^^^^^^ expected 6 parameters
error: aborting due to 2 previous errors
error: Could not compile `webrender`.
Caused by:

Emerentius · 2018-04-03T07:29:54Z

num-traits 0.1.43 re-exports some traits from 0.2. It's the semver trick.

Eijebong · 2018-04-03T10:49:14Z

Yeah the num-traits update is going to take some time, you can probably dupe it in the meantime (we'll probably have to do it in servo too :/)
For now we're blocked mainly on andersk/enum_primitive-rs#16
Once this one is out the way, I think the rest is pretty straightforward.

pcwalton · 2018-04-03T18:08:14Z

@glennw Updated to fix test build failure.

glennw · 2018-04-03T20:58:24Z

@Emerentius Ah, thanks for the info. So, this means we should, for now, white-list having duplicate versions of that crate, right? @pcwalton that can be done in https://github.com/servo/webrender/blob/master/servo-tidy.toml, if that's the correct thing to do here.

Eijebong · 2018-04-03T21:32:43Z

@glennw It is the right thing to do

pcwalton · 2018-04-03T21:35:39Z

@glennw Done.

glennw · 2018-04-03T21:58:20Z

I have pushed a (pending) try build to Gecko with this patch, just to make sure it doesn't break anything in the default code path:

https://hg.mozilla.org/try/rev/20feada4fb73b5467cd45bb274093a78674838d8
https://treeherder.mozilla.org/#/jobs?repo=try&revision=20feada4fb73b5467cd45bb274093a78674838d8

The WR CI build is failing on one of the configs that denies warnings:

error�(B: unused import: `glyph_cache::CachedGlyphData`�(B
  �(B--> �(Bwebrender/src/resource_cache.rs:24:5�(B
   �(B|�(B
24�(B �(B| �(Buse glyph_cache::CachedGlyphData;�(B
   �(B| �(B    �(B^^^^^^^^^^^^^^^^^^^^^^^^^^^^�(B
   �(B|�(B
   �(B= �(Bnote�(B: `-D unused-imports` implied by `-D warnings`�(B
error�(B: aborting due to previous error�(B
error: Could not compile `webrender`.


        Render glyphs with Pathfinder if the `pathfinder` feature flag is

enabled. This is very preliminary. Notable problems: * Intermediate textures are not cached. * Font appearance resembles that of macOS regardless of platform. * Subpixel AA is always used, even if disabled. * Emoji and other color glyphs will not render properly. We should fall back to the CPU for those. * There is no hinting or pixel snapping yet.

pcwalton · 2018-04-03T22:02:28Z

Pushed a new change that should eliminate that warning.

glennw · 2018-04-04T00:10:52Z

The failing tests on try appear to be unrelated intermittent failures.

@bors-servo r+ 🚀

bors-servo · 2018-04-04T00:10:53Z

📌 Commit e8b8082 has been approved by glennw

bors-servo · 2018-04-04T00:10:56Z

⌛ Testing commit e8b8082 with merge 7ca4558...


        Auto merge of #2566 - pcwalton:pathfinder-staging, r=glennw

@glennw

Render glyphs with Pathfinder if the `pathfinder` feature flag is enabled. This is very preliminary. Notable problems: * Intermediate textures are not cached. * Font appearance resembles that of macOS regardless of platform. * Subpixel AA is always used, even if disabled. * The `SourceTexture` extension is hacky. * Emoji and other color glyphs will not render properly. We should fall back to the CPU for those. * There is no hinting or pixel snapping yet. r? @glennw  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2566)

bors-servo · 2018-04-04T00:51:35Z

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: glennw
Pushing 7ca4558 to master...

ebraminio · 2019-06-01T06:50:03Z

#2566 (comment)
@pcwalton: Thanks for the work, HarfBuzz can work without freetype and working without others callbacks is suggested now actually, the only issue we don't have glyph outline fetch harfbuzz/harfbuzz#1686 yet and having that hopefully makes servo able to do better concurrent rendering in future

pcwalton requested a review from glennw Mar 23, 2018

pcwalton force-pushed the pcwalton:pathfinder-staging branch from a0011e9 to e1a9dc0 Mar 31, 2018

pcwalton force-pushed the pcwalton:pathfinder-staging branch from e1a9dc0 to 9a7610d Apr 2, 2018

pcwalton force-pushed the pcwalton:pathfinder-staging branch from 9a7610d to a921ed3 Apr 3, 2018

pcwalton force-pushed the pcwalton:pathfinder-staging branch from a921ed3 to 40f5ba1 Apr 3, 2018

pcwalton force-pushed the pcwalton:pathfinder-staging branch from 40f5ba1 to 5b96c96 Apr 3, 2018

pcwalton force-pushed the pcwalton:pathfinder-staging branch from 5b96c96 to e8b8082 Apr 3, 2018

ebraminio mentioned this pull request Jun 1, 2019

Unified API to fetch glyph countors harfbuzz/harfbuzz#1686

Open

servo / webrender

Render glyphs with Pathfinder if the `pathfinder` feature flag is enabled. #2566

Render glyphs with Pathfinder if the `pathfinder` feature flag is enabled. #2566

pcwalton commented Mar 23, 2018 •

edited by larsbergstrom

glennw commented Mar 23, 2018

glennw commented Mar 24, 2018

fschutt commented Mar 24, 2018 •

edited

pcwalton commented Mar 24, 2018

glennw commented Mar 26, 2018

glennw commented Mar 26, 2018

bors-servo commented Mar 26, 2018

pcwalton commented Mar 27, 2018

glennw commented Mar 27, 2018

bors-servo commented Apr 2, 2018

pcwalton commented Apr 2, 2018

glennw commented Apr 2, 2018

glennw commented Apr 3, 2018

glennw commented Apr 3, 2018 •

edited

pcwalton commented Apr 3, 2018 •

edited

glennw commented Apr 3, 2018

Emerentius commented Apr 3, 2018

Eijebong commented Apr 3, 2018

pcwalton commented Apr 3, 2018

glennw commented Apr 3, 2018

Eijebong commented Apr 3, 2018

pcwalton commented Apr 3, 2018

glennw commented Apr 3, 2018

pcwalton commented Apr 3, 2018

glennw commented Apr 4, 2018

bors-servo commented Apr 4, 2018

bors-servo commented Apr 4, 2018

bors-servo commented Apr 4, 2018

ebraminio commented Jun 1, 2019

servo / webrender

Join GitHub today

Render glyphs with Pathfinder if the `pathfinder` feature flag is enabled. #2566

Render glyphs with Pathfinder if the `pathfinder` feature flag is enabled. #2566

Conversation

pcwalton commented Mar 23, 2018 • edited by larsbergstrom

glennw commented Mar 23, 2018

glennw commented Mar 24, 2018

fschutt commented Mar 24, 2018 • edited

pcwalton commented Mar 24, 2018

glennw commented Mar 26, 2018

glennw commented Mar 26, 2018

bors-servo commented Mar 26, 2018

pcwalton commented Mar 27, 2018

glennw commented Mar 27, 2018

bors-servo commented Apr 2, 2018

pcwalton commented Apr 2, 2018

glennw commented Apr 2, 2018

glennw commented Apr 3, 2018

glennw commented Apr 3, 2018 • edited

pcwalton commented Apr 3, 2018 • edited

glennw commented Apr 3, 2018

Emerentius commented Apr 3, 2018

Eijebong commented Apr 3, 2018

pcwalton commented Apr 3, 2018

glennw commented Apr 3, 2018

Eijebong commented Apr 3, 2018

pcwalton commented Apr 3, 2018

glennw commented Apr 3, 2018

pcwalton commented Apr 3, 2018

glennw commented Apr 4, 2018

bors-servo commented Apr 4, 2018

bors-servo commented Apr 4, 2018

bors-servo commented Apr 4, 2018

ebraminio commented Jun 1, 2019

pcwalton commented Mar 23, 2018 •

edited by larsbergstrom

fschutt commented Mar 24, 2018 •

edited

glennw commented Apr 3, 2018 •

edited

pcwalton commented Apr 3, 2018 •

edited