From a2e3dd4e8636b7125140850dc4bc95433e801e77 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Sat, 6 Oct 2018 00:05:04 +0200 Subject: [PATCH 01/13] Rename byte_swap_and_premultiply to byte_swap_colors_inplace The function did not actually premultiply. --- components/net_traits/image/base.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/net_traits/image/base.rs b/components/net_traits/image/base.rs index 40b4d5d3c47..35b52e559fe 100644 --- a/components/net_traits/image/base.rs +++ b/components/net_traits/image/base.rs @@ -47,7 +47,7 @@ pub struct ImageMetadata { // reference count them. // TODO(pcwalton): Speed up with SIMD, or better yet, find some way to not do this. -fn byte_swap_and_premultiply(data: &mut [u8]) { +fn byte_swap_colors_inplace(data: &mut [u8]) { let length = data.len(); let mut i = 0; @@ -82,7 +82,7 @@ pub fn load_from_memory(buffer: &[u8]) -> Option { DynamicImage::ImageRgba8(rgba) => rgba, image => image.to_rgba(), }; - byte_swap_and_premultiply(&mut *rgba); + byte_swap_colors_inplace(&mut *rgba); Some(Image { width: rgba.width(), height: rgba.height(), From 784fbb2bc17d311fe3322cc48d2dca8a902161ca Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Sat, 6 Oct 2018 00:40:48 +0200 Subject: [PATCH 02/13] Merge some byte swap/premultiply functions in their own crate --- Cargo.lock | 8 ++++ components/canvas/Cargo.toml | 1 + components/canvas/canvas_paint_thread.rs | 3 +- components/canvas/lib.rs | 1 + components/canvas/webgl_thread.rs | 4 +- components/canvas_traits/canvas.rs | 35 ---------------- components/net/Cargo.toml | 1 + components/net/image_cache.rs | 27 +------------ components/net/lib.rs | 1 + components/net_traits/Cargo.toml | 1 + components/net_traits/image/base.rs | 21 +--------- components/net_traits/lib.rs | 1 + components/pixels/Cargo.toml | 10 +++++ components/pixels/lib.rs | 40 +++++++++++++++++++ components/script/Cargo.toml | 1 + .../script/dom/canvasrenderingcontext2d.rs | 5 ++- .../script/dom/webglrenderingcontext.rs | 20 ++++------ components/script/lib.rs | 1 + 18 files changed, 85 insertions(+), 96 deletions(-) create mode 100644 components/pixels/Cargo.toml create mode 100644 components/pixels/lib.rs diff --git a/Cargo.lock b/Cargo.lock index 35c513184b9..e80bb0441e1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -338,6 +338,7 @@ dependencies = [ "log 0.4.5 (registry+https://github.com/rust-lang/crates.io-index)", "num-traits 0.2.4 (registry+https://github.com/rust-lang/crates.io-index)", "offscreen_gl_context 0.21.0 (registry+https://github.com/rust-lang/crates.io-index)", + "pixels 0.0.1", "serde_bytes 0.10.4 (registry+https://github.com/rust-lang/crates.io-index)", "servo_config 0.0.1", "webrender 0.57.2 (git+https://github.com/servo/webrender)", @@ -2308,6 +2309,7 @@ dependencies = [ "msg 0.0.1", "net_traits 0.0.1", "openssl 0.9.24 (registry+https://github.com/rust-lang/crates.io-index)", + "pixels 0.0.1", "profile_traits 0.0.1", "serde 1.0.66 (registry+https://github.com/rust-lang/crates.io-index)", "serde_json 1.0.13 (registry+https://github.com/rust-lang/crates.io-index)", @@ -2351,6 +2353,7 @@ dependencies = [ "malloc_size_of_derive 0.0.1", "msg 0.0.1", "num-traits 0.2.4 (registry+https://github.com/rust-lang/crates.io-index)", + "pixels 0.0.1", "serde 1.0.66 (registry+https://github.com/rust-lang/crates.io-index)", "servo_arc 0.1.1", "servo_config 0.0.1", @@ -2657,6 +2660,10 @@ dependencies = [ "unicase 1.4.0 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "pixels" +version = "0.0.1" + [[package]] name = "pkg-config" version = "0.3.14" @@ -3028,6 +3035,7 @@ dependencies = [ "phf 0.7.21 (registry+https://github.com/rust-lang/crates.io-index)", "phf_codegen 0.7.21 (registry+https://github.com/rust-lang/crates.io-index)", "phf_shared 0.7.21 (registry+https://github.com/rust-lang/crates.io-index)", + "pixels 0.0.1", "profile_traits 0.0.1", "ref_filter_map 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)", "ref_slice 1.1.1 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/components/canvas/Cargo.toml b/components/canvas/Cargo.toml index 4006947acf6..de2073a1e28 100644 --- a/components/canvas/Cargo.toml +++ b/components/canvas/Cargo.toml @@ -24,6 +24,7 @@ ipc-channel = "0.11" log = "0.4" num-traits = "0.2" offscreen_gl_context = {version = "0.21", features = ["serde", "osmesa"]} +pixels = {path = "../pixels"} serde_bytes = "0.10" servo_config = {path = "../config"} webrender = {git = "https://github.com/servo/webrender"} diff --git a/components/canvas/canvas_paint_thread.rs b/components/canvas/canvas_paint_thread.rs index 90d338e9a4e..91164a47cf0 100644 --- a/components/canvas/canvas_paint_thread.rs +++ b/components/canvas/canvas_paint_thread.rs @@ -7,6 +7,7 @@ use canvas_data::*; use canvas_traits::canvas::*; use euclid::Size2D; use ipc_channel::ipc::{self, IpcSender}; +use pixels; use std::borrow::ToOwned; use std::collections::HashMap; use std::thread; @@ -141,7 +142,7 @@ impl<'a> CanvasPaintThread <'a> { let data = match imagedata { None => vec![0; image_size.width as usize * image_size.height as usize * 4], Some(mut data) => { - byte_swap(&mut data); + pixels::byte_swap_colors_inplace(&mut data); data.into() }, }; diff --git a/components/canvas/lib.rs b/components/canvas/lib.rs index 8da9544de6e..554f598403c 100644 --- a/components/canvas/lib.rs +++ b/components/canvas/lib.rs @@ -15,6 +15,7 @@ extern crate ipc_channel; #[macro_use] extern crate log; extern crate num_traits; extern crate offscreen_gl_context; +extern crate pixels; extern crate serde_bytes; extern crate servo_config; extern crate webrender; diff --git a/components/canvas/webgl_thread.rs b/components/canvas/webgl_thread.rs index 413566e7817..cd8dbde18d2 100644 --- a/components/canvas/webgl_thread.rs +++ b/components/canvas/webgl_thread.rs @@ -2,13 +2,13 @@ * 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/. */ -use canvas_traits::canvas::byte_swap; use canvas_traits::webgl::*; use euclid::Size2D; use fnv::FnvHashMap; use gleam::gl; use ipc_channel::ipc::IpcBytesSender; use offscreen_gl_context::{GLContext, GLContextAttributes, GLLimits, NativeGLContextMethods}; +use pixels; use std::thread; use super::gl_context::{GLContextFactory, GLContextWrapper}; use webrender; @@ -562,7 +562,7 @@ impl WebGLThread { let src_slice = &orig_pixels[src_start .. src_start + stride]; (&mut pixels[dst_start .. dst_start + stride]).clone_from_slice(&src_slice[..stride]); } - byte_swap(&mut pixels); + pixels::byte_swap_colors_inplace(&mut pixels); pixels } diff --git a/components/canvas_traits/canvas.rs b/components/canvas_traits/canvas.rs index 1fcf1a89a4a..6c7022c62a8 100644 --- a/components/canvas_traits/canvas.rs +++ b/components/canvas_traits/canvas.rs @@ -382,38 +382,3 @@ impl FromStr for CompositionOrBlending { Err(()) } } - -// TODO(pcwalton): Speed up with SIMD, or better yet, find some way to not do this. -pub fn byte_swap(data: &mut [u8]) { - let length = data.len(); - // FIXME(rust #27741): Range::step_by is not stable yet as of this writing. - let mut i = 0; - while i < length { - let r = data[i + 2]; - data[i + 2] = data[i + 0]; - data[i + 0] = r; - i += 4; - } -} - -pub fn multiply_u8_pixel(a: u8, b: u8) -> u8 { - return (a as u32 * b as u32 / 255) as u8; -} - -pub fn byte_swap_and_premultiply(data: &mut [u8]) { - let length = data.len(); - - let mut i = 0; - while i < length { - let r = data[i + 2]; - let g = data[i + 1]; - let b = data[i + 0]; - let a = data[i + 3]; - - data[i + 0] = multiply_u8_pixel(r, a); - data[i + 1] = multiply_u8_pixel(g, a); - data[i + 2] = multiply_u8_pixel(b, a); - - i += 4; - } -} diff --git a/components/net/Cargo.toml b/components/net/Cargo.toml index aca08703bae..4c0b2aa4e9b 100644 --- a/components/net/Cargo.toml +++ b/components/net/Cargo.toml @@ -34,6 +34,7 @@ mime_guess = "1.8.0" msg = {path = "../msg"} net_traits = {path = "../net_traits"} openssl = "0.9" +pixels = {path = "../pixels"} profile_traits = {path = "../profile_traits"} serde = "1.0" serde_json = "1.0" diff --git a/components/net/image_cache.rs b/components/net/image_cache.rs index 40ef8041ef7..ba0d679b0e3 100644 --- a/components/net/image_cache.rs +++ b/components/net/image_cache.rs @@ -9,6 +9,7 @@ use net_traits::image::base::{Image, ImageMetadata, PixelFormat, load_from_memor use net_traits::image_cache::{CanRequestImages, ImageCache, ImageResponder}; use net_traits::image_cache::{ImageOrMetadataAvailable, ImageResponse, ImageState}; use net_traits::image_cache::{PendingImageId, UsePlaceholder}; +use pixels; use servo_url::ServoUrl; use std::collections::HashMap; use std::collections::hash_map::Entry::{Occupied, Vacant}; @@ -52,7 +53,7 @@ fn set_webrender_image_key(webrender_api: &webrender_api::RenderApi, image: &mut let is_opaque = match image.format { PixelFormat::BGRA8 => { bytes.extend_from_slice(&*image.bytes); - premultiply(bytes.as_mut_slice()) + pixels::premultiply_inplace(bytes.as_mut_slice()) } PixelFormat::RGB8 => { for bgr in image.bytes.chunks(3) { @@ -86,30 +87,6 @@ fn set_webrender_image_key(webrender_api: &webrender_api::RenderApi, image: &mut image.id = Some(image_key); } -// Returns true if the image was found to be -// completely opaque. -fn premultiply(data: &mut [u8]) -> bool { - let mut is_opaque = true; - let length = data.len(); - - let mut i = 0; - while i < length { - let b = data[i + 0] as u32; - let g = data[i + 1] as u32; - let r = data[i + 2] as u32; - let a = data[i + 3] as u32; - - data[i + 0] = (b * a / 255) as u8; - data[i + 1] = (g * a / 255) as u8; - data[i + 2] = (r * a / 255) as u8; - - i += 4; - is_opaque = is_opaque && a == 255; - } - - is_opaque -} - // ====================================================================== // Aux structs and enums. // ====================================================================== diff --git a/components/net/lib.rs b/components/net/lib.rs index ab5c3bc74a2..285a755a9e9 100644 --- a/components/net/lib.rs +++ b/components/net/lib.rs @@ -27,6 +27,7 @@ extern crate mime_guess; extern crate msg; extern crate net_traits; extern crate openssl; +extern crate pixels; #[macro_use] extern crate profile_traits; #[macro_use] extern crate serde; diff --git a/components/net_traits/Cargo.toml b/components/net_traits/Cargo.toml index 9ae5042be69..641d77d8d0c 100644 --- a/components/net_traits/Cargo.toml +++ b/components/net_traits/Cargo.toml @@ -24,6 +24,7 @@ malloc_size_of = { path = "../malloc_size_of" } malloc_size_of_derive = { path = "../malloc_size_of_derive" } msg = {path = "../msg"} num-traits = "0.2" +pixels = {path = "../pixels"} serde = "1.0" servo_arc = {path = "../servo_arc"} servo_config = {path = "../config"} diff --git a/components/net_traits/image/base.rs b/components/net_traits/image/base.rs index 35b52e559fe..40d79b9ea74 100644 --- a/components/net_traits/image/base.rs +++ b/components/net_traits/image/base.rs @@ -4,6 +4,7 @@ use ipc_channel::ipc::IpcSharedMemory; use piston_image::{self, DynamicImage, ImageFormat}; +use pixels; use std::fmt; use webrender_api; @@ -46,24 +47,6 @@ pub struct ImageMetadata { // FIXME: Images must not be copied every frame. Instead we should atomically // reference count them. -// TODO(pcwalton): Speed up with SIMD, or better yet, find some way to not do this. -fn byte_swap_colors_inplace(data: &mut [u8]) { - let length = data.len(); - - let mut i = 0; - while i < length { - let r = data[i + 2]; - let g = data[i + 1]; - let b = data[i + 0]; - - data[i + 0] = r; - data[i + 1] = g; - data[i + 2] = b; - - i += 4; - } -} - pub fn load_from_memory(buffer: &[u8]) -> Option { if buffer.is_empty() { return None; @@ -82,7 +65,7 @@ pub fn load_from_memory(buffer: &[u8]) -> Option { DynamicImage::ImageRgba8(rgba) => rgba, image => image.to_rgba(), }; - byte_swap_colors_inplace(&mut *rgba); + pixels::byte_swap_colors_inplace(&mut *rgba); Some(Image { width: rgba.width(), height: rgba.height(), diff --git a/components/net_traits/lib.rs b/components/net_traits/lib.rs index 87dcf034caa..7066a3f1f28 100644 --- a/components/net_traits/lib.rs +++ b/components/net_traits/lib.rs @@ -17,6 +17,7 @@ extern crate ipc_channel; #[macro_use] extern crate malloc_size_of_derive; extern crate msg; extern crate num_traits; +extern crate pixels; #[macro_use] extern crate serde; extern crate servo_arc; extern crate servo_url; diff --git a/components/pixels/Cargo.toml b/components/pixels/Cargo.toml new file mode 100644 index 00000000000..49dcc3833cf --- /dev/null +++ b/components/pixels/Cargo.toml @@ -0,0 +1,10 @@ +[package] +name = "pixels" +version = "0.0.1" +authors = ["The Servo Project Developers"] +license = "MPL-2.0" +publish = false + +[lib] +name = "pixels" +path = "lib.rs" diff --git a/components/pixels/lib.rs b/components/pixels/lib.rs new file mode 100644 index 00000000000..d91b14cb650 --- /dev/null +++ b/components/pixels/lib.rs @@ -0,0 +1,40 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * 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/. */ + +// TODO(pcwalton): Speed up with SIMD, or better yet, find some way to not do this. +pub fn byte_swap_colors_inplace(pixels: &mut [u8]) { + assert!(pixels.len() % 4 == 0); + for rgba in pixels.chunks_mut(4) { + let b = rgba[0]; + rgba[0] = rgba[2]; + rgba[2] = b; + } +} + +pub fn byte_swap_and_premultiply_inplace(pixels: &mut [u8]) { + assert!(pixels.len() % 4 == 0); + for rgba in pixels.chunks_mut(4) { + let b = rgba[0]; + rgba[0] = multiply_u8_color(rgba[2], rgba[3]); + rgba[1] = multiply_u8_color(rgba[1], rgba[3]); + rgba[2] = multiply_u8_color(b, rgba[3]); + } +} + +/// Returns true if the pixels were found to be completely opaque. +pub fn premultiply_inplace(pixels: &mut [u8]) -> bool { + assert!(pixels.len() % 4 == 0); + let mut is_opaque = true; + for rgba in pixels.chunks_mut(4) { + rgba[0] = multiply_u8_color(rgba[0], rgba[3]); + rgba[1] = multiply_u8_color(rgba[1], rgba[3]); + rgba[2] = multiply_u8_color(rgba[2], rgba[3]); + is_opaque = is_opaque && rgba[3] == 255; + } + is_opaque +} + +pub fn multiply_u8_color(a: u8, b: u8) -> u8 { + return (a as u32 * b as u32 / 255) as u8; +} diff --git a/components/script/Cargo.toml b/components/script/Cargo.toml index ca859bb472a..078218d624b 100644 --- a/components/script/Cargo.toml +++ b/components/script/Cargo.toml @@ -75,6 +75,7 @@ num-traits = "0.2" offscreen_gl_context = {version = "0.21", features = ["serde"]} parking_lot = "0.6" phf = "0.7.18" +pixels = {path = "../pixels"} profile_traits = {path = "../profile_traits"} ref_filter_map = "1.0.1" ref_slice = "1.0" diff --git a/components/script/dom/canvasrenderingcontext2d.rs b/components/script/dom/canvasrenderingcontext2d.rs index 9adbaa3a734..15b6eb9560f 100644 --- a/components/script/dom/canvasrenderingcontext2d.rs +++ b/components/script/dom/canvasrenderingcontext2d.rs @@ -5,7 +5,7 @@ use canvas_traits::canvas::{Canvas2dMsg, CanvasMsg, CanvasId}; use canvas_traits::canvas::{CompositionOrBlending, FillOrStrokeStyle, FillRule}; use canvas_traits::canvas::{LineCapStyle, LineJoinStyle, LinearGradientStyle}; -use canvas_traits::canvas::{RadialGradientStyle, RepetitionStyle, byte_swap_and_premultiply}; +use canvas_traits::canvas::{RadialGradientStyle, RepetitionStyle}; use cssparser::{Parser, ParserInput, RGBA}; use cssparser::Color as CSSColor; use dom::bindings::cell::DomRefCell; @@ -41,6 +41,7 @@ use net_traits::image_cache::ImageResponse; use net_traits::image_cache::ImageState; use net_traits::image_cache::UsePlaceholder; use num_traits::ToPrimitive; +use pixels; use profile_traits::ipc as profiled_ipc; use script_traits::ScriptMsg; use servo_url::ServoUrl; @@ -410,7 +411,7 @@ impl CanvasRenderingContext2D { Some((mut data, size)) => { // Pixels come from cache in BGRA order and drawImage expects RGBA so we // have to swap the color values - byte_swap_and_premultiply(&mut data); + pixels::byte_swap_and_premultiply_inplace(&mut data); let size = Size2D::new(size.width as f64, size.height as f64); (data, size) }, diff --git a/components/script/dom/webglrenderingcontext.rs b/components/script/dom/webglrenderingcontext.rs index 7972201b9ce..a2862831f73 100644 --- a/components/script/dom/webglrenderingcontext.rs +++ b/components/script/dom/webglrenderingcontext.rs @@ -5,7 +5,6 @@ #[cfg(feature = "webgl_backtrace")] use backtrace::Backtrace; use byteorder::{ByteOrder, NativeEndian, WriteBytesExt}; -use canvas_traits::canvas::{byte_swap, multiply_u8_pixel}; use canvas_traits::webgl::{DOMToTextureCommand, Parameter, WebGLCommandBacktrace}; use canvas_traits::webgl::{TexParameter, WebGLCommand, WebGLContextShareMode, WebGLError}; use canvas_traits::webgl::{WebGLFramebufferBindingRequest, WebGLMsg, WebGLMsgSender}; @@ -65,6 +64,7 @@ use js::typedarray::{TypedArray, TypedArrayElementCreator}; use net_traits::image::base::PixelFormat; use net_traits::image_cache::ImageResponse; use offscreen_gl_context::{GLContextAttributes, GLLimits}; +use pixels; use script_layout_interface::HTMLCanvasDataSource; use serde::{Deserialize, Serialize}; use servo_config::prefs::PREFS; @@ -550,7 +550,7 @@ impl WebGLRenderingContext { _ => unimplemented!(), }; - byte_swap(&mut data); + pixels::byte_swap_colors_inplace(&mut data); (data, size, false) }, @@ -563,7 +563,7 @@ impl WebGLRenderingContext { } if let Some((mut data, size)) = canvas.fetch_all_data() { // Pixels got from Canvas have already alpha premultiplied - byte_swap(&mut data); + pixels::byte_swap_colors_inplace(&mut data); (data, size, true) } else { return Ok(None); @@ -679,15 +679,11 @@ impl WebGLRenderingContext { match (format, data_type) { (TexFormat::RGBA, TexDataType::UnsignedByte) => { - for rgba in pixels.chunks_mut(4) { - rgba[0] = multiply_u8_pixel(rgba[0], rgba[3]); - rgba[1] = multiply_u8_pixel(rgba[1], rgba[3]); - rgba[2] = multiply_u8_pixel(rgba[2], rgba[3]); - } + pixels::premultiply_inplace(pixels); }, (TexFormat::LuminanceAlpha, TexDataType::UnsignedByte) => { for la in pixels.chunks_mut(2) { - la[0] = multiply_u8_pixel(la[0], la[1]); + la[0] = pixels::multiply_u8_color(la[0], la[1]); } }, (TexFormat::RGBA, TexDataType::UnsignedShort5551) => { @@ -707,9 +703,9 @@ impl WebGLRenderingContext { let a = extend_to_8_bits(pix & 0x0f); NativeEndian::write_u16( rgba, - ((multiply_u8_pixel(r, a) & 0xf0) as u16) << 8 | - ((multiply_u8_pixel(g, a) & 0xf0) as u16) << 4 | - ((multiply_u8_pixel(b, a) & 0xf0) as u16) | + ((pixels::multiply_u8_color(r, a) & 0xf0) as u16) << 8 | + ((pixels::multiply_u8_color(g, a) & 0xf0) as u16) << 4 | + ((pixels::multiply_u8_color(b, a) & 0xf0) as u16) | ((a & 0x0f) as u16), ); } diff --git a/components/script/lib.rs b/components/script/lib.rs index 097f69d7808..a2ece5ce32c 100644 --- a/components/script/lib.rs +++ b/components/script/lib.rs @@ -78,6 +78,7 @@ extern crate num_traits; extern crate offscreen_gl_context; extern crate parking_lot; extern crate phf; +extern crate pixels; #[macro_use] extern crate profile_traits; extern crate ref_filter_map; From 19f40cdf0ba09a767e65ee3f0bd37622cc341bde Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Sat, 6 Oct 2018 01:10:34 +0200 Subject: [PATCH 03/13] Introduce ImageData::get_rect We use that to send only the pixels that will be actually drawn to the canvas thread in CanvasRenderingContext2d::PutImageData. We also make the canvas thread byte swap and premultiply colours in-place. --- components/canvas/canvas_data.rs | 45 ++++++------------- components/canvas/canvas_paint_thread.rs | 14 +----- components/canvas_traits/canvas.rs | 2 +- .../script/dom/canvasrenderingcontext2d.rs | 13 ++---- components/script/dom/imagedata.rs | 33 +++++++++++++- 5 files changed, 52 insertions(+), 55 deletions(-) diff --git a/components/canvas/canvas_data.rs b/components/canvas/canvas_data.rs index d4f083ae1e8..30e6218d604 100644 --- a/components/canvas/canvas_data.rs +++ b/components/canvas/canvas_data.rs @@ -13,6 +13,7 @@ use cssparser::RGBA; use euclid::{Transform2D, Point2D, Vector2D, Rect, Size2D}; use ipc_channel::ipc::{IpcBytesSender, IpcSender}; use num_traits::ToPrimitive; +use pixels; use serde_bytes::ByteBuf; use std::mem; use std::sync::Arc; @@ -451,42 +452,22 @@ impl<'a> CanvasData<'a> { // https://html.spec.whatwg.org/multipage/#dom-context-2d-putimagedata pub fn put_image_data( &mut self, - imagedata: Vec, + mut imagedata: Vec, offset: Vector2D, - image_data_size: Size2D, - dest_rect: Rect, + imagedata_size: Size2D, ) { - assert_eq!(image_data_size.width * image_data_size.height * 4, imagedata.len() as i32); - - let image_size = image_data_size; - - 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 = - Vec::with_capacity((dest_rect.size.width * dest_rect.size.height * 4) as usize); - - for _ in 0 .. dest_rect.size.height { - let mut src_offset = src_line; - for _ in 0 .. dest_rect.size.width { - let alpha = imagedata[src_offset + 3] as u16; - // add 127 before dividing for more accurate rounding - let premultiply_channel = |channel: u8| (((channel as u16 * alpha) + 127) / 255) as u8; - dest.push(premultiply_channel(imagedata[src_offset + 2])); - dest.push(premultiply_channel(imagedata[src_offset + 1])); - dest.push(premultiply_channel(imagedata[src_offset + 0])); - dest.push(imagedata[src_offset + 3]); - src_offset += 4; - } - src_line += (image_size.width * 4) as usize; - } - + assert_eq!(imagedata_size.area() * 4, imagedata.len() as i32); + pixels::byte_swap_and_premultiply_inplace(&mut imagedata); if let Some(source_surface) = self.drawtarget.create_source_surface_from_data( - &dest, - dest_rect.size, - dest_rect.size.width * 4, + &imagedata, + imagedata_size, + imagedata_size.width * 4, SurfaceFormat::B8G8R8A8) { - self.drawtarget.copy_surface(source_surface, Rect::from_size(dest_rect.size), offset.to_point()); + self.drawtarget.copy_surface( + source_surface, + Rect::from_size(imagedata_size), + offset.to_point(), + ); } } diff --git a/components/canvas/canvas_paint_thread.rs b/components/canvas/canvas_paint_thread.rs index 91164a47cf0..465d2cadd45 100644 --- a/components/canvas/canvas_paint_thread.rs +++ b/components/canvas/canvas_paint_thread.rs @@ -241,18 +241,8 @@ impl<'a> CanvasPaintThread <'a> { Canvas2dMsg::GetImageData(dest_rect, canvas_size, chan) => { self.canvas(canvas_id).image_data(dest_rect, canvas_size, chan) }, - Canvas2dMsg::PutImageData( - imagedata, - offset, - image_data_size, - dirty_rect, - ) => { - self.canvas(canvas_id).put_image_data( - imagedata.into(), - offset, - image_data_size, - dirty_rect, - ) + Canvas2dMsg::PutImageData(imagedata, offset, imagedata_size) => { + self.canvas(canvas_id).put_image_data(imagedata.into(), offset, imagedata_size) }, Canvas2dMsg::SetShadowOffsetX(value) => { self.canvas(canvas_id).set_shadow_offset_x(value) diff --git a/components/canvas_traits/canvas.rs b/components/canvas_traits/canvas.rs index 6c7022c62a8..b0c2fb4f2bc 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), QuadraticCurveTo(Point2D, Point2D), Rect(Rect), RestoreContext, diff --git a/components/script/dom/canvasrenderingcontext2d.rs b/components/script/dom/canvasrenderingcontext2d.rs index 15b6eb9560f..1f3b9ab92cd 100644 --- a/components/script/dom/canvasrenderingcontext2d.rs +++ b/components/script/dom/canvasrenderingcontext2d.rs @@ -1277,19 +1277,14 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { 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(); + let dirty_size = Size2D::new(dirty_width, dirty_height); + let dirty_rect = Rect::new(Point2D::new(dirty_x, dirty_y), dirty_size); // Step 7. self.send_canvas_2d_msg(Canvas2dMsg::PutImageData( - buffer.into(), + imagedata.get_rect(dirty_rect.try_cast().unwrap()).into(), origin.to_vector(), - imagedata_size, - Rect::new( - Point2D::new(dirty_x, dirty_y), - Size2D::new(dirty_width, dirty_height), - ), + dirty_size, )); self.mark_as_dirty(); } diff --git a/components/script/dom/imagedata.rs b/components/script/dom/imagedata.rs index ea65d533ffe..e219845675b 100644 --- a/components/script/dom/imagedata.rs +++ b/components/script/dom/imagedata.rs @@ -9,7 +9,7 @@ use dom::bindings::reflector::{Reflector, reflect_dom_object}; use dom::bindings::root::DomRoot; use dom::globalscope::GlobalScope; use dom_struct::dom_struct; -use euclid::Size2D; +use euclid::{Rect, Size2D}; use js::jsapi::{Heap, JSContext, JSObject}; use js::rust::Runtime; use js::typedarray::{Uint8ClampedArray, CreateWith}; @@ -149,9 +149,40 @@ impl ImageData { } } + #[allow(unsafe_code)] + pub fn get_rect(&self, rect: Rect) -> Vec { + unsafe { + assert!(!rect.is_empty()); + assert!(self.rect().contains_rect(&rect)); + assert!(!self.data.get().is_null()); + let cx = Runtime::get(); + assert!(!cx.is_null()); + typedarray!(in(cx) let array: Uint8ClampedArray = self.data.get()); + let slice = array.as_ref().unwrap().as_slice(); + let area = rect.size.area() as usize; + let first_column_start = rect.origin.x as usize * 4; + let row_length = self.width as usize * 4; + let first_row_start = rect.origin.y as usize * row_length; + if rect.origin.x == 0 && rect.size.width == self.width || rect.size.height == 1 { + let start = first_column_start + first_row_start; + // FIXME(nox): This should be a borrow. + return slice[start..start + area * 4].into(); + } + let mut data = Vec::with_capacity(area * 4); + for row in slice[first_row_start..].chunks(row_length).take(rect.size.height as usize) { + data.extend_from_slice(&row[first_column_start..][..rect.size.width as usize * 4]); + } + data + } + } + pub fn get_size(&self) -> Size2D { Size2D::new(self.Width(), self.Height()) } + + pub fn rect(&self) -> Rect { + Rect::from_size(self.get_size()) + } } impl ImageDataMethods for ImageData { From 75e6f5dfaabd8ff01916b929edaceedf47fe6309 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Sat, 6 Oct 2018 02:58:45 +0200 Subject: [PATCH 04/13] Avoid copying pixels in ctx.putImageData sometimes --- components/canvas/canvas_paint_thread.rs | 8 ++- components/canvas_traits/canvas.rs | 8 +-- .../script/dom/canvasrenderingcontext2d.rs | 5 +- components/script/dom/htmlcanvaselement.rs | 3 +- components/script/dom/imagedata.rs | 68 ++++++++++--------- .../script/dom/webglrenderingcontext.rs | 2 +- 6 files changed, 53 insertions(+), 41 deletions(-) diff --git a/components/canvas/canvas_paint_thread.rs b/components/canvas/canvas_paint_thread.rs index 465d2cadd45..428ca8ddd9f 100644 --- a/components/canvas/canvas_paint_thread.rs +++ b/components/canvas/canvas_paint_thread.rs @@ -241,8 +241,12 @@ impl<'a> CanvasPaintThread <'a> { Canvas2dMsg::GetImageData(dest_rect, canvas_size, chan) => { self.canvas(canvas_id).image_data(dest_rect, canvas_size, chan) }, - Canvas2dMsg::PutImageData(imagedata, offset, imagedata_size) => { - self.canvas(canvas_id).put_image_data(imagedata.into(), offset, imagedata_size) + Canvas2dMsg::PutImageData(receiver, offset, imagedata_size) => { + self.canvas(canvas_id).put_image_data( + receiver.recv().unwrap(), + offset, + imagedata_size, + ) }, Canvas2dMsg::SetShadowOffsetX(value) => { self.canvas(canvas_id).set_shadow_offset_x(value) diff --git a/components/canvas_traits/canvas.rs b/components/canvas_traits/canvas.rs index b0c2fb4f2bc..4833f26262f 100644 --- a/components/canvas_traits/canvas.rs +++ b/components/canvas_traits/canvas.rs @@ -4,7 +4,7 @@ use cssparser::RGBA; use euclid::{Transform2D, Point2D, Vector2D, Rect, Size2D}; -use ipc_channel::ipc::{IpcBytesSender, IpcSender}; +use ipc_channel::ipc::{IpcBytesReceiver, IpcBytesSender, IpcSender}; use serde_bytes::ByteBuf; use std::default::Default; use std::str::FromStr; @@ -19,7 +19,7 @@ pub enum FillRule { #[derive(Clone, Copy, Deserialize, Eq, Hash, MallocSizeOf, PartialEq, Serialize)] pub struct CanvasId(pub u64); -#[derive(Clone, Deserialize, Serialize)] +#[derive(Deserialize, Serialize)] pub enum CanvasMsg { Canvas2d(Canvas2dMsg, CanvasId), Create(IpcSender, Size2D, webrender_api::RenderApiSender, bool), @@ -34,7 +34,7 @@ pub struct CanvasImageData { pub image_key: webrender_api::ImageKey, } -#[derive(Clone, Deserialize, Serialize)] +#[derive(Deserialize, Serialize)] pub enum Canvas2dMsg { Arc(Point2D, f32, f32, f32, bool), ArcTo(Point2D, Point2D, f32), @@ -54,7 +54,7 @@ pub enum Canvas2dMsg { IsPointInPath(f64, f64, FillRule, IpcSender), LineTo(Point2D), MoveTo(Point2D), - PutImageData(ByteBuf, Vector2D, Size2D), + PutImageData(IpcBytesReceiver, Vector2D, Size2D), QuadraticCurveTo(Point2D, Point2D), Rect(Rect), RestoreContext, diff --git a/components/script/dom/canvasrenderingcontext2d.rs b/components/script/dom/canvasrenderingcontext2d.rs index 1f3b9ab92cd..69c7d4038cd 100644 --- a/components/script/dom/canvasrenderingcontext2d.rs +++ b/components/script/dom/canvasrenderingcontext2d.rs @@ -1197,6 +1197,7 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { } // https://html.spec.whatwg.org/multipage/#dom-context-2d-putimagedata + #[allow(unsafe_code)] fn PutImageData_( &self, imagedata: &ImageData, @@ -1281,11 +1282,13 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { let dirty_rect = Rect::new(Point2D::new(dirty_x, dirty_y), dirty_size); // Step 7. + let (sender, receiver) = ipc::bytes_channel().unwrap(); self.send_canvas_2d_msg(Canvas2dMsg::PutImageData( - imagedata.get_rect(dirty_rect.try_cast().unwrap()).into(), + receiver, origin.to_vector(), dirty_size, )); + sender.send(unsafe { &imagedata.get_rect(dirty_rect.try_cast().unwrap()) }).unwrap(); self.mark_as_dirty(); } diff --git a/components/script/dom/htmlcanvaselement.rs b/components/script/dom/htmlcanvaselement.rs index 09114a95d0a..c3aae4eed83 100644 --- a/components/script/dom/htmlcanvaselement.rs +++ b/components/script/dom/htmlcanvaselement.rs @@ -369,13 +369,14 @@ impl HTMLCanvasElementMethods for HTMLCanvasElement { // Step 3. let raw_data = match *self.context.borrow() { Some(CanvasContext::Context2d(ref context)) => { + // FIXME(nox): This shouldn't go through ImageData etc. let image_data = context.GetImageData( Finite::wrap(0f64), Finite::wrap(0f64), Finite::wrap(self.Width() as f64), Finite::wrap(self.Height() as f64), )?; - image_data.get_data_array() + image_data.to_vec() }, Some(CanvasContext::WebGL(ref context)) => { match context.get_image_data(self.Width(), self.Height()) { diff --git a/components/script/dom/imagedata.rs b/components/script/dom/imagedata.rs index e219845675b..7e615c0e5da 100644 --- a/components/script/dom/imagedata.rs +++ b/components/script/dom/imagedata.rs @@ -13,6 +13,7 @@ use euclid::{Rect, Size2D}; use js::jsapi::{Heap, JSContext, JSObject}; use js::rust::Runtime; use js::typedarray::{Uint8ClampedArray, CreateWith}; +use std::borrow::Cow; use std::default::Default; use std::ptr; use std::ptr::NonNull; @@ -137,43 +138,46 @@ impl ImageData { Self::new_with_jsobject(global, width, opt_height, Some(jsobject)) } + /// Nothing must change the array on the JS side while the slice is live. #[allow(unsafe_code)] - pub fn get_data_array(&self) -> Vec { - unsafe { - assert!(!self.data.get().is_null()); - let cx = Runtime::get(); - assert!(!cx.is_null()); - typedarray!(in(cx) let array: Uint8ClampedArray = self.data.get()); - let vec = array.unwrap().as_slice().to_vec(); - vec - } + pub unsafe fn as_slice(&self) -> &[u8] { + assert!(!self.data.get().is_null()); + let cx = Runtime::get(); + assert!(!cx.is_null()); + typedarray!(in(cx) let array: Uint8ClampedArray = self.data.get()); + let array = array.as_ref().unwrap(); + // NOTE(nox): This is just as unsafe as `as_slice` itself even though we + // are extending the lifetime of the slice, because the data in + // this ImageData instance will never change. The method is thus unsafe + // because the array may be manipulated from JS while the reference + // is live. + let ptr = array.as_slice() as *const _; + &*ptr } #[allow(unsafe_code)] - pub fn get_rect(&self, rect: Rect) -> Vec { - unsafe { - assert!(!rect.is_empty()); - assert!(self.rect().contains_rect(&rect)); - assert!(!self.data.get().is_null()); - let cx = Runtime::get(); - assert!(!cx.is_null()); - typedarray!(in(cx) let array: Uint8ClampedArray = self.data.get()); - let slice = array.as_ref().unwrap().as_slice(); - let area = rect.size.area() as usize; - let first_column_start = rect.origin.x as usize * 4; - let row_length = self.width as usize * 4; - let first_row_start = rect.origin.y as usize * row_length; - if rect.origin.x == 0 && rect.size.width == self.width || rect.size.height == 1 { - let start = first_column_start + first_row_start; - // FIXME(nox): This should be a borrow. - return slice[start..start + area * 4].into(); - } - let mut data = Vec::with_capacity(area * 4); - for row in slice[first_row_start..].chunks(row_length).take(rect.size.height as usize) { - data.extend_from_slice(&row[first_column_start..][..rect.size.width as usize * 4]); - } - data + pub fn to_vec(&self) -> Vec { + unsafe { self.as_slice().into() } + } + + #[allow(unsafe_code)] + pub unsafe fn get_rect(&self, rect: Rect) -> Cow<[u8]> { + assert!(!rect.is_empty()); + assert!(self.rect().contains_rect(&rect)); + let slice = self.as_slice(); + let area = rect.size.area() as usize; + let first_column_start = rect.origin.x as usize * 4; + let row_length = self.width as usize * 4; + let first_row_start = rect.origin.y as usize * row_length; + if rect.origin.x == 0 && rect.size.width == self.width || rect.size.height == 1 { + let start = first_column_start + first_row_start; + return Cow::Borrowed(&slice[start..start + area * 4]); } + let mut data = Vec::with_capacity(area * 4); + for row in slice[first_row_start..].chunks(row_length).take(rect.size.height as usize) { + data.extend_from_slice(&row[first_column_start..][..rect.size.width as usize * 4]); + } + data.into() } pub fn get_size(&self) -> Size2D { diff --git a/components/script/dom/webglrenderingcontext.rs b/components/script/dom/webglrenderingcontext.rs index a2862831f73..4e3658fa524 100644 --- a/components/script/dom/webglrenderingcontext.rs +++ b/components/script/dom/webglrenderingcontext.rs @@ -520,7 +520,7 @@ impl WebGLRenderingContext { ) -> Fallible, Size2D, bool)>> { Ok(Some(match source { TexImageSource::ImageData(image_data) => { - (image_data.get_data_array(), image_data.get_size(), false) + (image_data.to_vec(), image_data.get_size(), false) }, TexImageSource::HTMLImageElement(image) => { let document = document_from_node(&*self.canvas); From f13e35b2c55f6ee044373ef26874230800f11c00 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Sat, 6 Oct 2018 10:53:10 +0200 Subject: [PATCH 05/13] Always make sure we get a surface in CanvasData::put_image_data --- components/canvas/canvas_data.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/components/canvas/canvas_data.rs b/components/canvas/canvas_data.rs index 30e6218d604..7ccd7b6184e 100644 --- a/components/canvas/canvas_data.rs +++ b/components/canvas/canvas_data.rs @@ -458,17 +458,17 @@ impl<'a> CanvasData<'a> { ) { assert_eq!(imagedata_size.area() * 4, imagedata.len() as i32); pixels::byte_swap_and_premultiply_inplace(&mut imagedata); - if let Some(source_surface) = self.drawtarget.create_source_surface_from_data( - &imagedata, - imagedata_size, - imagedata_size.width * 4, - SurfaceFormat::B8G8R8A8) { - self.drawtarget.copy_surface( - source_surface, - Rect::from_size(imagedata_size), - offset.to_point(), - ); - } + let source_surface = self.drawtarget.create_source_surface_from_data( + &imagedata, + imagedata_size, + imagedata_size.width * 4, + SurfaceFormat::B8G8R8A8, + ).unwrap(); + self.drawtarget.copy_surface( + source_surface, + Rect::from_size(imagedata_size), + offset.to_point(), + ); } pub fn set_shadow_offset_x(&mut self, value: f64) { From 241dba064ded04b3b2f97b098db637ce58cf9e19 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Sat, 6 Oct 2018 11:41:48 +0200 Subject: [PATCH 06/13] Align ctx.createImageData and ctx.getImageData with the spec --- components/canvas/canvas_data.rs | 16 +--- components/canvas/canvas_paint_thread.rs | 7 +- components/canvas_traits/canvas.rs | 2 +- .../script/dom/canvasrenderingcontext2d.rs | 77 ++++++++----------- components/script/dom/htmlcanvaselement.rs | 9 +-- .../webidls/CanvasRenderingContext2D.webidl | 4 +- .../2d.imageData.create2.nonfinite.html.ini | 5 ++ .../2d.imageData.create2.zero.html.ini | 5 -- .../2d.imageData.get.nonfinite.html.ini | 5 ++ .../2d.imageData.get.zero.html.ini | 5 -- 10 files changed, 54 insertions(+), 81 deletions(-) create mode 100644 tests/wpt/metadata/2dcontext/pixel-manipulation/2d.imageData.create2.nonfinite.html.ini delete mode 100644 tests/wpt/metadata/2dcontext/pixel-manipulation/2d.imageData.create2.zero.html.ini create mode 100644 tests/wpt/metadata/2dcontext/pixel-manipulation/2d.imageData.get.nonfinite.html.ini delete mode 100644 tests/wpt/metadata/2dcontext/pixel-manipulation/2d.imageData.get.zero.html.ini diff --git a/components/canvas/canvas_data.rs b/components/canvas/canvas_data.rs index 7ccd7b6184e..e7841d1a53c 100644 --- a/components/canvas/canvas_data.rs +++ b/components/canvas/canvas_data.rs @@ -11,7 +11,7 @@ use azure::azure_hl::SurfacePattern; use canvas_traits::canvas::*; use cssparser::RGBA; use euclid::{Transform2D, Point2D, Vector2D, Rect, Size2D}; -use ipc_channel::ipc::{IpcBytesSender, IpcSender}; +use ipc_channel::ipc::IpcSender; use num_traits::ToPrimitive; use pixels; use serde_bytes::ByteBuf; @@ -440,15 +440,6 @@ impl<'a> CanvasData<'a> { chan.send(data).unwrap(); } - pub fn image_data( - &self, - dest_rect: Rect, - canvas_size: Size2D, - sender: IpcBytesSender, - ) { - sender.send(&self.read_pixels(dest_rect, canvas_size)).unwrap(); - } - // https://html.spec.whatwg.org/multipage/#dom-context-2d-putimagedata pub fn put_image_data( &mut self, @@ -526,9 +517,8 @@ impl<'a> CanvasData<'a> { /// canvas_size: The size of the canvas we're reading from /// read_rect: The area of the canvas we want to read from #[allow(unsafe_code)] - pub fn read_pixels(&self, read_rect: Rect, canvas_size: Size2D) -> Vec { - let canvas_size = canvas_size.to_i32(); - let canvas_rect = Rect::new(Point2D::new(0i32, 0i32), canvas_size); + pub fn read_pixels(&self, read_rect: Rect, canvas_size: Size2D) -> Vec { + let canvas_rect = Rect::from_size(canvas_size); let src_read_rect = canvas_rect.intersection(&read_rect).unwrap_or(Rect::zero()); if src_read_rect.is_empty() || canvas_size.width <= 0 && canvas_size.height <= 0 { diff --git a/components/canvas/canvas_paint_thread.rs b/components/canvas/canvas_paint_thread.rs index 428ca8ddd9f..dae7836fd71 100644 --- a/components/canvas/canvas_paint_thread.rs +++ b/components/canvas/canvas_paint_thread.rs @@ -163,7 +163,7 @@ impl<'a> CanvasPaintThread <'a> { ) => { let image_data = self.canvas(canvas_id).read_pixels( source_rect.to_i32(), - image_size, + image_size.to_i32(), ); self.canvas(other_canvas_id).draw_image( image_data.into(), @@ -238,8 +238,9 @@ impl<'a> CanvasPaintThread <'a> { Canvas2dMsg::SetGlobalComposition(op) => { self.canvas(canvas_id).set_global_composition(op) }, - Canvas2dMsg::GetImageData(dest_rect, canvas_size, chan) => { - self.canvas(canvas_id).image_data(dest_rect, canvas_size, chan) + Canvas2dMsg::GetImageData(dest_rect, canvas_size, sender) => { + let pixels = self.canvas(canvas_id).read_pixels(dest_rect, canvas_size); + sender.send(&pixels).unwrap(); }, Canvas2dMsg::PutImageData(receiver, offset, imagedata_size) => { self.canvas(canvas_id).put_image_data( diff --git a/components/canvas_traits/canvas.rs b/components/canvas_traits/canvas.rs index 4833f26262f..8e452c93aff 100644 --- a/components/canvas_traits/canvas.rs +++ b/components/canvas_traits/canvas.rs @@ -50,7 +50,7 @@ pub enum Canvas2dMsg { Fill, FillText(String, f64, f64, Option), FillRect(Rect), - GetImageData(Rect, Size2D, IpcBytesSender), + GetImageData(Rect, Size2D, IpcBytesSender), IsPointInPath(f64, f64, FillRule, IpcSender), LineTo(Point2D), MoveTo(Point2D), diff --git a/components/script/dom/canvasrenderingcontext2d.rs b/components/script/dom/canvasrenderingcontext2d.rs index 69c7d4038cd..633fbbc3f14 100644 --- a/components/script/dom/canvasrenderingcontext2d.rs +++ b/components/script/dom/canvasrenderingcontext2d.rs @@ -40,12 +40,11 @@ use net_traits::image_cache::ImageOrMetadataAvailable; use net_traits::image_cache::ImageResponse; use net_traits::image_cache::ImageState; use net_traits::image_cache::UsePlaceholder; -use num_traits::ToPrimitive; use pixels; use profile_traits::ipc as profiled_ipc; use script_traits::ScriptMsg; use servo_url::ServoUrl; -use std::{cmp, fmt, mem}; +use std::{fmt, mem}; use std::cell::Cell; use std::str::FromStr; use std::sync::Arc; @@ -1118,14 +1117,11 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { } // https://html.spec.whatwg.org/multipage/#dom-context-2d-createimagedata - fn CreateImageData(&self, sw: Finite, sh: Finite) -> Fallible> { - if *sw == 0.0 || *sh == 0.0 { + fn CreateImageData(&self, sw: i32, sh: i32) -> Fallible> { + if sw == 0 || sh == 0 { return Err(Error::IndexSize); } - - let sw = cmp::max(1, sw.abs().to_u32().unwrap()); - let sh = cmp::max(1, sh.abs().to_u32().unwrap()); - ImageData::new(&self.global(), sw, sh, None) + ImageData::new(&self.global(), sw.abs() as u32, sh.abs() as u32, None) } // https://html.spec.whatwg.org/multipage/#dom-context-2d-createimagedata @@ -1136,59 +1132,49 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { // https://html.spec.whatwg.org/multipage/#dom-context-2d-getimagedata fn GetImageData( &self, - sx: Finite, - sy: Finite, - sw: Finite, - sh: Finite, + mut sx: i32, + mut sy: i32, + mut sw: i32, + mut sh: i32, ) -> Fallible> { + if sw == 0 || sh == 0 { + return Err(Error::IndexSize); + } + if !self.origin_is_clean() { return Err(Error::Security); } - let mut sx = *sx; - let mut sy = *sy; - let mut sw = *sw; - let mut sh = *sh; - - if sw == 0.0 || sh == 0.0 { - return Err(Error::IndexSize); - } - - if sw < 0.0 { + if sw < 0 { sw = -sw; sx -= sw; } - if sh < 0.0 { + if sh < 0 { sh = -sh; sy -= sh; } - let sh = cmp::max(1, sh.to_u32().unwrap()); - let sw = cmp::max(1, sw.to_u32().unwrap()); - let (sender, receiver) = ipc::bytes_channel().unwrap(); - let dest_rect = Rect::new( - Point2D::new(sx.to_i32().unwrap(), sy.to_i32().unwrap()), - Size2D::new(sw as i32, sh as i32), - ); - let canvas_size = self - .canvas - .as_ref() - .map(|c| c.get_size()) - .unwrap_or(Size2D::zero()); - let canvas_size = Size2D::new(canvas_size.width as f64, canvas_size.height as f64); - self.send_canvas_2d_msg(Canvas2dMsg::GetImageData(dest_rect, canvas_size, sender)); + let dest_rect = Rect::new(Point2D::new(sx, sy), Size2D::new(sw, sh)); + // FIXME(nox): This is probably wrong when this is a context for an + // offscreen canvas. + let canvas_size = self.canvas.as_ref().map_or(Size2D::zero(), |c| c.get_size()); + self.send_canvas_2d_msg(Canvas2dMsg::GetImageData( + dest_rect, + canvas_size.to_i32(), + sender, + )); let mut data = receiver.recv().unwrap(); // Byte swap and unmultiply alpha. for chunk in data.chunks_mut(4) { - let (b, g, r, a) = (chunk[0], chunk[1], chunk[2], chunk[3]); - chunk[0] = UNPREMULTIPLY_TABLE[256 * (a as usize) + r as usize]; - chunk[1] = UNPREMULTIPLY_TABLE[256 * (a as usize) + g as usize]; - chunk[2] = UNPREMULTIPLY_TABLE[256 * (a as usize) + b as usize]; + let b = chunk[0]; + chunk[0] = UNPREMULTIPLY_TABLE[256 * (chunk[3] as usize) + chunk[2] as usize]; + chunk[1] = UNPREMULTIPLY_TABLE[256 * (chunk[3] as usize) + chunk[1] as usize]; + chunk[2] = UNPREMULTIPLY_TABLE[256 * (chunk[3] as usize) + b as usize]; } - ImageData::new(&self.global(), sw, sh, Some(data.to_vec())) + ImageData::new(&self.global(), sw as u32, sh as u32, Some(data.to_vec())) } // https://html.spec.whatwg.org/multipage/#dom-context-2d-putimagedata @@ -1265,11 +1251,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. + // FIXME(nox): This is probably wrong when this is a context for an + // offscreen canvas. 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(); + + // We take care of ignoring any pixel that would be drawn after the end + // of the canvas surface. dirty_width = dirty_width.min(drawable_size.width); dirty_height = dirty_height.min(drawable_size.height); diff --git a/components/script/dom/htmlcanvaselement.rs b/components/script/dom/htmlcanvaselement.rs index c3aae4eed83..78e74641347 100644 --- a/components/script/dom/htmlcanvaselement.rs +++ b/components/script/dom/htmlcanvaselement.rs @@ -14,7 +14,6 @@ use dom::bindings::codegen::Bindings::WebGLRenderingContextBinding::WebGLContext use dom::bindings::conversions::ConversionResult; use dom::bindings::error::{Error, Fallible}; use dom::bindings::inheritance::Castable; -use dom::bindings::num::Finite; use dom::bindings::reflector::DomObject; use dom::bindings::root::{Dom, DomRoot, LayoutDom}; use dom::bindings::str::{DOMString, USVString}; @@ -370,13 +369,7 @@ impl HTMLCanvasElementMethods for HTMLCanvasElement { let raw_data = match *self.context.borrow() { Some(CanvasContext::Context2d(ref context)) => { // FIXME(nox): This shouldn't go through ImageData etc. - let image_data = context.GetImageData( - Finite::wrap(0f64), - Finite::wrap(0f64), - Finite::wrap(self.Width() as f64), - Finite::wrap(self.Height() as f64), - )?; - image_data.to_vec() + context.GetImageData(0, 0, self.Width() as i32, self.Height() as i32)?.to_vec() }, Some(CanvasContext::WebGL(ref context)) => { match context.get_image_data(self.Width(), self.Height()) { diff --git a/components/script/dom/webidls/CanvasRenderingContext2D.webidl b/components/script/dom/webidls/CanvasRenderingContext2D.webidl index f772bd2e8db..82001645924 100644 --- a/components/script/dom/webidls/CanvasRenderingContext2D.webidl +++ b/components/script/dom/webidls/CanvasRenderingContext2D.webidl @@ -175,11 +175,11 @@ interface CanvasDrawImage { interface CanvasImageData { // pixel manipulation [Throws] - ImageData createImageData(double sw, double sh); + ImageData createImageData(long sw, long sh); [Throws] ImageData createImageData(ImageData imagedata); [Throws] - ImageData getImageData(double sx, double sy, double sw, double sh); + ImageData getImageData(long sx, long sy, long sw, long sh); void putImageData(ImageData imagedata, long dx, long dy); void putImageData(ImageData imagedata, long dx, long dy, diff --git a/tests/wpt/metadata/2dcontext/pixel-manipulation/2d.imageData.create2.nonfinite.html.ini b/tests/wpt/metadata/2dcontext/pixel-manipulation/2d.imageData.create2.nonfinite.html.ini new file mode 100644 index 00000000000..62382c779c2 --- /dev/null +++ b/tests/wpt/metadata/2dcontext/pixel-manipulation/2d.imageData.create2.nonfinite.html.ini @@ -0,0 +1,5 @@ +[2d.imageData.create2.nonfinite.html] + bug: https://github.com/web-platform-tests/wpt/issues/13393 + [createImageData() throws TypeError if arguments are not finite] + expected: FAIL + diff --git a/tests/wpt/metadata/2dcontext/pixel-manipulation/2d.imageData.create2.zero.html.ini b/tests/wpt/metadata/2dcontext/pixel-manipulation/2d.imageData.create2.zero.html.ini deleted file mode 100644 index ae36bc71a82..00000000000 --- a/tests/wpt/metadata/2dcontext/pixel-manipulation/2d.imageData.create2.zero.html.ini +++ /dev/null @@ -1,5 +0,0 @@ -[2d.imageData.create2.zero.html] - type: testharness - [createImageData(sw, sh) throws INDEX_SIZE_ERR if size is zero] - expected: FAIL - diff --git a/tests/wpt/metadata/2dcontext/pixel-manipulation/2d.imageData.get.nonfinite.html.ini b/tests/wpt/metadata/2dcontext/pixel-manipulation/2d.imageData.get.nonfinite.html.ini new file mode 100644 index 00000000000..dfb06a53b30 --- /dev/null +++ b/tests/wpt/metadata/2dcontext/pixel-manipulation/2d.imageData.get.nonfinite.html.ini @@ -0,0 +1,5 @@ +[2d.imageData.get.nonfinite.html] + bug: https://github.com/web-platform-tests/wpt/issues/13393 + [getImageData() throws TypeError if arguments are not finite] + expected: FAIL + diff --git a/tests/wpt/metadata/2dcontext/pixel-manipulation/2d.imageData.get.zero.html.ini b/tests/wpt/metadata/2dcontext/pixel-manipulation/2d.imageData.get.zero.html.ini deleted file mode 100644 index 1535daa7110..00000000000 --- a/tests/wpt/metadata/2dcontext/pixel-manipulation/2d.imageData.get.zero.html.ini +++ /dev/null @@ -1,5 +0,0 @@ -[2d.imageData.get.zero.html] - type: testharness - [getImageData() throws INDEX_SIZE_ERR if size is zero] - expected: FAIL - From e62dbabb46b8c5b6a5fc3dc6188976cbf2039d75 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Sun, 7 Oct 2018 02:52:06 +0200 Subject: [PATCH 07/13] Handle some transparent black cases in ctx.getImageData --- Cargo.lock | 3 ++ components/canvas/canvas_data.rs | 17 +------ components/pixels/Cargo.toml | 3 ++ components/pixels/lib.rs | 25 +++++++++ .../script/dom/canvasrenderingcontext2d.rs | 51 ++++++++++++++----- components/script/dom/imagedata.rs | 22 +------- 6 files changed, 72 insertions(+), 49 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e80bb0441e1..c999b84bb5c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2663,6 +2663,9 @@ dependencies = [ [[package]] name = "pixels" version = "0.0.1" +dependencies = [ + "euclid 0.19.0 (registry+https://github.com/rust-lang/crates.io-index)", +] [[package]] name = "pkg-config" diff --git a/components/canvas/canvas_data.rs b/components/canvas/canvas_data.rs index e7841d1a53c..cadce6ad941 100644 --- a/components/canvas/canvas_data.rs +++ b/components/canvas/canvas_data.rs @@ -526,22 +526,7 @@ impl<'a> CanvasData<'a> { } let data_surface = self.drawtarget.snapshot().get_data_surface(); - let src_data = unsafe { data_surface.data() }; - let stride = data_surface.stride(); - - //start offset of the copyable rectangle - let mut src = (src_read_rect.origin.y * stride + src_read_rect.origin.x * 4) as usize; - let mut image_data = Vec::with_capacity( - (src_read_rect.size.width * src_read_rect.size.height * 4) as usize, - ); - //copy the data to the destination vector - for _ in 0..src_read_rect.size.height { - let row = &src_data[src .. src + (4 * src_read_rect.size.width) as usize]; - image_data.extend_from_slice(row); - src += stride as usize; - } - - image_data + pixels::get_rect(unsafe { data_surface.data() }, canvas_size.to_u32(), read_rect.to_u32()).into_owned() } } diff --git a/components/pixels/Cargo.toml b/components/pixels/Cargo.toml index 49dcc3833cf..0c2e47b07bb 100644 --- a/components/pixels/Cargo.toml +++ b/components/pixels/Cargo.toml @@ -8,3 +8,6 @@ publish = false [lib] name = "pixels" path = "lib.rs" + +[dependencies] +euclid = "0.19" diff --git a/components/pixels/lib.rs b/components/pixels/lib.rs index d91b14cb650..dc6285ebaab 100644 --- a/components/pixels/lib.rs +++ b/components/pixels/lib.rs @@ -2,6 +2,31 @@ * 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/. */ +extern crate euclid; + +use euclid::{Rect, Size2D}; +use std::borrow::Cow; + +pub fn get_rect(pixels: &[u8], size: Size2D, rect: Rect) -> Cow<[u8]> { + assert!(!rect.is_empty()); + assert!(Rect::from_size(size).contains_rect(&rect)); + assert_eq!(pixels.len() % 4, 0); + assert_eq!(size.area() as usize, pixels.len() / 4); + let area = rect.size.area() as usize; + let first_column_start = rect.origin.x as usize * 4; + let row_length = size.width as usize * 4; + let first_row_start = rect.origin.y as usize * row_length; + if rect.origin.x == 0 && rect.size.width == size.width || rect.size.height == 1 { + let start = first_column_start + first_row_start; + return Cow::Borrowed(&pixels[start..start + area * 4]); + } + let mut data = Vec::with_capacity(area * 4); + for row in pixels[first_row_start..].chunks(row_length).take(rect.size.height as usize) { + data.extend_from_slice(&row[first_column_start..][..rect.size.width as usize * 4]); + } + data.into() +} + // TODO(pcwalton): Speed up with SIMD, or better yet, find some way to not do this. pub fn byte_swap_colors_inplace(pixels: &mut [u8]) { assert!(pixels.len() % 4 == 0); diff --git a/components/script/dom/canvasrenderingcontext2d.rs b/components/script/dom/canvasrenderingcontext2d.rs index 633fbbc3f14..f98da6f9a5e 100644 --- a/components/script/dom/canvasrenderingcontext2d.rs +++ b/components/script/dom/canvasrenderingcontext2d.rs @@ -1137,6 +1137,9 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { mut sw: i32, mut sh: i32, ) -> Fallible> { + // FIXME(nox): There are many arithmetic operations here that can + // overflow or underflow, this should probably be audited. + if sw == 0 || sh == 0 { return Err(Error::IndexSize); } @@ -1154,27 +1157,48 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { sy -= sh; } + let data_width = sw; + let data_height = sh; + + if sx < 0 { + sw += sx; + sx = 0; + } + if sy < 0 { + sh += sy; + sy = 0; + } + + if sw <= 0 || sh <= 0 { + // All the pixels are before the start of the canvas surface. + return ImageData::new(&self.global(), data_width as u32, data_height as u32, None); + } + let (sender, receiver) = ipc::bytes_channel().unwrap(); - let dest_rect = Rect::new(Point2D::new(sx, sy), Size2D::new(sw, sh)); + let src_rect = Rect::new(Point2D::new(sx, sy), Size2D::new(sw, sh)); // FIXME(nox): This is probably wrong when this is a context for an // offscreen canvas. - let canvas_size = self.canvas.as_ref().map_or(Size2D::zero(), |c| c.get_size()); - self.send_canvas_2d_msg(Canvas2dMsg::GetImageData( - dest_rect, - canvas_size.to_i32(), - sender, - )); - let mut data = receiver.recv().unwrap(); - - // Byte swap and unmultiply alpha. - for chunk in data.chunks_mut(4) { + let canvas_size = self.canvas + .as_ref() + .map_or(Size2D::zero(), |c| c.get_size()) + .try_cast().unwrap(); + let canvas_rect = Rect::from_size(canvas_size); + let read_rect = match src_rect.intersection(&canvas_rect) { + Some(rect) if !rect.is_empty() => rect, + _ => { + // All the pixels are past the end of the canvas surface. + return ImageData::new(&self.global(), data_width as u32, data_height as u32, None); + } + }; + self.send_canvas_2d_msg(Canvas2dMsg::GetImageData(read_rect, canvas_size, sender)); + let mut pixels = receiver.recv().unwrap(); + for chunk in pixels.chunks_mut(4) { let b = chunk[0]; chunk[0] = UNPREMULTIPLY_TABLE[256 * (chunk[3] as usize) + chunk[2] as usize]; chunk[1] = UNPREMULTIPLY_TABLE[256 * (chunk[3] as usize) + chunk[1] as usize]; chunk[2] = UNPREMULTIPLY_TABLE[256 * (chunk[3] as usize) + b as usize]; } - - ImageData::new(&self.global(), sw as u32, sh as u32, Some(data.to_vec())) + ImageData::new(&self.global(), sw as u32, sh as u32, Some(pixels.to_vec())) } // https://html.spec.whatwg.org/multipage/#dom-context-2d-putimagedata @@ -1197,6 +1221,7 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { // 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; diff --git a/components/script/dom/imagedata.rs b/components/script/dom/imagedata.rs index 7e615c0e5da..f72ea49c0ff 100644 --- a/components/script/dom/imagedata.rs +++ b/components/script/dom/imagedata.rs @@ -13,6 +13,7 @@ use euclid::{Rect, Size2D}; use js::jsapi::{Heap, JSContext, JSObject}; use js::rust::Runtime; use js::typedarray::{Uint8ClampedArray, CreateWith}; +use pixels; use std::borrow::Cow; use std::default::Default; use std::ptr; @@ -162,31 +163,12 @@ impl ImageData { #[allow(unsafe_code)] pub unsafe fn get_rect(&self, rect: Rect) -> Cow<[u8]> { - assert!(!rect.is_empty()); - assert!(self.rect().contains_rect(&rect)); - let slice = self.as_slice(); - let area = rect.size.area() as usize; - let first_column_start = rect.origin.x as usize * 4; - let row_length = self.width as usize * 4; - let first_row_start = rect.origin.y as usize * row_length; - if rect.origin.x == 0 && rect.size.width == self.width || rect.size.height == 1 { - let start = first_column_start + first_row_start; - return Cow::Borrowed(&slice[start..start + area * 4]); - } - let mut data = Vec::with_capacity(area * 4); - for row in slice[first_row_start..].chunks(row_length).take(rect.size.height as usize) { - data.extend_from_slice(&row[first_column_start..][..rect.size.width as usize * 4]); - } - data.into() + pixels::get_rect(self.as_slice(), self.get_size(), rect) } pub fn get_size(&self) -> Size2D { Size2D::new(self.Width(), self.Height()) } - - pub fn rect(&self) -> Rect { - Rect::from_size(self.get_size()) - } } impl ImageDataMethods for ImageData { From 77c28bdfc9050c645eaae02b0f11636fc73a11fb Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Mon, 8 Oct 2018 11:09:38 +0200 Subject: [PATCH 08/13] Abstract some stuff common to ctx.getImageData and ctx.putImageData --- components/canvas/canvas_data.rs | 27 +-- components/canvas/canvas_paint_thread.rs | 12 +- components/canvas_traits/canvas.rs | 6 +- .../script/dom/canvasrenderingcontext2d.rs | 199 +++++++----------- 4 files changed, 97 insertions(+), 147 deletions(-) diff --git a/components/canvas/canvas_data.rs b/components/canvas/canvas_data.rs index cadce6ad941..15f236e8c86 100644 --- a/components/canvas/canvas_data.rs +++ b/components/canvas/canvas_data.rs @@ -441,24 +441,20 @@ impl<'a> CanvasData<'a> { } // https://html.spec.whatwg.org/multipage/#dom-context-2d-putimagedata - pub fn put_image_data( - &mut self, - mut imagedata: Vec, - offset: Vector2D, - imagedata_size: Size2D, - ) { - assert_eq!(imagedata_size.area() * 4, imagedata.len() as i32); + pub fn put_image_data(&mut self, mut imagedata: Vec, rect: Rect) { + assert_eq!(imagedata.len() % 4, 0); + assert_eq!(rect.size.area() as usize, imagedata.len() / 4); pixels::byte_swap_and_premultiply_inplace(&mut imagedata); let source_surface = self.drawtarget.create_source_surface_from_data( &imagedata, - imagedata_size, - imagedata_size.width * 4, + rect.size.to_i32(), + rect.size.width as i32 * 4, SurfaceFormat::B8G8R8A8, ).unwrap(); self.drawtarget.copy_surface( source_surface, - Rect::from_size(imagedata_size), - offset.to_point(), + Rect::from_size(rect.size.to_i32()), + rect.origin.to_i32(), ); } @@ -517,14 +513,11 @@ impl<'a> CanvasData<'a> { /// canvas_size: The size of the canvas we're reading from /// read_rect: The area of the canvas we want to read from #[allow(unsafe_code)] - pub fn read_pixels(&self, read_rect: Rect, canvas_size: Size2D) -> Vec { + pub fn read_pixels(&self, read_rect: Rect, canvas_size: Size2D) -> Vec { let canvas_rect = Rect::from_size(canvas_size); - let src_read_rect = canvas_rect.intersection(&read_rect).unwrap_or(Rect::zero()); - - if src_read_rect.is_empty() || canvas_size.width <= 0 && canvas_size.height <= 0 { - return vec![]; + if canvas_rect.intersection(&read_rect).map_or(true, |rect| rect.is_empty()) { + return vec![]; } - let data_surface = self.drawtarget.snapshot().get_data_surface(); pixels::get_rect(unsafe { data_surface.data() }, canvas_size.to_u32(), read_rect.to_u32()).into_owned() } diff --git a/components/canvas/canvas_paint_thread.rs b/components/canvas/canvas_paint_thread.rs index dae7836fd71..21ea434a369 100644 --- a/components/canvas/canvas_paint_thread.rs +++ b/components/canvas/canvas_paint_thread.rs @@ -162,8 +162,8 @@ impl<'a> CanvasPaintThread <'a> { smoothing ) => { let image_data = self.canvas(canvas_id).read_pixels( - source_rect.to_i32(), - image_size.to_i32(), + source_rect.to_u32(), + image_size.to_u32(), ); self.canvas(other_canvas_id).draw_image( image_data.into(), @@ -242,12 +242,8 @@ impl<'a> CanvasPaintThread <'a> { let pixels = self.canvas(canvas_id).read_pixels(dest_rect, canvas_size); sender.send(&pixels).unwrap(); }, - Canvas2dMsg::PutImageData(receiver, offset, imagedata_size) => { - self.canvas(canvas_id).put_image_data( - receiver.recv().unwrap(), - offset, - imagedata_size, - ) + Canvas2dMsg::PutImageData(rect, receiver) => { + self.canvas(canvas_id).put_image_data(receiver.recv().unwrap(), rect); }, Canvas2dMsg::SetShadowOffsetX(value) => { self.canvas(canvas_id).set_shadow_offset_x(value) diff --git a/components/canvas_traits/canvas.rs b/components/canvas_traits/canvas.rs index 8e452c93aff..0bf4a4ac71c 100644 --- a/components/canvas_traits/canvas.rs +++ b/components/canvas_traits/canvas.rs @@ -3,7 +3,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ use cssparser::RGBA; -use euclid::{Transform2D, Point2D, Vector2D, Rect, Size2D}; +use euclid::{Transform2D, Point2D, Rect, Size2D}; use ipc_channel::ipc::{IpcBytesReceiver, IpcBytesSender, IpcSender}; use serde_bytes::ByteBuf; use std::default::Default; @@ -50,11 +50,11 @@ pub enum Canvas2dMsg { Fill, FillText(String, f64, f64, Option), FillRect(Rect), - GetImageData(Rect, Size2D, IpcBytesSender), + GetImageData(Rect, Size2D, IpcBytesSender), IsPointInPath(f64, f64, FillRule, IpcSender), LineTo(Point2D), MoveTo(Point2D), - PutImageData(IpcBytesReceiver, Vector2D, Size2D), + PutImageData(Rect, IpcBytesReceiver), QuadraticCurveTo(Point2D, Point2D), Rect(Rect), RestoreContext, diff --git a/components/script/dom/canvasrenderingcontext2d.rs b/components/script/dom/canvasrenderingcontext2d.rs index f98da6f9a5e..5aa1518932d 100644 --- a/components/script/dom/canvasrenderingcontext2d.rs +++ b/components/script/dom/canvasrenderingcontext2d.rs @@ -1130,13 +1130,7 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { } // https://html.spec.whatwg.org/multipage/#dom-context-2d-getimagedata - fn GetImageData( - &self, - mut sx: i32, - mut sy: i32, - mut sw: i32, - mut sh: i32, - ) -> Fallible> { + fn GetImageData(&self, sx: i32, sy: i32, sw: i32, sh: i32) -> Fallible> { // FIXME(nox): There are many arithmetic operations here that can // overflow or underflow, this should probably be audited. @@ -1148,57 +1142,30 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { return Err(Error::Security); } - if sw < 0 { - sw = -sw; - sx -= sw; - } - if sh < 0 { - sh = -sh; - sy -= sh; - } - - let data_width = sw; - let data_height = sh; - - if sx < 0 { - sw += sx; - sx = 0; - } - if sy < 0 { - sh += sy; - sy = 0; - } - - if sw <= 0 || sh <= 0 { - // All the pixels are before the start of the canvas surface. - return ImageData::new(&self.global(), data_width as u32, data_height as u32, None); - } - - let (sender, receiver) = ipc::bytes_channel().unwrap(); - let src_rect = Rect::new(Point2D::new(sx, sy), Size2D::new(sw, sh)); + let (origin, size) = adjust_size_sign(Point2D::new(sx, sy), Size2D::new(sw, sh)); // FIXME(nox): This is probably wrong when this is a context for an // offscreen canvas. - let canvas_size = self.canvas - .as_ref() - .map_or(Size2D::zero(), |c| c.get_size()) - .try_cast().unwrap(); - let canvas_rect = Rect::from_size(canvas_size); - let read_rect = match src_rect.intersection(&canvas_rect) { - Some(rect) if !rect.is_empty() => rect, - _ => { - // All the pixels are past the end of the canvas surface. - return ImageData::new(&self.global(), data_width as u32, data_height as u32, None); - } + let canvas_size = self.canvas.as_ref().map_or(Size2D::zero(), |c| c.get_size()); + let read_rect = match clip(origin, size, canvas_size) { + Some(rect) => rect, + None => { + // All the pixels are outside the canvas surface. + return ImageData::new(&self.global(), size.width, size.height, None); + }, }; + + let (sender, receiver) = ipc::bytes_channel().unwrap(); self.send_canvas_2d_msg(Canvas2dMsg::GetImageData(read_rect, canvas_size, sender)); - let mut pixels = receiver.recv().unwrap(); + let mut pixels = receiver.recv().unwrap().to_vec(); + for chunk in pixels.chunks_mut(4) { let b = chunk[0]; chunk[0] = UNPREMULTIPLY_TABLE[256 * (chunk[3] as usize) + chunk[2] as usize]; chunk[1] = UNPREMULTIPLY_TABLE[256 * (chunk[3] as usize) + chunk[1] as usize]; chunk[2] = UNPREMULTIPLY_TABLE[256 * (chunk[3] as usize) + b as usize]; } - ImageData::new(&self.global(), sw as u32, sh as u32, Some(pixels.to_vec())) + + ImageData::new(&self.global(), size.width, size.height, Some(pixels)) } // https://html.spec.whatwg.org/multipage/#dom-context-2d-putimagedata @@ -1213,17 +1180,17 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { imagedata: &ImageData, dx: i32, dy: i32, - mut dirty_x: i32, - mut dirty_y: i32, - mut dirty_width: i32, - mut dirty_height: i32, + dirty_x: i32, + dirty_y: i32, + dirty_width: i32, + 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 { + let imagedata_size = Size2D::new(imagedata.Width(), imagedata.Height()); + if imagedata_size.area() == 0 { return; } @@ -1233,76 +1200,37 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { // 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; - } - - // 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; - 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; - } - // FIXME(nox): This is probably wrong when this is a context for an // offscreen canvas. - 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(); + let canvas_size = self.canvas.as_ref().map_or(Size2D::zero(), |c| c.get_size()); - // We take care of ignoring any pixel that would be drawn after the end - // of the canvas surface. - 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; - } - - let dirty_size = Size2D::new(dirty_width, dirty_height); - let dirty_rect = Rect::new(Point2D::new(dirty_x, dirty_y), dirty_size); + // Steps 3-6. + let (src_origin, src_size) = adjust_size_sign( + Point2D::new(dirty_x, dirty_y), + Size2D::new(dirty_width, dirty_height), + ); + let src_rect = match clip(src_origin, src_size, imagedata_size) { + Some(rect) => rect, + None => return, + }; + let (dst_origin, _) = adjust_size_sign( + Point2D::new(dirty_x.saturating_add(dx), dirty_y.saturating_add(dy)), + Size2D::new(dirty_width, dirty_height), + ); + // By clipping to the canvas surface, we avoid sending any pixel + // that would fall outside it. + let dst_rect = match clip(dst_origin, src_rect.size, canvas_size) { + Some(rect) => rect, + None => return, + }; // Step 7. let (sender, receiver) = ipc::bytes_channel().unwrap(); - self.send_canvas_2d_msg(Canvas2dMsg::PutImageData( - receiver, - origin.to_vector(), - dirty_size, - )); - sender.send(unsafe { &imagedata.get_rect(dirty_rect.try_cast().unwrap()) }).unwrap(); + let pixels = unsafe { + &imagedata.get_rect(Rect::new(src_rect.origin, dst_rect.size)) + }; + self.send_canvas_2d_msg(Canvas2dMsg::PutImageData(dst_rect, receiver)); + sender.send(pixels).unwrap(); self.mark_as_dirty(); } @@ -1552,7 +1480,7 @@ fn is_rect_valid(rect: Rect) -> bool { rect.size.width > 0.0 && rect.size.height > 0.0 } -// https://html.spec.whatwg.org/multipage/#serialisation-of-a-colour +// https://html.spec.whatwg.org/multipage/#serialisation-of-a-color fn serialize(color: &RGBA, dest: &mut W) -> fmt::Result where W: fmt::Write, @@ -1583,3 +1511,36 @@ where ) } } + +fn adjust_size_sign( + mut origin: Point2D, + mut size: Size2D, +) -> (Point2D, Size2D) { + if size.width < 0 { + size.width = -size.width; + origin.x = origin.x.saturating_sub(size.width); + } + if size.height < 0 { + size.height = -size.height; + origin.y = origin.y.saturating_sub(size.height); + } + (origin, size.to_u32()) +} + +fn clip( + mut origin: Point2D, + mut size: Size2D, + surface: Size2D, +) -> Option> { + if origin.x < 0 { + size.width = size.width.saturating_sub(-origin.x as u32); + origin.x = 0; + } + if origin.y < 0 { + size.height = size.height.saturating_sub(-origin.y as u32); + origin.y = 0; + } + Rect::new(origin.to_u32(), size) + .intersection(&Rect::from_size(surface)) + .filter(|rect| !rect.is_empty()) +} From 551c405b0fc03350109b560f75169edf43284297 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Mon, 8 Oct 2018 12:19:10 +0200 Subject: [PATCH 09/13] Avoid ctx.getImageData in canvas.toDataURL --- .../script/dom/canvasrenderingcontext2d.rs | 35 ++++++++++++------- components/script/dom/htmlcanvaselement.rs | 16 ++++----- 2 files changed, 29 insertions(+), 22 deletions(-) diff --git a/components/script/dom/canvasrenderingcontext2d.rs b/components/script/dom/canvasrenderingcontext2d.rs index 5aa1518932d..2617f9e7ed5 100644 --- a/components/script/dom/canvasrenderingcontext2d.rs +++ b/components/script/dom/canvasrenderingcontext2d.rs @@ -575,6 +575,28 @@ impl CanvasRenderingContext2D { fn set_origin_unclean(&self) { self.origin_clean.set(false) } + + pub fn get_rect(&self, rect: Rect) -> Vec { + assert!(self.origin_is_clean()); + + // FIXME(nox): This is probably wrong when this is a context for an + // offscreen canvas. + let canvas_size = self.canvas.as_ref().map_or(Size2D::zero(), |c| c.get_size()); + assert!(Rect::from_size(canvas_size).contains_rect(&rect)); + + let (sender, receiver) = ipc::bytes_channel().unwrap(); + self.send_canvas_2d_msg(Canvas2dMsg::GetImageData(rect, canvas_size, sender)); + let mut pixels = receiver.recv().unwrap().to_vec(); + + for chunk in pixels.chunks_mut(4) { + let b = chunk[0]; + chunk[0] = UNPREMULTIPLY_TABLE[256 * (chunk[3] as usize) + chunk[2] as usize]; + chunk[1] = UNPREMULTIPLY_TABLE[256 * (chunk[3] as usize) + chunk[1] as usize]; + chunk[2] = UNPREMULTIPLY_TABLE[256 * (chunk[3] as usize) + b as usize]; + } + + pixels + } } pub trait LayoutCanvasRenderingContext2DHelpers { @@ -1154,18 +1176,7 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { }, }; - let (sender, receiver) = ipc::bytes_channel().unwrap(); - self.send_canvas_2d_msg(Canvas2dMsg::GetImageData(read_rect, canvas_size, sender)); - let mut pixels = receiver.recv().unwrap().to_vec(); - - for chunk in pixels.chunks_mut(4) { - let b = chunk[0]; - chunk[0] = UNPREMULTIPLY_TABLE[256 * (chunk[3] as usize) + chunk[2] as usize]; - chunk[1] = UNPREMULTIPLY_TABLE[256 * (chunk[3] as usize) + chunk[1] as usize]; - chunk[2] = UNPREMULTIPLY_TABLE[256 * (chunk[3] as usize) + b as usize]; - } - - ImageData::new(&self.global(), size.width, size.height, Some(pixels)) + ImageData::new(&self.global(), size.width, size.height, Some(self.get_rect(read_rect))) } // https://html.spec.whatwg.org/multipage/#dom-context-2d-putimagedata diff --git a/components/script/dom/htmlcanvaselement.rs b/components/script/dom/htmlcanvaselement.rs index 78e74641347..798c4e9d93c 100644 --- a/components/script/dom/htmlcanvaselement.rs +++ b/components/script/dom/htmlcanvaselement.rs @@ -7,7 +7,6 @@ use canvas_traits::canvas::{CanvasMsg, CanvasId, FromScriptMsg}; use canvas_traits::webgl::WebGLVersion; use dom::attr::Attr; use dom::bindings::cell::DomRefCell; -use dom::bindings::codegen::Bindings::CanvasRenderingContext2DBinding::CanvasRenderingContext2DMethods; use dom::bindings::codegen::Bindings::HTMLCanvasElementBinding; use dom::bindings::codegen::Bindings::HTMLCanvasElementBinding::{HTMLCanvasElementMethods, RenderingContext}; use dom::bindings::codegen::Bindings::WebGLRenderingContextBinding::WebGLContextAttributes; @@ -27,7 +26,7 @@ use dom::virtualmethods::VirtualMethods; use dom::webgl2renderingcontext::WebGL2RenderingContext; use dom::webglrenderingcontext::{LayoutCanvasWebGLRenderingContextHelpers, WebGLRenderingContext}; use dom_struct::dom_struct; -use euclid::Size2D; +use euclid::{Rect, Size2D}; use html5ever::{LocalName, Prefix}; use image::ColorType; use image::png::PNGEncoder; @@ -354,10 +353,8 @@ impl HTMLCanvasElementMethods for HTMLCanvasElement { _quality: HandleValue, ) -> Fallible { // Step 1. - if let Some(CanvasContext::Context2d(ref context)) = *self.context.borrow() { - if !context.origin_is_clean() { - return Err(Error::Security); - } + if !self.origin_is_clean() { + return Err(Error::Security); } // Step 2. @@ -366,10 +363,9 @@ impl HTMLCanvasElementMethods for HTMLCanvasElement { } // Step 3. - let raw_data = match *self.context.borrow() { + let file = match *self.context.borrow() { Some(CanvasContext::Context2d(ref context)) => { - // FIXME(nox): This shouldn't go through ImageData etc. - context.GetImageData(0, 0, self.Width() as i32, self.Height() as i32)?.to_vec() + context.get_rect(Rect::from_size(self.get_size())) }, Some(CanvasContext::WebGL(ref context)) => { match context.get_image_data(self.Width(), self.Height()) { @@ -392,7 +388,7 @@ impl HTMLCanvasElementMethods for HTMLCanvasElement { // FIXME: Only handle image/png for now. let mut png = Vec::new(); PNGEncoder::new(&mut png) - .encode(&raw_data, self.Width(), self.Height(), ColorType::RGBA(8)) + .encode(&file, self.Width(), self.Height(), ColorType::RGBA(8)) .unwrap(); let mut url = "data:image/png;base64,".to_owned(); // FIXME(nox): Should this use base64::URL_SAFE? From 05ef233097e17c3cdd0000f434d1592e8e26ff54 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Mon, 8 Oct 2018 13:13:24 +0200 Subject: [PATCH 10/13] Add a couple of bug links --- components/script/dom/htmlcanvaselement.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/components/script/dom/htmlcanvaselement.rs b/components/script/dom/htmlcanvaselement.rs index 798c4e9d93c..5cc28fcf461 100644 --- a/components/script/dom/htmlcanvaselement.rs +++ b/components/script/dom/htmlcanvaselement.rs @@ -387,6 +387,8 @@ impl HTMLCanvasElementMethods for HTMLCanvasElement { // FIXME: Only handle image/png for now. let mut png = Vec::new(); + // FIXME(nox): https://github.com/PistonDevelopers/image-png/issues/86 + // FIXME(nox): https://github.com/PistonDevelopers/image-png/issues/87 PNGEncoder::new(&mut png) .encode(&file, self.Width(), self.Height(), ColorType::RGBA(8)) .unwrap(); From 6c469b90b1ae34bddcb7da19eacfa6ad4467cf35 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Mon, 8 Oct 2018 13:49:58 +0200 Subject: [PATCH 11/13] Share some code between 2D canvas and WebGL --- components/canvas/webgl_thread.rs | 27 +++----- components/canvas_traits/webgl.rs | 4 +- components/pixels/lib.rs | 20 +++++- .../script/dom/canvasrenderingcontext2d.rs | 24 +------ components/script/dom/htmlcanvaselement.rs | 4 +- .../script/dom/webglrenderingcontext.rs | 63 +++++++------------ 6 files changed, 58 insertions(+), 84 deletions(-) diff --git a/components/canvas/webgl_thread.rs b/components/canvas/webgl_thread.rs index cd8dbde18d2..d00ae02871b 100644 --- a/components/canvas/webgl_thread.rs +++ b/components/canvas/webgl_thread.rs @@ -6,7 +6,6 @@ use canvas_traits::webgl::*; use euclid::Size2D; use fnv::FnvHashMap; use gleam::gl; -use ipc_channel::ipc::IpcBytesSender; use offscreen_gl_context::{GLContext, GLContextAttributes, GLLimits, NativeGLContextMethods}; use pixels; use std::thread; @@ -791,8 +790,16 @@ impl WebGLImpl { ctx.gl().pixel_store_i(name, val), WebGLCommand::PolygonOffset(factor, units) => ctx.gl().polygon_offset(factor, units), - WebGLCommand::ReadPixels(x, y, width, height, format, pixel_type, ref chan) => { - Self::read_pixels(ctx.gl(), x, y, width, height, format, pixel_type, chan) + WebGLCommand::ReadPixels(rect, format, pixel_type, ref sender) => { + let pixels = ctx.gl().read_pixels( + rect.origin.x as i32, + rect.origin.y as i32, + rect.size.width as i32, + rect.size.height as i32, + format, + pixel_type, + ); + sender.send(&pixels).unwrap(); } WebGLCommand::RenderbufferStorage(target, format, width, height) => ctx.gl().renderbuffer_storage(target, format, width, height), @@ -1335,20 +1342,6 @@ impl WebGLImpl { } } - fn read_pixels( - gl: &gl::Gl, - x: i32, - y: i32, - width: i32, - height: i32, - format: u32, - pixel_type: u32, - chan: &IpcBytesSender, - ) { - let result = gl.read_pixels(x, y, width, height, format, pixel_type); - chan.send(&result).unwrap() - } - fn finish(gl: &gl::Gl, chan: &WebGLSender<()>) { gl.finish(); chan.send(()).unwrap(); diff --git a/components/canvas_traits/webgl.rs b/components/canvas_traits/webgl.rs index 4c392972b8e..43a24771ae1 100644 --- a/components/canvas_traits/webgl.rs +++ b/components/canvas_traits/webgl.rs @@ -2,7 +2,7 @@ * 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/. */ -use euclid::Size2D; +use euclid::{Rect, Size2D}; use gleam::gl; use ipc_channel::ipc::{IpcBytesReceiver, IpcBytesSender}; use offscreen_gl_context::{GLContextAttributes, GLLimits}; @@ -227,7 +227,7 @@ pub enum WebGLCommand { GetRenderbufferParameter(u32, u32, WebGLSender), PolygonOffset(f32, f32), RenderbufferStorage(u32, u32, i32, i32), - ReadPixels(i32, i32, i32, i32, u32, u32, IpcBytesSender), + ReadPixels(Rect, u32, u32, IpcBytesSender), SampleCoverage(f32, bool), Scissor(i32, i32, u32, u32), StencilFunc(u32, i32, u32), diff --git a/components/pixels/lib.rs b/components/pixels/lib.rs index dc6285ebaab..de29e5fa2ee 100644 --- a/components/pixels/lib.rs +++ b/components/pixels/lib.rs @@ -4,7 +4,7 @@ extern crate euclid; -use euclid::{Rect, Size2D}; +use euclid::{Point2D, Rect, Size2D}; use std::borrow::Cow; pub fn get_rect(pixels: &[u8], size: Size2D, rect: Rect) -> Cow<[u8]> { @@ -63,3 +63,21 @@ pub fn premultiply_inplace(pixels: &mut [u8]) -> bool { pub fn multiply_u8_color(a: u8, b: u8) -> u8 { return (a as u32 * b as u32 / 255) as u8; } + +pub fn clip( + mut origin: Point2D, + mut size: Size2D, + surface: Size2D, +) -> Option> { + if origin.x < 0 { + size.width = size.width.saturating_sub(-origin.x as u32); + origin.x = 0; + } + if origin.y < 0 { + size.height = size.height.saturating_sub(-origin.y as u32); + origin.y = 0; + } + Rect::new(origin.to_u32(), size) + .intersection(&Rect::from_size(surface)) + .filter(|rect| !rect.is_empty()) +} diff --git a/components/script/dom/canvasrenderingcontext2d.rs b/components/script/dom/canvasrenderingcontext2d.rs index 2617f9e7ed5..dacabcccce5 100644 --- a/components/script/dom/canvasrenderingcontext2d.rs +++ b/components/script/dom/canvasrenderingcontext2d.rs @@ -1168,7 +1168,7 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { // FIXME(nox): This is probably wrong when this is a context for an // offscreen canvas. let canvas_size = self.canvas.as_ref().map_or(Size2D::zero(), |c| c.get_size()); - let read_rect = match clip(origin, size, canvas_size) { + let read_rect = match pixels::clip(origin, size, canvas_size) { Some(rect) => rect, None => { // All the pixels are outside the canvas surface. @@ -1220,7 +1220,7 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { Point2D::new(dirty_x, dirty_y), Size2D::new(dirty_width, dirty_height), ); - let src_rect = match clip(src_origin, src_size, imagedata_size) { + let src_rect = match pixels::clip(src_origin, src_size, imagedata_size) { Some(rect) => rect, None => return, }; @@ -1230,7 +1230,7 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D { ); // By clipping to the canvas surface, we avoid sending any pixel // that would fall outside it. - let dst_rect = match clip(dst_origin, src_rect.size, canvas_size) { + let dst_rect = match pixels::clip(dst_origin, src_rect.size, canvas_size) { Some(rect) => rect, None => return, }; @@ -1537,21 +1537,3 @@ fn adjust_size_sign( } (origin, size.to_u32()) } - -fn clip( - mut origin: Point2D, - mut size: Size2D, - surface: Size2D, -) -> Option> { - if origin.x < 0 { - size.width = size.width.saturating_sub(-origin.x as u32); - origin.x = 0; - } - if origin.y < 0 { - size.height = size.height.saturating_sub(-origin.y as u32); - origin.y = 0; - } - Rect::new(origin.to_u32(), size) - .intersection(&Rect::from_size(surface)) - .filter(|rect| !rect.is_empty()) -} diff --git a/components/script/dom/htmlcanvaselement.rs b/components/script/dom/htmlcanvaselement.rs index 5cc28fcf461..8f572336573 100644 --- a/components/script/dom/htmlcanvaselement.rs +++ b/components/script/dom/htmlcanvaselement.rs @@ -368,13 +368,13 @@ impl HTMLCanvasElementMethods for HTMLCanvasElement { context.get_rect(Rect::from_size(self.get_size())) }, Some(CanvasContext::WebGL(ref context)) => { - match context.get_image_data(self.Width(), self.Height()) { + match context.get_image_data(self.get_size()) { Some(data) => data, None => return Ok(USVString("data:,".into())), } }, Some(CanvasContext::WebGL2(ref context)) => { - match context.base_context().get_image_data(self.Width(), self.Height()) { + match context.base_context().get_image_data(self.get_size()) { Some(data) => data, None => return Ok(USVString("data:,".into())), } diff --git a/components/script/dom/webglrenderingcontext.rs b/components/script/dom/webglrenderingcontext.rs index 4e3658fa524..bfccd5e6f68 100644 --- a/components/script/dom/webglrenderingcontext.rs +++ b/components/script/dom/webglrenderingcontext.rs @@ -52,7 +52,7 @@ use dom::webgluniformlocation::WebGLUniformLocation; use dom::webglvertexarrayobjectoes::WebGLVertexArrayObjectOES; use dom::window::Window; use dom_struct::dom_struct; -use euclid::Size2D; +use euclid::{Point2D, Rect, Size2D}; use half::f16; use ipc_channel::ipc; use js::jsapi::{JSContext, JSObject, Type}; @@ -1070,7 +1070,7 @@ impl WebGLRenderingContext { // can fail and that it is UB what happens in that case. // // https://www.khronos.org/registry/webgl/specs/latest/1.0/#2.2 - pub fn get_image_data(&self, width: u32, height: u32) -> Option> { + pub fn get_image_data(&self, mut size: Size2D) -> Option> { handle_potential_webgl_error!(self, self.validate_framebuffer(), return None); let (fb_width, fb_height) = handle_potential_webgl_error!( @@ -1078,15 +1078,12 @@ impl WebGLRenderingContext { self.get_current_framebuffer_size().ok_or(InvalidOperation), return None ); - let width = cmp::min(width, fb_width as u32); - let height = cmp::min(height, fb_height as u32); + size.width = cmp::min(size.width, fb_width as u32); + size.height = cmp::min(size.height, fb_height as u32); let (sender, receiver) = ipc::bytes_channel().unwrap(); self.send_command(WebGLCommand::ReadPixels( - 0, - 0, - width as i32, - height as i32, + Rect::from_size(size), constants::RGBA, constants::UNSIGNED_BYTE, sender, @@ -2879,45 +2876,29 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext { return self.webgl_error(InvalidOperation); } - let mut src_x = x; - let mut src_y = y; - let mut src_width = width; - let mut src_height = height; + let src_origin = Point2D::new(x, y); + let src_size = Size2D::new(width as u32, height as u32); + let fb_size = Size2D::new(fb_width as u32, fb_height as u32); + let src_rect = match pixels::clip(src_origin, src_size, fb_size) { + Some(rect) => rect, + None => return, + }; + let mut dest_offset = 0; - - if src_x < 0 { - if src_width <= -src_x { - return; - } - dest_offset += bytes_per_pixel * -src_x; - src_width += src_x; - src_x = 0; + if x < 0 { + dest_offset += -x * bytes_per_pixel; } - if src_y < 0 { - if src_height <= -src_y { - return; - } - dest_offset += row_len * -src_y; - src_height += src_y; - src_y = 0; - } - - if src_x + src_width > fb_width { - src_width = fb_width - src_x; - } - if src_y + src_height > fb_height { - src_height = fb_height - src_y; + if y < 0 { + dest_offset += -y * row_len; } let (sender, receiver) = ipc::bytes_channel().unwrap(); - self.send_command(WebGLCommand::ReadPixels( - src_x, src_y, src_width, src_height, format, pixel_type, sender, - )); - + self.send_command(WebGLCommand::ReadPixels(src_rect, format, pixel_type, sender)); let src = receiver.recv().unwrap(); - let src_row_len = (src_width * bytes_per_pixel) as usize; - for i in 0..src_height { - let dest_start = (dest_offset + i * dest_stride) as usize; + + let src_row_len = src_rect.size.width as usize * bytes_per_pixel as usize; + for i in 0..src_rect.size.height { + let dest_start = dest_offset as usize + i as usize * dest_stride as usize; let dest_end = dest_start + src_row_len; let src_start = i as usize * src_row_len; let src_end = src_start + src_row_len; From 2bf4fcd9bd09f6f3301854c7bbca9e41409b0187 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Tue, 9 Oct 2018 11:03:47 +0200 Subject: [PATCH 12/13] Remove some condition in CanvasData::write_image The surface creation should never fail. --- components/canvas/canvas_data.rs | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/components/canvas/canvas_data.rs b/components/canvas/canvas_data.rs index 15f236e8c86..d4a3bcc9a1a 100644 --- a/components/canvas/canvas_data.rs +++ b/components/canvas/canvas_data.rs @@ -639,23 +639,23 @@ fn write_image( } else { Filter::Point }; - // azure_hl operates with integers. We need to cast the image size let image_size = image_size.to_i32(); - if let Some(source_surface) = - draw_target.create_source_surface_from_data(&image_data, - image_size, - image_size.width * 4, - SurfaceFormat::B8G8R8A8) { - let draw_surface_options = DrawSurfaceOptions::new(filter, true); - let draw_options = DrawOptions::new(global_alpha, composition_op, AntialiasMode::None); - - draw_target.draw_surface(source_surface, - dest_rect.to_azure_style(), - image_rect.to_azure_style(), - draw_surface_options, - draw_options); - } + let source_surface = draw_target.create_source_surface_from_data( + &image_data, + image_size, + image_size.width * 4, + SurfaceFormat::B8G8R8A8, + ).unwrap(); + let draw_surface_options = DrawSurfaceOptions::new(filter, true); + let draw_options = DrawOptions::new(global_alpha, composition_op, AntialiasMode::None); + draw_target.draw_surface( + source_surface, + dest_rect.to_azure_style(), + image_rect.to_azure_style(), + draw_surface_options, + draw_options, + ); } pub trait PointToi32 { From c53db64e63f3268d90f45c00f36f94941796b104 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Tue, 9 Oct 2018 11:04:33 +0200 Subject: [PATCH 13/13] Return input as is when there is no cropping to be done --- components/canvas/canvas_data.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/components/canvas/canvas_data.rs b/components/canvas/canvas_data.rs index d4a3bcc9a1a..0ce0b352ccc 100644 --- a/components/canvas/canvas_data.rs +++ b/components/canvas/canvas_data.rs @@ -590,6 +590,9 @@ fn crop_image( // We're going to iterate over a pixel values array so we need integers let crop_rect = crop_rect.to_i32(); let image_size = image_size.to_i32(); + if crop_rect == Rect::from_size(image_size) { + return image_data; + } // Assuming 4 bytes per pixel and row-major order for storage // (consecutive elements in a pixel row of the image are contiguous in memory) let stride = image_size.width * 4;