1899266 - Make ScaleOffset::(un)map_rect() operate on min and max points rather than origin and size

Status: NEW

(Core :: Graphics: WebRender, task)

Description

[Jamie Nicol [:jnicol]]

In bug 1894106 we are seeing seams when zooming with low-quality pinch zoom enabled. One of the causes of these seams is that ScaleOffset::unmap_rect() and ScaleOffset::map_rect() internally convert the min and max points to an origin and size, then perform the transform, then convert back to min and max points. Presumably this is a hangover from when we used "origin and size" Rect types rather than Box2D.

Due to floating point innaccuracies, it is therefore possible that mapping two rects that are adjacent (eg rect1.max.y == rect2.min.y) will result in two output rects that are no longer adjacent. This causes seams. We should instead operate directly on the min and max points rather than internally computing a size.

Unfortunately this causes a non-obvious reftest failure, so I am splitting this out in to its own bug separate from bug 1894106

Comment 1

[Jamie Nicol [:jnicol]]

Attachment: failure log

Link to the failure: https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/DmBGhldlSMWFZ89C6lz5tw/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

Also added as an attachment for when the treeherder logs are removed

Comment 2

[Jamie Nicol [:jnicol]]

As seen in the above logs, making map_rect() and unmap_rect() operate on the min and max points rather than origin and size causes reftests/boxshadow/scale.yaml to fail. Some of the box shadow edges are thicker/heavier than others as opposed to all 4 edges being the same.

If we reduce the testcase to just two box shadows it's a bit easier to debug:

---
root:
  items:
    - type: stacking-context
      transform: scale(0.3)
      items:
        - type: box-shadow
          bounds: [ 50, 50, 100, 100 ]
          color: black
          blur-radius: 1
          clip-mode: inset
    - type: stacking-context
      items:
        - type: box-shadow
          bounds: [ 50, 400, 100, 100 ]
          color: black
          blur-radius: 1
          clip-mode: inset

In this case, the 2nd box shadow is rendered incorrectly (top and left edges are thicker than bottom and right). Interestingly, removing the first box shadow from the test case makes the second one render correctly.

The effect of changing the map_rect and unmap_rect implementations is that the rect calculated here for the first box shadow changes from Box2D((49.9999961853027344, 49.9999961853027344), (149.9999847412109375, 149.9999847412109375)) to Box2D((49.9999961853027344, 49.9999961853027344), (150.0000000000000000, 150.0000000000000000)) (ie the next highest representable number)

If we follow the values that are derived from this rect, we eventually get to here. With the old unmap/map_rect implementation, the cache_size and original_alloc_size for the first box shadow are calculated as 16x16, whereas with the new implementation they are 15x15. For the second box shadow they are 15x15 in both cases.

This means that with the old implementations, the cache keys differ for both box shadows, meaning that here we create separate rounded rects and blur render tasks as the input to the box shadow. With the new implementations, we reuse the same render task as the input to both box shadows. This explains why removing the first box shadow from the test case makes the second one render correctly - as we will now create the input tasks specifically for the second box shadow.

Comment 3

[Jamie Nicol [:jnicol]]

I don't think that using the cached blurs is the root problem here, but it may be papering over / uncovering issues. If we checkout current mozilla/central, without any of my changes to map/unmap_rect, and reduce the testcase to only contain the two scale(0.6) boxshadows, then they render correctly. However, if you then remove the leftmost one of them, the remaining right-hand-side one now renders incorrectly. So the only reason it is being rendered correctly currently is because it is using the cached render task from the boxshadow next to it

Comment 4

[Jamie Nicol [:jnicol]]

Glenn, do you know what the expected rendering of these box shadows with fractional offsets/sizes is? Do we expect all 4 edges to have an equal width/weight? Looking at the reference image it appears that we probably do, but..
a) note the two smallest box shadows in the top-left do not (the top edge is thick and the bottom edge is thin)
b) the rest of them appear to only be correct by accident - removing the left column of items from the yaml causes the right column to render incorrectly. they only currently render correctly currently because they reuse the left column's cached blur render tasks.

If I completely remove the fract_offset and fract_size variables from the calculations here, then everything appears to render correctly. Both with and without my changes to map_rect(). Even the top-left tiny ones now have equal edges. It doesn't appear to cause any regressions on try, and in fact causes a test to pass.

Do you remember the motivation behind using the fract_offset and fract_size? Am I missing something here?

Comment 5

[Glenn Watson [:gw]]

We do have a number of box-shadow bugs on file related to incorrect rendering at various scale / fractional values. I'm working on an updated box-shadow implementation as part of my current work, which aims to resolve these. That is to say, I wouldn't worry too much about the current box-shadow impl being wrong in some of those cases, as they should get resolved in the next couple of weeks, if I can get this stuff landed.