750598 - Use better heuristics for when to use linear vs nearest when drawing images

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

WebKit has a bunch of heuristics it uses when choosing a filtering mode. We should do something similar. These should let us take care of the cnn.com backgrounds with nearest without impacting other more noticeable places.

http://git.chromium.org/gitweb/?p=external/Webkit.git;a=blob;f=Source/WebCore/platform/graphics/skia/ImageSkia.cpp;h=4732d1d22c7f53dc06fe44ad5e760ffdd25d7b11;hb=HEAD#l73

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add some heuristics to catch cases when we can use nearest

The heuristics in WebKit ended up being weaker than I originally though. For example, they don't catch cnn tiled images. I've added an additional case that will catch these tiled images on cnn.com

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 620051 [details] [diff] [review]
Add some heuristics to catch cases when we can use nearest

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

gfxPattern.h changes missing?

I'm not sure it's a good idea to slavishly copy the Webkit code here, especially all these commented-out fragments. Maybe just provide a reference to the original code and rewrite this to be as simple as possible?

::: gfx/thebes/gfxUtils.cpp
@@ +418,5 @@
> +#ifdef MOZ_GFX_OPTIMIZE_MOBILE
> +static gfxPattern::GraphicsFilter reduceResamplingFilter(gfxPattern::GraphicsFilter filter,
> +                                                         int imgWidth, int imgHeight,
> +                                                         float sourceWidth, float sourceHeight,
> +                                                         float destWidth, float destHeight)

Follow naming conventions

@@ +436,5 @@
> +        }
> +    */
> +
> +    int destIWidth = static_cast<int>(destWidth);
> +    int destIHeight = static_cast<int>(destHeight);

Shouldn't these be the size in device coordinates, after applying the CTM? Currently it looks like you're using user-space size.

Maybe some heuristics want the device-space size and others the user-space size?

Comment 3

[Jeff Muizelaar [:jrmuizel]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> Comment on attachment 620051 [details] [diff] [review]
> Add some heuristics to catch cases when we can use nearest
> 
> Review of attachment 620051 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> gfxPattern.h changes missing?

I didn't change anything in gfxPattern.h

> I'm not sure it's a good idea to slavishly copy the Webkit code here,
> especially all these commented-out fragments. Maybe just provide a reference
> to the original code and rewrite this to be as simple as possible?

I originally thought the heuristics were going to be more useful than they are and I would be able to use the Webkit code mostly unchanged. I'm ok with doing a minimal version.

> @@ +436,5 @@
> > +        }
> > +    */
> > +
> > +    int destIWidth = static_cast<int>(destWidth);
> > +    int destIHeight = static_cast<int>(destHeight);
> 
> Shouldn't these be the size in device coordinates, after applying the CTM?
> Currently it looks like you're using user-space size.

Isn't our CTM always Identity when this function is called? (DrawImageInternal & nsLayoutUtils::DrawPixelSnapped)

> Maybe some heuristics want the device-space size and others the user-space
> size?

This is probably true, however, I may just take out the ones that want the userspace size.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Attachment: version 2 - simplified

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 620111 [details] [diff] [review]
version 2 - simplified

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

