1494403 - (blob-recoord) blob image re-coordination

Status: RESOLVED FIXED

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

Description

[Jeff Muizelaar [:jrmuizel]]

We want to adjust some of the coordinate spaces in blob images to better align with what Gecko needs and to make it so that scrolling and resizing of the blob images works well.

Comment 1

[Markus Stange [:mstange]]

Here's the plan:

At the moment, blob images only have a size. They are sized to the bounds of the nsDisplaySVGWrapper display item. (0, 0) inside the blob's coordinate space is at the top left corner of the nsDisplaySVGWrapper's bounds. This means that, if the nsDisplaySVGWrapper's bounds expand / such that the origin of the bounds rectangle changes, this affects the coordinate space of the blob. Any items inside the blob will now be treated as having "moved" because their coordinates relative to the blob's origin have changed. This causes unnecessary invalidations in many legitimate cases. For example, when scrolling up, display items can get added at the top of a blob, which affects the wrapper's bounds.

We want to change this such that (0, 0) inside the blob coordinate space refers to (0, 0) in SVG user space. SVG elements create reference frames now, and that reference frame's position is the (0, 0) that we want here.

In order to do this, we need to give the blob image an origin in this space. We still want to size the blob image to the bounds of the nsDisplaySVGWrapper item. So instead of giving the blob image just a "size", we need to give it a "rect". This rectangle describes the area that is covered by the blob image in blob image space. And we need to update Gecko to set coordinates on the blob content items that are relative to (0, 0) instead of being relative to the blob's bounds. This part should be pure code removal.

Then we need to make sure that resizing the blob does not invalidate unchanged pixels in the middle of the image. This means 1) ensuring that Gecko doesn't send unnecessarily large invalid rects with its updates and 2) teaching WebRender not to discard tiles / pixels for parts of a blob image that haven't changed even when the blob's rect has changed.

We might also need to take a look at how blob merging handles this case.

Comment 2

[Aria Beingessner [:Gankra]]

Attachment: Bug 1494403 - WIP refactor dirty rects to be simpler/clearer.

Previously dirty rects were set up in a subtle way where early in the
pipeline None is used to mean "no information" and therefore "all clean",
but then later on is used to mean "all dirty" on the assumption that if
a texture is still being handled, some dirty information must have been
sent. None is therefore used by the internals as a way to signal
things like missing cache entries without actually bothering to look up 
the dimensions of the image (note that "clean" can always be expressed
without knowing dimensions, as it is always the same empty rect).

I change the system to just consistently use a custom enum that has
AllDirty and SomeDirty(empty_rect) to represent these two states. As
a result the code seems much easier to understand.

This is WIP because I probably want to make the dirty rect signed so
that it can be in the same space as the visible rect introduced in the
following commit. Also I probably need to rework how dirty rects are
passed for tiles, for similar reasons.

Comment 3

[Aria Beingessner [:Gankra]]

Attachment: Bug 1494403 - WIP recoordination, still need to rework wr image cache to handle visible rect properly.

Depends on D9559

Comment 4

[Nicolas Silva [:nical]]

Attachment: Bug 1494403 - Separate the Blob related apis. r=jrmuizel

This commit contains the Gecko-side changes from WebRender PR#3277:
 - Dedicated DirtyRect type.
 - Separate the blob image APIs from regular image ones.

Comment 5

[Nicolas Silva [:nical]]

Try push with the Gecko and WebRender API changes around blob images and dirty rects: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c468b1b643bb7fb6fcebccb0dc8ea4001c50f3d6

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df30b0f614c9
Separate the Blob related apis. r=jrmuizel

Comment 7

[Dorel Luca [:dluca]]

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

Comment 8

[Jeff Muizelaar [:jrmuizel]]

I'm working on rebasing D9560/"WIP recoordination, still need to rework wr image cache to handle visible rect properly."

Comment 9

[Jeff Muizelaar [:jrmuizel]]

I have the gecko side rebased and partially debugged. I'm running into an issue with code like:
  gfxRect maskSurfaceRect = context->GetClipExtents(gfxContext::eDeviceSpace);
getting confused about what the extents of the draw target are. I'll need to think more about how to solve that.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Attachment: Gecko side v2

This version of the patch seems to basically work and the most egregious test failures are fixed. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47b2af11447879688df9e19c3abfd6f0dca68c07&selectedJob=217592705

Comment 11

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> I have the gecko side rebased and partially debugged. I'm running into an
> issue with code like:
>   gfxRect maskSurfaceRect =
> context->GetClipExtents(gfxContext::eDeviceSpace);
> getting confused about what the extents of the draw target are. I'll need to
> think more about how to solve that.

