Closed Bug 1425484 Opened 2 years ago Closed 2 years ago

Reuse shared surfaces in blob images instead of copying

Categories

(Core :: Graphics: WebRender, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: aosmond, Assigned: aosmond)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(3 files, 8 obsolete files)

3.07 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
8.15 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
29.88 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
If a shared surface is referenced in a blob image recording, it is currently copied when it is already mapped into the GPU process memory space. We should use SharedSurfacesChild/Parent on the record and replay paths to avoid the data copy.
An example of where this triggers is https://dev.w3.org/SVG/profiles/1.1F2/test/svg/struct-image-04-t.svg.
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Whiteboard: [gfx-noted]
Fix how it would be used with printing. Just use it for WebRender.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=834fd33f0993e82002dabb342eb174e146606e2a
Attachment #8937085 - Attachment is obsolete: true
Attachment #8937083 - Flags: review?(jmuizelaar)
Attachment #8937084 - Flags: review?(jmuizelaar)
Comment on attachment 8937088 [details] [diff] [review]
0003-Bug-1425484-Part-3.-Integrate-shared-surfaces-with-t.patch, v2

I think there might have been some rule against using layers/etc headers from 2d files?
Attachment #8937088 - Flags: review?(jmuizelaar)
(In reply to Andrew Osmond [:aosmond] from comment #6)
> Comment on attachment 8937088 [details] [diff] [review]
> 0003-Bug-1425484-Part-3.-Integrate-shared-surfaces-with-t.patch, v2
> 
> I think there might have been some rule against using layers/etc headers
> from 2d files?

Yep.
Moved the relevant code into gfx/layers/wr and rejigged the subclassing.
Attachment #8937088 - Attachment is obsolete: true
Attachment #8937088 - Flags: review?(jmuizelaar)
Attachment #8937753 - Flags: review?(jmuizelaar)
Unbitrot.
Attachment #8937083 - Attachment is obsolete: true
Attachment #8937083 - Flags: review?(jmuizelaar)
Attachment #8962448 - Flags: review?(jmuizelaar)
Unbitrot.
Attachment #8937084 - Attachment is obsolete: true
Attachment #8937084 - Flags: review?(jmuizelaar)
Attachment #8962449 - Flags: review?(jmuizelaar)
Comment on attachment 8962449 [details] [diff] [review]
0002-Bug-1425484-Part-2.-Allow-using-SharedSurfacesParent.patch, v2

Whoops, obsoleted the wrong attachment.
Attachment #8962449 - Attachment is obsolete: false
Attachment #8962449 - Flags: review?(jmuizelaar)
Attachment #8937753 - Attachment is obsolete: true
Attachment #8937753 - Flags: review?(jmuizelaar)
Attachment #8962448 - Attachment is obsolete: true
Attachment #8962448 - Flags: review?(jmuizelaar)
Attachment #8962449 - Attachment is obsolete: true
Attachment #8962449 - Flags: review?(jmuizelaar)
(In reply to Andrew Osmond [:aosmond] from comment #15)
> Created attachment 8970964 [details] [diff] [review]
> 0003-Bug-1425484-Part-3.-Integrate-shared-surfaces-with-t.patch, v5
> 
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=aed0613083fed55212086f56c1d805855a5d2517

In addition to the rebasing and making it work with blob invalidation, it now keeps the surfaces alive at least as long as the WebRenderFallback/GroupData objects.
Attachment #8970961 - Flags: review?(jmuizelaar)
Attachment #8970963 - Flags: review?(jmuizelaar)
Attachment #8970964 - Flags: review?(jmuizelaar)
Attachment #8970963 - Flags: review?(jmuizelaar) → review+
Attachment #8970961 - Flags: review?(jmuizelaar) → review+
Attachment #8970964 - Flags: review?(jmuizelaar) → review+
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f389cb76ed88
Part 1. Allow using SharedSurfacesChild::Share with the external image ID directly. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/fae8c28c3966
Part 2. Allow using SharedSurfacesParent off the compositor thread. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2a3cbc041dd
Part 3. Integrate shared surfaces with the blob image recordings. r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/f389cb76ed88
https://hg.mozilla.org/mozilla-central/rev/fae8c28c3966
https://hg.mozilla.org/mozilla-central/rev/b2a3cbc041dd
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Sigh, this was in the try run, but I missed it buried among the other (real) intermittents. Looks like it failed to convert the given external image ID to a surface, which very much surprises me. Investigating.
If I can't quickly resolve this, I'll land a temp tweak to just disable the use of shared surfaces rather than back the whole thing out.
Reproduced locally in rr. I was expecting a second owner of the surface to come in and keep it alive, but that did not happen. I may need to manually force SharedSurfacesParent::Share with an image key ID to avoid this race, which will kick in the machinery to keep the surface alive until the epoch expires.
There are also crashtest and reftest crashing on "mozilla::wr::Moz2DRenderCallback"
:aosmond, can you please take a look at those too?

Crashtest log link: https://treeherder.mozilla.org/logviewer.html#?job_id=175922895&repo=mozilla-inbound&lineNumber=19989
Reftest log link: https://treeherder.mozilla.org/logviewer.html#?job_id=175928369&repo=mozilla-inbound&lineNumber=45418
(In reply to Narcis Beleuzu [:NarcisB] from comment #23)
> There are also crashtest and reftest crashing on
> "mozilla::wr::Moz2DRenderCallback"
> :aosmond, can you please take a look at those too?
> 
> Crashtest log link:
> https://treeherder.mozilla.org/logviewer.html#?job_id=175922895&repo=mozilla-
> inbound&lineNumber=19989
> Reftest log link:
> https://treeherder.mozilla.org/logviewer.html#?job_id=175928369&repo=mozilla-
> inbound&lineNumber=45418

Those are me too. Put together a patch that I think should fix it:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa496a863b80004b229184c2993dbb2de72d6767

Mechanisms are all there to avoid this, I just didn't cover all the cases where I needed to use it...
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bba12a996009
Part 4. Followup to fix shared surface lifetime issue with blob images. r=aosmond
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3e2845ac301
Part 5. Correct prototype of TakeExternalSurfaces. r=aosmond
Clearing NIs, this was resolved.
Flags: needinfo?(aosmond)
Flags: needinfo?(amkatz)
No longer blocks: 1450375
You need to log in before you can comment on or make changes to this bug.