From 62ea3c093a48362578b1f49d5a65270702015839 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Wed, 3 Oct 2018 12:11:28 +0200 Subject: [PATCH 1/3] Move canvas.putImageData checks to the DOM side --- components/canvas/canvas_data.rs | 47 +----------- .../script/dom/canvasrenderingcontext2d.rs | 71 ++++++++++++++++--- 2 files changed, 61 insertions(+), 57 deletions(-) diff --git a/components/canvas/canvas_data.rs b/components/canvas/canvas_data.rs index c91dbd8c826..c422cdd0aec 100644 --- a/components/canvas/canvas_data.rs +++ b/components/canvas/canvas_data.rs @@ -454,57 +454,12 @@ impl<'a> CanvasData<'a> { imagedata: Vec, offset: Vector2D, image_data_size: Size2D, - mut dirty_rect: Rect + dirty_rect: Rect ) { - if image_data_size.width <= 0.0 || image_data_size.height <= 0.0 { - return - } - assert_eq!(image_data_size.width * image_data_size.height * 4.0, imagedata.len() as f64); - // Step 1. TODO (neutered data) - - // Step 2. - if dirty_rect.size.width < 0.0f64 { - dirty_rect.origin.x += dirty_rect.size.width; - dirty_rect.size.width = -dirty_rect.size.width; - } - - if dirty_rect.size.height < 0.0f64 { - dirty_rect.origin.y += dirty_rect.size.height; - dirty_rect.size.height = -dirty_rect.size.height; - } - - // Step 3. - if dirty_rect.origin.x < 0.0f64 { - dirty_rect.size.width += dirty_rect.origin.x; - dirty_rect.origin.x = 0.0f64; - } - - if dirty_rect.origin.y < 0.0f64 { - dirty_rect.size.height += dirty_rect.origin.y; - dirty_rect.origin.y = 0.0f64; - } - - // Step 4. - if dirty_rect.max_x() > image_data_size.width { - dirty_rect.size.width = image_data_size.width - dirty_rect.origin.x; - } - - if dirty_rect.max_y() > image_data_size.height { - dirty_rect.size.height = image_data_size.height - dirty_rect.origin.y; - } - - // 5) If either dirtyWidth or dirtyHeight is negative or zero, - // stop without affecting any bitmaps - if dirty_rect.size.width <= 0.0 || dirty_rect.size.height <= 0.0 { - return - } - - // Step 6. let dest_rect = dirty_rect.translate(&offset).to_i32(); - // azure_hl operates with integers. We need to cast the image size let image_size = image_data_size.to_i32(); let first_pixel = dest_rect.origin - offset.to_i32(); diff --git a/components/script/dom/canvasrenderingcontext2d.rs b/components/script/dom/canvasrenderingcontext2d.rs index 2b364c3702d..3482c3d925a 100644 --- a/components/script/dom/canvasrenderingcontext2d.rs +++ b/components/script/dom/canvasrenderingcontext2d.rs @@ -1214,19 +1214,68 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { dirty_width: Finite, dirty_height: Finite, ) { - let data = imagedata.get_data_array(); - let offset = Vector2D::new(*dx, *dy); - let image_data_size = Size2D::new(imagedata.Width() as f64, imagedata.Height() as f64); + let imagedata_size = Size2D::new(imagedata.Width() as f64, imagedata.Height() as f64); + if imagedata_size.width <= 0. || imagedata_size.height <= 0. { + return; + } - let dirty_rect = Rect::new( - Point2D::new(*dirty_x, *dirty_y), - Size2D::new(*dirty_width, *dirty_height), - ); + let mut dirty_x = *dirty_x; + let mut dirty_y = *dirty_y; + let mut dirty_width = *dirty_width; + let mut dirty_height = *dirty_height; + + // Step 1. + // Done later. + + // Step 2. + // TODO: throw InvalidState if buffer is detached. + + // Step 3. + if dirty_width < 0. { + dirty_x += dirty_width; + dirty_width = -dirty_width; + } + if dirty_height < 0. { + dirty_y += dirty_height; + dirty_height = -dirty_height; + } + + // Step 4. + if dirty_x < 0. { + dirty_width += dirty_x; + dirty_x = 0.; + } + if dirty_y < 0. { + dirty_height += dirty_y; + dirty_y = 0.; + } + + // Step 5. + if dirty_x + dirty_width > imagedata_size.width { + dirty_width = imagedata_size.width - dirty_x; + } + if dirty_y + dirty_height > imagedata_size.height { + dirty_height = imagedata_size.height - dirty_y; + } + + // Step 6. + if dirty_width <= 0. || dirty_height <= 0. { + return; + } + + // FIXME(nox): There is no need to make a Vec of all the pixels + // if we didn't want to put the entire image. + let buffer = imagedata.get_data_array(); + + // Step 7. self.send_canvas_2d_msg(Canvas2dMsg::PutImageData( - data.into(), - offset, - image_data_size, - dirty_rect, + buffer.into(), + Vector2D::new(*dx, *dy), + imagedata_size, + Rect::new( + Point2D::new(dirty_x, dirty_y), + Size2D::new(dirty_width, dirty_height), + ), )); self.mark_as_dirty(); } From 3d910feb3a59ced307b5152b2f3b74c7725322c0 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Thu, 4 Oct 2018 12:11:39 +0200 Subject: [PATCH 2/3] Align canvas.putImageData with spec The arguments are supposed to be long values, not floats. --- components/canvas/canvas_data.rs | 15 +++--- components/canvas_traits/canvas.rs | 2 +- .../script/dom/canvasrenderingcontext2d.rs | 49 +++++++------------ .../webidls/CanvasRenderingContext2D.webidl | 8 +-- .../2d.imageData.put.nonfinite.html.ini | 5 ++ 5 files changed, 35 insertions(+), 44 deletions(-) create mode 100644 tests/wpt/metadata/2dcontext/pixel-manipulation/2d.imageData.put.nonfinite.html.ini diff --git a/components/canvas/canvas_data.rs b/components/canvas/canvas_data.rs index c422cdd0aec..31db18a2e0b 100644 --- a/components/canvas/canvas_data.rs +++ b/components/canvas/canvas_data.rs @@ -452,17 +452,16 @@ impl<'a> CanvasData<'a> { pub fn put_image_data( &mut self, imagedata: Vec, - offset: Vector2D, - image_data_size: Size2D, - dirty_rect: Rect + offset: Vector2D, + image_data_size: Size2D, + dirty_rect: Rect, ) { - assert_eq!(image_data_size.width * image_data_size.height * 4.0, imagedata.len() as f64); + assert_eq!(image_data_size.width * image_data_size.height * 4, imagedata.len() as i32); - let dest_rect = dirty_rect.translate(&offset).to_i32(); + let dest_rect = dirty_rect.translate(&offset); + let image_size = image_data_size; - let image_size = image_data_size.to_i32(); - - let first_pixel = dest_rect.origin - offset.to_i32(); + let first_pixel = dest_rect.origin - offset; let mut src_line = (first_pixel.y * (image_size.width * 4) + first_pixel.x * 4) as usize; let mut dest = diff --git a/components/canvas_traits/canvas.rs b/components/canvas_traits/canvas.rs index ec3f623e634..1fcf1a89a4a 100644 --- a/components/canvas_traits/canvas.rs +++ b/components/canvas_traits/canvas.rs @@ -54,7 +54,7 @@ pub enum Canvas2dMsg { IsPointInPath(f64, f64, FillRule, IpcSender), LineTo(Point2D), MoveTo(Point2D), - PutImageData(ByteBuf, Vector2D, Size2D, Rect), + PutImageData(ByteBuf, Vector2D, Size2D, Rect), QuadraticCurveTo(Point2D, Point2D), Rect(Rect), RestoreContext, diff --git a/components/script/dom/canvasrenderingcontext2d.rs b/components/script/dom/canvasrenderingcontext2d.rs index 3482c3d925a..31298ccb648 100644 --- a/components/script/dom/canvasrenderingcontext2d.rs +++ b/components/script/dom/canvasrenderingcontext2d.rs @@ -1191,39 +1191,26 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { } // https://html.spec.whatwg.org/multipage/#dom-context-2d-putimagedata - fn PutImageData(&self, imagedata: &ImageData, dx: Finite, dy: Finite) { - self.PutImageData_( - imagedata, - dx, - dy, - Finite::wrap(0f64), - Finite::wrap(0f64), - Finite::wrap(imagedata.Width() as f64), - Finite::wrap(imagedata.Height() as f64), - ) + fn PutImageData(&self, imagedata: &ImageData, dx: i32, dy: i32) { + self.PutImageData_(imagedata, dx, dy, 0, 0, imagedata.Width() as i32, imagedata.Height() as i32) } // https://html.spec.whatwg.org/multipage/#dom-context-2d-putimagedata fn PutImageData_( &self, imagedata: &ImageData, - dx: Finite, - dy: Finite, - dirty_x: Finite, - dirty_y: Finite, - dirty_width: Finite, - dirty_height: Finite, + dx: i32, + dy: i32, + mut dirty_x: i32, + mut dirty_y: i32, + mut dirty_width: i32, + mut dirty_height: i32, ) { - let imagedata_size = Size2D::new(imagedata.Width() as f64, imagedata.Height() as f64); - if imagedata_size.width <= 0. || imagedata_size.height <= 0. { + let imagedata_size = Size2D::new(imagedata.Width() as i32, imagedata.Height() as i32); + if imagedata_size.width <= 0 || imagedata_size.height <= 0 { return; } - let mut dirty_x = *dirty_x; - let mut dirty_y = *dirty_y; - let mut dirty_width = *dirty_width; - let mut dirty_height = *dirty_height; - // Step 1. // Done later. @@ -1231,23 +1218,23 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { // TODO: throw InvalidState if buffer is detached. // Step 3. - if dirty_width < 0. { + if dirty_width < 0 { dirty_x += dirty_width; dirty_width = -dirty_width; } - if dirty_height < 0. { + if dirty_height < 0 { dirty_y += dirty_height; dirty_height = -dirty_height; } // Step 4. - if dirty_x < 0. { + if dirty_x < 0 { dirty_width += dirty_x; - dirty_x = 0.; + dirty_x = 0; } - if dirty_y < 0. { + if dirty_y < 0 { dirty_height += dirty_y; - dirty_y = 0.; + dirty_y = 0; } // Step 5. @@ -1259,7 +1246,7 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { } // Step 6. - if dirty_width <= 0. || dirty_height <= 0. { + if dirty_width <= 0 || dirty_height <= 0 { return; } @@ -1270,7 +1257,7 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { // Step 7. self.send_canvas_2d_msg(Canvas2dMsg::PutImageData( buffer.into(), - Vector2D::new(*dx, *dy), + Vector2D::new(dx, dy), imagedata_size, Rect::new( Point2D::new(dirty_x, dirty_y), diff --git a/components/script/dom/webidls/CanvasRenderingContext2D.webidl b/components/script/dom/webidls/CanvasRenderingContext2D.webidl index a27cc600e40..f772bd2e8db 100644 --- a/components/script/dom/webidls/CanvasRenderingContext2D.webidl +++ b/components/script/dom/webidls/CanvasRenderingContext2D.webidl @@ -180,11 +180,11 @@ interface CanvasImageData { ImageData createImageData(ImageData imagedata); [Throws] ImageData getImageData(double sx, double sy, double sw, double sh); - void putImageData(ImageData imagedata, double dx, double dy); + void putImageData(ImageData imagedata, long dx, long dy); void putImageData(ImageData imagedata, - double dx, double dy, - double dirtyX, double dirtyY, - double dirtyWidth, double dirtyHeight); + long dx, long dy, + long dirtyX, long dirtyY, + long dirtyWidth, long dirtyHeight); }; enum CanvasLineCap { "butt", "round", "square" }; diff --git a/tests/wpt/metadata/2dcontext/pixel-manipulation/2d.imageData.put.nonfinite.html.ini b/tests/wpt/metadata/2dcontext/pixel-manipulation/2d.imageData.put.nonfinite.html.ini new file mode 100644 index 00000000000..45786171579 --- /dev/null +++ b/tests/wpt/metadata/2dcontext/pixel-manipulation/2d.imageData.put.nonfinite.html.ini @@ -0,0 +1,5 @@ +[2d.imageData.put.nonfinite.html] + bug: https://github.com/web-platform-tests/wpt/issues/13393 + [putImageData() throws TypeError if arguments are not finite] + expected: FAIL + From 82c7d71811c072a29fb73df49664490dcba2d82f Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Thu, 4 Oct 2018 16:29:45 +0200 Subject: [PATCH 3/3] Improve gl.putImageData This commit should allow us to send smaller blobs to the canvas thread, I made it into its own commit just to try=wpt. --- components/canvas/canvas_data.rs | 9 ++---- .../script/dom/canvasrenderingcontext2d.rs | 30 +++++++++++++++++-- 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/components/canvas/canvas_data.rs b/components/canvas/canvas_data.rs index 31db18a2e0b..d4f083ae1e8 100644 --- a/components/canvas/canvas_data.rs +++ b/components/canvas/canvas_data.rs @@ -454,14 +454,13 @@ impl<'a> CanvasData<'a> { imagedata: Vec, offset: Vector2D, image_data_size: Size2D, - dirty_rect: Rect, + dest_rect: Rect, ) { assert_eq!(image_data_size.width * image_data_size.height * 4, imagedata.len() as i32); - let dest_rect = dirty_rect.translate(&offset); let image_size = image_data_size; - let first_pixel = dest_rect.origin - offset; + let first_pixel = dest_rect.origin; let mut src_line = (first_pixel.y * (image_size.width * 4) + first_pixel.x * 4) as usize; let mut dest = @@ -487,9 +486,7 @@ impl<'a> CanvasData<'a> { dest_rect.size, dest_rect.size.width * 4, SurfaceFormat::B8G8R8A8) { - self.drawtarget.copy_surface(source_surface, - Rect::new(Point2D::new(0, 0), dest_rect.size), - dest_rect.origin); + self.drawtarget.copy_surface(source_surface, Rect::from_size(dest_rect.size), offset.to_point()); } } diff --git a/components/script/dom/canvasrenderingcontext2d.rs b/components/script/dom/canvasrenderingcontext2d.rs index 31298ccb648..9adbaa3a734 100644 --- a/components/script/dom/canvasrenderingcontext2d.rs +++ b/components/script/dom/canvasrenderingcontext2d.rs @@ -31,7 +31,7 @@ use dom::htmlcanvaselement::{CanvasContext, HTMLCanvasElement}; use dom::imagedata::ImageData; use dom::node::{Node, NodeDamage, window_from_node}; use dom_struct::dom_struct; -use euclid::{Transform2D, Point2D, Vector2D, Rect, Size2D, vec2}; +use euclid::{Transform2D, Point2D, Rect, Size2D, vec2}; use ipc_channel::ipc::{self, IpcSender}; use net_traits::image::base::PixelFormat; use net_traits::image_cache::CanRequestImages; @@ -1206,6 +1206,9 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { mut dirty_width: i32, mut dirty_height: i32, ) { + // FIXME(nox): There are many arithmetic operations here that can + // overflow or underflow, this should probably be audited. + let imagedata_size = Size2D::new(imagedata.Width() as i32, imagedata.Height() as i32); if imagedata_size.width <= 0 || imagedata_size.height <= 0 { return; @@ -1227,6 +1230,21 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { dirty_height = -dirty_height; } + // Ignore any pixel that would be drawn before the beginning of the + // canvas surface. + let mut dest_x = dx + dirty_x; + let mut dest_y = dy + dirty_y; + if dest_x < 0 { + dirty_x -= dest_x; + dirty_width += dest_x; + dest_x = 0; + } + if dest_y < 0 { + dirty_y -= dest_y; + dirty_height += dest_y; + dest_y = 0; + } + // Step 4. if dirty_x < 0 { dirty_width += dirty_x; @@ -1245,6 +1263,14 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { dirty_height = imagedata_size.height - dirty_y; } + // We take care of ignoring any pixel that would be drawn after the end + // of the canvas surface. + let canvas_size = self.canvas.as_ref().map_or(Size2D::zero(), |c| c.get_size()).to_i32(); + let origin = Point2D::new(dest_x, dest_y); + let drawable_size = (origin - canvas_size.to_vector().to_point()).to_size().abs(); + dirty_width = dirty_width.min(drawable_size.width); + dirty_height = dirty_height.min(drawable_size.height); + // Step 6. if dirty_width <= 0 || dirty_height <= 0 { return; @@ -1257,7 +1283,7 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { // Step 7. self.send_canvas_2d_msg(Canvas2dMsg::PutImageData( buffer.into(), - Vector2D::new(dx, dy), + origin.to_vector(), imagedata_size, Rect::new( Point2D::new(dirty_x, dirty_y),