Closed Bug 1704792 Opened 6 months ago Closed 5 months ago

Support putting SVG images into blob recordings

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
90 Branch
Tracking Status
firefox90 --- disabled
firefox91 --- disabled

People

(Reporter: aosmond, Assigned: aosmond)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

Attachments

(7 files, 8 obsolete files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

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).

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

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 :).

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

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.

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

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.

Attachment #9215452 - Attachment description: Bug 1704792 - Part 3. Add SourceSurfaceBlobImage to manage the blob recording for SVG images. → Bug 1704792 - Part 6. Add SourceSurfaceBlobImage to manage the blob recording for SVG images.
Attachment #9215454 - Attachment description: Bug 1704792 - Part 5. Add plumbing for display list building to support blobs for SVG images. → Bug 1704792 - Part 7. Add plumbing for display list building to support blobs for SVG images.
Attachment #9215455 - Attachment description: Bug 1704792 - Part 6. Make display list items request blobs for SVG images. → Bug 1704792 - Part 8. Make display list items request blobs for SVG images.
Attachment #9215453 - Attachment description: Bug 1704792 - Part 4. Add caching support for SourceSurfaceBlobImage. → Bug 1704792 - Part 9. Add caching support for SourceSurfaceBlobImage.
Attachment #9215456 - Attachment description: Bug 1704792 - Part 7. Allow animated SVG images to use blob recordings. → Bug 1704792 - Part 10. Allow animated SVG images to use blob recordings.

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.

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

Attachment #9215451 - Attachment description: Bug 1704792 - Part 2. Split AutoRestoreSVGState into its own header. → Bug 1704792 - Part 1. Split AutoRestoreSVGState into its own header.
Attachment #9215567 - Attachment description: Bug 1704792 - Part 3. Move SVGDrawingCallback declaration to own header. → Bug 1704792 - Part 2. Move SVGDrawingCallback declaration to own header.
Attachment #9215568 - Attachment description: Bug 1704792 - Part 4. Move VectorImage::mIsDrawing to SVGDocumentWrapper. → Bug 1704792 - Part 3. Move VectorImage::mIsDrawing to SVGDocumentWrapper.
Attachment #9215569 - Attachment description: Bug 1704792 - Part 5. Fix how VectorImage::GetFrameInternal did not honour aWhichFrame. → Bug 1704792 - Part 4. Fix how VectorImage::GetFrameInternal did not honour aWhichFrame.
Attachment #9215452 - Attachment description: Bug 1704792 - Part 6. Add SourceSurfaceBlobImage to manage the blob recording for SVG images. → Bug 1704792 - Part 5. Add SourceSurfaceBlobImage to manage the blob recording for SVG images.
Attachment #9215454 - Attachment description: Bug 1704792 - Part 7. Add plumbing for display list building to support blobs for SVG images. → Bug 1704792 - Part 6. Add plumbing for display list building to support blobs for SVG images.
Attachment #9215455 - Attachment description: Bug 1704792 - Part 8. Make display list items request blobs for SVG images. → Bug 1704792 - Part 7. Make display list items request blobs for SVG images.
Attachment #9215453 - Attachment description: Bug 1704792 - Part 9. Add caching support for SourceSurfaceBlobImage. → Bug 1704792 - Part 8. Add caching support for SourceSurfaceBlobImage.
Attachment #9215456 - Attachment description: Bug 1704792 - Part 10. Allow animated SVG images to use blob recordings. → Bug 1704792 - Part 9. Allow animated SVG images to use blob recordings.
Attachment #9215607 - Attachment description: Bug 1704792 - Part 11. Ensure we invalidate any blob recordings inside the cache. → Bug 1704792 - Part 10. Ensure we invalidate any blob recordings inside the cache.
Attachment #9215450 - Attachment is obsolete: true

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.

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.
Depends on: 1708443

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.

Attachment #9215451 - Attachment is obsolete: true

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.

Attachment #9215567 - Attachment is obsolete: true

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.

Attachment #9215568 - Attachment is obsolete: true

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.

Attachment #9215569 - Attachment is obsolete: true

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

Attachment #9215452 - Attachment description: Bug 1704792 - Part 5. Add SourceSurfaceBlobImage to manage the blob recording for SVG images. → Bug 1704792 - Part 1. Add SourceSurfaceBlobImage to manage the blob recording for SVG images.
Attachment #9215454 - Attachment description: Bug 1704792 - Part 6. Add plumbing for display list building to support blobs for SVG images. → Bug 1704792 - Part 2. Add plumbing for display list building to support blobs for SVG images.
Attachment #9215455 - Attachment description: Bug 1704792 - Part 7. Make display list items request blobs for SVG images. → Bug 1704792 - Part 3. Make display list items request blobs for SVG images.
Attachment #9215456 - Attachment description: Bug 1704792 - Part 9. Allow animated SVG images to use blob recordings. → Bug 1704792 - Part 4. Allow animated SVG images to use blob recordings.

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

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.

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.

Attachment #9215453 - Attachment is obsolete: true
Attachment #9215607 - Attachment is obsolete: true
Attachment #9215456 - Attachment is obsolete: true
Attachment #9221611 - Attachment description: Bug 1704792 - Part 5. Add ImageIntRegion, an integer variant of ImageRegion. → Bug 1704792 - Part 4. Add ImageIntRegion, an integer variant of ImageRegion.
Attachment #9221612 - Attachment description: Bug 1704792 - Part 6. Add plumbing to move/store ImageIntRegion. → Bug 1704792 - Part 5. Add plumbing to move/store ImageIntRegion.
Attachment #9221613 - Attachment description: Bug 1704792 - Part 7. Add calculation for ImageIntRegion for SVG images. → Bug 1704792 - Part 6. Add calculation for ImageIntRegion for SVG images.
Attachment #9221614 - Attachment description: Bug 1704792 - Part 8. Integrate use of ImageIntRegion with WebRender display lists. → Bug 1704792 - Part 7. Integrate use of ImageIntRegion with WebRender display lists.
Pushed by aosmond@mozilla.com:
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
Regressions: 1711164

== 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

Blocks: 1711948
Regressions: 1712855
See Also: → 1713350
Regressions: 1713350
See Also: 1713350
Regressions: 1713418
Regressions: 1713002
You need to log in before you can comment on or make changes to this bug.