Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add (un)premultiply tables for canvas {Get,Put}ImageData operations #8086

Merged
merged 2 commits into from
Oct 22, 2015
Merged

add (un)premultiply tables for canvas {Get,Put}ImageData operations #8086

merged 2 commits into from
Oct 22, 2015

Conversation

froydnj
Copy link
Contributor

@froydnj froydnj commented Oct 19, 2015

Pretty straightforward use of lookup tables to replace a bunch of expensive float operations.

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 19, 2015
@jdm
Copy link
Member

jdm commented Oct 19, 2015

Let's move it out of script/dom/ into dom/; the ideal is that the only files in dom/ are implementing specific web APIs. Otherwise this looks fine!

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 19, 2015
@jdm
Copy link
Member

jdm commented Oct 19, 2015

@froydnj Also, are we sure that the table doesn't get inlined? pub static would be guaranteed to avoid that.

@froydnj
Copy link
Contributor Author

froydnj commented Oct 19, 2015

Let's move it out of script/dom/ into dom/; the ideal is that the only files in dom/ are implementing specific web APIs.

Which dom/ directory are you talking about here? The only dom/ directory I see is components/script/dom/, so I am a little confused where else I should put the new file.

@jdm
Copy link
Member

jdm commented Oct 19, 2015

Erk, I meant out of script/dom/ and into script/.

@froydnj froydnj force-pushed the premultiply-tables branch from 9111261 to d6851cb Compare October 20, 2015 00:52
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Oct 20, 2015
@froydnj
Copy link
Contributor Author

froydnj commented Oct 20, 2015

The pub static thing is a good point; changed. Too much C++, not enough Rust.

Maybe there should be a lint check or a warning for const non-scalar objects?

@jdm jdm mentioned this pull request Oct 20, 2015
Closed
@jdm
Copy link
Member

jdm commented Oct 20, 2015

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit d6851cb has been approved by jdm

@bors-servo
Copy link
Contributor

⌛ Testing commit d6851cb with merge b877cb5...

@highfive 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 Oct 20, 2015
bors-servo pushed a commit that referenced this pull request Oct 20, 2015
Auto merge of #8086 - froydnj:premultiply-tables, r=jdm
b877cb5 
add (un)premultiply tables for canvas {Get,Put}ImageData operations

Pretty straightforward use of lookup tables to replace a bunch of expensive float operations.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8086)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 20, 2015
@froydnj
Copy link
Contributor Author

froydnj commented Oct 20, 2015

OK, so the previous push had green tests, but this push (which essentially did s/const/static/) is broken. Looking at the failing tests, it does look like they might be affected by this change, but I am also confused why the tests were green prior. Did a different set of tests get run, or are the failing tests known intermittents?

@jdm
Copy link
Member

jdm commented Oct 20, 2015

Oh, travis only builds Servo; it doesn't run any tests.

@jdm
Copy link
Member

jdm commented Oct 20, 2015

Also note that the reported tests are PASS expected FAIL, i.e. the result was known to be wrong previously, and these changes have corrected the underlying problem. Please update the appropriate ini files :)

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Oct 20, 2015
@froydnj
Copy link
Contributor Author

froydnj commented Oct 20, 2015

\o/ for optimizations and correctness!

@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 20, 2015

How did we fail them before? Rounding issues?

@froydnj
Copy link
Contributor Author

froydnj commented Oct 20, 2015

How did we fail them before? Rounding issues?

Not sure, #5030 doesn't seem to explain why they were XFAIL'd. putImageData(getImageData()) seems likely to suffer from rounding issues; putImageData on non-solid data...maybe there were some rounding issues there too?

@nox
Copy link
Contributor

nox commented Oct 21, 2015

@froydnj Could you squash the two last commits together before someone r+ it? I don't like commits which aren't with their expectation changes.

@nox nox added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 21, 2015
@froydnj froydnj force-pushed the premultiply-tables branch from cde8858 to 9e348e2 Compare October 21, 2015 22:31
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 21, 2015
@froydnj
Copy link
Contributor Author

froydnj commented Oct 21, 2015

@froydnj Could you squash the two last commits together before someone r+ it? I don't like commits which aren't with their expectation changes.

Sure.

@nox nox removed the S-needs-squash Some (or all) of the commits in the PR should be combined. label Oct 21, 2015
@nox
Copy link
Contributor

nox commented Oct 21, 2015

@bors-servo r=jdm

@bors-servo
Copy link
Contributor

📌 Commit 9e348e2 has been approved by jdm

@highfive 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 Oct 21, 2015
@nox nox assigned jdm Oct 21, 2015
@bors-servo
Copy link
Contributor

⌛ Testing commit 9e348e2 with merge 9a34dec...

bors-servo pushed a commit that referenced this pull request Oct 21, 2015
Auto merge of #8086 - froydnj:premultiply-tables, r=jdm
9a34dec 
add (un)premultiply tables for canvas {Get,Put}ImageData operations

Pretty straightforward use of lookup tables to replace a bunch of expensive float operations.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8086)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

@bors-servo bors-servo merged commit 9e348e2 into servo:master Oct 22, 2015
@michaelwu michaelwu mentioned this pull request Feb 11, 2016
Closed
@magni- magni- mentioned this pull request Mar 8, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jdm jdm

Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@froydnj @jdm @bors-servo @Ms2ger @nox @highfive