Final compile fixes for pathfinder #2723

fschutt · 2018-05-03T06:42:52Z

Closes #2645.

Note: On Windows, pathfinder won't yet run properly, partly due to https://github.com/pcwalton/pathfinder/pull/82 and partly due to some RenderTask-related bugs (currently it crashes with "unexpected render task: glyph task") in webrender::render_task::RenderTask::uv_rect_kind.

However, pathfinder now at least compiles on Windows, Linux and Mac. Also added various #[derive(Debug)] necessary for debugging the pathfinder crashes.

This change is


        Implemented ThreadSafePathfinderFontContext to fix feature="pathfinde…

…r" compile errors on Windows Fixes #2645. PathfinderFontContext can contain a raw *mut IDWriteFactory (on Windows). However, since we know that it is wrapped in a Mutex here, it is safe to assume that the struct is thread-safe. PathfinderComPtr::new is an unsafe function, so we have to wrap it in an unsafe block.


        Derive Debug for various RenderTask-related structs

glennw · 2018-05-03T06:46:52Z

r? @pcwalton

kvark · 2018-05-03T14:58:00Z

Can we have that path tested on CI?

fschutt · 2018-05-03T15:00:26Z

Yes although, as I said, it will crash at runtime. Compilation tests should work, though.

fschutt · 2018-05-03T15:23:55Z

@kvark Would it be enough to do only appveyor + taskcluster? Linux + macOS would already be tested on taskcluster, so no need for travis to re-check for that...


        Added --no-default-features --features pathfinder to CI

Verify that the patfinder feature compiles on Windows, Mac and Linux

kvark · 2018-05-03T15:38:34Z

@fschutt I think we should keep Travis is sync with TaskCluster in what it's building for as long as Travis is useful, to avoid confusion at least.


        Updated pathfinder in Cargo.lock, refactored conditional imports

kvark · 2018-05-03T17:49:44Z

webrender/src/glyph_rasterizer.rs

+        use std::ops::Deref;
+        use tiling::RenderTargetKind;
+        use webrender_api::{DeviceIntPoint, DevicePixel};
+    } else if #[cfg(not(feature = "pathfinder"))] {


does this need if after else?

kvark · 2018-05-03T17:49:44Z

webrender/src/glyph_rasterizer.rs

 use tiling::SpecialRenderPasses;
-#[cfg(feature = "pathfinder")]
-use webrender_api::{DeviceIntPoint, DevicePixel};
+cfg_if! {


The amount of diversion between the pathfinder and whatnot features of this module is pretty high. We should consider splitting those into separate modules, potentially leaving something shared in another module. This would be cleaner than multiple #[cfg(feature = "pathfinder")] spread over the code.

Yeah ok, but this is going to take a while to split up the files and resolve all the errors. I'll do two seperate files for the pathfinder and the no_pathfinder configuration.

btw, is there any difference between use api:: and use webrender_api:: or are they the same?

No, this is because of this line in "lib.rs": pub use webrender_api as api;
I'd much prefer us just doing pub extern crate webrender_api as api; in the first place, without any ambiguity.

But, are you ok with the rest of the changes? If I push the refactored glyph_rasterizer module, this will create a huge commit, so I'd prefer a review before I split up the module. Semanticall it'll all be the same, just the PR will be larger.

Yes, although TaskCluster CI is not ok :)

pcwalton · 2018-05-04T01:12:03Z

webrender/src/glyph_rasterizer/glyph_rasterizer_ext_pathfinder.rs

+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+//! Module only available when pathfinder is activated


How about just naming this pathfinder.rs? It's shorter :)


        Refactored glyph_rasterizer into folder with seperate modules

This commit doesn't change any semantics, but it exposes the files `glyph_rasterizer_ext_no_pathfinder` (containing only the items previously marked with cfg(not(feature = "pathfinder")) and the `glyph_rasterizer_pathfinder`, which has all previous cfg(feature = "pathfinder") items. The public / private visibilities of the structs are the same as before.

fschutt · 2018-05-04T13:49:52Z

@kvark @pcwalton All done, tests passed. Ready to merge.

kvark · 2018-05-04T17:04:09Z

Reviewed 2 of 4 files at r1, 3 of 3 files at r2, 2 of 3 files at r3, 1 of 6 files at r4, 3 of 6 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

appveyor.yml, line 23 at r5 (raw file):

  - cd ../webrender
  - cargo test --verbose
  - cargo build --verbose --no-default-features --features pathfinder

somewhat less relevant, I wonder if we can use cargo check instead of cargo build on CI to speed it up a bit

webrender/src/glyph_rasterizer/no_pathfinder.rs, line 24 at r5 (raw file):

use std::collections::hash_map::Entry;

