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

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P2
normal
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: aosmond, Assigned: aosmond)

Tracking

Trunk
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

10 months ago
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.
(Assignee)

Updated

10 months ago
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
(Assignee)

Comment 1

10 months ago
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...).
Priority: -- → P1
Whiteboard: [gfx-noted]
This doesn't sound like a user-facing bug. At best it's going to be a perf improvement; dropping to P2.
Priority: P1 → P2
(Assignee)

Comment 4

10 months ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(Assignee)

Comment 5

10 months ago
Attachment #8987056 - Attachment is obsolete: true
Attachment #8991013 - Flags: review?(nical.bugzilla)

Updated

10 months ago
Attachment #8991013 - Flags: review?(nical.bugzilla) → review+

Updated

10 months ago
Attachment #8991014 - Flags: review?(nical.bugzilla) → review+

Comment 8

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

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6ac886d5a6ad
https://hg.mozilla.org/mozilla-central/rev/b91599130fba
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.