Skip to content

Conversation

@demo99
Copy link
Contributor

@demo99 demo99 commented Nov 9, 2017

The result of hue-rotate and saturate was wrong. I also create color matrix in vertex shader for those matrix type filters.

After the fix, the gecko looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca4753a3913013cdc80bd5cbba81f2213ce9ae51&selectedJob=143243269


This change is Reviewable

@glennw
Copy link
Member

glennw commented Nov 9, 2017

Thanks for the PR!

There seem to be several unexpected fails in filter tests in that try run that look related though?

Also, do the tests check for correctly working with pre-multiplied alpha? We should make sure we have some coverage for those too if not already.

@demo99
Copy link
Contributor Author

demo99 commented Nov 9, 2017

Oh, I think there is a bug after I move color matrix to vertex shader. I will fix it and add premultiplied alpha test as well.

@glennw
Copy link
Member

glennw commented Nov 9, 2017

Looks good! Do we have / need a gecko try run for the updated PR?

@demo99
Copy link
Contributor Author

demo99 commented Nov 10, 2017

The try result is same:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=191652942d7af413180593f77738a71239d7d98d&selectedJob=143539747

There are 5 unexpected fails. The max difference is only 1~2. So I will use fuzz for those tests.

@glennw
Copy link
Member

glennw commented Nov 10, 2017

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit f2909b4 has been approved by glennw

@bors-servo
Copy link
Contributor

⌛ Testing commit f2909b4 with merge 66ddb1f...

bors-servo pushed a commit that referenced this pull request Nov 10, 2017
Fix hue-rotate and saturate filter

The result of hue-rotate and saturate was wrong. I also create color matrix in vertex shader for those matrix type filters.

After the fix, the gecko looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca4753a3913013cdc80bd5cbba81f2213ce9ae51&selectedJob=143243269

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2017)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: glennw
Pushing 66ddb1f to master...

@bors-servo bors-servo merged commit f2909b4 into servo:master Nov 10, 2017
@staktrace
Copy link
Contributor

@demo99 This seems to have caused shader compilation failures on windows. See https://bugzilla.mozilla.org/show_bug.cgi?id=1415150#c7 - the try push that revealed it is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffb402d6fefebf42d40c88889b97a86340fda4fe

@mstange
Copy link
Contributor

mstange commented Nov 10, 2017

There's a variable with the name c at

case 3:
// HueRotate
float c = cos(vAmount * 0.01745329251);
float s = sin(vAmount * 0.01745329251);

Maybe variable declarations aren't allowed within switch case branches without a nested scope, similar to C++?

@kvark
Copy link
Member

kvark commented Nov 10, 2017

@mstange that's a good candidate! Filed #2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants