Do not use lookup tables for put_image_data #9911

magni- · 2016-03-08T09:27:26Z

"This is the first Rust code I have ever written" (and also my first time writing real browser code, it's been quite the learning experience).

Some questions:

For really fast CPU results, use integer SIMD instructions to handle more than one pixel at a time.

This was out of the scope of #9599, right? I started looking into doing that, but it seems to be a lot more work than the E-easy label would suggest. std::simd is marked as "unstable", is that a blocker?

128 can be added before dividing to round more accurately.

@michaelwu, what did you mean by that?
#9599 mentions #8086, but this isn't just a revert of that PR, correct? I didn't touch script/ at all. Do I need to re-add the tests here: https://github.com/servo/servo/pull/8086/files#diff-d6e07693f6e7f8f93e2113a102d1d405L1 ?

Also, the #9599 is Do not use lookup tables for {Get,Put}ImageData operations, but we only use lookup tables for the Put, not the Get, right?

Sorry for all the noobish questions.

wafflespeanut · 2016-03-08T14:15:59Z

r? @michaelwu

michaelwu · 2016-03-08T16:39:53Z

Thanks for the PR!

128 can be added before dividing to round more accurately.

@michaelwu, what did you mean by that?

The math rounds down by default, so if the result was something like 5.75 in floating point, the final result would be 5 rather than 6. Adding 128 before dividing allows us to round up in these cases. From a floating point POV, it looks like you're adding .5 before rounding down.

#9599 mentions #8086, but this isn't just a revert of that PR, correct? I didn't touch script/ at all. Do I need to re-add the tests here: https://github.com/servo/servo/pull/8086/files#diff-d6e07693f6e7f8f93e2113a102d1d405L1 ?

Correct. You shouldn't need to put those tests back on the failing list if the math here is correct. However, you might need to do the rounding thing I suggested to get the results accurate enough to not fail tests.

Also, the #9599 is Do not use lookup tables for {Get,Put}ImageData operations, but we only use lookup tables for the Put, not the Get, right?

It looks like we only use the table for the Get. Either way - your PR is on the right track.

jdm · 2016-03-08T16:41:10Z

See also components/script/unpremultiplytable.rs.

michaelwu · 2016-03-08T16:42:32Z

For really fast CPU results, use integer SIMD instructions to handle more than one pixel at a time.

This was out of the scope of #9599, right? I started looking into doing that, but it seems to be a lot more work than the E-easy label would suggest. std::simd is marked as "unstable", is that a blocker?

Yeah don't worry about it. It's tricky since you have to worry about alignment too.

magni- · 2016-03-09T06:38:04Z

Out of curiosity, why is the premultiplication for put_image_data implemented in components/canvas but the unpremultiplication (postdivision?) for GetImageData implemented in components/script?

The math rounds down by default, so if the result was something like 5.75 in floating point, the final result would be 5 rather than 6. Adding 128 before dividing allows us to round up in these cases. From a floating point POV, it looks like you're adding .5 before rounding down.

Ahh gotcha, makes a lot more sense now.

magni- · 2016-03-09T06:40:30Z

components/canvas/canvas_paint_thread.rs

@@ -795,6 +790,11 @@ fn is_zero_size_gradient(pattern: &Pattern) -> bool {
    false
 }

+fn premultiply_channel_by_alpha(channel: u8, alpha: u16) -> u8 {


I felt like this could be spun out into a helper, not least because those ((((s looked potentially confusing, but happy to put it back in put_image_data.

Can you put this function in the block where it's used?

jdm · 2016-03-09T18:41:57Z

@bors-servo: try

bors-servo · 2016-03-09T18:41:59Z

⌛ Trying commit b1e087f with merge b01dd4a...

@michaelwu

Don't use lookup tables for put_image_data and GetImageData Fixes #9599 "This is the first Rust code I have ever written" (and also my first time writing real browser code, it's been quite the learning experience). Some questions: > For really fast CPU results, use integer SIMD instructions to handle more than one pixel at a time. This was out of the scope of #9599, right? I started looking into doing that, but it seems to be a lot more work than the `E-easy` label would suggest. [`std::simd`](https://doc.rust-lang.org/1.0.0/std/simd/index.html) is marked as "unstable", is that a blocker? > 128 can be added before dividing to round more accurately. @michaelwu, what did you mean by that? Also, the #9599 is `Do not use lookup tables for {Get,Put}ImageData operations`, but we only use lookup tables for the `Put`, not the `Get`, right? Sorry for all the noobish questions.  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9911)

bors-servo · 2016-03-09T19:22:31Z

💔 Test failed - mac-rel-wpt

jdm · 2016-03-09T19:38:00Z

 ▶ Unexpected subtest result in /2dcontext/pixel-manipulation/2d.imageData.put.dirty.rect2.html:
  │ FAIL [expected PASS] putImageData() only modifies areas inside the dirty rectangle, using x and y
  │   → assert_approx_equals: Red channel of the pixel at (50, 25) expected 0 +/- 2 but got 255
  │ 
  │ _assertPixelApprox@http://web-platform.test:8000/common/canvas-tests.js:46:5
  │ @http://web-platform.test:8000/2dcontext/pixel-manipulation/2d.imageData.put.dirty.rect2.html:17:1
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1381:20
  │ _addTest/</<@http://web-platform.test:8000/common/canvas-tests.js:61:13
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1381:20
  └ _addTest/<@http://web-platform.test:8000/common/canvas-tests.js:58:9

  ▶ Unexpected subtest result in /2dcontext/pixel-manipulation/2d.imageData.put.unchanged.html:
  │ FAIL [expected PASS] putImageData(getImageData(...), ...) has no effect
  │   → assert_equals: olddata["14"] === imgdata2.data["14"] (got 85[number], expected 0[number]) expected 0 but got 85
  │ 
  │ _assertSame@http://web-platform.test:8000/common/canvas-tests.js:16:5
  │ @http://web-platform.test:8000/2dcontext/pixel-manipulation/2d.imageData.put.unchanged.html:20:5
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1381:20
  │ _addTest/</<@http://web-platform.test:8000/common/canvas-tests.js:61:13
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1381:20
  └ _addTest/<@http://web-platform.test:8000/common/canvas-tests.js:58:9

magni- · 2016-03-10T07:50:23Z

components/script/dom/canvasrenderingcontext2d.rs

-            chunk[2] = UNPREMULTIPLY_TABLE[256 * alpha + chunk[2] as usize];
+            let alpha = chunk[3] as u16;
+            if alpha != 0 {
+                chunk[0] = ((chunk[0] as u16 * 255 + (alpha / 2)) / alpha) as u8;


After lots of Googling, I took this from https://bugzilla.mozilla.org/show_bug.cgi?id=659725#c31. Not sure what the best solution here is though.

michaelwu · 2016-03-12T16:01:51Z

@bors-servo: try

bors-servo · 2016-03-12T16:01:53Z

⌛ Trying commit 33f67b0 with merge 9c328d6...

@michaelwu

Don't use lookup tables for put_image_data and GetImageData Fixes #9599 "This is the first Rust code I have ever written" (and also my first time writing real browser code, it's been quite the learning experience). Some questions: > For really fast CPU results, use integer SIMD instructions to handle more than one pixel at a time. This was out of the scope of #9599, right? I started looking into doing that, but it seems to be a lot more work than the `E-easy` label would suggest. [`std::simd`](https://doc.rust-lang.org/1.0.0/std/simd/index.html) is marked as "unstable", is that a blocker? > 128 can be added before dividing to round more accurately. @michaelwu, what did you mean by that? Also, the #9599 is `Do not use lookup tables for {Get,Put}ImageData operations`, but we only use lookup tables for the `Put`, not the `Get`, right? Sorry for all the noobish questions.  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9911)

bors-servo · 2016-03-12T16:42:41Z

💔 Test failed - mac-rel-wpt

bors-servo · 2016-03-25T11:17:51Z

💔 Test failed - linux-rel

magni- · 2016-03-25T11:32:37Z

Hmm, three timeouts that I don't think have anything to do with my PR?

  ▶ TIMEOUT [expected OK] /websockets/opening-handshake/003-sets-origin.worker

  ▶ Unexpected subtest result in /websockets/opening-handshake/003-sets-origin.worker:
  │ TIMEOUT [expected PASS] W3C WebSocket API - origin set in a Worker
  └   → Test timed out

  ▶ TIMEOUT [expected FAIL] /css21_dev/html4/border-color-applies-to-013.htm
  │ 
  │ Shutting down the Constellation after generating an output file or exit flag specified
  │ thread 'Constellation' panicked at 'called `Result::unwrap()` on an `Err` value: IoError(Error { repr: Os { code: 104, message: "Connection reset by peer" } })', ../src/libcore/result.rs:746
  │ stack backtrace:
  │    1:     0x7f0edc6d1930 - sys::backtrace::tracing::imp::write::h495bc662e480d01f2cv
  │    2:     0x7f0edc6d678f - panicking::default_handler::_$u7b$$u7b$closure$u7d$$u7d$::closure.44522
  │    3:     0x7f0edc6d6408 - panicking::default_handler::h4e73712d1e927dfeH0z
  │    4:     0x7f0edc6c06cc - sys_common::unwind::begin_unwind_inner::h0e1be209f5e9e87dg2t
  │    5:     0x7f0edc6c1678 - sys_common::unwind::begin_unwind_fmt::h45dc2fac9b5e0d23m1t
  │    6:     0x7f0edc6d0c31 - rust_begin_unwind
  │    7:     0x7f0edc70e1af - panicking::panic_fmt::h40f5ec0cdc3fc429FRL
  │    8:     0x7f0edba1f84e - result::unwrap_failed::h18184822212557296049
  │    9:     0x7f0edba48999 - font_cache_thread::FontCacheThread::exit::h2850cfb8b5add96auMn
  │   10:     0x7f0edad396f5 - constellation::Constellation<LTF, STF>::handle_exit::h15804920269366392541
  │   11:     0x7f0edad16916 - constellation::Constellation<LTF, STF>::handle_request::h11164905505392349147
  │   12:     0x7f0edad0b2f7 - sys_common::unwind::try::try_fn::h1825336841370962927
  │   13:     0x7f0edc6d0bbb - __rust_try
  │   14:     0x7f0edc6d0b4d - sys_common::unwind::inner_try::he9b0b11c6b5b4e7fiZt
  │   15:     0x7f0edad0c1fa - boxed::F.FnBox<A>::call_box::h17027304325024401966
  │   16:     0x7f0edc6d4d59 - sys::thread::Thread::new::thread_start::hf1ffcc04a41608eb9Xy
  │   17:     0x7f0ed8968181 - start_thread
  │   18:     0x7f0ed847f47c - __clone
  └   19:                0x0 - <unknown>

jdm · 2016-03-25T22:32:31Z

Yes, those are known intermittent failures.

magni- · 2016-04-01T00:36:21Z

@bors-servo: try

@michaelwu

Do not use lookup tables for put_image_data Fixes #9599 "This is the first Rust code I have ever written" (and also my first time writing real browser code, it's been quite the learning experience). Some questions: > For really fast CPU results, use integer SIMD instructions to handle more than one pixel at a time. This was out of the scope of #9599, right? I started looking into doing that, but it seems to be a lot more work than the `E-easy` label would suggest. [`std::simd`](https://doc.rust-lang.org/1.0.0/std/simd/index.html) is marked as "unstable", is that a blocker? > 128 can be added before dividing to round more accurately. @michaelwu, what did you mean by that? Also, the #9599 is `Do not use lookup tables for {Get,Put}ImageData operations`, but we only use lookup tables for the `Put`, not the `Get`, right? Sorry for all the noobish questions.  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9911)

bors-servo · 2016-04-01T00:36:24Z

⌛ Trying commit 3055581 with merge cb2cb9a...

bors-servo · 2016-04-01T01:53:07Z

☀️ Test successful - android, arm32, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, status-appveyor

magni- · 2016-04-01T02:07:50Z

@michaelwu Despite the S-needs-rebase label, this PR is ready for 👀 now 😄

michaelwu · 2016-04-01T15:49:07Z

@bors-servo: r=michaelwu

bors-servo · 2016-04-01T15:49:08Z

📌 Commit 3055581 has been approved by michaelwu

bors-servo · 2016-04-01T15:49:10Z

⌛ Testing commit 3055581 with merge 983956a...

@michaelwu

Do not use lookup tables for put_image_data Fixes #9599 "This is the first Rust code I have ever written" (and also my first time writing real browser code, it's been quite the learning experience). Some questions: > For really fast CPU results, use integer SIMD instructions to handle more than one pixel at a time. This was out of the scope of #9599, right? I started looking into doing that, but it seems to be a lot more work than the `E-easy` label would suggest. [`std::simd`](https://doc.rust-lang.org/1.0.0/std/simd/index.html) is marked as "unstable", is that a blocker? > 128 can be added before dividing to round more accurately. @michaelwu, what did you mean by that? Also, the #9599 is `Do not use lookup tables for {Get,Put}ImageData operations`, but we only use lookup tables for the `Put`, not the `Get`, right? Sorry for all the noobish questions.  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9911)

bors-servo · 2016-04-01T16:42:10Z

☀️ Test successful - android, arm32, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, status-appveyor

bors-servo · 2016-04-01T16:42:12Z

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

jdm · 2016-04-01T18:04:45Z

@bors-servo: retry

Intermittent "Test was successful, but fast-forwarding failed: 422 Update is not a fast forward" homu#24

bors-servo · 2016-04-01T18:27:20Z

⌛ Testing commit 3055581 with merge 4e4a213...

@michaelwu

Do not use lookup tables for put_image_data Fixes #9599 "This is the first Rust code I have ever written" (and also my first time writing real browser code, it's been quite the learning experience). Some questions: > For really fast CPU results, use integer SIMD instructions to handle more than one pixel at a time. This was out of the scope of #9599, right? I started looking into doing that, but it seems to be a lot more work than the `E-easy` label would suggest. [`std::simd`](https://doc.rust-lang.org/1.0.0/std/simd/index.html) is marked as "unstable", is that a blocker? > 128 can be added before dividing to round more accurately. @michaelwu, what did you mean by that? Also, the #9599 is `Do not use lookup tables for {Get,Put}ImageData operations`, but we only use lookup tables for the `Put`, not the `Get`, right? Sorry for all the noobish questions.  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9911)

bors-servo · 2016-04-01T19:34:30Z

☀️ Test successful - android, arm32, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, status-appveyor

highfive assigned wafflespeanut Mar 8, 2016

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

magni- changed the title ~~Dont use lookup tables for put_image_data~~ Don't use lookup tables for put_image_data Mar 8, 2016

highfive assigned michaelwu and unassigned wafflespeanut Mar 8, 2016

magni- force-pushed the dont_use_lookup_tables branch from 5abc32f to 368f532 Compare March 9, 2016 06:36

magni- reviewed Mar 9, 2016
View reviewed changes

magni- changed the title ~~Don't use lookup tables for put_image_data~~ Don't use lookup tables for put_image_data and GetImageData Mar 9, 2016

magni- force-pushed the dont_use_lookup_tables branch from 368f532 to b1e087f Compare March 9, 2016 08:29

highfive added the S-tests-failed The changes caused existing tests to fail. label Mar 9, 2016

magni- force-pushed the dont_use_lookup_tables branch from b1e087f to 33f67b0 Compare March 10, 2016 06:58

highfive removed the S-tests-failed The changes caused existing tests to fail. label Mar 10, 2016

magni- reviewed Mar 10, 2016
View reviewed changes

highfive added the S-tests-failed The changes caused existing tests to fail. label Mar 12, 2016

highfive added the S-tests-failed The changes caused existing tests to fail. label Mar 25, 2016

magni- mentioned this pull request Mar 25, 2016

Add S-needs-rebase label when homu posts "Merge conflict" comment servo/highfive#24

Open

magni- added 3 commits March 31, 2016 14:20

Don't use premultiply table for put_image_data

1e8e5d6

Clean up spacing

86dcef1

Skip failing test

3055581

magni- force-pushed the dont_use_lookup_tables branch from faccaf8 to 3055581 Compare March 31, 2016 05:20

highfive removed the S-tests-failed The changes caused existing tests to fail. label Mar 31, 2016

jdm removed the S-needs-rebase There are merge conflict errors. label Apr 1, 2016

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

jdm mentioned this pull request Apr 1, 2016

Intermittent "Test was successful, but fast-forwarding failed: 422 Update is not a fast forward" servo/homu#24

Open

bors-servo merged commit 3055581 into servo:master Apr 1, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not use lookup tables for put_image_data #9911

Do not use lookup tables for put_image_data #9911

magni- commented Mar 8, 2016

wafflespeanut commented Mar 8, 2016

michaelwu commented Mar 8, 2016

jdm commented Mar 8, 2016

michaelwu commented Mar 8, 2016

magni- commented Mar 9, 2016

magni- Mar 9, 2016

michaelwu Mar 9, 2016

jdm commented Mar 9, 2016

bors-servo commented Mar 9, 2016

bors-servo commented Mar 9, 2016

jdm commented Mar 9, 2016

magni- Mar 10, 2016

michaelwu commented Mar 12, 2016

bors-servo commented Mar 12, 2016

bors-servo commented Mar 12, 2016

bors-servo commented Mar 25, 2016

magni- commented Mar 25, 2016

jdm commented Mar 25, 2016

magni- commented Apr 1, 2016

bors-servo commented Apr 1, 2016

bors-servo commented Apr 1, 2016

magni- commented Apr 1, 2016

michaelwu commented Apr 1, 2016

bors-servo commented Apr 1, 2016

bors-servo commented Apr 1, 2016

bors-servo commented Apr 1, 2016

bors-servo commented Apr 1, 2016

jdm commented Apr 1, 2016

bors-servo commented Apr 1, 2016

bors-servo commented Apr 1, 2016

Do not use lookup tables for put_image_data #9911

Do not use lookup tables for put_image_data #9911

Conversation

magni- commented Mar 8, 2016

wafflespeanut commented Mar 8, 2016

michaelwu commented Mar 8, 2016

jdm commented Mar 8, 2016

michaelwu commented Mar 8, 2016

magni- commented Mar 9, 2016

magni- Mar 9, 2016

Choose a reason for hiding this comment

michaelwu Mar 9, 2016

Choose a reason for hiding this comment

jdm commented Mar 9, 2016

bors-servo commented Mar 9, 2016

bors-servo commented Mar 9, 2016

jdm commented Mar 9, 2016

magni- Mar 10, 2016

Choose a reason for hiding this comment

michaelwu commented Mar 12, 2016

bors-servo commented Mar 12, 2016

bors-servo commented Mar 12, 2016

bors-servo commented Mar 25, 2016

magni- commented Mar 25, 2016

jdm commented Mar 25, 2016

magni- commented Apr 1, 2016

bors-servo commented Apr 1, 2016

bors-servo commented Apr 1, 2016

magni- commented Apr 1, 2016

michaelwu commented Apr 1, 2016

bors-servo commented Apr 1, 2016

bors-servo commented Apr 1, 2016

bors-servo commented Apr 1, 2016

bors-servo commented Apr 1, 2016

jdm commented Apr 1, 2016

bors-servo commented Apr 1, 2016

bors-servo commented Apr 1, 2016