NCSU Canvas Rendering Project Initial Steps #20447

Brody-Eastwood · 2018-03-27T02:53:48Z

Implements the initial steps from:

https://github.com/servo/servo/wiki/Canvas-rendering-project

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2018-03-27T02:53:54Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @pcwalton (or someone else) soon.

highfive · 2018-03-27T02:53:54Z

Heads up! This PR modifies the following files:

@emilio: components/layout/fragment.rs, components/layout/display_list/builder.rs
@fitzgen: components/script/dom/paintrenderingcontext2d.rs, components/script/dom/canvasrenderingcontext2d.rs, components/script_traits/script_msg.rs, components/script/dom/bindings/trace.rs, components/script/dom/htmlcanvaselement.rs
@KiChjang: components/script/dom/paintrenderingcontext2d.rs, components/script/dom/canvasrenderingcontext2d.rs, components/script_traits/script_msg.rs, components/script/dom/bindings/trace.rs, components/script/dom/htmlcanvaselement.rs
@asajeffrey: components/script/dom/paintrenderingcontext2d.rs, components/script/dom/canvasrenderingcontext2d.rs, components/constellation/constellation.rs, components/script/dom/bindings/trace.rs, components/script/dom/htmlcanvaselement.rs
@cbrewster: components/constellation/constellation.rs
@paulrouget: components/constellation/constellation.rs

highfive · 2018-03-27T02:53:56Z

Warning

These commits modify layout and script code, but no tests are modified. Please consider adding a test!

jdm

Great work!

jdm · 2018-03-27T03:04:15Z

components/canvas/canvas_paint_thread.rs

@@ -99,7 +100,8 @@ impl<'a> CanvasPaintState<'a> {
 impl<'a> CanvasPaintThread<'a> {
    fn new(size: Size2D<i32>,
           webrender_api_sender: webrender_api::RenderApiSender,
-           antialias: AntialiasMode) -> CanvasPaintThread<'a> {
+           antialias: AntialiasMode,
+              canvas_id: CanvasId) -> CanvasPaintThread<'a> {


nit: indentation

jdm · 2018-03-27T03:04:45Z

components/canvas/canvas_paint_thread.rs

        }
    }

    /// Creates a new `CanvasPaintThread` and returns an `IpcSender` to
    /// communicate with it.
    pub fn start(size: Size2D<i32>,
                 webrender_api_sender: webrender_api::RenderApiSender,
-                 antialias: bool)
+                 antialias: bool,
+                  canvas_id: CanvasId)


nit: indentation

jdm · 2018-03-27T03:08:23Z

components/constellation/constellation.rs

