blob image re-coordination
Categories
(Core :: Graphics: WebRender, defect, P2)
Tracking
()
People
(Reporter: jrmuizel, Assigned: nical)
References
(Depends on 1 open bug, Blocks 3 open bugs)
Details
(Whiteboard: [wr-q4])
Attachments
(3 files, 7 obsolete files)
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•6 years ago
|
Comment 1•6 years ago
|
||
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.
Updated•6 years ago
|
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Updated•6 years ago
|
Comment 2•6 years ago
|
||
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•6 years ago
|
||
Depends on D9559
Reporter | ||
Updated•6 years ago
|
Assignee | ||
Comment 4•6 years 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 years 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
Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/df30b0f614c9 Separate the Blob related apis. r=jrmuizel
Comment 7•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/df30b0f614c9
Reporter | ||
Updated•6 years ago
|
Updated•6 years ago
|
Reporter | ||
Comment 8•6 years 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 years 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 | ||
Comment 10•5 years ago
|
||
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
Reporter | ||
Comment 11•5 years 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 years 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 years 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 years 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 years 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•5 years ago
|
Reporter | ||
Comment 16•5 years ago
|
||
Assignee | ||
Comment 17•5 years ago
|
||
Reporter | ||
Updated•5 years ago
|
Comment 18•5 years 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•5 years ago
|
||
bugherder |
Reporter | ||
Updated•5 years ago
|
Reporter | ||
Comment 20•5 years ago
|
||
Updated•5 years ago
|
Reporter | ||
Updated•5 years ago
|
Reporter | ||
Updated•5 years ago
|
Reporter | ||
Comment 21•5 years ago
|
||
Reporter | ||
Comment 22•5 years ago
|
||
And now a different approach to the device offset stuff:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a02c2940a93f4b427ccb75a3b323fe120bc7e64f
Reporter | ||
Comment 23•5 years ago
|
||
Reporter | ||
Comment 24•5 years ago
|
||
It looks like things were broken somewhat by the "Use CreateClippedDrawTarget more." from bug 1495170
Updated•5 years ago
|
Assignee | ||
Comment 25•5 years ago
|
||
Reporter | ||
Updated•5 years ago
|
Reporter | ||
Updated•5 years ago
|
Assignee | ||
Comment 26•5 years ago
|
||
Assignee | ||
Comment 27•5 years ago
|
||
Assignee | ||
Comment 28•5 years ago
|
||
Reporter | ||
Comment 29•5 years ago
|
||
Here's a version rebased on top of central/CreateClippedDrawTarget https://treeherder.mozilla.org/#/jobs?repo=try&revision=958792c35edd196bfe1ecb4343b06776af0c12d3
Reporter | ||
Comment 30•5 years ago
|
||
Here's the latest reftest run. I believe the failures are probably fuzzable.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d25df7f130dbd2e3ca4c6e6dd9efb790f8367155
Reporter | ||
Comment 31•5 years ago
|
||
I plan on breaking this up into three main parts to aid review and help with the bisection of any regressions:
- Remove the top-left offset from the recording and apply it during playback: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9a1ecc45af3a1e549420effe5d73278c7ba86a1
- Remove the top-left offset from the bounds in the index: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48e85a3476c6ae0c5ec76ee8a73c7d0b62e78028
- 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.
Reporter | ||
Updated•5 years ago
|
Reporter | ||
Updated•5 years ago
|
Updated•5 years ago
|
Reporter | ||
Comment 32•5 years ago
|
||
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.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Reporter | ||
Updated•5 years ago
|
Updated•5 years ago
|
Reporter | ||
Comment 33•5 years ago
|
||
This is done.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Description
•