Make offscreen canvas rendering context use offscreen canvas' size #24518

bblanke · 2019-10-21T16:46:30Z

Updated the offscreen canvas rendering context to use the offscreen canvas' size. This involved upgrading several methods to accept u64 sizes.

Additionally, the code in OffscreenCanvas::SetWidth() and OffscreenCanvas::SetHeight() was updated to send CanvasMsg::Recreate to the canvas paint thread.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Make offscreen canvas rendering context use offscreen canvas' size #24465
These changes do not require tests because the purpose of this was to tidy up code

…o using canvas size where needed; made canvas definite instead of optional. still need to change sizing in associated classes to use u64 instead of u32 removed size from offscreencanvasrenderingcontext2d.rs; now getting canvas size through referenced canvas Updated pixels::clip to use u64 data types and added trait to canvasrenderingcontext2d.rs to convert u32 sizes to u64 added Size2D<u32> to Size2D<u64> conversion to webglrenderingcontext and converted u32 sizes to u64 where relevant updated get_rect to use u64. beginning to make type changes to canvas.rs Updated canvas, canvas_data, and canvas_paint_thread to use u64 when relevant Added ability to Recreate canvas when OffscreenCanvas::SetWidth() or OffscreenCanvas::SetHeight() are called Fixed formatting; passes ./mach test-tidy 🎉

highfive · 2019-10-21T16:46:38Z

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

highfive · 2019-10-21T16:46:39Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/canvasrenderingcontext2d.rs, components/script/dom/offscreencanvas.rs, components/script/dom/imagedata.rs, components/script/dom/offscreencanvasrenderingcontext2d.rs, components/script/dom/webglrenderingcontext.rs
@KiChjang: components/script/dom/canvasrenderingcontext2d.rs, components/script/dom/offscreencanvas.rs, components/script/dom/imagedata.rs, components/script/dom/offscreencanvasrenderingcontext2d.rs, components/script/dom/webglrenderingcontext.rs

highfive · 2019-10-21T16:46:41Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify script code, but no tests are modified. Please consider adding a test!

jdm

Looks great! I just have a couple pieces of cleanup to suggest.

jdm · 2019-10-21T16:50:21Z

components/canvas/canvas_paint_thread.rs

                let image_data = self
                    .canvas(canvas_id)
-                    .read_pixels(source_rect.to_u32(), image_size.to_u32());
+                    .read_pixels(source_rect_u64, image_size.to_u64());


This could be source_rect.to_u64(), right?

jdm · 2019-10-21T16:51:00Z

components/canvas/canvas_data.rs

@@ -1189,3 +1189,26 @@ impl RectToi32 for Rect<f64> {
        )
    }
 }
+
+pub trait Size2DExt {


Let's use pub(crate) here and for RectExt_.

jdm · 2019-10-21T16:51:20Z

components/canvas/canvas_paint_thread.rs

@@ -207,3 +211,13 @@ impl<'a> CanvasPaintThread<'a> {
        self.canvases.get_mut(&canvas_id).expect("Bogus canvas id")
    }
 }
+
+pub trait Size2DExt {


We can have use crate::canvas_data::Size2DExt; instead of duplicating this.

jdm · 2019-10-21T16:56:23Z

components/script/dom/canvasrenderingcontext2d.rs

@@ -2123,3 +2136,13 @@ fn adjust_size_sign(
    }
    (origin, size.to_u32())
 }