::: gfx/thebes/gfxUtils.cpp
@@ +415,5 @@
>  }
>  
> +/* These heuristics are based on Source/WebCore/platform/graphics/skia/ImageSkia.cpp:computeResamplingMode() */
> +#ifdef MOZ_GFX_OPTIMIZE_MOBILE
> +static gfxPattern::GraphicsFilter reduceResamplingFilter(gfxPattern::GraphicsFilter filter,

names still need fixing here

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> > Shouldn't these be the size in device coordinates, after applying the CTM?
> > Currently it looks like you're using user-space size.
> 
> Isn't our CTM always Identity when this function is called?
> (DrawImageInternal & nsLayoutUtils::DrawPixelSnapped)

No. Anyway what really matters is that aFill is in user space.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #3)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> > > Shouldn't these be the size in device coordinates, after applying the CTM?
> > > Currently it looks like you're using user-space size.
> > 
> > Isn't our CTM always Identity when this function is called?
> > (DrawImageInternal & nsLayoutUtils::DrawPixelSnapped)
> 
> No. Anyway what really matters is that aFill is in user space.

When isn't our CTM identity?

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Attachment: v3 - with the last parameter rename

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> When isn't our CTM identity?

When we're rendering non-active CSS transforms, for example.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 620120 [details] [diff] [review]
v3 - with the last parameter rename

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

::: gfx/thebes/gfxUtils.cpp
@@ +432,5 @@
> +
> +    // The percent change below which we will not resample. This usually means
> +    // an off-by-one error on the web page, and just doing nearest neighbor
> +    // sampling is usually good enough.
> +    const float kFractionalChangeThreshold = 0.025f;

You're not using this -- remove it?

I think we should probably do this optimization though, given SkiaImage thinks it's worth doing.

@@ +448,5 @@
> +    // common cases where resampling won't give us anything, since it is much
> +    // slower than drawing stretched.
> +    if (aImgWidth == destIWidth && aImgHeight == destIHeight) {
> +        // We don't need to resample if the source and destination are the same.
> +        return gfxPattern::FILTER_NEAREST;

This check should definitely look at the device pixel size.

(Also, if the CTM is a rotation/skew, is NEAREST bad?)

@@ +458,5 @@
> +        || destIHeight <= kSmallImageSizeThreshold) {
> +        // Never resample small images. These are often used for borders and
> +        // rules (think 1x1 images used to make lines).
> +        return gfxPattern::FILTER_NEAREST;
> +    }

What's the rationale for small dest sizes here? A large image resized to a small destination doesn't match the justification in the comment.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> Comment on attachment 620120 [details] [diff] [review]
> v3 - with the last parameter rename
> 
> Review of attachment 620120 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxUtils.cpp
> @@ +432,5 @@
> > +
> > +    // The percent change below which we will not resample. This usually means
> > +    // an off-by-one error on the web page, and just doing nearest neighbor
> > +    // sampling is usually good enough.
> > +    const float kFractionalChangeThreshold = 0.025f;
> 
> You're not using this -- remove it?
> 
> I think we should probably do this optimization though, given SkiaImage
> thinks it's worth doing.

Afaict, this optimization is more for keeping them doing a high quality resample instead of not using Linear. i.e. if someone has <img width=99> when the actual image size is 100, they will use linear instead of higher quality. Otherwise the reduction only happens for backgrounds which I think are unlikely to be slightly mis-sized. 

> @@ +448,5 @@
> > +    // common cases where resampling won't give us anything, since it is much
> > +    // slower than drawing stretched.
> > +    if (aImgWidth == destIWidth && aImgHeight == destIHeight) {
> > +        // We don't need to resample if the source and destination are the same.
> > +        return gfxPattern::FILTER_NEAREST;
> 
> This check should definitely look at the device pixel size.

Ah true. I think I'll just remove this check, if the sizes do align properly pixman will do the right thing anyways.

> (Also, if the CTM is a rotation/skew, is NEAREST bad?)
> 
> @@ +458,5 @@
> > +        || destIHeight <= kSmallImageSizeThreshold) {
> > +        // Never resample small images. These are often used for borders and
> > +        // rules (think 1x1 images used to make lines).
> > +        return gfxPattern::FILTER_NEAREST;
> > +    }
> 
> What's the rationale for small dest sizes here? A large image resized to a
> small destination doesn't match the justification in the comment.

It's true the rational doesn't match but I think the behaviour still makes sense.
If you're drawing to a very small destination it's unlikely you need anything smoothly interpolated. That being said, I'm not sure it's a very common case.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #11)
> Afaict, this optimization is more for keeping them doing a high quality
> resample instead of not using Linear. i.e. if someone has <img width=99>
> when the actual image size is 100, they will use linear instead of higher
> quality. Otherwise the reduction only happens for backgrounds which I think
> are unlikely to be slightly mis-sized.

OK.

> > @@ +458,5 @@
> > > +        || destIHeight <= kSmallImageSizeThreshold) {
> > > +        // Never resample small images. These are often used for borders and
> > > +        // rules (think 1x1 images used to make lines).
> > > +        return gfxPattern::FILTER_NEAREST;
> > > +    }
> > 
> > What's the rationale for small dest sizes here? A large image resized to a
> > small destination doesn't match the justification in the comment.
> 
> It's true the rational doesn't match but I think the behaviour still makes
> sense.
> If you're drawing to a very small destination it's unlikely you need
> anything smoothly interpolated. That being said, I'm not sure it's a very
> common case.

