1401672 - We seem to duplicate gmail images

Status: RESOLVED FIXED

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

Description

[Jeff Muizelaar [:jrmuizel]]

If you look at the yaml here: https://jrmuizel.github.io/webrender/gmail.zip we have multiple copies of the same image. This could be a yaml problem but if it's not we should stop doing it.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

This can be reproduced using: https://jrmuizel.github.io/webrender/sprite.html

Comment 2

[Jeff Muizelaar [:jrmuizel]]

I confirmed that we seem to be sending this multiple times.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

I believe this will probably be fixed by Andrew's shared image stuff. I.e. we should be maintaining the image key on the image lib object and not on the display item.

Comment 4

[Sotaro Ikeda [:sotaro]]

:jrmuizel, could we close this bug, since bug 1331944 is addressed?

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Probably. I'll do a new recording and confirm that it's fixed.

Comment 6

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

(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)
> I believe this will probably be fixed by Andrew's shared image stuff. I.e.
> we should be maintaining the image key on the image lib object and not on
> the display item.

FWIW, it doesn't maintain the image key in the imagelib surface. It does maintain an external image ID, which is mapped to as many image keys as needed by the display items. If recording is turned on, it still dupes the image data in WebRenderBridgeParent::AddExternalImage; all you saved was the initial copy from the content process to the GPU process.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(In reply to Andrew Osmond [:aosmond] from comment #6)
> 
> FWIW, it doesn't maintain the image key in the imagelib surface. It does
> maintain an external image ID, which is mapped to as many image keys as
> needed by the display items. If recording is turned on, it still dupes the
> image data in WebRenderBridgeParent::AddExternalImage; all you saved was the
> initial copy from the content process to the GPU process.

What prevents us from reusing the same image key for all the display items?

Comment 8

[Jeff Muizelaar [:jrmuizel]]

FWIW, this bug is contributing to our low motionmark scores. It uses the same image a bunch.

Comment 9

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

Attachment: 0002-Bug-1401672-Make-display-items-for-the-same-WebRende.patch

Seems to avoid any duplicate images in the yaml on gmail :D.

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

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Is anything blocking this from further progress?

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8927846 [details] [diff] [review]
0002-Bug-1401672-Make-display-items-for-the-same-WebRende.patch

Review of attachment 8927846 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/SharedSurfacesChild.cpp
@@ +275,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  auto i = aKeys.Length();
> +  while (i > 0) {

for key in aKeys

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ad4e810bab1
Make display items for the same WebRenderBridgeParent/Child share the ImageKey for shared surfaces. r=jrmuizel

Comment 13

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/0ad4e810bab1

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Not sure we can have QE test this without specially instrumented builds, so I would make this qe-verify-

Comment 15

[Cornel Ionce [:noni] [Hubs QA]]

Thanks Milan, updating the qe-flag