1570081 - Support changing the bounds of blob image visible rects.

Status: RESOLVED FIXED

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

Description

[Nicolas Silva [:nical]]

WebRrender-side changes needed for the blob-image recoordination work.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Bug 1570081 - [WIP] fixes

Comment 2

[Nicolas Silva [:nical]]

Attachment: Bug 1570081 - Invalidate blob tiles accordingly when bounds change.

Comment 3

[Nicolas Silva [:nical]]

Attachment: Bug 1570081 - Support changing the blob image visible rect in WebRender. r=jrmuizel

Comment 4

[Nicolas Silva [:nical]]

Attachment: Bug 1570081 - Pass the tile's blob-space offset to the blob renderer.

Comment 5

[Nicolas Silva [:nical]]

Attachment: Bug 1570081 - Remove assumptions in blob tiling code about single tile offsets.

Depends on D40050

Comment 6

[Nicolas Silva [:nical]]

Attachment: Bug 1570081 - Wrench rawtest changes. r=jrmuizel

Depends on D40826

Comment 7

[Nicolas Silva [:nical]]

Attachment: Bug 1570081 - Remove unnecessary viewport_tiles field.

Depends on D41387

Comment 8

[Nicolas Silva [:nical]]

Attachment: Bug 1570081 - Update reftests. r=jrmuizel

Depends on D41388

Comment 9

[Nicolas Silva [:nical]]

Attachment: Bug 1570081 - Don't insert blob images with empty visible rects. r=jrmuizel

Depends on D41389

Comment 10

[Nicolas Silva [:nical]]

Attachment: Bug 1570081 - Put the blob dirty rect in the same space as the drawing commands and visible rect. r=jrmuizel

Depends on D43222

Comment 11

[Nicolas Silva [:nical]]

Quick note about the invalidation issue. I don't know how this works but it looks like in WebRenderCommandsBuilder.cpp we invalidate more than we need to. While scrolling a large SVG we hit this call to InvalidateRect: https://searchfox.org/mozilla-central/rev/325c1a707819602feff736f129cb36055ba6d94f/gfx/layers/wr/WebRenderCommandBuilder.cpp#545 and I suspect that here (and in other places?) we don't want to use the full clipped image bounds and instead some thing that would be the intersection of the the clipped image bounds before and after scrolling.
Commenting out that line causes the blob to never update (because we don't update mValidRect at all during scrolling) so we get empty tiles past the initial visible area.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

(In reply to Nicolas Silva [:nical] from comment #11)

Quick note about the invalidation issue. I don't know how this works but it looks like in WebRenderCommandsBuilder.cpp we invalidate more than we need to. While scrolling a large SVG we hit this call to InvalidateRect: https://searchfox.org/mozilla-central/rev/325c1a707819602feff736f129cb36055ba6d94f/gfx/layers/wr/WebRenderCommandBuilder.cpp#545 and I suspect that here (and in other places?) we don't want to use the full clipped image bounds and instead some thing that would be the intersection of the the clipped image bounds before and after scrolling.
Commenting out that line causes the blob to never update (because we don't update mValidRect at all during scrolling) so we get empty tiles past the initial visible area.

Indeed. I'll investigate today.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

So a piece that I forgot about that we'll need, is to make sure our invalid rect is intersected appropriately.

Today we intersect the invalid rect with the bounds of blob image image, but instead we need to intersect it with the intersection of the old bounds and the new bounds because that's only area that it makes sense to invalidate. Does that make sense? I'll try to put together a patch for it.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Attachment: Untested patch that handles makes the invalidation make more sense in the blob resizing world

I haven't even tried to build this, but it should roughly do what we want here.

Some notable changes:

• mImageBounds changes to be the an un-unit-ed mLayerBounds instead of the visibleRect. This should reduce some unnecessary invalidation
• we don't intersect with mClippedGroupBounds. We shouldn't need to anymore because the visible area should already be tight
• add a mLastVisibleRect that tracks the previous visible rect
• we store a mPreservedRect which is the intersection between the current visiblerect and the last one
• all calls to InvalidateRect are intersected with mPreservedRect

Possible areas of trouble:

• new blob images don't have an invalid rect the size of the whole image. Instead it will be empty.
• obsolete assertions might trigger.

Comment 15

[Nicolas Silva [:nical]]

Attachment: Bug 1570081 - Fix a number of issues with the tile decomposition code. r=jrmuizel

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Latest try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ee4a038cc15eca090b38c862877344bc63a5f84

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/afa98bb10628
Fix a number of issues with the tile decomposition code. r=jrmuizel

Comment 18

[Nicolas Silva [:nical]]

Attachment: Bug 1570081 - Drop an assertion. r=jrmuizel

Comment 19

[Noemi Erli[:noemi_erli]]

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/548e4395ee1a
Support changing the blob image visible rect in WebRender. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/214c815015a8
Wrench rawtest changes. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/ba19640abf33
Update reftests. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/14a4451b2781
Don't insert blob images with empty visible rects. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/8d4baf4d063a
Put the blob dirty rect in the same space as the drawing commands and visible rect. r=jrmuizel

Comment 21

[Andreea Pavel [:apavel]]

Backed out for assertion failure at WebRenderCommandBuilder.cpp

Push that started the failures https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=8d4baf4d063ac13cb7b0ef7e1457d43a91f8fbe5&selectedJob=266487618

Failure log https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=266487618&repo=autoland&lineNumber=26696

Backout: https://hg.mozilla.org/integration/autoland/rev/c2abef0bfc0b855a8f0b50e96dabe1f6a4973de2

***this failed on multiple test suites (crashtests, reftests, mochitests) both tier1 and tier2

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32951c9cc186
Support changing the blob image visible rect in WebRender. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/775c0840f9cb
Wrench rawtest changes. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/aac21319b917
Update reftests. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/aeeb928c954f
Don't insert blob images with empty visible rects. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/e906f82ffa90
Put the blob dirty rect in the same space as the drawing commands and visible rect. r=jrmuizel

Comment 23

[Nicolas Silva [:nical]]

Attachment: Bug 1570081 - Adjust a reftest. r=jnicol,aosmond

Comment 24

[Bogdan Tara[:bogdan_tara | bogdant]]

The patch could not be landed:
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. applying /tmp/tmpgkppmL layout/reftests/border-image/reftest.list Hunk #1 FAILED at 48. 1 out of 1 hunk FAILED -- saving rejects to file layout/reftests/border-image/reftest.list.rej abort: patch command failed: exited with status 256

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0aa2e90830bd
Adjust a reftest. r=jnicol,aosmond

Comment 27

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/32951c9cc186
https://hg.mozilla.org/mozilla-central/rev/775c0840f9cb
https://hg.mozilla.org/mozilla-central/rev/aac21319b917
https://hg.mozilla.org/mozilla-central/rev/aeeb928c954f
https://hg.mozilla.org/mozilla-central/rev/e906f82ffa90
https://hg.mozilla.org/mozilla-central/rev/0aa2e90830bd