1425484 - Reuse shared surfaces in blob images instead of copying

Status: RESOLVED FIXED

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

Description

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

An example of where this triggers is https://dev.w3.org/SVG/profiles/1.1F2/test/svg/struct-image-04-t.svg.

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0001-Bug-1425484-Part-1.-Allow-using-SharedSurfacesChild-.patch, v1

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0002-Bug-1425484-Part-2.-Allow-using-SharedSurfacesParent.patch, v1

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0003-Bug-1425484-Part-3.-Integrate-shared-surfaces-with-t.patch, v1

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

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0003-Bug-1425484-Part-3.-Integrate-shared-surfaces-with-t.patch, v2

Fix how it would be used with printing. Just use it for WebRender.

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

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

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?

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0003-Bug-1425484-Part-3.-Integrate-shared-surfaces-with-t.patch, v3

Moved the relevant code into gfx/layers/wr and rejigged the subclassing.

Comment 9

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0001-Bug-1425484-Part-1.-Allow-using-SharedSurfacesChild-.patch, v2

Unbitrot.

Comment 10

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0002-Bug-1425484-Part-2.-Allow-using-SharedSurfacesParent.patch, v2

Unbitrot.

Comment 11

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0003-Bug-1425484-Part-3.-Integrate-shared-surfaces-with-t.patch, v4

Unbitrot.

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

Comment 12

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8962449 [details] [diff] [review]
0002-Bug-1425484-Part-2.-Allow-using-SharedSurfacesParent.patch, v2

Whoops, obsoleted the wrong attachment.

Comment 13

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0001-Bug-1425484-Part-1.-Allow-using-SharedSurfacesChild-.patch, v3

Comment 14

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0002-Bug-1425484-Part-2.-Allow-using-SharedSurfacesParent.patch, v3

Comment 15

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0003-Bug-1425484-Part-3.-Integrate-shared-surfaces-with-t.patch, v5

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

Comment 16

[Andrew Osmond [:aosmond] (he/him)]

(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.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/f389cb76ed88
https://hg.mozilla.org/mozilla-central/rev/fae8c28c3966
https://hg.mozilla.org/mozilla-central/rev/b2a3cbc041dd

Comment 19

[Cristian Brindusan [:cbrindusan]]

Hi,

Can you please take a look at these tier2 failures that started with your push?

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b2a3cbc041dd835893b261ff64caa8120468f887&filter-searchStr=f3cdd594f3536ed5c748e4c6011c309a5962963e&selectedJob=175934909

Comment 20

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 21

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 22

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 23

[Narcis Beleuzu [:NarcisB]]

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

Comment 24

[Andrew Osmond [:aosmond] (he/him)]

(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...

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bba12a996009
Part 4. Followup to fix shared surface lifetime issue with blob images. r=aosmond

Comment 26

[Natalia Csoregi [:nataliaCs]]

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3e2845ac301
Part 5. Correct prototype of TakeExternalSurfaces. r=aosmond

Comment 28

[Tiberius Oros[:tiberius_oros]]

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

Comment 29

[Andrew Osmond [:aosmond] (he/him)]

Clearing NIs, this was resolved.