The best solution seems to be to reuse the SetDeviceOffset support gfxContext.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

The remaining interesting failures are:
 /css/CSS2/inline-svg-intrinsic-size-100-percent-1.html
 layout/reftests/image-element/element-paint-native-widget.html
 layout/reftests/svg/pattern-big-image.html

Comment 13

[Jeff Muizelaar [:jrmuizel]]

We also seem to be hitting Hit 'MOZ_CRASH(Manual eviction requires cleanup) at gfx/wr/webrender/src/resource_cache.rs:219'

Comment 14

[Nicolas Silva [:nical]]

> We also seem to be hitting Hit 'MOZ_CRASH(Manual eviction requires cleanup) at gfx/wr/webrender/src/resource_cache.rs:219'

I got this one to reproduce in rr. Looking into it.

Comment 15

[Nicolas Silva [:nical]]

> I got this one to reproduce in rr. Looking into it.

Nevermind, the one I have in rr is during rawtests and the issue was that some of the tests weren't cleaning the blob image keys themselves (cf. https://github.com/servo/webrender/pull/3470), so it wasn't an issue with webrender's resource cache. In that try run the blob image key leak is likely happening on the gecko side.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Attachment: Gecko side v3

Comment 17

[Nicolas Silva [:nical]]

Attachment: Bug 1494403 - Support arbitrary tiling origins and negative tile offsets in the tile decomposition algorithm. r=kvark

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2812c159321
Support arbitrary tiling origins and negative tile offsets in the tile decomposition algorithm. r=kvark

Comment 19

[Raul Gurzau (:RaulG)]

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

Comment 20

[Jeff Muizelaar [:jrmuizel]]

Current try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae7f150677f0a67bab9377e490bacfd250d2312d

Comment 21

[Jeff Muizelaar [:jrmuizel]]

With some more fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dec3c47a3bce8741f05bd8893fef2ef32d0f1299&selectedJob=233960090

Comment 22

[Jeff Muizelaar [:jrmuizel]]

And now a different approach to the device offset stuff:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a02c2940a93f4b427ccb75a3b323fe120bc7e64f

Comment 23

[Jeff Muizelaar [:jrmuizel]]

And here they all pass:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d89ccad36b444b082af4661dc20fefcbb33e90aa&selectedJob=235568897

Comment 24

[Jeff Muizelaar [:jrmuizel]]

It looks like things were broken somewhat by the "Use CreateClippedDrawTarget more." from bug 1495170

Comment 25

[Nicolas Silva [:nical]]

Attachment: Bug 1494403 - [WIP] Handle top and left partial blob tiles. r=jrmuizel

Comment 26

[Nicolas Silva [:nical]]

Attachment: Bug 1494403 - Rename blob visible area into active rect, pass it to create/update_blob_image. r=jrmuizel

Comment 27

[Nicolas Silva [:nical]]

Attachment: Bug 1494403 - Jeff's blob recoordination patch 'v3' rebased.

Comment 28

[Nicolas Silva [:nical]]

Attachment: Bug 1494403 - Invalidate tiles as needed when blob image active area changes.

Comment 29

[Jeff Muizelaar [:jrmuizel]]

Here's a version rebased on top of central/CreateClippedDrawTarget https://treeherder.mozilla.org/#/jobs?repo=try&revision=958792c35edd196bfe1ecb4343b06776af0c12d3

Comment 30

[Jeff Muizelaar [:jrmuizel]]

Here's the latest reftest run. I believe the failures are probably fuzzable.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d25df7f130dbd2e3ca4c6e6dd9efb790f8367155

Comment 31

[Jeff Muizelaar [:jrmuizel]]

I plan on breaking this up into three main parts to aid review and help with the bisection of any regressions:

1. Remove the top-left offset from the recording and apply it during playback: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9a1ecc45af3a1e549420effe5d73278c7ba86a1
2. Remove the top-left offset from the bounds in the index: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48e85a3476c6ae0c5ec76ee8a73c7d0b62e78028
3. Switch to orienting things around the visible rect instead of the item bounds. https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfe0937ace60f44f827f793817553194a9a165c9&selectedJob=253943326

The final results seems to pass tests, the earlier pieces still need some work.

Comment 32

[Jeff Muizelaar [:jrmuizel]]

Here's what I have left rebased on top of current m-c
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0f6623d5daa701bd55660322cfd21b98cd03291

https://hg.mozilla.org/try/rev/b5a827aa98c91c4c0619ea2e474769dd437108a0 is really the only interesting part.

Comment 33

[Jeff Muizelaar [:jrmuizel]]

This is done.