Closed Bug 1711061 Opened 4 years ago Closed 3 years ago

Refactor image integration with WebRender

Categories

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

Product:
Core
Component:
Graphics: WebRender
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
96 Branch
Tracking Flags:
Tracking Status
firefox96 --- fixed

People

(Reporter: aosmond, Assigned: aosmond)

References

(Blocks 1 open bug)

Details

Attachments

(13 files)

Bug 1711061 - Part 1. Remove SourceSurfaceMappedData surface deduplication from memory reports.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1711061 - Part 2. Add imgIContainer::GetImageProvider skeleton.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1711061 - Part 3. Add WebRenderImageProvider and implement for rasterized providers.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1711061 - Part 5. Expose an ImageProviderId for tracking surface ownership.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1711061 - Part 6. Implement RasterImage::GetImageProvider.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1711061 - Part 7. Remove support for ImageContainer-based SVG image blob recordings.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1711061 - Part 8. Rename SourceSurfaceBlobImage.h/cpp to BlobSurfaceProvider.h/cpp.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1711061 - Part 10. Implement BlobSurfaceProvider for non-rasterized blob recordings.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1711061 - Part 9. Add blob recording support to SurfaceCache.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1711061 - Part 11. Implement VectorImage::GetImageProvider.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1711061 - Part 12. Change the display list to use WebRenderImageProvider.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1711061 - Part 13. Remove the now unused ImageContainer and related code for images.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1711061 - Part 4. Implement AnimationSurfaceProvider for animated rasterized images.
48 bytes, text/x-phabricator-request
Details | Review

We use ImageContainers and SourceSurface's to integrate Raster/VectorImage with the WebRender display list generation. There is lots of indirection here we could probably avoid, e.g. ImageContainer will always contain just one SourceSurface, which is always SourceSurfaceSharedData or SourceSurfaceBlobImage. Maybe we can do better here and avoid the overhead as well as make the code look better.

We no longer use SourceSurfaceMappedData because we only support
SourceSurfaceSharedData-backed imgFrame in the SurfaceCache now.

In later parts in this series, GetImageProvider will replace
GetImageContainerAtSize. This will be a more specialized and lower
overhead means to get a wr::ImageKey for a particular surface.

This provides the framework to allow ISurfaceProvider objects to
implement WebRenderImageProvider. It is straightforward for rasterized
providers (DecodedSurfaceProvider, SimpleSurfaceProvider and
AnimationSurfaceProvider). Later parts in this series will provide the
necessary changes for blob recordings.

This will be used by layers as a replacement for ImageContainer's
ContainerProducerID for tracking if the same imgIRequest/imgIContainer
own the cached WebRenderImageProvider.

This will be replaced by a WebRenderImageProvider-based implementation
in a later part in this series.

Rename the file in preparation for the switch over to
WebRenderImageProvider.

This WebRenderImageProvider/ISurfaceProvider subclass provides the
implementation for blob recordings. This is mostly just taking the
functionality that was previously in SourceSurfaceBlobImage.

Now that we no longer have the extra layer of ImageContainers providing
a superficial level of caching/reuse of existing blob recordings, we
need some way to share recordings. This part adds support to
SurfaceCache to store BlobSurfaceProvider objects.

This includes the specialized code for invalidating SVG images. In
particular this is useful for animated SVG images. In general we want to
avoid changing the image key whenever possible so that we avoid
reallocating the underlying buffers in the compositor process for the
rasterized blob images.

We also need to track the ImageIntRegion used by the recording. If a
caller only wants a slice of the SVG image, then we need to track this
differentiation in our cache entries. At this time, we don't allow
substitutes for entries with a region exclusion.

