Factor-of-2 scaling or similar for vector images

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
10 months ago

People

(Reporter: aosmond, Assigned: aosmond)

Tracking

unspecified
mozilla64
All
Unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(3 attachments, 4 obsolete attachments)

Scrolling through likes like http://makeyourmoneymatter.org/ can cause us to rasterize SVGs at many similar sizes, spending an awful amount of cycles to do so. It would be good to make this both look good, and be fast. It was not done with factor-of-2 downscale limiting like with raster images because vector images may not have a rational aspect ratio. But something similar should be considered.
Priority: -- → P3
See Also: → 1370412
Whiteboard: [gfx-noted]
Here's a test case that helps to show of different browser's behaviour: https://jrmuizel.github.io/implementation-tests/svg-image-scale-animation.html
Dusting off my old patch and testing with this today reveals a few problems:

1) We won't allocate surfaces larger than the texture size. We won't fallback either. It uses TEMPORARY_ERROR, so it attempts to recreate it every time. This will manifest as a missing image. This needs to be fixed separately.

2) Using factor of 2 scaling helps with the test case in comment 1, but it is not sufficient. This is because uploading the tiles is quite expensive and we do it on every loop of zooming in on the SVG. There is a quite noticeable pause while the uploads happen. Forcing the texture cache to be bigger, and stop evicting the tiles, makes it go smoother, but we are still a little slower compared to Chrome.

3) Chrome takes a different approach. It has a maximum size it rasterizes the SVG to, and it is just upscaled from thereon. It doesn't look as crisp as Firefox in this case, but the animation is so much smoother, you barely even notice.
Also,

