The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 14

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

unspecified
mozilla15
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14 fixed, blocking-fennec1.0 beta+)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
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
(Assignee)

Updated

5 years ago
Blocks: 748922
Assignee: nobody → jmuizelaar
blocking-fennec1.0: --- → beta+
(Assignee)

Comment 1

5 years ago
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
Attachment #620051 - Flags: review?(roc)
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?
Attachment #620051 - Flags: review?(roc) → review-
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
Created attachment 620111 [details] [diff] [review]
version 2 - simplified
Attachment #620051 - Attachment is obsolete: true
Attachment #620111 - Flags: review?(roc)
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
(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.
(Assignee)

Comment 7

5 years ago
(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?
(Assignee)

Comment 8

5 years ago
Created attachment 620120 [details] [diff] [review]
v3 - with the last parameter rename
Attachment #620111 - Attachment is obsolete: true
Attachment #620111 - Flags: review?(roc)
Attachment #620120 - Flags: review?(roc)
(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 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.
(Assignee)

Comment 11

5 years ago
(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.
(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.
(Assignee)

Comment 13

5 years ago
Created attachment 620140 [details] [diff] [review]
v4 - more removals
Attachment #620120 - Attachment is obsolete: true
Attachment #620120 - Flags: review?(roc)
Attachment #620140 - Flags: review?(roc)
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.
(Assignee)

Updated

5 years ago
Attachment #620140 - Flags: review?(roc)
(Assignee)

Comment 15

5 years ago
Created attachment 620141 [details] [diff] [review]
v5
Attachment #620140 - Attachment is obsolete: true
Attachment #620141 - Flags: review?(roc)
(Assignee)

Comment 16

5 years ago
Created attachment 620142 [details] [diff] [review]
v6
Attachment #620141 - Attachment is obsolete: true
Attachment #620141 - Flags: review?(roc)
Attachment #620142 - Flags: review?(roc)
Attachment #620142 - Flags: review?(roc) → review+

Updated

5 years ago
Whiteboard: [has reviewed patch]
(Assignee)

Comment 17

5 years ago
I'm running into a reftest failure that's keeping this from landing.
(Assignee)

Updated

5 years ago
Depends on: 751668
(Assignee)

Comment 18

5 years ago
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
Target Milestone: --- → mozilla15
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
Target Milestone: mozilla15 → ---
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

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

Updated

5 years ago
Whiteboard: [has reviewed patch] → [inboud]

Updated

5 years ago
Whiteboard: [inboud] → [inbound]
(Assignee)

Comment 22

5 years ago
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
Attachment #620142 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/4c6759dcecd3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Backed out: https://hg.mozilla.org/mozilla-central/rev/6a9a1e259aeb
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla15 → ---
https://hg.mozilla.org/mozilla-central/rev/b94e64ad7e76
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Attachment #620142 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 26

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/99152272ca3e
status-firefox14: --- → fixed
Depends on: 811675
You need to log in before you can comment on or make changes to this bug.