pub(in super) fn create_pathfinder_font_context() -> Result<(), ResourceCacheError> {

why do we need pathfinder-related methods in this module at all?

Comments from Reviewable

fschutt · 2018-05-05T10:37:22Z

Review status: all files reviewed at latest revision, 5 unresolved discussions.

appveyor.yml, line 23 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

somewhat less relevant, I wonder if we can use cargo check instead of cargo build on CI to speed it up a bit

I think we can change from "cargo build" to "cargo check" whenever we are not doing anything with the final binary. I.e. we just see if it compiles, then throw the binary away. I am not aware of checks where you have to compile the crate in order to verify that it compiles. I'll change it for all of the CI.

webrender/src/glyph_rasterizer/no_pathfinder.rs, line 24 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

why do we need pathfinder-related methods in this module at all?

The FontContexts struct currently contains a pathfinder_context field, although that is unused on non-pathfinder builds. I'll change that, thanks.

Comments from Reviewable


        Moved from `cargo build` to `cargo check` to speed up CI

fschutt · 2018-05-07T13:44:29Z

@kvark Just notifying you that the issues are fixed now. I changed cargo build to cargo check and removed the pathfinder-related functions from the no_pathfinder module.

kvark · 2018-05-07T16:19:39Z

Looks like we are almost there!

Reviewed 5 of 6 files at r6.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

webrender/src/glyph_rasterizer/mod.rs, line 355 at r6 (raw file):

                worker_contexts: contexts,
                shared_context: Mutex::new(shared_context),
                pathfinder_context: create_pathfinder_font_context()?,

let's put the cfg on those extra fields as opposed to the whole structure initialization

Comments from Reviewable


        Removed pathfinder-related functions from no_pathfinder module

This now uses compile-time flags instead of an empty `()` field for the pathfinder_context.

fschutt · 2018-05-08T13:24:34Z

Review status: 10 of 11 files reviewed at latest revision, 4 unresolved discussions.

webrender/src/glyph_rasterizer.rs, line 47 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

does this need if after else?

Done.

webrender/src/glyph_rasterizer/mod.rs, line 355 at r6 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

let's put the cfg on those extra fields as opposed to the whole structure initialization

Done.

Comments from Reviewable

kvark · 2018-05-08T13:28:38Z

Reviewed 1 of 1 files at r7.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

kvark · 2018-05-08T13:30:11Z

As this is just moving things around, I don't expect any reftest failures, but we'll stay on guard.
Thank you for pushing this to the end!
@bors-servo r+

bors-servo · 2018-05-08T13:30:12Z

📌 Commit f06f503 has been approved by kvark

bors-servo · 2018-05-08T13:58:11Z

⌛ Testing commit f06f503 with merge 1eb46da...


        Auto merge of #2723 - fschutt:pathfinder_final_compile_fix_windows, r…

…=kvark Final compile fixes for pathfinder Closes #2645. Note: On Windows, pathfinder won't yet run properly, partly due to https://github.com/pcwalton/pathfinder/pull/82 and partly due to some `RenderTask`-related bugs (currently it crashes with "unexpected render task: glyph task") in ` webrender::render_task::RenderTask::uv_rect_kind`. However, pathfinder now at least compiles on Windows, Linux and Mac. Also added various `#[derive(Debug)]` necessary for debugging the pathfinder crashes.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2723)

bors-servo · 2018-05-08T15:21:51Z

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 1eb46da to master...

fschutt added 2 commits May 3, 2018

Derive Debug for various RenderTask-related structs

Loading status checks…

ca8f177

Added --no-default-features --features pathfinder to CI

Loading status checks…

c2989a3

Verify that the patfinder feature compiles on Windows, Mac and Linux

Updated pathfinder in Cargo.lock, refactored conditional imports

Loading status checks…

f7474a8

kvark reviewed May 3, 2018

View changes

kvark approved these changes May 3, 2018

View changes

pcwalton reviewed May 4, 2018

View changes

Moved from `cargo build` to `cargo check` to speed up CI

c48c4b6

Removed pathfinder-related functions from no_pathfinder module

Loading status checks…

f06f503

This now uses compile-time flags instead of an empty `()` field for the pathfinder_context.

fschutt mentioned this pull request May 30, 2018

webrender crashes with feature=pathfinder on Windows #2791

Open

servo / webrender

Final compile fixes for pathfinder #2723

Final compile fixes for pathfinder #2723

fschutt commented May 3, 2018 •

edited by larsbergstrom

glennw commented May 3, 2018

kvark commented May 3, 2018

fschutt commented May 3, 2018

fschutt commented May 3, 2018

kvark commented May 3, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

fschutt commented May 4, 2018

kvark commented May 4, 2018

fschutt commented May 5, 2018

fschutt commented May 7, 2018

kvark commented May 7, 2018

fschutt commented May 8, 2018

kvark commented May 8, 2018

kvark commented May 8, 2018

bors-servo commented May 8, 2018

bors-servo commented May 8, 2018

bors-servo commented May 8, 2018

servo / webrender

Join GitHub today

Final compile fixes for pathfinder #2723

Final compile fixes for pathfinder #2723

Conversation

fschutt commented May 3, 2018 • edited by larsbergstrom

glennw commented May 3, 2018

kvark commented May 3, 2018

fschutt commented May 3, 2018

fschutt commented May 3, 2018

kvark commented May 3, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

fschutt commented May 4, 2018

kvark commented May 4, 2018

fschutt commented May 5, 2018

fschutt commented May 7, 2018

kvark commented May 7, 2018

fschutt commented May 8, 2018

kvark commented May 8, 2018

kvark commented May 8, 2018

bors-servo commented May 8, 2018

bors-servo commented May 8, 2018

bors-servo commented May 8, 2018

fschutt commented May 3, 2018 •

edited by larsbergstrom