OK. I could go either way. If you keep that check, then it should check the device-space size, and you should fix the comment or add another one.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Attachment: v4 - more removals

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 620140 [details] [diff] [review]
v4 - more removals

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

::: gfx/thebes/gfxUtils.cpp
@@ +415,5 @@
>  }
>  
> +/* These heuristics are based on Source/WebCore/platform/graphics/skia/ImageSkia.cpp:computeResamplingMode() */
> +#ifdef MOZ_GFX_OPTIMIZE_MOBILE
> +static gfxPattern::GraphicsFilter reduceResamplingFilter(gfxPattern::GraphicsFilter aFilter,

ReduceResamplingFilter

@@ +444,5 @@
> +        || destIWidth <= kSmallImageSizeThreshold
> +        || destIHeight <= kSmallImageSizeThreshold) {
> +        // Never resample small images. These are often used for borders and
> +        // rules (think 1x1 images used to make lines).
> +        return gfxPattern::FILTER_NEAREST;

Like I said, fix comment and make dest check in device space. Otherwise drop it.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Attachment: v5

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Attachment: v6

Comment 17

[Jeff Muizelaar [:jrmuizel]]

I'm running into a reftest failure that's keeping this from landing.

Comment 18

[Jeff Muizelaar [:jrmuizel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c6759dcecd3

This will cause a large tcheckerboard regression and a smaller tcheck2 regression.
The tcheckerboard regression comes from switching to bilinear for the background image on timecube. We catch the large background image on tcheck2 (cnn) but there are some other images that will slow down. The slow down is exagerated on tegra because we don't have NEON there. (The patch on bug 563874 does help the non-tegra case noticeably)

We'll see how bad this is in practice

Comment 19

[Ed Morley [:emorley]]

Sorry, had to back out for Android reftest-1 failures in background-size-zoom-repeat.html:

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4c6759dcecd3
eg https://tbpl.mozilla.org/php/getParsedLog.php?id=11458289&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a9a1e259aeb

Comment 20

[Ed Morley [:emorley]]

jprmc: for https://bugzilla.mozilla.org/show_bug.cgi?id=750598, does https://bugzilla.mozilla.org/show_bug.cgi?id=751668 not fix the problem?
jprmc: i believe those two patches go together

Bug 751668 landed first a few pushes prior, so the failures in comment 19 were with both together.

Comment 21

[JP Rosevear [:jpr]]

Jeff appears to have re-landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b94e64ad7e76

Comment 22

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 620142 [details] [diff] [review]
v6

[Approval Request Comment]
Regression caused by (bug #): turning off bilinear for backgrounds
User impact if declined: blocky images
Testing completed (on m-c, etc.): mozilla-inbound
Risk to taking this patch (and alternatives if risky): Only effects mobile, even there low risk the only thing that this should cause is higher quality/slower rendering of images.
String changes made by this patch: none

Note: this requires bug 751668

Comment 23

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/4c6759dcecd3

Comment 24

[(no longer active)]

Backed out: https://hg.mozilla.org/mozilla-central/rev/6a9a1e259aeb

Comment 25

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/b94e64ad7e76

Comment 26

[Jeff Muizelaar [:jrmuizel]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/99152272ca3e