added ordered dithering to gradient-like effects #966
Conversation
|
Thanks for looking into this issue! I'm a bit concerned about the extra shader costs of the approach, since it seems to introduce quite a few more instructions and also an extra texture fetch (although that should be in cache almost all the time). We're not currently using sRGB or any of the usual techniques to reduce banding. It seems like that may be a better approach to fixing this - thoughts @squarewave and @kvark ? I'm hoping to land some benchmark / profiling support in wrench in the next couple of days, which would allow us to quantify if this has any noticeable impact on framerates. |
|
While I'm on board with using sRGB, I'm not familiar with how it would reduce this kind of banding. At a certain point a monitor can only represent so many steps between two colors regardless of color space, and they'll be far enough apart from each other that the human eye will pick up on it. Dithering is the only way I know of to compensate for this, but I'm by no means an expert. Quantifying would be great! I definitely felt uncertain while writing this what kind of impact it would have. However, I do know that Chrome is currently using an ordered dither on Windows for this purpose. (Tested by just screen-shotting a gentle gradient in Chrome and zooming in.) |
There was a problem hiding this comment.
@glennw good point about sRGB!
A gradient is linearly interpolated, so we'll need to convert it to sRGB space in order for the linearity to show properly on the screen. Currently we don't touch GL_FRAMEBUFFER_SRGB, and it's false by default. I think Gecko folks would want to use a real gamma conversion table (instead of GL's built-in), so we'd need this mode supported as well. (Which, btw, may be conveniently combined with the post-fx pass of #957). cc @nical @jrmuizel
As for this PR, I think it's good to go. There is a few nits spotted, but I'm not too worried about the implications of adding it:
- complexity wise, having the dither ability is going to be handy anyway
- performance wise, it only affects the blur, box shadows, and dither shaders
webrender/res/prim_shared.glsl
Outdated
|
|
||
| vec4 dither(vec4 color) { | ||
| const float matrix_size = 8; | ||
| // TODO: should we mask it here or set the texture's wrapping to GL_REPEAT? |
There was a problem hiding this comment.
I don't think we need a texture sample at all. We could just texelFetch instead
webrender/res/prim_shared.glsl
Outdated
| // TODO: should we mask it here or set the texture's wrapping to GL_REPEAT? | ||
| const int matrix_mask = 7; | ||
|
|
||
| vec2 pos = vec2(int(gl_FragCoord.x) & matrix_mask, int(gl_FragCoord.y) & matrix_mask); |
There was a problem hiding this comment.
nit: could just do ivec2(gl_FragCoord.xy) & ivec2(matrix_mask)
|
One thing that remains, which I just finished diagnosing. For angle and radial gradients, we pre-build a texture for the gradient like so:
fn fill_colors(&mut self, start_idx: usize, end_idx: usize, start_color: &ColorF, end_color: &ColorF) -> usize {
if start_idx >= end_idx {
return start_idx;
}
// Calculate the color difference for individual steps in the ramp.
let inv_steps = 1.0 / (end_idx - start_idx) as f32;
let step_r = (end_color.r - start_color.r) * inv_steps;
let step_g = (end_color.g - start_color.g) * inv_steps;
let step_b = (end_color.b - start_color.b) * inv_steps;
let step_a = (end_color.a - start_color.a) * inv_steps;
let mut cur_color = *start_color;
let mut cur_packed_color = PackedTexel::from_color(&cur_color);
// Walk the ramp writing start and end colors for each entry.
for entry in &mut self.colors[start_idx..end_idx] {
entry.start_color = cur_packed_color;
cur_color.r += step_r;
cur_color.g += step_g;
cur_color.b += step_b;
cur_color.a += step_a;
cur_packed_color = PackedTexel::from_color(&cur_color);
entry.end_color = cur_packed_color;
}
end_idx
}Filling the texture out with this data causes it to be prematurely quantized, meaning our dithering doesn't really do too much. I feel like there's a number of ways we could tackle this, but I just wanted to see if anything was already in the works to use a different mechanism for angle and radial gradients. |
|
@squarewave Nicely spotted! Alternatively, we could provide 2 textures: one carrying just the segment index, and another actually providing the segment data. The pixel shader would then read the relevant segment and interpolate with code between the start/end colors. |
|
@squarewave I added a benchmark module here - #969. Perhaps we could add a couple of gradient related benchmarks? Maybe a full screen linear gradient and a full screen angle gradient? |
|
@kvark - good ideas! I went with increasing the bpp for now since it's much simpler to do, but I do wonder if this solution is viable on Android and other architectures which I'm not very set up to test. Thoughts? Also, here are some results of the benchmarks (seems to be an impact - not sure how to assess how major it is) Before: [
{
"name": "benchmarks\\gradients.yaml",
"composite_time_ns": 460775,
"paint_time_ns": 308330,
"draw_calls": 3
},
{
"name": "benchmarks\\gradients.yaml",
"composite_time_ns": 542940,
"paint_time_ns": 307200,
"draw_calls": 3
},
{
"name": "benchmarks\\gradients.yaml",
"composite_time_ns": 446595,
"paint_time_ns": 307322,
"draw_calls": 3
},
]After: [
{
"name": "benchmarks\\gradients.yaml",
"composite_time_ns": 589848,
"paint_time_ns": 435536,
"draw_calls": 3
},
{
"name": "benchmarks\\gradients.yaml",
"composite_time_ns": 590947,
"paint_time_ns": 435269,
"draw_calls": 3
},
{
"name": "benchmarks\\gradients.yaml",
"composite_time_ns": 588059,
"paint_time_ns": 435696,
"draw_calls": 3
},
] |
|
@squarewave I don't think |
|
Done. Also, I split out the performance benchmarks for the two cases. Before (no dithering - BGRA8 texture): {
"tests": [
{
"name": "benchmarks\\aligned-gradient.yaml",
"composite_time_ns": 704614,
"paint_time_ns": 1049088,
"draw_calls": 2
},
{
"name": "benchmarks\\unaligned-gradient.yaml",
"composite_time_ns": 975543,
"paint_time_ns": 988997,
"draw_calls": 2
},
{
"name": "benchmarks\\simple-batching.yaml",
"composite_time_ns": 308162,
"paint_time_ns": 406149,
"draw_calls": 1
},
{
"name": "benchmarks\\large-clip-rect.yaml",
"composite_time_ns": 1028642,
"paint_time_ns": 618181,
"draw_calls": 10
}
]
}
After (dithering - RGBA16 texture): {
"tests": [
{
"name": "benchmarks\\aligned-gradient.yaml",
"composite_time_ns": 732871,
"paint_time_ns": 1239525,
"draw_calls": 2
},
{
"name": "benchmarks\\unaligned-gradient.yaml",
"composite_time_ns": 1453478,
"paint_time_ns": 1243402,
"draw_calls": 2
},
{
"name": "benchmarks\\simple-batching.yaml",
"composite_time_ns": 501355,
"paint_time_ns": 500986,
"draw_calls": 1
},
{
"name": "benchmarks\\large-clip-rect.yaml",
"composite_time_ns": 999048,
"paint_time_ns": 617669,
"draw_calls": 10
}
]
} |
|
(One last note, the benchmarks are fairly noisy - I don't know if that's just my machine or if it's just the nature of the domain. Just thought I'd mention it.) |
|
Code looks great! |
|
From what I can tell it's not supported: http://docs.gl/es3/glTexImage2D Should I just fall back to RGBA8 on these? |
|
☔ The latest upstream changes (presumably #965) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@squarewave it doesn't support the normalized version, but there is |
|
So, unfortunately linear texture filtering doesn't seem to work on non-normalized integer formats. The only mention I could find on this was this thread, but I confirmed it by simply increasing GRADIENT_DATA_RESOLUTION until the banding I was seeing went away. We could manually interpolate in the shader, but I don't have a good intuition as far as how performant that would be. Might be time to go to the dual-texture approach. Thoughts? |
|
@squarewave argh, you are right. I forgot that you need interpolation... |
squarewave
force-pushed
the
master
branch
2 times, most recently
from
5948ae4
to
19aa850
Compare
|
@squarewave Thanks for the update! Are you able to run this against the Servo WPT/CSS test suite locally to make sure there's no regressions there? If that's difficult / not possible - just let me know and I'll pull this branch locally sometime today and run the test suite here. |
|
@glennw barring unexpected things I should be able to do that tonight :) |
|
☔ The latest upstream changes (presumably #984) made this pull request unmergeable. Please resolve the merge conflicts. |
Not sure how to interpret these results. I loaded several of the tests up with |
|
Those are probably intermittents unrelated to your change, I think. Did you run them in the release builder (the |
|
So, coincidentally I ran the wpt tests on my Macbook so I could do other things while they ran, and two gradient tests failed. I then ran the wrench tests on my Macbook and they don't display. Something about my Macbook doesn't seem to like the RGABU16 texture, and it just silently fails and reads 0,0,0,65535 (all black with full alpha), which is somewhat perplexing. Any thoughts? The machine it works on is using an NVIDIA GTX 960, and the Macbook is running an AMD R9 M370DX. Is there anywhere that vendors document what is and isn't supported by their drivers or is it really the total chaos that it feels like? |
|
@squarewave could you capture it with |
|
@squarewave thank you! I haven't found anything obviously wrong with either the code or the state in the capture. Do you mind sharing the repro steps? I have a mac at home, so I can check it out locally on the weekend. |
|
Sure - I've just been doing this: cd /path/to/wrench
cargo run show benchmarks/unaligned-gradient.yamlYou should then see a big black rectangle in the top left of the window. |
|
@squarewave I ran it on Mac mini 2010. No black rectangle. |
added ordered dithering to gradient-like effects Closes #929 It's a bit rough around the edges, so any criticism is completely welcome - just hoping to help out! <!-- 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/966) <!-- Reviewable:end -->
|
☀️ Test successful - status-travis |
|
Unfortunately this seems to have broken the Would you be able to take a look and see what's causing the differences? I'll can hold off on updating WR in Servo for a bit, or we can temporarily revert this if it's going to take a while to sort out. |
|
Also FYI this seems to have caused some reftest failures in Gecko, see https://bugzilla.mozilla.org/show_bug.cgi?id=1349692#c5 and subsequent comments. The reftest failures are caused by a few pixels having a slightly different value (visually I can't tell the difference) so we can just fuzz the difference in the reftest harness. However it might be indicative of a deeper problem, so just a heads-up for that. |
|
@glennw @staktrace the PR does change the way of rendering gradients, so perhaps the image refs just need to be updated, given the new results are of better quality. I'm yet to look into the exact failures.... |
|
@staktrace This is my first patch in this area, so I don't know a lot about the reftests, but what units is "max difference" in? I see one of the tests failed with a max difference of 1, and the other failed with a max difference of 14. I would expect the maximum difference between a pixel with dithering and a pixel without dithering to be 1/256 (or 1 if not normalized) for each channel, so 14 seems to be above that. Otherwise, visually imperceptible differences are expected. |
|
The "max difference" is the maximum difference in one of the (r,g,b) components. So if a pixel in one was rgb(1,1,1) and the corresponding pixel in the other was rgb(2,3,15) that would be a max difference of 14. |
|
Also, with the reftests, if you click on the "bar chart" icon it will take you to the reftest analyzer where you can examine the differences in more detail. Here is one example: link |
|
I'm a bit confused by what I'm seeing here. I would expect that on release, boxshadow-inset-large-offset-ref.html and boxshadow-inset-large-offset.html would be identical, but they're not. There's a visible difference on the left side of the div where the box shadow is fading out where it shouldn't be. |
|
One thing that we can easily do in order to reduce the difference is to handle the case specifically when a gradient source = destination. If you look at the "linear_gradients_parsing_a.html", the rectangles there are just supposed to be filled with solid colors, so dithering doesn't give us much. |
|
One thing to note about boxshadow-inset-large-offset is that it's already marked fuzzy with a max difference of 13 (and with 13612 pixels allowed to be fuzzy when webrender is enabled). So actually your patch just increased the fuzz difference from 13 to 14. Sorry for not checking that earlier. |
|
Ah okay - then yeah both of these look good to me. @kvark, if we're seeing any visual differences in a gradient where source == destination, then I think that's an indicator of a bug, since our dithering should only be able to push non-integer values up or down. If that's what you're seeing, I think the bug is in my dither function: vec4 dither(vec4 color) {
const int matrix_mask = 7;
ivec2 pos = ivec2(gl_FragCoord.xy) & ivec2(matrix_mask);
float noise_factor = 4.0 / 255.0;
float noise = texelFetch(sDither, pos, 0).r * noise_factor; // <---
return color + vec4(noise, noise, noise, 0);
}I suspect it should be float noise = (texelFetch(sDither, pos, 0).r - 0.5) * noise_factor;so that we're pushing the value up or down by a value between 0 and 0.5, rather than pushing it up by a value between 0 and 1. I think my thought process was that it would be flooring the result in order to quantize it rather than rounding. |
|
@squarewave oh, nice find! Hope that fixes everything :) looking forward to see your PR |
|
There's also a number of WPT reftests that are new failures - I suspect they are the same issue as you've mentioned above - they are comparing gradients with flat colors to a solid color rectangle. Once we get the fix above in a PR, I'll run those WPT tests locally and make sure they become passes too :) |
|
Quick update on this: last night I was digging pretty deep for little imperfections in this, and I managed to clear up all the things I saw, but I can't figure out why some of my solutions work. I can submit a PR with the changes and some comments documenting my confusion if you like. Highlights:
|
|
@squarewave we've got to dig it up and figure out why the expectations are wrong. I bet something with the way colors are generated might be off. If the shader sampling is misbehaving, we might test it by forcing the colors to be black and white and measuring the result from a snapshot. I'm going to have another look at the code later today. |
|
@kvark yeah I did quite a few experiments to try to coax the underlying problems out, so any help you can provide would be wonderful. To avoid duplicating efforts, one thing I did notice is that there was a bug in my fn extract_bytes(color: &ColorF, shift_by: i32) -> PackedTexel {
PackedTexel {
b: ((0.5 + color.b * COLOR_FLOAT_TO_FIXED_WIDE).floor() as u32 >> shift_by & 0xff) as u8,
g: ((0.5 + color.g * COLOR_FLOAT_TO_FIXED_WIDE).floor() as u32 >> shift_by & 0xff) as u8,
r: ((0.5 + color.r * COLOR_FLOAT_TO_FIXED_WIDE).floor() as u32 >> shift_by & 0xff) as u8,
a: ((0.5 + color.a * COLOR_FLOAT_TO_FIXED_WIDE).floor() as u32 >> shift_by & 0xff) as u8,
}
}With |
…. r=jrmuizel The fuzziness was introduced by servo/webrender#966 and might be fixed in the future, see discussion starting at servo/webrender#966 (comment) MozReview-Commit-ID: AIx6FY8XAiK
|
@squarewave I've found 2 things so far.
edit - actual shader code: float normalization_factor = 255.0 / 64.0; // bring it into 0-1 range
float noise_factor = 0.5 / 256.0; // half-unit difference noise
float noise = texelFetch(sDither, pos, 0).r * (normalization_factor * noise_factor); |
|
Eh, the noise factor is not that wrong, but there is a bit of fine logic missing. PR is incoming. |
Proper dither re-enabled Follow up to #966, reverts #1042 ~~WIP - still being tested~~ cc @squarewave r? @glennw <!-- 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/1045) <!-- Reviewable:end -->
Proper dither re-enabled Follow up to #966, reverts #1042 ~~WIP - still being tested~~ cc @squarewave r? @glennw <!-- 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/1045) <!-- Reviewable:end -->
…. r=jrmuizel The fuzziness was introduced by servo/webrender#966 and might be fixed in the future, see discussion starting at servo/webrender#966 (comment) MozReview-Commit-ID: AIx6FY8XAiK
…. r=jrmuizel The fuzziness was introduced by servo/webrender#966 and might be fixed in the future, see discussion starting at servo/webrender#966 (comment) MozReview-Commit-ID: AIx6FY8XAiK UltraBlame original commit: e0e0b41946f231db12a8d9584f7961022d96f8b9
…. r=jrmuizel The fuzziness was introduced by servo/webrender#966 and might be fixed in the future, see discussion starting at servo/webrender#966 (comment) MozReview-Commit-ID: AIx6FY8XAiK UltraBlame original commit: e0e0b41946f231db12a8d9584f7961022d96f8b9
…. r=jrmuizel The fuzziness was introduced by servo/webrender#966 and might be fixed in the future, see discussion starting at servo/webrender#966 (comment) MozReview-Commit-ID: AIx6FY8XAiK UltraBlame original commit: e0e0b41946f231db12a8d9584f7961022d96f8b9
Closes #929
It's a bit rough around the edges, so any criticism is completely welcome - just hoping to help out!
This change is