1496540 - Fix layout/reftests/border-radius/corner-3.html on WebRender

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P3)

Description

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&bugfiler=&group_state=expanded&revision=4aee7c413aa978c5cd41f6065c654d55025b70b0&selectedJob=203453745

Comment 1

[Kartikaya Gupta (email:[e-mail])]

I think this failure warrants some investigation before we ride the trains.

Comment 2

[Jamie Nicol [:jnicol]]

The problem here is that the top-right corner overlaps with the top-left corner and the left edge. (And equivalent for the bottom-right corner.) In webrender we just blit the segments over eachother. Because the colour is translucent blue this causes overlapping portions to look more blue. And also the left-side's rectangular corners fill pixels that lie outside of the right-side's curve, hence the funny shape.

Note that in gecko we do not have this problem because, as the border is a single colour, we simply fill a path. However, if we make the different edges have different colours (e.g. "border-color: rgba(255, 0, 0, 0.5) rgba(0, 255, 0, 0.5) rgba(0, 0, 255, 0.5);") we do indeed see the same issue.

Glenn, Matt, any suggestions on how to fix this?

Comment 3

[Matt Woodrow (:mattwoodrow)]

I can't find anything particular in the specs about how this should be handled, but it looks like blink/WebKit matches non-WR behaviour here. Given that, and that we have specific tests for this, I do think we should fix this within WR.

gw is probably the right person to decide how to fix this. Maybe reduce the length of some of the edges such that they touch instead of overlap?

Comment 4

[Glenn Watson [:gw]]

I haven't taken a really detailed look into it - but reducing the sizes of the edges to enforce no overlap sounds like it'd probably be the best solution, if feasible.

Comment 5

[Jamie Nicol [:jnicol]]

In this example we can reduce the width of the top-right and bottom-right corners, so that they no longer overlap with the left corners or edge. That would fix the overdraw. But we still need something to shape the top-left and bottom-left corners to the curve of the top-right and bottom-right.

I don't think it's possible any other way: eg Making the top-left and bottom-left have width=0 would fix the shape. But you'd still get overdraw where the top-right corner overlaps with the left edge. If you then shrunk the left-edge you'd have a gap.

Comment 6

[Jamie Nicol [:jnicol]]

Attachment: Bug 1496540 - Handle overlapping border corners in webrender

When some of a border's corners have a border-radius, and that radius
is larger than the sum of the border width and element size, then it
results in the corners of the border overlapping. Webrender draws
borders by rasterizing each segment individually in to the cache, then
compositing them together. In this overlapping case, this has 2
problems:

a) we composite overlapping segments on top of eachother
b) corner segments are not correctly clipped to the curve of the
overlapping adjacent corners

This patch allows corner segments to be clipped by their adjacent
corners. We provide the outer corner position and radii of the
adjacent corners to the border shader, which then applies those clips,
if required, along with the segment's own corner clip when rasterizing
the segment.

As the adjacent corners now affect the result of the cached segment,
they are added to the cache key.

We continue to rasterize the entire segment in to the cache as before,
but now modify the local rect and texel rect of the BrushSegment so
that it only composites the subportion of the corner segment which
does not overlap with the opposite edges of the border.

Comment 7

[Jamie Nicol [:jnicol]]

Hi Glenn, sorry this took while to finish.

Requested changes made, and wrench reftest added.

I had to change the code which clamps the h/v_adjacent_corner_outer to the edge of the segment for non-overlapping rects. Instead of clamping to min_x or min_y, it has to be min_x - 1 or min_y - 1. (But for corners on the other side, max_x or max_y do not need +1.) This fixed a reftest, and I think makes sense: min_x is the first pixel of the rect, and we want to use the closest pixel which isn't actually inside the rect. max_x on the other hand is already the first pixel outside of the rect.

I had to fuzz the corner-3 reftest, the one this patch is enabling. See here: https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/bpqYdvO2SmKdFXBVa80TGA/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

The pixels on the outside of the curve were expected, but the two on the inside less so. They are the pixels at inside edge of the bottom-right and top-right corner curves, and they are being anti-aliased. I verified this happens without my patches on a more regular shaped border. In this case it just looks a bit different because it's on the inside of a shape. I assume that's okay, unless you have a suggestion on how to avoid that?

Comment 8

[Glenn Watson [:gw]]

Sounds good, thanks! I accepted the patch in differential.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f57d14800cbb
Handle overlapping border corners in webrender r=gw

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for wrench failures in border-with-rounded-clip.yaml and transforms/border-zoom.yaml:

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Cretry&group_state=expanded&selectedJob=224015736&revision=f57d14800cbb5cf9f7c135d7e32ea5728fe7b377
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=224027289&repo=autoland