Attachment #9242858 - Attachment description: Bug 1711061 - Part 4. Expose an ImageProviderId for tracking surface ownership. → Bug 1711061 - Part 5. Expose an ImageProviderId for tracking surface ownership.
Attachment #9242859 - Attachment description: Bug 1711061 - Part 5. Implement RasterImage::GetImageProvider. → Bug 1711061 - Part 6. Implement RasterImage::GetImageProvider.
Attachment #9242860 - Attachment description: Bug 1711061 - Part 6. Remove support for ImageContainer-based SVG image blob recordings. → Bug 1711061 - Part 7. Remove support for ImageContainer-based SVG image blob recordings.
Attachment #9242861 - Attachment description: Bug 1711061 - Part 7. Rename SourceSurfaceBlobImage.h/cpp to BlobSurfaceProvider.h/cpp. → Bug 1711061 - Part 8. Rename SourceSurfaceBlobImage.h/cpp to BlobSurfaceProvider.h/cpp.
Attachment #9242862 - Attachment description: Bug 1711061 - Part 8. Implement BlobSurfaceProvider for non-rasterized blob recordings. → Bug 1711061 - Part 9. Implement BlobSurfaceProvider for non-rasterized blob recordings.
Attachment #9242863 - Attachment description: Bug 1711061 - Part 9. Add blob recording support to SurfaceCache. → Bug 1711061 - Part 10. Add blob recording support to SurfaceCache.
Attachment #9242864 - Attachment description: Bug 1711061 - Part 10. Implement VectorImage::GetImageProvider. → Bug 1711061 - Part 11. Implement VectorImage::GetImageProvider.
Attachment #9242865 - Attachment description: Bug 1711061 - Part 11. Change the display list to use WebRenderImageProvider. → Bug 1711061 - Part 12. Change the display list to use WebRenderImageProvider.
Attachment #9242866 - Attachment description: Bug 1711061 - Part 12. Remove the now unused ImageContainer code in image/. → Bug 1711061 - Part 13. Remove the now unused ImageContainer and related code for images.
Attachment #9242863 - Attachment description: Bug 1711061 - Part 10. Add blob recording support to SurfaceCache. → Bug 1711061 - Part 9. Add blob recording support to SurfaceCache.
Attachment #9242862 - Attachment description: Bug 1711061 - Part 9. Implement BlobSurfaceProvider for non-rasterized blob recordings. → Bug 1711061 - Part 10. Implement BlobSurfaceProvider for non-rasterized blob recordings.
Pushed by aosmond@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8d5261184226 Part 1. Remove SourceSurfaceMappedData surface deduplication from memory reports. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/cb00177e655e Part 2. Add imgIContainer::GetImageProvider skeleton. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/ac2792d42be2 Part 3. Add WebRenderImageProvider and implement for rasterized providers. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/6b2bc6c16b9f Part 4. Implement AnimationSurfaceProvider for animated rasterized images. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/da992effeff1 Part 5. Expose an ImageProviderId for tracking surface ownership. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/9af5b3f2225f Part 6. Implement RasterImage::GetImageProvider. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/d4b3d0fa1771 Part 7. Remove support for ImageContainer-based SVG image blob recordings. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/f3b2379d971b Part 8. Rename SourceSurfaceBlobImage.h/cpp to BlobSurfaceProvider.h/cpp. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/27565d5ae08d Part 9. Add blob recording support to SurfaceCache. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/000940244dcf Part 10. Implement BlobSurfaceProvider for non-rasterized blob recordings. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/343746ffae73 Part 11. Implement VectorImage::GetImageProvider. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/cb7721d7ce49 Part 12. Change the display list to use WebRenderImageProvider. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/5988220f56b8 Part 13. Remove the now unused ImageContainer and related code for images. r=tnikkel

Backed out for causing browser-chrome failures in test/performance/browser_startup_images

Backout link

Push with failures

Failure log

Flags: needinfo?(aosmond)

I think this is because I didn't add a NotifyDrawingObservers call in GetImageProvider

