1469964 - Shared surfaces repeatably regenerate image key and reupload whole surface if slow to decode

Status: RESOLVED FIXED

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

Description

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

If an image is slow to decode (e.g. very large), it will call SourceSurfaceSharedData::Invalidate many times while it is already being displayed by WebRender. As implemented today, this causes us to regenerate the ImageKey key and reupload the entire texture, when only a few rows may have changed.

Instead we should:

1) Use UpdateExternalImage when we need to invalidate instead of deleting the ImageKey and calling AddExternalImage again. While UpdateExternalImage exists in the compositor process, this needs to be exposed via IpcResourceUpdateQueue.

2) Add expose a dirty rect parameter for UpdateExternalImage. Imagelib already knows at the time it calls SourceSurfaceSharedData::Invalidate the region of the image which invalidated. This will allow WebRender to only reupload the rows that changed.

Comment 1

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

Attachment: [WIP] 0001-Bug-1469964-Avoid-ever-deleting-the-image-keys-for-S.patch, v1

Still a work in progress, as I am hitting the "BUG: handle not requested earlier in frame" panic after switching to UpdateExternalImage. I also suspect progressive images may have some trouble with how I tried to avoid reuploading the entire image again when decoding has completed (mDecoded == mFrameRect, but we may still need to post an invalid rect for the rewritten rows in imgFrame::Finish...).

Comment 2

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

Attachment: [WIP] 0001-Bug-1469964-Avoid-ever-deleting-the-image-keys-for-S.patch, v2

Comment 3

[Kartikaya Gupta (email:[e-mail])]

This doesn't sound like a user-facing bug. At best it's going to be a perf improvement; dropping to P2.

Comment 4

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> This doesn't sound like a user-facing bug. At best it's going to be a perf
> improvement; dropping to P2.

Yes, there is no correctness aspect to this, just performance.

Comment 5

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

Attachment: 0001-Bug-1469964-Part-1.-Expose-WebRender-plumbing-for-pa.patch, v1

Comment 6

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

Attachment: 0002-Bug-1469964-Part-2.-Make-shared-surfaces-use-externa.patch, v1

Comment 7

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

Looks like I have a few failures to fix:

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c4e88af19efcbbd5448f922fbe06fe5687ac41b&selectedJob=187815192

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ac886d5a6ad
Part 1. Expose WebRender plumbing for partial updates for external images. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/b91599130fba
Part 2. Make shared surfaces use external image update mechanism. r=nical

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/6ac886d5a6ad
https://hg.mozilla.org/mozilla-central/rev/b91599130fba