Please ignore the first failure mentioned in the log, that one will soon be fixed by a merge. Second and third failure are:
REFTEST TEST-UNEXPECTED-FAIL | reftests/clip/border-with-rounded-clip.yaml == reftests/clip/border-with-rounded-clip.png | image comparison, max difference: 255, number of differing pixels: 16104
REFTEST TEST-UNEXPECTED-FAIL | reftests/transforms/border-zoom.yaml == reftests/transforms/border-zoom.png | image comparison, max difference: 255, number of differing pixels: 13254

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b1df68082c2f
Backed out changeset f57d14800cbb for wrench failures in border-with-rounded-clip.yaml and transforms/border-zoom.yaml

Comment 12

[Jamie Nicol [:jnicol]]

border-with-rounded-clip.yaml is failing because the size of the border is 100x200, but the border widths 300. With the new code in create_border_segments and add_corner_segment, each corner now decides it overlaps with the opposite edges, so does not render. Changing the top and bottom widths to 100 (half the height) and the left and right widths to 50 (half the width) fixes this. I assume/hope we'd never get in to this situation by accident with a real display list, and it's just something we have to bear in mind when writing wrench reftests.

border-zoom.yaml is failing because the texel rect set on the corner's BrushSegment is in local layout pixels, but I think the image shader expects them in device pixels. These don't match up in this reftest because of the zoom transform. I also noticed this morning on my hidpi laptop lots of wrench tests fail for the same reason, despite being fine on my low-res desktop.

So I think I need to find a way to scale the texture rect.

Comment 13

[Jamie Nicol [:jnicol]]

We can set the correct texel rect fairly easily if we normalize BrushSegment.extra_data, so that 0 is the start of the segment and 1 is the end of the segment. Then modify the shader to calculate the device pixel co-ordinates from that (where it already adjusts them to be absolute rather than segment-relative.) I think image borders would be the only other things affected by this, and presumably they shouldn't be too hard to update.

Does that sound like an okay solution Glenn?

If not, do you have any other suggestions on how to set the texture rect in device pixels? I don't think we know the correct scale at the time the BrushSegments are created.

Comment 14

[Glenn Watson [:gw]]

I agree about the border-with-rounded-clip.yaml - we should be fine to just update the yaml to a more reasonable set of inputs.

I'm less sure about the border-zoom issue. The brush_image shader should expect them in local space, as far as I know. I just did a quick test of running border-zoom in my local machine, with various different DPI ratios, and it seems to work fine.

Could you expand a bit on the existing issue with this (esp. how to repro it), and I can look another look?

Comment 15

[Jamie Nicol [:jnicol]]

Arggh it's one thing after another with this bug!

Try push here with the changes we discussed on irc: https://treeherder.mozilla.org/#/jobs?repo=try&revision=af538cebb3862f491aa843ba4923f8c8e1b95e3c&selectedJob=224764402

border-suite-2 I think we should fuzz

border-image-crash is strange. the image-width/height in the yaml is 32, so that's what we think it is in NinePatchDescriptor::create_segments. So I now normalize the slices by dividing by 32. But the image's size is actually 96x96, so the shader un-normalizes by multiplying by 96. So we end up sampling a completely different bit of the image. I think the reason it's failing is that we now end up sampling outside of the segment (because of the 36 slice), which produces different results between the two runs.

border-with-rounded-clip is the one which I previously had to change the border widths so that the segments no longer overlap. That had the effect that we now draw 4 non-overlapping segments to produce the final result, whereas previously each segment was the full size and was drawn on top of eachother. This drawing on top of eachother produced different results along the anti-aliased edge of the clip. With my patches the result now matches what you'd get with just a clipped rect rather than border, but doesn't match the reference png. Should I update the png?

Comment 16

[Jamie Nicol [:jnicol]]

[1] https://searchfox.org/mozilla-central/source/gfx/wr/webrender_api/src/display_item.rs#348

The documentation for NinePatchBorder[1] says if the source is an image then it will be stretched to fill the size given by width x height. But I don't think webrender currently does this. If it did, the 36 slice would currently sample outwith the image like it does with my change, making the results inconsistent and the test fail.

I looked up the blame for this test and it seems you added it when fixing an overflow for when the slice > size. So changing that part of the test obviously isn't an option, but is it possible to run the test but ignore the result?

Comment 17

[Glenn Watson [:gw]]

Looks like we can re-generate the PNGs for border-suite-2 (instead of fuzzing) and also for border-with-rounded-clip.

For border-image-crash, if I understand what you said we normalize / denormalize by a different factor between the cpu / shader code? Is that just a bug we can fix?

It does kind of look like it's just sampling garbage, maybe the test is just bad? I haven't looked at it in detail yet.

Comment 18

[Glenn Watson [:gw]]

Oh, I missed your second part of the question - yes, we can ignore that part of the result, if we don't care about the pixels themselves in this test case. What we could do is add a rect to the reftest, that is solid white and masks out the pixels we don't care about?

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b04c7dcb1a43
Handle overlapping border corners in webrender r=gw

Comment 20

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/b04c7dcb1a43