+
+pub trait Size2DExt {


Let's define these in one file in components/script/ and import them from there in the other files in this crate.

jdm · 2019-10-21T17:58:44Z

components/canvas/canvas_data.rs

+    }
+}
+
+pub trait RectExt_ {


I filed servo/euclid#374 if you felt like submitting these methods upstream.

I submitted PR #375!

jdm · 2019-10-21T18:01:05Z

@bors-servo try=wpt

bors-servo · 2019-10-21T18:01:12Z

⌛ Trying commit 5ad0303 with merge e773ed0...

Make offscreen canvas rendering context use offscreen canvas' size Updated the offscreen canvas rendering context to use the offscreen canvas' size. This involved upgrading several methods to accept u64 sizes. Additionally, the code in OffscreenCanvas::SetWidth() and OffscreenCanvas::SetHeight() was updated to send CanvasMsg::Recreate to the canvas paint thread. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #24465 - [X] These changes do not require tests because the purpose of this was to tidy up code

bors-servo · 2019-10-21T18:29:23Z

💔 Test failed - linux-rel-css

jdm · 2019-10-21T18:33:47Z

  ▶ CRASH [expected OK] /2dcontext/compositing/2d.composite.canvas.source-over.html
  │ 
  │ 
  │ 
  │ 
  │ 
  │ assertion failed: Rect::from_size(size).contains_rect(&rect) (thread CanvasThread, at components/pixels/lib.rs:28)
  │ stack backtrace:
  │    0: servo::main::{{closure}}
  │    1: std::panicking::rust_panic_with_hook
  │              at src/libstd/panicking.rs:477
  │    2: std::panicking::begin_panic
  │    3: pixels::rgba8_get_rect
  │    4: canvas::canvas_data::CanvasData::read_pixels::{{closure}}
  │    5: canvas::azure_backend::<impl canvas::canvas_data::GenericDrawTarget for azure::azure_hl::DrawTarget>::snapshot_data
  │    6: canvas::canvas_data::CanvasData::read_pixels
  │    7: std::sys_common::backtrace::__rust_begin_short_backtrace
  │    8: __rust_maybe_catch_panic
  │              at src/libpanic_unwind/lib.rs:80
  │    9: core::ops::function::FnOnce::call_once{{vtable.shim}}
  │   10: <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once
  │              at /rustc/084beb83e0e87d673d5fabc844d28e8e8ae2ab4c/src/liballoc/boxed.rs:922
  │   11: <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once
  │              at /rustc/084beb83e0e87d673d5fabc844d28e8e8ae2ab4c/src/liballoc/boxed.rs:922
  │       std::sys_common::thread::start_thread
  │              at src/libstd/sys_common/thread.rs:13
  │       std::sys::unix::thread::Thread::new::thread_start
  │              at src/libstd/sys/unix/thread.rs:79
  │   12: start_thread
  │   13: clone

jdm · 2019-10-21T18:36:40Z

Looks like we'll want to be a bit more careful about our implementation of to_u64. Let's base it off of the same conversion methods from upstream: https://github.com/servo/euclid/blob/6dcc86f0e5370b5be45a955239131293bd336c66/src/rect.rs#L539-L546

Make offscreen canvas rendering context use offscreen canvas' size; Consolidate size helpers  Addresses issues raised in the review of PR #24518 and includes changes to 17 tests' metadata for those that now PASS. Contains fixes in PR #24518: Updated the offscreen canvas rendering context to use the offscreen canvas' size. This involved upgrading several methods to accept u64 sizes. Additionally, the code in OffscreenCanvas::SetWidth() and OffscreenCanvas::SetHeight() was updated to send CanvasMsg::Recreate to the canvas paint thread. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #24465  - [X] There are tests for these changes – 17 were updated to PASS

Make offscreen canvas rendering context use offscreen canvas' size; Consolidate size helpers  Addresses issues raised in the review of PR #24518 and includes changes to 17 tests' metadata for those that now PASS. Contains fixes in PR #24518: Updated the offscreen canvas rendering context to use the offscreen canvas' size. This involved upgrading several methods to accept u64 sizes. Additionally, the code in OffscreenCanvas::SetWidth() and OffscreenCanvas::SetHeight() was updated to send CanvasMsg::Recreate to the canvas paint thread. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #24465 and fix #24536  - [X] There are tests for these changes – 17 were updated to PASS

highfive assigned paulrouget Oct 21, 2019

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 21, 2019

jdm requested changes Oct 21, 2019

View reviewed changes

jdm changed the title ~~Fix: Make offscreen canvas rendering context use offscreen canvas' size #24465~~ Make offscreen canvas rendering context use offscreen canvas' size Oct 21, 2019

highfive added the S-tests-failed The changes caused existing tests to fail. label Oct 21, 2019

jdm assigned jdm and unassigned paulrouget Oct 21, 2019

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 Oct 21, 2019

bblanke mentioned this pull request Oct 22, 2019

Make offscreen canvas rendering context use offscreen canvas' size; Consolidate size helpers #24524

Merged

4 tasks

bblanke closed this Oct 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make offscreen canvas rendering context use offscreen canvas' size #24518

Make offscreen canvas rendering context use offscreen canvas' size #24518

bblanke commented Oct 21, 2019

highfive commented Oct 21, 2019

highfive commented Oct 21, 2019

highfive commented Oct 21, 2019

jdm left a comment

jdm Oct 21, 2019

jdm Oct 21, 2019

jdm Oct 21, 2019

jdm Oct 21, 2019

jdm Oct 21, 2019

bblanke Oct 21, 2019

jdm commented Oct 21, 2019

bors-servo commented Oct 21, 2019

bors-servo commented Oct 21, 2019

jdm commented Oct 21, 2019

jdm commented Oct 21, 2019

Make offscreen canvas rendering context use offscreen canvas' size #24518

Make offscreen canvas rendering context use offscreen canvas' size #24518

Conversation

bblanke commented Oct 21, 2019

highfive commented Oct 21, 2019

highfive commented Oct 21, 2019

highfive commented Oct 21, 2019

jdm left a comment

Choose a reason for hiding this comment

jdm Oct 21, 2019

Choose a reason for hiding this comment

jdm Oct 21, 2019

Choose a reason for hiding this comment

jdm Oct 21, 2019

Choose a reason for hiding this comment

jdm Oct 21, 2019

Choose a reason for hiding this comment

jdm Oct 21, 2019

Choose a reason for hiding this comment

bblanke Oct 21, 2019

Choose a reason for hiding this comment

jdm commented Oct 21, 2019

bors-servo commented Oct 21, 2019

bors-servo commented Oct 21, 2019

jdm commented Oct 21, 2019

jdm commented Oct 21, 2019