Last Comment Bug 750598 - Use better heuristics for when to use linear vs nearest when drawing images
: Use better heuristics for when to use linear vs nearest when drawing images
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Jeff Muizelaar [:jrmuizel]
:
Mentors:
Depends on: 751668 811675
Blocks: pixelated
  Show dependency treegraph
 
Reported: 2012-04-30 21:08 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2012-11-14 10:00 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
beta+


Attachments
Add some heuristics to catch cases when we can use nearest (9.03 KB, patch)
2012-05-01 13:42 PDT, Jeff Muizelaar [:jrmuizel]
roc: review-
Details | Diff | Review
version 2 - simplified (7.07 KB, patch)
2012-05-01 15:56 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Review
v3 - with the last parameter rename (7.08 KB, patch)
2012-05-01 16:11 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Review
v4 - more removals (6.44 KB, patch)
2012-05-01 16:57 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Review
v5 (5.98 KB, patch)
2012-05-01 17:03 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Review
v6 (5.98 KB, patch)
2012-05-01 17:05 PDT, Jeff Muizelaar [:jrmuizel]
roc: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Review

Description Jeff Muizelaar [:jrmuizel] 2012-04-30 21:08:45 PDT
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] 2012-05-01 13:42:46 PDT
Created attachment 620051 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2012-05-01 14:51:36 PDT
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] 2012-05-01 15:40:32 PDT
(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] 2012-05-01 15:56:09 PDT
Created attachment 620111 [details] [diff] [review]
version 2 - simplified
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-01 16:01:21 PDT
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) (Exited; email my personal email if necessary) 2012-05-01 16:03:18 PDT
(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] 2012-05-01 16:10:52 PDT
(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] 2012-05-01 16:11:57 PDT
Created attachment 620120 [details] [diff] [review]
v3 - with the last parameter rename
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-01 16:14:07 PDT
(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) (Exited; email my personal email if necessary) 2012-05-01 16:20:05 PDT
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] 2012-05-01 16:53:07 PDT
(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) (Exited; email my personal email if necessary) 2012-05-01 16:55:37 PDT
(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] 2012-05-01 16:57:03 PDT
Created attachment 620140 [details] [diff] [review]
v4 - more removals
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-01 16:58:47 PDT
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] 2012-05-01 17:03:15 PDT
Created attachment 620141 [details] [diff] [review]
v5
Comment 16 Jeff Muizelaar [:jrmuizel] 2012-05-01 17:05:22 PDT
Created attachment 620142 [details] [diff] [review]
v6
Comment 17 Jeff Muizelaar [:jrmuizel] 2012-05-02 14:46:10 PDT
I'm running into a reftest failure that's keeping this from landing.
Comment 18 Jeff Muizelaar [:jrmuizel] 2012-05-03 22:42:41 PDT
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 20 Ed Morley [:emorley] 2012-05-04 04:39:29 PDT
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] 2012-05-04 06:20:12 PDT
Jeff appears to have re-landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b94e64ad7e76
Comment 22 Jeff Muizelaar [:jrmuizel] 2012-05-04 10:38:25 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2012-05-04 13:34:16 PDT
https://hg.mozilla.org/mozilla-central/rev/4c6759dcecd3
Comment 24 :Ehsan Akhgari (busy, don't ask for review please) 2012-05-04 13:37:26 PDT
Backed out: https://hg.mozilla.org/mozilla-central/rev/6a9a1e259aeb
Comment 25 :Ehsan Akhgari (busy, don't ask for review please) 2012-05-04 13:38:44 PDT
https://hg.mozilla.org/mozilla-central/rev/b94e64ad7e76
Comment 26 Jeff Muizelaar [:jrmuizel] 2012-05-07 14:19:18 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/99152272ca3e

Note You need to log in before you can comment on or make changes to this bug.