Ensure SVGs prefer data surfaces whenever possible

RESOLVED FIXED in Firefox 59

Status

()

Core
Graphics: WebRender
P1
normal
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: aosmond, Assigned: aosmond)

Tracking

unspecified
mozilla59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [wr-mvp])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 months ago
In imgFrame::InitWithDrawable, we call gfxPlatform::CanRenderContentToDataSurface() to determine whether or not we should render the SVG to a DataSourceSurface. imgFrame is the only consumer of this API. When we cannot render to a DataSourceSurface, we need to take a snapshot of the DrawTarget after rendering the SVG to get a SourceSurface to put into the SurfaceCache. It by default returns false, and exceptions are made for Mac and Android.

Linux should always return true, because it uses Skia as its content backend just like Mac and Android. It is only on Windows that we could potentially use D2D.

For WebRender however, it would be preferable if we could always render to a DataSourceSurface, because that allows us to use SourceSurfaceSharedData. That in turn allows us to avoid copies of the image data itself in both content and GPU processes, and avoid duplicate ImageKeys for the same image because we had to fallback to using blob images or ImageBridgeChild. This will have the side effect of using Skia to rasterize SVGs on Windows whereas before we may have used D2D if available.
(Assignee)

Updated

2 months ago
Assignee: nobody → aosmond
Status: NEW → ASSIGNED

Updated

2 months ago
Priority: -- → P1
Whiteboard: [wr-mvp]
(Assignee)

Comment 2

2 months ago
Created attachment 8931075 [details] [diff] [review]
0004-Bug-1419767-gfxPlatform-CanRenderContentToDataSurfac.patch, v2

I didn't think clearly when I rewrote the now obsoleted patch. The original method didn't need the backend type passed in as it as all hard coded to return true/false based on platform. Now it properly passes in the desired backend to use with data surfaces. Hopefully this greens the reftests.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=df4512f237325d9cf63f1c74ebe5b165b9b1df33
Attachment #8930920 - Attachment is obsolete: true
(Assignee)

Comment 3

2 months ago
As it stands, I expect the Windows QR SVG reftests will need some fuzz because of this, since it ends up using Skia instead of D2D to rasterize SVGs now.

There are two alternatives that come to mind: one, take out the check for if we are using WR; it will at least start using shared surfaces on Linux and on Windows if D2D was disabled in the content process. I expect that would mean SVGs will use PImageBridge/Textures to shuttle the D2D surfaces across.

Or two, continue using D2D to generate the surface, but do what the texture code will do anyways, and copy *that* into a shared surface. This does at least have the advantage over textures by sharing the memory instead of duping it in both the content and GPU processes, as well as enabling sharing the image keys across display items.
(Assignee)

Comment 4

2 months ago
Created attachment 8931472 [details] [diff] [review]
0004-Bug-1419767-Remove-gfxPlatform-CanRenderContentToDat.patch, v3

Let's not get too ambitious with this. For the moment I just want it to be correct without changing the world :). Right now on Linux we don't use shared surfaces (because nobody implemented CanRenderContentToDataSurface for it) and we don't use volatile surfaces on Windows if it disabled D2D. Let's switch to Factory::DoesBackendSupportDataDrawtarget straight up, as that is maintained and does what we want better.
Attachment #8931075 - Attachment is obsolete: true
Attachment #8931472 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

2 months ago
Summary: Ensure SVGs prefer data surfaces with WebRender → Ensure SVGs prefer data surfaces whenever possible

Updated

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

Comment 5

2 months ago
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3cd5881c7ee
Remove gfxPlatform::CanRenderContentToDataSurface. r=nical

Comment 6

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c3cd5881c7ee
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.