Flags: needinfo?(aosmond)
Pushed by aosmond@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/41ba2e2a2d13 Part 1. Remove SourceSurfaceMappedData surface deduplication from memory reports. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/0145b538175b Part 2. Add imgIContainer::GetImageProvider skeleton. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/d453c5219255 Part 3. Add WebRenderImageProvider and implement for rasterized providers. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/b964576ae53d Part 4. Implement AnimationSurfaceProvider for animated rasterized images. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/be73004c0850 Part 5. Expose an ImageProviderId for tracking surface ownership. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/38890cc266fb Part 6. Implement RasterImage::GetImageProvider. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/96a0ef35881b Part 7. Remove support for ImageContainer-based SVG image blob recordings. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/a37b3091281d Part 8. Rename SourceSurfaceBlobImage.h/cpp to BlobSurfaceProvider.h/cpp. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/1014a95f540e Part 9. Add blob recording support to SurfaceCache. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/7b471990ea86 Part 10. Implement BlobSurfaceProvider for non-rasterized blob recordings. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/3687e798f665 Part 11. Implement VectorImage::GetImageProvider. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/aced4b672fb4 Part 12. Change the display list to use WebRenderImageProvider. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/c4f073f7e3a3 Part 13. Remove the now unused ImageContainer and related code for images. r=tnikkel
Regressions: 1738181
Regressions: 1738267

Backed out on @aosmond's request

Backout link: https://hg.mozilla.org/integration/autoland/rev/a02c0af6e80cc31fe0d9a8572b2ddac8d564ed35

Status: RESOLVED → REOPENED
status-firefox95: fixed → ---
Flags: needinfo?(aosmond)
Resolution: FIXED → ---
Target Milestone: 95 Branch → ---

(In reply to Sandor Molnar from comment #20)

Backed out on @aosmond's request

Backout link: https://hg.mozilla.org/integration/autoland/rev/a02c0af6e80cc31fe0d9a8572b2ddac8d564ed35

== Change summary for alert #32251 (as of Thu, 04 Nov 2021 11:07:36 GMT) ==

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
6% instagram SpeedIndex android-hw-g5-7-0-arm7-shippable-qr warm webrender 454.31 -> 428.83

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

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:aosmond, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(tnikkel)
Flags: needinfo?(aosmond)

Patches aren't relanded because of regressions they cause that need to be fixed.

Flags: needinfo?(tnikkel)

It appears the cause of the regressions is because of this bail out condition:

https://searchfox.org/mozilla-central/rev/519a8915c5538be2f03813e05c1a7bd8dc9f49c8/gfx/layers/wr/WebRenderUserData.cpp#50

We return false to indicate we should invalidate but we don't schedule a paint like the other paths. With the old ImageContainer code, we would always return an ImageContainer from GetImageContainerAtSize, even if it was empty because we haven't decoded a surface yet. This would ensure we have a WebRenderImageData object attached to the nsImageFrame and we would reach this line instead:

https://searchfox.org/mozilla-central/rev/519a8915c5538be2f03813e05c1a7bd8dc9f49c8/gfx/layers/wr/WebRenderUserData.cpp#68

since we WebRenderImageData::mKey is Nothing().

Flags: needinfo?(aosmond)
Pushed by aosmond@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a7c6389a767f Part 1. Remove SourceSurfaceMappedData surface deduplication from memory reports. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/41590355b534 Part 2. Add imgIContainer::GetImageProvider skeleton. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/de019933be1f Part 3. Add WebRenderImageProvider and implement for rasterized providers. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/d6a41b6555bb Part 4. Implement AnimationSurfaceProvider for animated rasterized images. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/43278d9a5da0 Part 5. Expose an ImageProviderId for tracking surface ownership. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/3b3078df854d Part 6. Implement RasterImage::GetImageProvider. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/7d2c3c3a5339 Part 7. Remove support for ImageContainer-based SVG image blob recordings. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/889fcfb23434 Part 8. Rename SourceSurfaceBlobImage.h/cpp to BlobSurfaceProvider.h/cpp. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/7503e1289b92 Part 9. Add blob recording support to SurfaceCache. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/d7f9835a65e8 Part 10. Implement BlobSurfaceProvider for non-rasterized blob recordings. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/466af290693c Part 11. Implement VectorImage::GetImageProvider. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/2eef1762a4e8 Part 12. Change the display list to use WebRenderImageProvider. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/3fe9e1643aaf Part 13. Remove the now unused ImageContainer and related code for images. r=tnikkel
Depends on: 1743574
Regressions: 1743574
Regressions: 1743761
No longer regressions: 1743574
See Also: → 1745475
Regressions: 1748216
Regressions: 1772665
No longer regressions: 1772665
Regressions: 1780311
Regressions: 1805599
Regressions: 1831457
Regressions: 1891672
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: