Bug 1494403 (blob-re-coord)

blob image re-coordination

REOPENED
Assigned to

Status

()

defect
P2
normal
REOPENED
8 months ago
3 days ago

People

(Reporter: jrmuizel, Assigned: nical)

Tracking

(Depends on 1 bug, Blocks 9 bugs, {leave-open})

64 Branch
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 wontfix, firefox65 fixed, firefox66 wontfix, firefox67 affected)

Details

(Whiteboard: [wr-q2][wr-june])

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

8 months ago
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.
(Reporter)

Updated

8 months ago
Blocks: 1429930
(Reporter)

Updated

8 months ago
Blocks: 1493466
(Reporter)

Updated

8 months ago
Blocks: 1477369
(Reporter)

Updated

8 months ago
No longer blocks: 1477369
(Reporter)

Updated

8 months ago
Blocks: 1494408
(Reporter)

Updated

8 months ago
Priority: -- → P2
Summary: [meta] blob image re-coordination → blob image re-coordination
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.
Priority: P2 → P3
(Reporter)

Updated

8 months ago
Blocks: 1391838
(Reporter)

Updated

8 months ago
Blocks: 1425871
(Reporter)

Updated

8 months ago
Assignee: nobody → a.beingessner
Priority: P3 → P2
(Reporter)

Updated

8 months ago
Blocks: 1494924
(Reporter)

Updated

8 months ago
Blocks: 1470219
(Reporter)

Updated

8 months ago
Blocks: 1477371
(Reporter)

Updated

8 months ago
Blocks: 1477374
(Reporter)

Updated

8 months ago
Alias: blob-re-coord
(Reporter)

Updated

8 months ago
Blocks: 1478125
(Reporter)

Updated

7 months ago
Blocks: 1484863
(Reporter)

Updated

7 months ago
Blocks: 1477369
(Reporter)

Updated

7 months ago
Blocks: 1494173
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.
(Reporter)

Updated

6 months ago
Assignee: a.beingessner → nical.bugzilla
(Assignee)

Comment 4

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

Comment 5

6 months ago
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

6 months ago
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/df30b0f614c9
Separate the Blob related apis. r=jrmuizel

Comment 7

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/df30b0f614c9
Status: NEW → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
(Reporter)

Updated

6 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 8

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

Comment 9

5 months ago
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.
(Reporter)

Updated

5 months ago
Depends on: 1513772
(Reporter)

Comment 10

5 months ago
Posted patch Gecko side v2 (obsolete) — Splinter Review
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
Attachment #9019430 - Attachment is obsolete: true
(Reporter)

Comment 11

5 months ago
(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.
(Reporter)

Comment 12

5 months ago
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
(Reporter)

Comment 13

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

Comment 14

5 months ago
> 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.
(Assignee)

Comment 15

5 months ago
> 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.
(Reporter)

Updated

4 months ago
Priority: P2 → P3
(Reporter)

Comment 16

4 months ago
Posted patch Gecko side v3Splinter Review
Attachment #9032073 - Attachment is obsolete: true
(Reporter)

Updated

4 months ago
Priority: P3 → P4

Comment 18

4 months ago
Pushed by nsilva@mozilla.com:
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

4 months ago
bugherder
Status: REOPENED → RESOLVED
Last Resolved: 6 months ago4 months ago
Resolution: --- → FIXED
(Reporter)

Updated

4 months ago
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
(Reporter)

Updated

3 months ago
Blocks: wr-68
No longer blocks: stage-wr-trains
Priority: P4 → P2
(Reporter)

Comment 24

2 months ago

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

(Reporter)

Updated

2 months ago
Depends on: 1539702
Whiteboard: [wr-q2][wr-june]
You need to log in before you can comment on or make changes to this bug.