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.

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

Fix srgb/linear conversion filters. #3240

Merged
merged 1 commit into from Oct 30, 2018

Conversation

Projects
None yet
5 participants
@nical
Collaborator

nical commented Oct 29, 2018

I made a very silly mistake in the srgb/linear conversion filters of swapping c1 and c2 (see the patch). The mistake was symetrical so it passed the tests, but the filter ended up merely quantifying the input color space into 256/13 values which isn't very useful and makes most color matrix filter applied in linear space totally wrong (as soon as the resulting colors fall out of the 0..256/13 range).

So here is the fix. I was going to introduce the if_then_else macro in another thing I'm working on but it's a good fit here because the argument positions are less prone to the error I made.

r? anyone


This change is Reviewable

@emilio

emilio approved these changes Oct 29, 2018

Is there any chance of adding a test for this?

@nical

This comment has been minimized.

Collaborator

nical commented Oct 29, 2018

Some gecko reftests will cover this as soon as I enable using SVG color matrix filters in WR. We can still add a reftest that tests the exact values of the srg-to-linear conversion in the webrender repo but I'm not too worried about that.

@emilio

This comment has been minimized.

Member

emilio commented Oct 29, 2018

Ok, wfm.

@bors-servo r+

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 29, 2018

📌 Commit b7ee2c6 has been approved by emilio

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 29, 2018

⌛️ Testing commit b7ee2c6 with merge 983c78a...

bors-servo added a commit that referenced this pull request Oct 29, 2018

Auto merge of #3240 - nical:srgb-linear-fix, r=emilio
Fix srgb/linear conversion filters.

I made a *very* silly mistake in the srgb/linear conversion filters of swapping c1 and c2 (see the patch). The mistake was symetrical so it passed the tests, but the filter ended up merely quantifying the input color space into 256/13 values which isn't very useful and makes most color matrix filter applied in linear space totally wrong (as soon as the resulting colors fall out of the 0..256/13 range).

So here is the fix. I was going to introduce the `if_then_else` macro in another thing I'm working on but it's a good fit here because the argument positions are less prone to the error I made.

r? anyone

<!-- 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/3240)
<!-- Reviewable:end -->
@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 30, 2018

@bors-servo retry

@gw3583 gw3583 closed this Oct 30, 2018

@gw3583 gw3583 reopened this Oct 30, 2018

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 30, 2018

@bors-servo r=emilio

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 30, 2018

📌 Commit b49a8c6 has been approved by emilio

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 30, 2018

⌛️ Testing commit b49a8c6 with merge ad6cec7...

bors-servo added a commit that referenced this pull request Oct 30, 2018

Auto merge of #3240 - nical:srgb-linear-fix, r=emilio
Fix srgb/linear conversion filters.

I made a *very* silly mistake in the srgb/linear conversion filters of swapping c1 and c2 (see the patch). The mistake was symetrical so it passed the tests, but the filter ended up merely quantifying the input color space into 256/13 values which isn't very useful and makes most color matrix filter applied in linear space totally wrong (as soon as the resulting colors fall out of the 0..256/13 range).

So here is the fix. I was going to introduce the `if_then_else` macro in another thing I'm working on but it's a good fit here because the argument positions are less prone to the error I made.

r? anyone

<!-- 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/3240)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 30, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: emilio
Pushing ad6cec7 to master...

@bors-servo bors-servo merged commit b49a8c6 into servo:master Oct 30, 2018

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@nical nical deleted the nical:srgb-linear-fix branch Oct 30, 2018

///
/// Note: Some older android devices don't support mix with bvec. If we ever run into them
/// the only option we have is to polyfill it with a branch per component.
#define if_then_else(cond, then_branch, else_branch) mix(else_branch, then_branch, cond)

This comment has been minimized.

@kvark

kvark Oct 30, 2018

Member

d'oh, why don't we just use step instead of lessThanEqual so that bvec isn't involved?
I also don't think we should be having the one-line wrappers around GLSL functions, fwiw

This comment has been minimized.

@nical

nical Oct 31, 2018

Collaborator

I wasn't aware that one can use step with bvec. It's not clear from the glsl docs that you could do that whereas it is explicitly specified for mix.
We do want component-wise selection here so bvec is needed.

I stand by wrapping the single line function because the function will most likely be used all over the place to avoid timing attacks, using mix directly is error prone (the bug this PR is fixing) and this might need to be polyfilled if we support low end android devices.

This comment has been minimized.

@kvark

kvark Oct 31, 2018

Member

What I'm saying is: we don't need bvec at all. We can just use mix and step without triggering the bugtrap GLSL booleans are.

This comment has been minimized.

@nical

nical Oct 31, 2018

Collaborator

Not sure I follow, you want to do it manually for each individual channel?

This comment has been minimized.

@kvark

kvark Nov 1, 2018

Member

I'm saying that instead of doing this:

mix(c1, c2, lessThanEqual(color, vec3(0.04045)));

we could be doing this:

mix(c1, c2, step(color, vec3(0.04045)));

And it would not need any wrappers or messing around bvec

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