DrawTargetSkia misaligns repeated linear gradients

RESOLVED INVALID

Status

()

P3
normal
RESOLVED INVALID
2 years ago
6 months ago

People

(Reporter: mstange, Assigned: lsalzman)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox52 wontfix)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Created attachment 8809146 [details] [diff] [review]
failing gtest

I ran into this in bug 1315113: If a linear gradient is drawn with ExtendMode::Repeat, and it's longer than 256px, then the first pixel row is drawn with the color of the opposite end of the gradient.

E.g. if you have a gradient from red at the top to blue at the bottom, then drawing that gradient will paint one pixel row of blue at the top, and then red just under that blue row, and then the regular shading from red to blue towards the bottom.

I've written a simple standalone test which you can run by applying this patch and executing "mach gtest Moz2D.DrawTargetSkia".
> Test (GradientWithExtendRepeat): Verify Pixel (0x0) Failed. Expected (255,0,0,255)  Got (0,0,255,255)
> FAILED
Priority: -- → P3
Whiteboard: [gfx-noted]
(Assignee)

Comment 1

2 years ago
(In reply to Markus Stange [:mstange] from comment #0)
> Created attachment 8809146 [details] [diff] [review]
> failing gtest
> 
> I ran into this in bug 1315113: If a linear gradient is drawn with
> ExtendMode::Repeat, and it's longer than 256px, then the first pixel row is
> drawn with the color of the opposite end of the gradient.
> 
> E.g. if you have a gradient from red at the top to blue at the bottom, then
> drawing that gradient will paint one pixel row of blue at the top, and then
> red just under that blue row, and then the regular shading from red to blue
> towards the bottom.
> 
> I've written a simple standalone test which you can run by applying this
> patch and executing "mach gtest Moz2D.DrawTargetSkia".
> > Test (GradientWithExtendRepeat): Verify Pixel (0x0) Failed. Expected (255,0,0,255)  Got (0,0,255,255)
> > FAILED

This patch doesn't work because it is missing the requisite files, i.e. TestDrawTargetSkia.cpp/.h
Flags: needinfo?(mstange)
(Assignee)

Comment 2

2 years ago
Also, a simple standalone html testcase would probably be easier for me to debug here, and would give a better indicator of how this happens in the wild.
(Reporter)

Comment 3

2 years ago
Created attachment 8813319 [details] [diff] [review]
failing gtest

An HTML testcase is in attachment 8807353 [details] - at some zoom levels, there is a green line in the red part.
Attachment #8809146 - Attachment is obsolete: true
Flags: needinfo?(mstange)
(Assignee)

Comment 4

2 years ago
(In reply to Markus Stange [:mstange] from comment #3)
> Created attachment 8813319 [details] [diff] [review]
> failing gtest
> 
> An HTML testcase is in attachment 8807353 [details] - at some zoom levels,
> there is a green line in the red part.

So what is going on here is that this testcase is causing us to render the middle row of the gradient with different color stops than the pure red/green and using a repeat tile mode. Due to imprecision in the coordinate transforms with fixed point, we can end up sampling slightly across the repeat boundary within Skia's gradient lookup tables. The easiest workaround here that doesn't require rewriting Skia's gradients is to ensure we use clamped gradients wherever possible so that we don't accidentally wrap.
(Assignee)

Comment 5

2 years ago
Given the circumstances that there isn't an easy fix within Skia for this, Markus and I agreed we'll just retire this one as WONTFIX and address the CSS rendering issue in a different bug.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
(Assignee)

Comment 6

2 years ago
Created attachment 8813842 [details] [diff] [review]
fix clamping of vertical gradient lerp in Skia

This patch does not fix the html testcase, since the problem there is imprecision in the coordinate transforms that can't be resolved in this way.

However, this should partly address the issue turned up by the gtest when the coordinate imprecision is not an issue.

The problem occurs when the calculated table index sits right on the boundary between 255 and 0, i.e. the table index is precisely 0. Then when we offset this by half an index in preparation for the bilinear filtering that happens below, it underflows to the 255th index instead. There code nearby that clamps the neighboring sample to be <= 255, so we essentially get a hard-edge that is wrongly clamped to the 255th index.

This fixes that situation by doing the offset only after we've applied the repeat proc, and making sure we don't underflow from below. That way it will sample the 0th index with a hard edge like we expect.
Assignee: nobody → lsalzman
Status: RESOLVED → REOPENED
Attachment #8813842 - Flags: review?(mstange)
Resolution: WONTFIX → ---
(Reporter)

Comment 7

2 years ago
Comment on attachment 8813842 [details] [diff] [review]
fix clamping of vertical gradient lerp in Skia

I have a hard time evaluating whether this is worth doing given that we're going to have to work around it anyway. Deferring to Jeff.
Attachment #8813842 - Flags: review?(mstange) → review?(jmuizelaar)
(Assignee)

Comment 8

2 years ago
(In reply to Markus Stange [:mstange] from comment #7)
> Comment on attachment 8813842 [details] [diff] [review]
> fix clamping of vertical gradient lerp in Skia
> 
> I have a hard time evaluating whether this is worth doing given that we're
> going to have to work around it anyway. Deferring to Jeff.

I guess it should be noted, the reason I think it is worth making this change is because that half-index offsetting we do is a local patch to our version of Skia which upstream seemed unreceptive to integrating, so we're already maintaining it anyway. So no real harm in making it better.
(Reporter)

Comment 9

2 years ago
Jeff, here's the IRC log from when Lee explained the patch to me:

> https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/effects/gradients/SkLinearGradient.cpp?q=shadespan_linear_vertical_lerp&redirect_type=direct#192
> so line 192
> the original version was just: unsigned fullIndex = proc(SkGradFixedToFixed(fx));
> that is to say, it indexed the gradient lookup table below using coordinates reminiscent of nearest filtering
> but what is happening is that it's actually doing a bilinear filter there
> so the bilinear filtering was sampling half a pixel too far in advance
> which would cause gradients to be offset by half a pixel
> so we fixed that in our local version by subtracting off that half a pixel before sampling, like the normal bilinear filtering shaders do
> proc in this case is a small little function implementing the repeat tile mode
> static inline SkFixed repeat_tileproc(SkFixed x) {
>     return x & 0xFFFF;
> }
> if the input before we subtract the half-pixel is already wrong, because of the imprecise coord transforms, there is nothing you can do
> you get the weird lines popping up
> but
> where i noticed things can go wrong
> imagine fx is very close to 0
> so that it's value is less than half
> once you subtract off the half increment, it will end up outputting the 255 index from repeat_tileproc
> in combination with the clamping happening down on line 198
> it basically lerps between index 255 and index 255
> which is not the desired effect...
> in that case we would have wanted it to use index 0
> and clamp from below, not from above
> to get a hard edge at the repeat boundary
> so if the original index without offseting would have been 0, we want it to stay 0 instead of turning to 255
> or to show visually
> 255 | 0 | 1 | ... | 255 |
> so the input fx sits right at the line dividing 255 and 0
> we want that to fall into the 0 bucket
> but the offseting screws that up in combination with the clamping
Mass wontfix for bugs affecting firefox 52.
status-firefox52: affected → wontfix
(Assignee)

Comment 11

6 months ago
This particular code no longer exists in recent versions of Skia anymore. So I am going to mark this as invalid for now. We can file a new bug if something similar turns up in Skia's new code.
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago6 months ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.