Closed Bug 1401672 Opened 2 years ago Closed 2 years ago

We seem to duplicate gmail images

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 --- unaffected
firefox58 --- unaffected
firefox59 --- fixed

People

(Reporter: jrmuizel, Assigned: aosmond)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [wr-mvp] [gfx-noted])

Attachments

(1 file)

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.
Blocks: 1401574
This can be reproduced using: https://jrmuizel.github.io/webrender/sprite.html
I confirmed that we seem to be sending this multiple times.
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.
Depends on: 1331944
Whiteboard: [wr-mvp] [triage]
Priority: -- → P3
Whiteboard: [wr-mvp] [triage] → [wr-reserve]
Whiteboard: [wr-reserve] → [gfx-noted]
Priority: P3 → P2
Whiteboard: [gfx-noted] → [wr-mvp] [gfx-noted]
:jrmuizel, could we close this bug, since bug 1331944 is addressed?
Flags: needinfo?(jmuizelaar)
Probably. I'll do a new recording and confirm that it's fixed.
Flags: needinfo?(jmuizelaar)
(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.
(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?
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Priority: P2 → P1
FWIW, this bug is contributing to our low motionmark scores. It uses the same image a bunch.
Is anything blocking this from further progress?
Flags: needinfo?(aosmond)
Flags: needinfo?(aosmond)
Attachment #8927846 - Flags: review?(jmuizelaar)
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
Attachment #8927846 - Flags: review?(jmuizelaar) → review+
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
https://hg.mozilla.org/mozilla-central/rev/0ad4e810bab1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Not sure we can have QE test this without specially instrumented builds, so I would make this qe-verify-
Thanks Milan, updating the qe-flag
Flags: qe-verify+ → qe-verify-
You need to log in before you can comment on or make changes to this bug.