-                                              opts::get().enable_canvas_antialiasing);
-        if let Err(e) = response_sender.send(sender) {
+                                              opts::get().enable_canvas_antialiasing,self.canvas_id.clone());
+        if let Err(e) = response_sender.send((sender,self.canvas_id.clone())) {


nit: add a space after , in both these lines.

jdm · 2018-03-27T03:09:29Z

components/layout/display_list/builder.rs

@@ -1800,7 +1800,7 @@ impl FragmentDisplayListBuilding for Fragment {
                            let ipc_renderer = ipc_renderer.lock().unwrap();
                            let (sender, receiver) = ipc::channel().unwrap();
                            ipc_renderer
-                                .send(CanvasMsg::FromLayout(FromLayoutMsg::SendData(sender)))
+                                .send(CanvasMsg::FromLayout(FromLayoutMsg::SendData(sender), canvas_fragment_info.canvas_id.clone()))


nit: move this argument onto the next line so it's easier to read.

jdm · 2018-03-27T03:10:34Z

components/script/dom/canvasrenderingcontext2d.rs

@@ -133,7 +134,8 @@ impl CanvasRenderingContext2D {
        let script_to_constellation_chan = global.script_to_constellation_chan();
        debug!("Asking constellation to create new canvas thread.");
        script_to_constellation_chan.send(ScriptMsg::CreateCanvasPaintThread(size, sender)).unwrap();
-        let ipc_renderer = receiver.recv().unwrap();
+        let response = receiver.recv().unwrap();
+        let (ipc_renderer, canvas_id) = response;


These lines can be combined:

let (ipc_render, canvas_id) = receiver.recv().unwrap();

jdm · 2018-03-27T03:18:51Z

components/script_traits/script_msg.rs

@@ -11,7 +11,7 @@ use LayoutControlMsg;
 use LoadData;
 use WorkerGlobalScopeInit;
 use WorkerScriptLoadOrigin;
-use canvas_traits::canvas::CanvasMsg;
+use canvas_traits::canvas::{CanvasMsg,CanvasId};


nit: space after ,

jdm · 2018-03-27T03:18:58Z

components/script_traits/script_msg.rs

@@ -79,7 +79,7 @@ pub enum ScriptMsg {
    ChangeRunningAnimationsState(AnimationState),
    /// Requests that a new 2D canvas thread be created. (This is done in the constellation because
    /// 2D canvases may use the GPU and we don't want to give untrusted content access to the GPU.)
-    CreateCanvasPaintThread(Size2D<i32>, IpcSender<IpcSender<CanvasMsg>>),
+    CreateCanvasPaintThread(Size2D<i32>, IpcSender<(IpcSender<CanvasMsg>,CanvasId)>),


nit: space after ,

jdm · 2018-03-27T03:19:21Z

tests/html/test_canvas_drawimage_canvas.html

-  try {
-    ctx2.drawImage.apply(ctx2, args);
+    try {
+        ctx2.drawImage.apply(ctx2, args);


nit: revert the formatting changes in this file.

jdm · 2018-03-27T03:19:45Z

tests/html/test_canvas_drawimage_canvas_timed.html

+};
+
+    var t0 = performance.now();     //save starting time
+        drawImage(25, 25);            // The source canvas will copied to the 0,0 position of the destination canvas


nit: odd indentation on this line.

jdm · 2018-03-27T03:20:19Z

tests/html/test_canvas_drawimage_canvas_timed.html

+
+  args.unshift(canvas1);
+    try {
+        ctx2.drawImage.apply(ctx2, args);


nit: odd indentation on these lines.

jdm · 2018-03-28T12:55:28Z

./mach test-tidy is complaining:

./components/layout/fragment.rs:11: missing space after ,
./components/script/dom/bindings/trace.rs:34: use statement is not in alphabetical order
	expected: canvas_traits::canvas::CanvasId
	found: canvas_traits::canvas::{CanvasGradientStop, LinearGradientStyle, RadialGradientStyle}

Please squash all the commits into one as well.

pmocher · 2018-03-30T03:39:35Z

Hi! This is my first time squashing all of the commits into one. I probably did something that I wasn't supposed to.

jdm · 2018-03-30T03:43:08Z

Yeah, if the pull request still shows >1 commit after refreshing, it didn't achieve the desired effect. Be sure to run git rebase -i 5f62a1789a9bf1f4ddc736dd131d0fe4269242c3^ and mark all the commits as s instead of pick except for the topmost one.

pmocher · 2018-03-30T03:48:34Z

I see. I'll do that now.

servo-wpt-sync · 2018-03-30T04:45:05Z

Error syncing changes upstream. Logs saved in error-snapshot-1522386139768.

servo-wpt-sync · 2018-03-30T04:49:08Z

Error syncing changes upstream. Logs saved in error-snapshot-1522386382447.

servo-wpt-sync · 2018-03-30T04:51:18Z

Error syncing changes upstream. Logs saved in error-snapshot-1522386512547.

pmocher · 2018-03-30T04:57:09Z

It seems as though I've messed something up.

jdm · 2018-03-30T05:07:54Z

Indeed. I recommend running git reflog, copying the hash of the entry that corresponds to the last commit you made that appears in that list of git actions, and running git reset --hard [hash]. This should reset the state of your repository to the pre-squash state.

jdm · 2018-03-30T23:04:57Z

Ok, all of the automated tests that use a canvas are panicking with stacks like this:

  ▶ CRASH [expected PASS] /2dcontext/line-styles/canvas_linestyles_linecap_001.htm
  │ 
  │ VMware, Inc.
  │ softpipe
  │ 3.3 (Core Profile) Mesa 17.3.0-devel
  │ assertion failed: thread_state::get().is_script() (thread LayoutThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(NonZero(NonZero(1))) }, at components/script/dom/bindings/cell.rs:98)
  │ stack backtrace:
  │    0:     0x7f418c1b2d6c - backtrace::backtrace::trace::heb9efa3021143bc0
  │    1:     0x7f418c1b20f2 - backtrace::capture::Backtrace::new::hd20e533c9c999e36
  │    2:     0x7f41891ae90c - servo::main::{{closure}}::h96f2fd035380f7d9
  │    3:     0x7f418c1c1905 - std::panicking::rust_panic_with_hook::h489260a27605eec3
  │                         at libstd/panicking.rs:577
  │    4:     0x7f418a2c1fc3 - std::panicking::begin_panic::h88970b163c1446a9
  │    5:     0x7f4189b2a7dd - script::dom::htmlcanvaselement::HTMLCanvasElement::get_canvas_id::h8af515f893db25df
  │    6:     0x7f4189824166 - <script::dom::bindings::root::LayoutDom<script::dom::htmlcanvaselement::HTMLCanvasElement> as script::dom::htmlcanvaselement::LayoutHTMLCanvasElementHelpers>::data::hb960f9960efcb3dc
  │    7:     0x7f418982de02 - <script::dom::bindings::root::LayoutDom<script::dom::node::Node> as script::dom::node::LayoutNodeHelpers>::canvas_data::hc4d0763ec1019568
  │    8:     0x7f41895358fe - <layout::construct::FlowConstructor<'a, ConcreteThreadSafeLayoutNode>>::build_fragment_for_block::h313458c542f016e8
  │    9:     0x7f41895234bf - <layout::construct::FlowConstructor<'a, ConcreteThreadSafeLayoutNode> as layout::traversal::PostorderNodeMutTraversal<ConcreteThreadSafeLayoutNode>>::process::ha6229ff0a27676fd
  │   10:     0x7f4189630eef - style::traversal::DomTraversal::handle_postorder_traversal::h992f91b2749e6bd8
  │   11:     0x7f41895ca3dc - style::driver::traverse_dom::h9b6af5485b4e1784
  │   12:     0x7f41894da9d0 - layout_thread::LayoutThread::handle_reflow::hafe580cf2b79e1af
  │   13:     0x7f41894d447c - layout_thread::LayoutThread::handle_request_helper::hb3abd95d5d0b09c0
  │   14:     0x7f41894d2d79 - layout_thread::LayoutThread::start::h9537b41eb8b948cf
  │   15:     0x7f41895ba063 - profile_traits::mem::ProfilerChan::run_with_memory_reporting::hc7bbe6eff5174296
  │   16:     0x7f418953f62b - std::sys_common::backtrace::__rust_begin_short_backtrace::h7bf083075dc64c41
  │   17:     0x7f41894b54c7 - _ZN3std9panicking3try7do_call17hdeaf1406e5993f92E.llvm.9F59020
  │   18:     0x7f418c1eaf9e - __rust_maybe_catch_panic
  │                         at libpanic_unwind/lib.rs:102
  │   19:     0x7f418959f23a - <F as alloc::boxed::FnBox<A>>::call_box::hdb8301d823bd613e
  │   20:     0x7f418c1d98a7 - <alloc::boxed::Box<alloc::boxed::FnBox<A, Output$u3d$R$GT$$u20$$u2b$$u20$$u27$a$GT$$u20$as$u20$core..ops..function..FnOnce$LT$A$GT$$GT$::call_once::h4bbca053e9002218
  │                         at /checkout/src/liballoc/boxed.rs:798
  │                          - std::sys_common::thread::start_thread::h3301b0b4cb89e2c1
  │                         at libstd/sys_common/thread.rs:24
  │   21:     0x7f418c1c1f68 - std::sys::unix::thread::Thread::new::thread_start::h6f2358c4012c0e12
  │                         at libstd/sys/unix/thread.rs:90
  │   22:     0x7f418809f183 - start_thread
  │   23:     0x7f4186966ffc - clone
  │   24:                0x0 - <unknown>

This means that LayoutHTMLCanvasElementHelpers::data is invoking HTMLCanvasElement::get_canvas_id, which calls DomRefCell::borrow on self.context. This fails assertions we have in place about which thread is active when this borrow operation occurs, so you'll need to write a get_canvas_id_for_layout helper method that duplicates get_canvas_id but uses borrow_for_layout instead of borrow. Sorry, it's not a great system and we haven't spent time on creating something with fewer surprises.

jdm · 2018-03-30T23:06:07Z

cc @SimonSapin so you're aware of what's happening in this PR.

jdm · 2018-04-01T16:21:49Z

@bors-servo r+

bors-servo · 2018-04-01T16:21:49Z

📌 Commit 3b5cdec has been approved by jdm

bors-servo · 2018-04-01T16:53:10Z

⌛ Testing commit 3b5cdec with merge 3381ade...

NCSU Canvas Rendering Project Initial Steps  Implements the initial steps from: https://github.com/servo/servo/wiki/Canvas-rendering-project ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20447)

bors-servo · 2018-04-01T17:18:49Z

💔 Test failed - linux-rel-css

jdm · 2018-04-02T02:19:07Z

Note: to run these 2d and 3d canvas tests locally, use ./mach test-wpt tests/wpt/web-platform-tests/2dcontext/transformations/canvas_transformations_reset_001.html and ./mach test-wpt tests/wpt/mozilla/tests/webgl/conformance-2.0.0/conformance2/samplers/sampler-drawing-test.html. This will verify that any changes made actually allow the tests to run correctly.

jdm · 2018-04-02T02:36:54Z

components/script/dom/htmlcanvaselement.rs

+        unsafe {
+            let canvas = &*self.unsafe_get();
+            if let &Some(CanvasContext::Context2d(ref context)) = canvas.context.borrow_for_layout() {
+                context.get_canvas_id()


Sorry, one more change is required. There needs to be a get_canvas_id_for_layout method added to LayoutCanvasRenderingContext2DHelpers in canvasrenderingcontext2d.rs. This line should then read context.to_layout().get_canvas_id().

Brody-Eastwood · 2018-04-03T02:04:03Z

Those tests succeeded locally! I'm really hoping we'll see success fully this time!

jdm · 2018-04-03T02:24:30Z

@bors-servo r+

bors-servo · 2018-04-03T02:24:31Z

📌 Commit 8a1590e has been approved by jdm

bors-servo · 2018-04-03T02:24:35Z

⌛ Testing commit 8a1590e with merge 9cd60c8...

NCSU Canvas Rendering Project Initial Steps  Implements the initial steps from: https://github.com/servo/servo/wiki/Canvas-rendering-project ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20447)

bors-servo · 2018-04-03T03:24:34Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 9cd60c8 to master...

highfive assigned pcwalton Mar 27, 2018

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 27, 2018

jdm requested changes Mar 27, 2018

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 27, 2018

jdm assigned jdm and unassigned pcwalton Mar 27, 2018

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Mar 28, 2018

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 28, 2018

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Mar 30, 2018

pmocher force-pushed the master branch from bad5c08 to 33bb495 Compare March 30, 2018 03:35

pmocher force-pushed the master branch from 33bb495 to 0e0b729 Compare March 30, 2018 04:44

pmocher force-pushed the master branch from 0e0b729 to 56712ef Compare March 30, 2018 04:48

pmocher force-pushed the master branch from 56712ef to 535d8e1 Compare March 30, 2018 04:51

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Apr 1, 2018

Brody-Eastwood force-pushed the master branch from 57166fe to 3b5cdec Compare April 1, 2018 14:50

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 1, 2018

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Apr 1, 2018

jdm requested changes Apr 2, 2018

View reviewed changes

Add unique canvas IDs to all canvas operations.

8a1590e

Brody-Eastwood force-pushed the master branch from 3b5cdec to 8a1590e Compare April 3, 2018 02:02

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Apr 3, 2018

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 3, 2018

bors-servo merged commit 8a1590e into servo:master Apr 3, 2018

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 3, 2018

NCSU Canvas Rendering Project Initial Steps #20447

NCSU Canvas Rendering Project Initial Steps #20447

Conversation

Brody-Eastwood commented Mar 27, 2018 • edited by SimonSapin

highfive commented Mar 27, 2018

highfive commented Mar 27, 2018

highfive commented Mar 27, 2018

jdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdm commented Mar 28, 2018

pmocher commented Mar 30, 2018

jdm commented Mar 30, 2018

pmocher commented Mar 30, 2018

servo-wpt-sync commented Mar 30, 2018

servo-wpt-sync commented Mar 30, 2018

servo-wpt-sync commented Mar 30, 2018

pmocher commented Mar 30, 2018

jdm commented Mar 30, 2018

jdm commented Mar 30, 2018 • edited

jdm commented Mar 30, 2018

jdm commented Apr 1, 2018

bors-servo commented Apr 1, 2018

bors-servo commented Apr 1, 2018

bors-servo commented Apr 1, 2018

jdm commented Apr 2, 2018

Choose a reason for hiding this comment

Brody-Eastwood commented Apr 3, 2018

jdm commented Apr 3, 2018

bors-servo commented Apr 3, 2018

bors-servo commented Apr 3, 2018

bors-servo commented Apr 3, 2018

Brody-Eastwood commented Mar 27, 2018 •

edited by SimonSapin

jdm commented Mar 30, 2018 •

edited