Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use raqote for canvas 2D rendering #23601

Closed
wants to merge 8 commits into from
Closed

Conversation

@pylbrecht
Copy link
Contributor

pylbrecht commented Jun 20, 2019


  • ./mach build -d --features raqote_backend does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #23431

This change is Reviewable

@pylbrecht
Copy link
Contributor Author

pylbrecht commented Jun 20, 2019

I openend this PR to track my progress.

@pylbrecht
Copy link
Contributor Author

pylbrecht commented Jun 23, 2019

I ran into some problems while trying to implement GenericPathBuilder for raqote::PathBuilder:
Unfortunately I missed the fact that raqote::PathBuilder's methods require a &mut self when creating GenericPathBuilder. I was just paying attention to azure's code, which is happy with just &self.
I feel the solution to this is changing the GenericPathBuilder trait (and adapting the implementation for azure::PathBuilder, if want to preserve successful builds for both azure and raqote). Is there an easier way to fix this?

@jdm
Copy link
Member

jdm commented Jun 23, 2019

The other option is using RefCell, which can call borrow_mut to obtain a &mut value.

@jdm jdm removed the S-awaiting-review label Jun 24, 2019
@pylbrecht pylbrecht force-pushed the pylbrecht:raqote branch from c3a5733 to fd44556 Jun 27, 2019
@jdm jdm removed the S-awaiting-review label Jun 28, 2019
@pylbrecht
Copy link
Contributor Author

pylbrecht commented Jun 30, 2019

I'm currently facing similar issues implementing GenericDrawTarget for raqote::DrawTarget as I did while implementing GenericPathBuilder for raqote::PathBuilder:
Some methods of raqote::DrawTarget take a &mut self, where GenericDrawTarget takes just &self.

Changing the interface of GenericDrawTarget to take &mut self results in many errors in CanvasData's methods, which themselves call methods on the drawtarget member.
It's usually in the form of the one below:

error[E0502]: cannot borrow `*self.drawtarget` as mutable because it is also borrowed as immutable
   --> components/canvas/canvas_data.rs:663:9
    |
663 |         self.drawtarget.push_clip(&self.path());
    |         ^^^^^^^^^^^^^^^^---------^^----^^^^^^^^
    |         |               |          |
    |         |               |          immutable borrow occurs here
    |         |               immutable borrow later used by call
    |         mutable borrow occurs here

error: aborting due to previous error

I fiddled with this for some time now, but I'm not getting anywhere without some help.
How do I get around this ownership issue?

@jdm
Copy link
Member

jdm commented Jul 1, 2019

let path = self.path();
self.drawtarget.push_clio(&path);
@pylbrecht
Copy link
Contributor Author

pylbrecht commented Jul 1, 2019

I tried that before and it resulted in the same error:

error[E0502]: cannot borrow `*self.drawtarget` as mutable because it is also borrowed as immutable
   --> components/canvas/canvas_data.rs:664:9
    |
663 |         let path = self.path();
    |                    ---- immutable borrow occurs here
664 |         self.drawtarget.push_clip(&path);
    |         ^^^^^^^^^^^^^^^^---------^^^^^^^
    |         |               |
    |         |               immutable borrow later used by call
    |         mutable borrow occurs here

error: aborting due to previous error

I feel it may be easier to discuss this on IRC, so I try to hit you up in #servo this week. Currently my "contribution time" is limited to the weekends, but for the sake of not being blocked, I'm going to make some extra time for this during (your) work hours, if that's fine with you?

@pylbrecht pylbrecht force-pushed the pylbrecht:raqote branch from 28e3e22 to 46caa12 Jul 2, 2019
@pylbrecht
Copy link
Contributor Author

pylbrecht commented Jul 2, 2019

Update: I pushed the latest changes (46caa12), which make some of GenericDrawTarget's methods take &mut self, so I can implement the trait for raqote (some methods of raqote::DrawTarget require &mut self).
Unfortunately this results in some ownership issues, which I wasn't able to figure out by myself.

@jdm agreed to help me out by looking over these changes, when he's free, but I guess anyone's opinion is welcome. My Rust skills are still on a beginner level, so these issues might be a no-brainer.

@jdm
Copy link
Member

jdm commented Jul 4, 2019

@pylbrecht I resolved the remaining errors stemming from the mutability changes, and I dealt with the warning about the raqote GenericPathBuilder implementation recursively calling itself. This required adding a bit of indirection via an Option to support the fact that raqote::PathBuilder::finish consumes the calling object, unlike azure.

@pylbrecht
Copy link
Contributor Author

pylbrecht commented Jul 4, 2019

@jdm, thank you! I'll try to comprehend your changes, so I get an idea of how to approach such errors by myself in the future. Thanks again for your quick response!

@jdm jdm removed the S-awaiting-review label Jul 4, 2019
@jdm jdm removed the S-awaiting-review label Jul 10, 2019
@jdm
Copy link
Member

jdm commented Aug 8, 2019

Closing since #23936 is the most up to date version of this work.

@jdm jdm closed this Aug 8, 2019
bors-servo added a commit that referenced this pull request Aug 21, 2019
Implement the raqote backend

Rebased version of #23601 with some more stuff implemented. This passes some (not many) wpt tests.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23936)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Aug 22, 2019
Implement the raqote backend

Rebased version of #23601 with some more stuff implemented. This passes some (not many) wpt tests.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23936)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Aug 22, 2019
Implement the raqote backend

Rebased version of #23601 with some more stuff implemented. This passes some (not many) wpt tests.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23936)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.