1704792 - Support putting SVG images into blob recordings

Status: RESOLVED FIXED

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

Description

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

Inline SVGs are currently put into blob recordings, but SVG images are rasterized on the main thread in the content process. As the first step for deferrable blobs, we should allow them to be saved as blob recordings, and rasterized on the scene builder thread instead of the main thread. This has the advantage of 1) not rasterizing in the content process main thread and splitting the work across multiple worker threads, 2) allows us to make us of blob image tiling to reduce how much work we may need to do (don't need all the tiles) and reduce how much contiguous memory we require (tiles are smaller).

Comment 1

[Robert Longson [:longsonr]]

Is this related to active images i.e. bug 1555356 that I turned off in bug 1684625 because of multiple regressions.

Comment 2

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

This affects SVG images, so those included on a page like raster image, e.g. via the img src attribute, or in CSS via url. Bug 1684625's example shows a nested inline SVG. I'm not changing the active image logic path that you touched, although I'm not entirely sure how it will interact with nested SVG images.... a good test case to check out :).

Comment 3

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

I'll attach a WIP in case you want to take a look.

Comment 4

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

Attachment: Bug 1704792 - Part 1. Eliminate use of std::vector in blob recording plumbing.

We have clang plugins that prevent us from moving STL objects due to
some implementations not supporting move. Let's just use nsTArray so
that we can actually enforce moving in a later patch.

Comment 5

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

Attachment: Bug 1704792 - Part 1. Split AutoRestoreSVGState into its own header.

This will be used in a later patch in the series when creating a
recording from an SVG image.

Comment 6

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

Attachment: Bug 1704792 - Part 1. Add SourceSurfaceBlobImage to manage the blob recording for SVG images.

Comment 7

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

Attachment: Bug 1704792 - Part 8. Add caching support for SourceSurfaceBlobImage.

Comment 8

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

Attachment: Bug 1704792 - Part 2. Add plumbing for display list building to support blobs for SVG images.

Comment 9

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

Attachment: Bug 1704792 - Part 3. Make display list items request blobs for SVG images.

Comment 10

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

Attachment: Bug 1704792 - Part 4. Allow animated SVG images to use blob recordings.

Comment 11

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

Attachment: Bug 1704792 - Part 2. Move SVGDrawingCallback declaration to own header.

Comment 12

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

Attachment: Bug 1704792 - Part 3. Move VectorImage::mIsDrawing to SVGDocumentWrapper.

Moving mIsDrawing to SVGDocumentWrapper allows us to avoid depending on
VectorImage in a future patch, and instead only keep a reference to
SVGDocumentWrapper. This helps avoid a circular dependency.

Comment 13

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

Attachment: Bug 1704792 - Part 4. Fix how VectorImage::GetFrameInternal did not honour aWhichFrame.

Comment 14

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

Attachment: Bug 1704792 - Part 10. Ensure we invalidate any blob recordings inside the cache.

The SurfaceCache may contain blob image surfaces which are not inside
any ImageContainer. As such, when an invalidation is requested, we need
to ensure we mark those surfaces as dirty so that if a new image
container is created and a cached surface is placed inside of it, the
caller will regenerate the blob recording as expected.

Comment 15

[Timothy Nikkel (:tnikkel)]

A high level overview of what/how you implemented would be helpful for reviewing.

Comment 16

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

Blob images are currently used for inline SVGs and for corner cases where we don't support rendering particular display items and/or configurations thereof with WebRender. They are recordings of drawing commands which are rasterized on worker threads during scene building in the compositor process.

SVG images are currently rasterized on the main thread of the content process. This work moves them to use the same infrastructure that existing blob images use.

SourceSurfaceBlobImage keeps the image key bindings alive for the recordings for each the layer manager that uses the SVG image, as well as is responsible for generating the recording itself. This effectively moves the drawing logic out of imgFrame to SourceSurfaceBlobImage post 100% WebRender.

The only way to get a SourceSurfaceBlobImage is to request an ImageContainer with the flag imgIContainer::FLAG_RECORD_BLOB set. This is used on the various display item's CreateWebRenderCommands path.

The VectorImage is responsible for marking the SourceSurfaceBlobImage objects as dirty if there is an invalidation event (e.g. if it is animated, etc). When the blob image key is requested during display list building via the SharedSurfacesParent::ShareBlob wrapper, the SourceSurfaceBlobImage will check if the recording is dirty, and if so, regenerate the recording and update it in the resource queue, in addition to returning the blob image key itself.

Without the RECORD_BLOB flag, consumers will continue to get rasterized versions of the SVG in the content process. This will ensure things such as canvas continue to work as expected.

We store the SourceSurfaceBlobImage object in the cache like we do other surfaces, with the addition of a new flag SurfaceFlags::RECORD_BLOB to differentiate them in the cache. This will allow us to continue tracking images in the memory reports, as well as avoid being too aggressive about purging recordings. If we hit memory pressure, they will still be purged like they are now however.

This work also moves animated SVG images from using fallback blob images to explicit blob images using SourceSurfaceBlobImage. Right now we don't generate a dirty rect between frame updates, so we must replay the whole recording, but it does at least allow us to reuse the blob image key and allocated texture on the GPU.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

I was thinking some more about how we should handle large SVG sprite sheets.

There are a couple of options that I see:

1. Record and rasterize separate blobs for each rectangle of the the image used.
2. Record a single blob for the whole image and rasterize different parts of it as needed.
3. Record a single blob and have a single rasterization that includes only the tiles needed to supply all of the sprites used.

Comment 18

[Phabricator Automation]

Comment on attachment 9215451 [details]
Bug 1704792 - Part 1. Split AutoRestoreSVGState into its own header.

Revision D111834 was moved to bug 1708443. Setting attachment 9215451 [details] to obsolete.

Comment 19

[Phabricator Automation]

Comment on attachment 9215567 [details]
Bug 1704792 - Part 2. Move SVGDrawingCallback declaration to own header.

Revision D111902 was moved to bug 1708443. Setting attachment 9215567 [details] to obsolete.

Comment 20

[Phabricator Automation]

Comment on attachment 9215568 [details]
Bug 1704792 - Part 3. Move VectorImage::mIsDrawing to SVGDocumentWrapper.

Revision D111903 was moved to bug 1708443. Setting attachment 9215568 [details] to obsolete.

Comment 21

[Phabricator Automation]

Comment on attachment 9215569 [details]
Bug 1704792 - Part 4. Fix how VectorImage::GetFrameInternal did not honour aWhichFrame.

Revision D111904 was moved to bug 1708443. Setting attachment 9215569 [details] to obsolete.

Comment 22

[Jeff Muizelaar [:jrmuizel]]

I made two test cases that show the different off the different performance characteristics of the different implementation choices:

• lots of expensive sprites fast in Firefox slow in Chrome
• lots of large but inexpensive sprites fast in Chrome slow in Firefox

Comment 23

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

Attachment: Bug 1704792 - Part 4. Add ImageIntRegion, an integer variant of ImageRegion.

Comment 24

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

Attachment: Bug 1704792 - Part 5. Add plumbing to move/store ImageIntRegion.

This patch has no functional change beyond changing prototypes and
adding storage for ImageIntRegion for each ImageContainer.

Comment 25

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

Attachment: Bug 1704792 - Part 6. Add calculation for ImageIntRegion for SVG images.

This patch produces an ImageIntRegion in
ComputeImageContainerDrawingParameters which will allow a future patch
in this series to produce partial blob recordings for SVG images when
used as an atlas or similar.

Comment 26

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

Attachment: Bug 1704792 - Part 7. Integrate use of ImageIntRegion with WebRender display lists.

This patch hooks up the ImageIntRegion to the blob recording and makes
any necessary adjusts to the display list creation to take advantage of
it.

Comment 27

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

try with the caching patches: https://treeherder.mozilla.org/jobs?repo=try&revision=6af878d5a37fe7ec99977743e3afd2aced1a3a73
try without the caching patches: https://treeherder.mozilla.org/jobs?repo=try&revision=b261c1d72cd7acd721e03d44d2dd05d8777c9e6c

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2990e1f17a38
Part 1. Add SourceSurfaceBlobImage to manage the blob recording for SVG images. r=jrmuizel,nical
https://hg.mozilla.org/integration/autoland/rev/d029d8beb9c7
Part 2. Add plumbing for display list building to support blobs for SVG images. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/95855d1cc4ea
Part 3. Make display list items request blobs for SVG images. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/8a96f0170744
Part 4. Add ImageIntRegion, an integer variant of ImageRegion. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/91a7204d340a
Part 5. Add plumbing to move/store ImageIntRegion. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/d5870b192d9b
Part 6. Add calculation for ImageIntRegion for SVG images. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/6b7633663e92
Part 7. Integrate use of ImageIntRegion with WebRender display lists. r=jrmuizel

Comment 29

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/2990e1f17a38
https://hg.mozilla.org/mozilla-central/rev/d029d8beb9c7
https://hg.mozilla.org/mozilla-central/rev/95855d1cc4ea
https://hg.mozilla.org/mozilla-central/rev/8a96f0170744
https://hg.mozilla.org/mozilla-central/rev/91a7204d340a
https://hg.mozilla.org/mozilla-central/rev/d5870b192d9b
https://hg.mozilla.org/mozilla-central/rev/6b7633663e92

Comment 30

[Andra Esanu (needinfo me)]

== Change summary for alert #30126 (as of Wed, 19 May 2021 08:59:01 GMT) ==

Improvements:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
5%	Images		windows10-64-shippable-qr		7,429,081.84 -> 7,063,978.40
5%	Images		windows10-64-shippable-qr		7,426,860.59 -> 7,062,847.22
3%	Images		linux1804-64-shippable-qr		5,103,777.57 -> 4,935,042.06

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=30126