Closed Bug 1711061 Opened 3 years ago Closed 2 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, Regressed 3 open bugs)

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: