Get vector images working with WebRender

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jrmuizel, Assigned: aosmond)

Tracking

(Blocks 1 bug, {stale-bug})

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 unaffected, firefox57 unaffected, firefox59 fixed)

Details

(Whiteboard: [wr-mvp] gfx-noted)

Attachments

(8 attachments, 60 obsolete attachments)

5.34 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
1.43 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
4.46 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
7.94 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
9.33 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
29.60 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
8.49 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
6.36 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
This currently puts the bullets on a wikipedia page in PaintedLayers
Here's the part that fails:

       nsCOMPtr<imgIContainer> srcImage;
       requestProxy->GetImage(getter_AddRefs(srcImage));
       if (srcImage && !srcImage->IsImageContainerAvailable(aManager, imgIContainer::FLAG_NONE)) {
Assignee: nobody → aosmond
Flags: needinfo?(aosmond)
Status: NEW → ASSIGNED
Priority: -- → P3
Whiteboard: gfx-noted
Fix an accidental condition change, should solve the crashes on the previous try.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93944639bb2c30c59487d2d1f826c6e950c75cc5
Attachment #8872436 - Attachment is obsolete: true
Add maximum texture size checks to mirror RasterImage.
Attachment #8872439 - Attachment is obsolete: true
Minor fix, shouldn't have an impact, but if somehow GetFrameInternal was called where mFullyLoaded was set, it would incorrectly return BAD_IMAGE instead of NOT_READY. The callers checked this already though.
Attachment #8872437 - Attachment is obsolete: true
Attachment #8872591 - Flags: review?(tnikkel)
Attachment #8872613 - Flags: review?(tnikkel)
Attachment #8872438 - Flags: review?(tnikkel)
Attachment #8872612 - Flags: review?(tnikkel)
Attachment #8872440 - Flags: review?(tnikkel)
Comment on attachment 8872612 [details] [diff] [review]
Part 4. Implement VectorImage::GetImageContainer and VectorImage::IsImageContainerAvailable., v2

This will be changing as part of bug 1368776 so maybe the review for these bits as not necessary.
Attachment #8872612 - Flags: review?(tnikkel)
Attachment #8872438 - Flags: review?(tnikkel)
Comment on attachment 8872591 [details] [diff] [review]
Part 1. VectorImage::GetFrameAtSize should not create a DrawTarget if using cached surface., v2

>+VectorImage::DrawInternal(const SVGDrawingParameters& aParams,

>   // Set context paint (if specified) on the document:
>   Maybe<AutoSetRestoreSVGContextPaint> autoContextPaint;
>-  if (haveContextPaint && !blockContextPaint) {
>-    autoContextPaint.emplace(aSVGContext->GetContextPaint(),
>+  if (aContextPaint) {
>+    autoContextPaint.emplace(aParams.svgContext->GetContextPaint(),
>                              mSVGDocumentWrapper->GetDocument());

This actually changes us from using aSVGContext always to the new svg context if it exists.
Attachment #8872591 - Flags: review?(tnikkel)
Attachment #8872440 - Flags: review?(tnikkel)
Attachment #8872613 - Flags: review?(tnikkel)
(In reply to Timothy Nikkel (:tnikkel) from comment #11)
> Comment on attachment 8872591 [details] [diff] [review]
> Part 1. VectorImage::GetFrameAtSize should not create a DrawTarget if using
> cached surface., v2
> 
> >+VectorImage::DrawInternal(const SVGDrawingParameters& aParams,
> 
> >   // Set context paint (if specified) on the document:
> >   Maybe<AutoSetRestoreSVGContextPaint> autoContextPaint;
> >-  if (haveContextPaint && !blockContextPaint) {
> >-    autoContextPaint.emplace(aSVGContext->GetContextPaint(),
> >+  if (aContextPaint) {
> >+    autoContextPaint.emplace(aParams.svgContext->GetContextPaint(),
> >                              mSVGDocumentWrapper->GetDocument());
> 
> This actually changes us from using aSVGContext always to the new svg
> context if it exists.

Noted thanks. I already fixed it in the rewrite which should also make it much easier to review :).
Attachment #8872438 - Attachment is obsolete: true
Attachment #8872440 - Attachment is obsolete: true
Attachment #8872591 - Attachment is obsolete: true
Attachment #8872612 - Attachment is obsolete: true
Attachment #8872613 - Attachment is obsolete: true
Merge error.
Attachment #8874534 - Attachment is obsolete: true
Merge error.
Attachment #8874537 - Attachment is obsolete: true
(In reply to Andrew Osmond [:aosmond] from comment #30)
> Created attachment 8874563 [details] [diff] [review]
> Part 10. Mark VectorImage as surface as shared when used outside an image
> layer., v4
> 
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7038bf7ce454143b28df67bd4626225afebaac1a

It appears that downscale-on-decode is necessary for vector image image container support. Not for downscaling, but for *upscaling*. Without it, images look blurry in the reftest. Sigh.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f57a27d8e313e0ad557105b85912731bfe2b75ea

Updated part 5 from downscale-on-decode, which forces imgIContainer::GetImageContainer uses from nsDisplayImage and related to use said functionality, fixes at least some of these reftests, or all of them if we are lucky.
Okay try again, my branch didn't have the latest mImageContainer changes merged in, but when I pushed to try I updated and broke the build.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ed8335bbcf61c316af3fdec6099c68b51a47430
Depends on: 1368776
Attachment #8874531 - Attachment is obsolete: true
Attachment #8874533 - Attachment is obsolete: true
Attachment #8874556 - Attachment is obsolete: true
Attachment #8874557 - Attachment is obsolete: true
Attachment #8874558 - Attachment is obsolete: true
Attachment #8874559 - Attachment is obsolete: true
Attachment #8874560 - Attachment is obsolete: true
Attachment #8874561 - Attachment is obsolete: true
Attachment #8874562 - Attachment is obsolete: true
Attachment #8874563 - Attachment is obsolete: true
This requires support for multi-sized image containers being added in bug 1368776. This is particularly important for vector images because not only can they be downscaled, they can also be upscaled, and without that, they may look blurry (because they were rendered at their "native" size, but then the graphics code later had to upscale the rasterized version for the actual display).
See Also: → 1367987
Priority: P3 → P1
Whiteboard: gfx-noted → [wr-mvp] gfx-noted
Target Milestone: --- → mozilla57
Attachment #8898890 - Attachment is obsolete: true
Attachment #8926535 - Flags: review?(tnikkel)
Comment on attachment 8926534 [details] [diff] [review]
Part 1. VectorImage::GetFrameAtSize should not create a DrawTarget if using cached surface., v8

>@@ -746,33 +746,46 @@ VectorImage::GetFrameAtSize(const IntSize& aSize,
>+  SVGDrawingParameters params(context, aSize, ImageRegion::Create(aSize),
>+                              SamplingFilter::POINT, Nothing(),
>+                              mSVGDocumentWrapper->GetCurrentTime(),
>+                              aFlags, 1.0);

Draw uses 0.0f as the time if the first frame is requested, shouldn't we also?

The fact that we pass in Nothing for the svg context smells like a bug here. Draw uses the svg context to determine if we should use context painting. The fact that we don't have an svg context here suggests that if we layerize vector images with context painting we will break them.
Attachment #8926535 - Flags: review?(tnikkel) → review+
This had a very subtle bug where the SVGDrawingParameters used in GetFrameInternal used "Nothing()" directly for its constructor / Maybe<SVGImageContext> parameter. The problem being that SVGDrawingParameters stores it as a reference instead of making a copy. On debug builds no problem, on optimized, crash. Part 7 will change this again.
Attachment #8926537 - Attachment is obsolete: true
Attachment #8926537 - Flags: review?(tnikkel)
Attachment #8928657 - Flags: review?(tnikkel)
SVGImageContext is sort of important for SVG. This refactors GetImageContainerAtSize to add said parameter so that callers can pass it in to get the container they really expect to get.
Blocks: 1417624
Blocks: 1337552
Comment on attachment 8928658 [details] [diff] [review]
Part 7. Add an SVGImageContext parameter to imgIContainer::GetImageContainerAtSize., v1

Don't we need the svg context for all ways that we can't get image data then? GetFrame, GetFrameAtSize, and GetImageContainer?
Attachment #8926534 - Flags: review?(tnikkel)
It shouldn't give a container for animated SVGs. Yet anyways.
Attachment #8926538 - Attachment is obsolete: true
Attachment #8926538 - Flags: review?(tnikkel)
Make this work as intended -- it was marking surfaces as shared on the image container code path (doh!). Also it wasn't always returning the cached surface (the one that could be a SourceSurfaceSharedData) and instead was using DrawTarget::Snapshot(). There will need to be *another* change to make imgFrame::InitWithDrawable to always use a shared surface (as of right now it only does so on Android and Mac OS X due to the gfxPlatform::CanRenderContentToDataSurface call).
Attachment #8926539 - Attachment is obsolete: true
Attachment #8926539 - Flags: review?(tnikkel)
Rebased.

(In reply to Timothy Nikkel (:tnikkel) from comment #61)
> Comment on attachment 8928658 [details] [diff] [review]
> Part 7. Add an SVGImageContext parameter to
> imgIContainer::GetImageContainerAtSize., v1
> 
> Don't we need the svg context for all ways that we can't get image data
> then? GetFrame, GetFrameAtSize, and GetImageContainer?

For the moment, I'm happy to deprecate those APIs. The WebRender path only requires GetImageContainerAtSize as far as I know, at this very moment. To see how we are using these new APIs, check out the patches in bug 1183378.
Attachment #8928658 - Attachment is obsolete: true
The SVGImageContext is actually restricted, so we need to abstract out the checks from Draw into its own function that can be shared with GetFrameInternal.
In the RasterImage image container refactoring, we already moved the NotifyDrawingObservers API to a shared location. Let's actually use that here.
Update this so that it masks off FLAG_FORCE_PRESERVEASPECTRATIO_NONE before passing to GetImageContainerImpl. This is important because it will still match an image container with a new SVG context after applying the flag as another call with the same SVG context (after application) without the flag.
Attachment #8929115 - Attachment is obsolete: true
I was thinking this might help debug the remaining failures but thus far, no dice. Making the SVG context info match still results in different results from VectorImage::GetImageContainerAtSize vs Draw. Still useful stuff for the future probably.
(In reply to Timothy Nikkel (:tnikkel) from comment #11)
> Comment on attachment 8872591 [details] [diff] [review]
> Part 1. VectorImage::GetFrameAtSize should not create a DrawTarget if using
> cached surface., v2
> 
> >+VectorImage::DrawInternal(const SVGDrawingParameters& aParams,
> 
> >   // Set context paint (if specified) on the document:
> >   Maybe<AutoSetRestoreSVGContextPaint> autoContextPaint;
> >-  if (haveContextPaint && !blockContextPaint) {
> >-    autoContextPaint.emplace(aSVGContext->GetContextPaint(),
> >+  if (aContextPaint) {
> >+    autoContextPaint.emplace(aParams.svgContext->GetContextPaint(),
> >                              mSVGDocumentWrapper->GetDocument());
> 
> This actually changes us from using aSVGContext always to the new svg
> context if it exists.

Hm. But the new SVG context is a copy of the old one with the ContextPaint filtered out if blockContextPaint == true. It shouldn't have a behaviour change.
Fix how GetFrameAtSize should pass Nothing() to the SVGDrawingParameters, as it keeps a reference, not a copy. Instead put SVGImageContext on the stack and take a reference to *that*.
Attachment #8926534 - Attachment is obsolete: true
Attachment #8929246 - Flags: review?(tnikkel)
Attachment #8929105 - Flags: review?(tnikkel)
Attachment #8929107 - Flags: review?(tnikkel)
Attachment #8929113 - Flags: review?(tnikkel)
Attachment #8929114 - Flags: review?(tnikkel)
Attachment #8929116 - Flags: review?(tnikkel)
Attachment #8929132 - Flags: review?(tnikkel)
Attachment #8929149 - Flags: review?(tnikkel)
(In reply to Timothy Nikkel (:tnikkel) from comment #58)
> Comment on attachment 8926534 [details] [diff] [review]
> Part 1. VectorImage::GetFrameAtSize should not create a DrawTarget if using
> cached surface., v8
> 
> >@@ -746,33 +746,46 @@ VectorImage::GetFrameAtSize(const IntSize& aSize,
> >+  SVGDrawingParameters params(context, aSize, ImageRegion::Create(aSize),
> >+                              SamplingFilter::POINT, Nothing(),
> >+                              mSVGDocumentWrapper->GetCurrentTime(),
> >+                              aFlags, 1.0);
> 
> Draw uses 0.0f as the time if the first frame is requested, shouldn't we
> also?
> 

Fixed.

> The fact that we pass in Nothing for the svg context smells like a bug here.
> Draw uses the svg context to determine if we should use context painting.
> The fact that we don't have an svg context here suggests that if we layerize
> vector images with context painting we will break them.

You are correct. Parts 7 and 8 should address this.
Attachment #8929248 - Flags: review?(tnikkel)
Attachment #8929248 - Attachment description: 0018-Bug-1366097-Part-1.-VectorImage-GetFrameAtSize-shoul.patch → Part 1. VectorImage::GetFrameAtSize should not create a DrawTarget if using cached surface., v10
Attachment #8929246 - Attachment is obsolete: true
Attachment #8929246 - Flags: review?(tnikkel)
Rebase.
Attachment #8929105 - Attachment is obsolete: true
Attachment #8929105 - Flags: review?(tnikkel)
Attachment #8929249 - Flags: review?(tnikkel)
Too many patches in my queue. Sigh. *Real* patch this time.
Attachment #8929249 - Attachment is obsolete: true
Attachment #8929249 - Flags: review?(tnikkel)
Attachment #8929250 - Flags: review?(tnikkel)
Attachment #8929113 - Attachment is obsolete: true
Attachment #8929113 - Flags: review?(tnikkel)
Attachment #8929251 - Flags: review?(tnikkel)
Attachment #8929248 - Flags: review?(tnikkel) → review+
Rebase.
Attachment #8929114 - Attachment is obsolete: true
Attachment #8929114 - Flags: review?(tnikkel)
Attachment #8929252 - Flags: review?(tnikkel)
Comment on attachment 8926536 [details] [diff] [review]
Part 3. Make VectorImage use ImageResource::mHasSize and mSize to cache its size information., v8

Talked to Andrews, the plan is to skip this patch as it doesn't seem important.
Attachment #8926536 - Flags: review?(tnikkel)
Comment on attachment 8929250 [details] [diff] [review]
Part 4. Implement VectorImage::GetFrameInternal as required by ImageResource for image containers., v12

Can we keep the RecoverFromLostSurfaces call in LookupCacheSurface so all callers don't have to repeat it?
Attachment #8929250 - Flags: review?(tnikkel) → review+
Attachment #8929107 - Flags: review?(tnikkel) → review+
Comment on attachment 8929251 [details] [diff] [review]
Part 6. Mark VectorImage as surface as shared when used outside an image layer., v10

It's quite confusing to call MarkSurfaceShared to say that the surface isn't going to be shared.
Attachment #8929251 - Flags: review?(tnikkel) → review+
Attachment #8929252 - Flags: review?(tnikkel) → review+
(In reply to Andrew Osmond [:aosmond] from comment #65)
> (In reply to Timothy Nikkel (:tnikkel) from comment #61)
> > Comment on attachment 8928658 [details] [diff] [review]
> > Part 7. Add an SVGImageContext parameter to
> > imgIContainer::GetImageContainerAtSize., v1
> > 
> > Don't we need the svg context for all ways that we can't get image data
> > then? GetFrame, GetFrameAtSize, and GetImageContainer?
> 
> For the moment, I'm happy to deprecate those APIs. The WebRender path only
> requires GetImageContainerAtSize as far as I know, at this very moment. To
> see how we are using these new APIs, check out the patches in bug 1183378.

Okay, so either we need a bug filed to either convert or deprecate these functions.
Attachment #8929253 - Flags: review?(tnikkel) → review+
Attachment #8929116 - Flags: review?(tnikkel) → review+
Attachment #8929149 - Flags: review?(tnikkel) → review+
(In reply to Timothy Nikkel (:tnikkel) from comment #79)
> Comment on attachment 8929250 [details] [diff] [review]
> Part 4. Implement VectorImage::GetFrameInternal as required by ImageResource
> for image containers., v12
> 
> Can we keep the RecoverFromLostSurfaces call in LookupCacheSurface so all
> callers don't have to repeat it?

I can do one better. I'm sure changed the prototype of LookupCacheSurface made sense at one point, but now I've reverted it back because it added no value as this currently stand.
Attachment #8929250 - Attachment is obsolete: true
Attachment #8929547 - Flags: review+
Attachment #8926535 - Attachment is obsolete: true
Attachment #8926536 - Attachment is obsolete: true
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb77f75f95d9
Part 1. VectorImage::GetFrameAtSize should not create a DrawTarget if using cached surface. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/12fc7f8b8930
Part 2. Implement VectorImage::GetFrameInternal as required by ImageResource for image containers. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4b811e0b860
Part 3. Implement VectorImage::GetImageContainerAtSize and VectorImage::IsImageContainerAvailableAtSize. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/79b165219828
Part 4. Mark VectorImage as surface as shared when used outside an image layer. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/54d6ad659f11
Part 5. Add an SVGImageContext parameter to imgIContainer::GetImageContainerAtSize. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/e65c223336d9
Part 6. Restrict use of SVGImageContext in VectorImage::GetImageContainerAtSize to permitted URIs. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/71c36630c141
Part 7. VectorImage::Show should delegate to ImageResource::NotifyDrawingObservers. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/3395c151dabb
Part 8. Improve image memory reports to include SVG context information. r=tnikkel
Target Milestone: mozilla57 → mozilla59
You need to log in before you can comment on or make changes to this bug.