Jeff Muizelaar [:jrmuizel] Reporter
	Description • 7 years ago

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.

	Jeff Muizelaar [:jrmuizel] Reporter
	Comment 1 • 7 years ago

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

	Jeff Muizelaar [:jrmuizel] Reporter
	Comment 2 • 7 years ago

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

	Jeff Muizelaar [:jrmuizel] Reporter
	Comment 3 • 7 years ago

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.

	Sotaro Ikeda [:sotaro]
	Comment 4 • 7 years ago

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

	Jeff Muizelaar [:jrmuizel] Reporter
	Comment 5 • 7 years ago

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

	Andrew Osmond [:aosmond] (he/him) Assignee
	Comment 6 • 7 years ago

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

	Jeff Muizelaar [:jrmuizel] Reporter
	Comment 7 • 7 years ago

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

	Jeff Muizelaar [:jrmuizel] Reporter
	Comment 8 • 7 years ago

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

	Andrew Osmond [:aosmond] (he/him) Assignee
	Comment 9 • 7 years ago

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

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

	Jeff Muizelaar [:jrmuizel] Reporter
	Comment 10 • 7 years ago

Is anything blocking this from further progress?

	Jeff Muizelaar [:jrmuizel] Reporter
	Comment 11 • 7 years ago

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

	Pulsebot
	Comment 12 • 7 years ago

Pushed by aosmond@gmail.com:
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

	Cristian Brindusan [:cbrindusan]
	Comment 13 • 7 years ago
bugherder

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

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 14 • 7 years ago

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

	Cornel Ionce [:noni] [Hubs QA]
	Comment 15 • 7 years ago

Thanks Milan, updating the qe-flag