1456558 - Factor-of-2 scaling or similar for vector images

Status: RESOLVED FIXED

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

Description

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

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

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

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

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0001-Bug-1456558-WIP.patch, v2

Whoops, small mistake, broke WR.

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

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

Comment 7

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0001-Bug-1456558-Part-1.-Move-the-allowed-raster-size-cal.patch, v1

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0002-Bug-1456558-Part-2.-Implement-factor-of-2-scaling-su.patch, v1

Comment 9

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0003-Bug-1456558-Part-3.-Implement-factor-of-2-scaling-su.patch, v1

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

Comment 10

[Timothy Nikkel (:tnikkel)]

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

Comment 11

[Timothy Nikkel (: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.

Comment 12

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0002-Bug-1456558-Part-2.-Implement-factor-of-2-scaling-su.patch, v2

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

Comment 13

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0003-Bug-1456558-Part-3.-Implement-factor-of-2-scaling-su.patch, v2

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Andreea Pavel [:apavel]]

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

Comment 16

[Andrew Osmond [:aosmond] (he/him)]

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

Comment 17

[Andrew Osmond [:aosmond] (he/him)]

green crashtest try by just shifting the assert location: https://treeherder.mozilla.org/#/jobs?repo=try&revision=02500ff1cdd64e4c5153aa51c9bd7be99d434277

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/55c2dd9bdb34
https://hg.mozilla.org/mozilla-central/rev/9884e2d81cbe
https://hg.mozilla.org/mozilla-central/rev/1e53e6af4223