4) The first pass is always even worse than the subsequent passes. Presumably this is because it is doing the rasterization of the SVG on the main thread. It would be nice if we could take a page from normal raster images, always do a recording of SVGs on the main thread, do the paint on the image decoder threads, and allow substitutions so that we can up/downscale a fully rasterized version while we wait for the image decoder threads to complete their tasks. This has an advantage over OMTP because it won't hold up the generation of the frame on the "perfect" SVG rasterization.
The only thing missing here that I think we would like to do is actually switch to fallback / refuse to rasterize a surface if we are above the gfxPrefs::ImageCacheFactor2MaxUpscaleSize() threshold instead of just stopping scaling. That would prevent things from looking blurry if we zoom in a lot.
Assignee: nobody → aosmond
Whoops, small mistake, broke WR.
Attachment #9008468 - Attachment is obsolete: true
(In reply to Andrew Osmond [:aosmond] from comment #4)
> Created attachment 9008468 [details] [diff] [review]
> 0001-Bug-1456558-WIP.patch, v1
> 
> The only thing missing here that I think we would like to do is actually
> switch to fallback / refuse to rasterize a surface if we are above the
> gfxPrefs::ImageCacheFactor2MaxUpscaleSize() threshold instead of just
> stopping scaling. That would prevent things from looking blurry if we zoom
> in a lot.

After talking with gankro, it seems blob image tiling isn't work, so fallback might not work as well as I had hoped. I believe what I have attached to the bug is good enough to ship however.
Attachment #9010023 - Flags: review?(tnikkel)
Attachment #9010025 - Flags: review?(tnikkel)
Attachment #9010026 - Flags: review?(tnikkel)
Attachment #9010023 - Flags: review?(tnikkel) → review+
Comment on attachment 9010025 [details] [diff] [review]
0002-Bug-1456558-Part-2.-Implement-factor-of-2-scaling-su.patch, v1

Review of attachment 9010025 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/SurfaceCache.cpp
@@ +444,3 @@
>      // the native size (and w/h ratio) to provide a frame of reference to what
>      // are "good" sizes. While it is desirable to have a similar mechanism as
>      // that for raster images, it will need a different approach.

Update this comment too please.

@@ +448,5 @@
>      NotNull<CachedSurface*> current = WrapNotNull(first.UserData());
>      Image* image = static_cast<Image*>(current->GetImageKey());
>      size_t nativeSizes = image->GetNativeSizesLength();
>      if (nativeSizes == 0) {
> +      thresholdSurfaces += 1;

Raster images that are in error and dynamic images still return 0 for GetNativeSizesLength, so we probably only want to increment thresholdSurfaces for vector images. And still return if GetNativeSizesLength is zero for non-vector images.

@@ +538,5 @@
> +
> +    // Whether or not we are in factor of 2 mode, vector image rasterization is
> +    // clamped at a configured maximum if the caller is willing to accept
> +    // substitutes.
> +    if (mFactor2MatchAR) {

So mFactor2MatchAR just means "this is a vector image" as this has nothing to do with aspect ratio, we should just rename it.

@@ +581,3 @@
>          NS_FAILED(image->GetHeight(&factorSize.height)) ||
>          factorSize.IsEmpty()) {
>        // We should not have entered factor of 2 mode without a valid size, and

Note in this comment that this will need to be changed to deal with vector images with no size.

@@ +593,5 @@
> +      //
> +      //     delta = nativeWidth/nativeHeight - desiredWidth/desiredHeight
> +      //
> +      //     delta*nativeHeight*desiredHeight = nativeWidth*desiredHeight
> +      //                                      - desiredHeight*nativeHeight

desiredHeight should be desiredWidth
Attachment #9010025 - Flags: review?(tnikkel)
Comment on attachment 9010026 [details] [diff] [review]
0003-Bug-1456558-Part-3.-Implement-factor-of-2-scaling-su.patch, v1

Review of attachment 9010026 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/SVGDrawingParameters.h
@@ +51,5 @@
>      }
>    }
>  
>    gfxContext*                   context;
> +  IntSize                       size; // Size to prepare a surface at.

prepare as in rasterize?

::: image/VectorImage.cpp
@@ +817,3 @@
>      LookupCachedSurface(aSize, aSVGContext, aFlags);
>    if (sourceSurface) {
> +    return MakeTuple(ImgDrawResult::SUCCESS, decodeSize, std::move(sourceSurface));

It's hard to review these changes because there doesn't seem to be any description of what GetFrameInternal is supposed to be returning, specifically in the size part of the tuple.

@@ +1136,3 @@
>  
> +  IntSize decodeSize = result.SuggestedSize().IsEmpty()
> +                       ? aSize : result.SuggestedSize();

It's hard to review if this line is correct because the comment for mSuggestedSize in LookupResult don't tell us when it is empty.

Based on this function it seems as though it is "mSuggestedSize will be the size of the returned surface if the result is SUBSTITUTE_BECAUSE_BEST. It will be empty for EXACT, and can contain a non-empty size possibly different from the returned surface (if any) for all other results".

::: image/VectorImage.h
@@ +92,5 @@
>                            const IntSize& aSize,
>                            uint32_t aFlags) override;
>  
>    /// Attempt to find a matching cached surface in the SurfaceCache.
> +  Tuple<RefPtr<SourceSurface>, IntSize>

What does the size we are returning represent? Add a comment.
Attachment #9010026 - Flags: review?(tnikkel)
(In reply to Timothy Nikkel (:tnikkel) from comment #10)
> Comment on attachment 9010025 [details] [diff] [review]
> 0002-Bug-1456558-Part-2.-Implement-factor-of-2-scaling-su.patch, v1
> 
> Review of attachment 9010025 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/SurfaceCache.cpp
> @@ +444,3 @@
> >      // the native size (and w/h ratio) to provide a frame of reference to what
> >      // are "good" sizes. While it is desirable to have a similar mechanism as
> >      // that for raster images, it will need a different approach.
> 
> Update this comment too please.
> 

Done.

> @@ +448,5 @@
> >      NotNull<CachedSurface*> current = WrapNotNull(first.UserData());
> >      Image* image = static_cast<Image*>(current->GetImageKey());
> >      size_t nativeSizes = image->GetNativeSizesLength();
> >      if (nativeSizes == 0) {
> > +      thresholdSurfaces += 1;
> 
> Raster images that are in error and dynamic images still return 0 for
> GetNativeSizesLength, so we probably only want to increment
> thresholdSurfaces for vector images. And still return if
> GetNativeSizesLength is zero for non-vector images.
> 

Done.

> @@ +538,5 @@
> > +
> > +    // Whether or not we are in factor of 2 mode, vector image rasterization is
> > +    // clamped at a configured maximum if the caller is willing to accept
> > +    // substitutes.
> > +    if (mFactor2MatchAR) {
> 
> So mFactor2MatchAR just means "this is a vector image" as this has nothing
> to do with aspect ratio, we should just rename it.
> 

Renamed to mIsVectorImage.

> @@ +581,3 @@
> >          NS_FAILED(image->GetHeight(&factorSize.height)) ||
> >          factorSize.IsEmpty()) {
> >        // We should not have entered factor of 2 mode without a valid size, and
> 
> Note in this comment that this will need to be changed to deal with vector
> images with no size.
> 

Done.

> @@ +593,5 @@
> > +      //
> > +      //     delta = nativeWidth/nativeHeight - desiredWidth/desiredHeight
> > +      //
> > +      //     delta*nativeHeight*desiredHeight = nativeWidth*desiredHeight
> > +      //                                      - desiredHeight*nativeHeight
> 
> desiredHeight should be desiredWidth

Done.
Attachment #9010025 - Attachment is obsolete: true
Attachment #9010737 - Flags: review?(tnikkel)
(In reply to Timothy Nikkel (:tnikkel) from comment #11)
> Comment on attachment 9010026 [details] [diff] [review]
> 0003-Bug-1456558-Part-3.-Implement-factor-of-2-scaling-su.patch, v1
> 
> Review of attachment 9010026 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/SVGDrawingParameters.h
> @@ +51,5 @@
> >      }
> >    }
> >  
> >    gfxContext*                   context;
> > +  IntSize                       size; // Size to prepare a surface at.
> 
> prepare as in rasterize?
> 

Yes, done.

> ::: image/VectorImage.cpp
> @@ +817,3 @@
> >      LookupCachedSurface(aSize, aSVGContext, aFlags);
> >    if (sourceSurface) {
> > +    return MakeTuple(ImgDrawResult::SUCCESS, decodeSize, std::move(sourceSurface));
> 
> It's hard to review these changes because there doesn't seem to be any
> description of what GetFrameInternal is supposed to be returning,
> specifically in the size part of the tuple.
> 

I documented GetFrameInternal. Hopefully it is clearer. The size returned is more descriptive than anything else. The rationale is that we want to share the same ImageContainer for many different sizes, if they end up producing the same best fit result.

> @@ +1136,3 @@
> >  
> > +  IntSize decodeSize = result.SuggestedSize().IsEmpty()
> > +                       ? aSize : result.SuggestedSize();
> 
> It's hard to review if this line is correct because the comment for
> mSuggestedSize in LookupResult don't tell us when it is empty.
> 
> Based on this function it seems as though it is "mSuggestedSize will be the
> size of the returned surface if the result is SUBSTITUTE_BECAUSE_BEST. It
> will be empty for EXACT, and can contain a non-empty size possibly different
> from the returned surface (if any) for all other results".
> 

Your understanding is correct :). I incorporated that into the comment for mSuggestedSize.

> ::: image/VectorImage.h
> @@ +92,5 @@
> >                            const IntSize& aSize,
> >                            uint32_t aFlags) override;
> >  
> >    /// Attempt to find a matching cached surface in the SurfaceCache.
> > +  Tuple<RefPtr<SourceSurface>, IntSize>
> 
> What does the size we are returning represent? Add a comment.

Done.
Attachment #9010026 - Attachment is obsolete: true
Attachment #9010738 - Flags: review?(tnikkel)
Attachment #9010737 - Flags: review?(tnikkel) → review+
Attachment #9010738 - Flags: review?(tnikkel) → review+
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f209a9d848f4
Part 1. Move the allowed raster size calculation from imgFrame to SurfaceCache. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/af9fc3daf97c
Part 2. Implement factor of 2 scaling support for SVGs in the surface cache. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/70d8f11cf6e8
Part 3. Implement factor of 2 scaling support for SVGs in VectorImage. r=tnikkel
(In reply to Andreea Pavel [:apavel] from comment #15)
> Backed out for crashtest assertion failres
> 
> Push with failures: 
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&resultStatus=testfailed,busted,
> exception&classifiedState=unclassified&group_state=expanded&selectedJob=20058
> 2510&revision=70d8f11cf6e8278b8e1509a69bd02adc0168e606
> 
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=200582510&repo=mozilla-
> inbound
> 
> Backout:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> fac7810a8f317798d8d78e6cdd4034bf18385001

Hmm, this is failing for RasterImages. I guess you can ask for an illegal size of an image, if the native image size itself is illegal. This can't happen on the VectorImage path, since I added an explicit check for it in VectorImage::UseSurfaceCacheForSize. I guess I can move the assert to the if clause just below it.
Flags: needinfo?(aosmond)
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55c2dd9bdb34
Part 1. Move the allowed raster size calculation from imgFrame to SurfaceCache. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/9884e2d81cbe
Part 2. Implement factor of 2 scaling support for SVGs in the surface cache. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e53e6af4223
Part 3. Implement factor of 2 scaling support for SVGs in VectorImage. r=tnikkel
https://hg.mozilla.org/mozilla-central/rev/55c2dd9bdb34
https://hg.mozilla.org/mozilla-central/rev/9884e2d81cbe
https://hg.mozilla.org/mozilla-central/rev/1e53e6af4223
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.