Use nearest neighbor filtering for non-translated quads

Use nearest neighbor filtering for non-translated quads

Draw tiled layer quads that only have a translation transformation
component using nearest neighbor filtering instead of bilinear
filtering. This has two advantages:

1. On some GPUs this can reduce memory bandwidth usage due to increased
   texture cache hit rate.

2. Linear filtering is known to give slightly different results on
   different GPUs because of differences in the texture sampling
   hardware. Avoiding bilinear filtering in the common case (i.e., when
   CSS transformation aren't used) makes WebKit layout test pixel dumps
   better comparable across different devices.

TEST=ResourceProviderTest.ScopedSampler, GLRendererTest2.activeTextureState, manual testing with composited websites.

BUG=


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170944

[jamesr, 2012-11-09 18:40:09]

https://codereview.chromium.org/11358181/diff/1/cc/tiled_layer_impl.cc
File cc/tiled_layer_impl.cc (right):

https://codereview.chromium.org/11358181/diff/1/cc/tiled_layer_impl.cc#newcod...
cc/tiled_layer_impl.cc:201: const GLint textureFilter =
m_tiler->hasBorderTexels() && !drawTransform().isIdentityOrTranslation() ?
GL_LINEAR : GL_NEAREST;
for non-integer translations we still need GL_LINEAR, don't we?

https://codereview.chromium.org/11358181/diff/1/cc/tiled_layer_impl_unittest.cc
File cc/tiled_layer_impl_unittest.cc (right):

https://codereview.chromium.org/11358181/diff/1/cc/tiled_layer_impl_unittest....
cc/tiled_layer_impl_unittest.cc:22: using namespace WebKit;
using statements aren't style guide compliant (neither are the other two, but
we'll have to fix those). add using declarations for the types you need

[Sami, 2012-11-11 16:56:26]

http://codereview.chromium.org/11358181/diff/1/cc/tiled_layer_impl.cc
File cc/tiled_layer_impl.cc (right):

http://codereview.chromium.org/11358181/diff/1/cc/tiled_layer_impl.cc#newcode201
cc/tiled_layer_impl.cc:201: const GLint textureFilter =
m_tiler->hasBorderTexels() && !drawTransform().isIdentityOrTranslation() ?
GL_LINEAR : GL_NEAREST;
On 2012/11/09 18:40:09, jamesr wrote:
> for non-integer translations we still need GL_LINEAR, don't we?

Excellent point. I've changed this to use isIntegerTranslation() instead.

http://codereview.chromium.org/11358181/diff/1/cc/tiled_layer_impl_unittest.cc
File cc/tiled_layer_impl_unittest.cc (right):

http://codereview.chromium.org/11358181/diff/1/cc/tiled_layer_impl_unittest.c...
cc/tiled_layer_impl_unittest.cc:22: using namespace WebKit;
On 2012/11/09 18:40:09, jamesr wrote:
> using statements aren't style guide compliant (neither are the other two, but
> we'll have to fix those). add using declarations for the types you need

Done.

[jamesr, 2012-11-16 01:33:19]

Hmmmmmm - to cut down on texParameteri calls I've refactored this bit of code
(specifically textureFilter doesn't exist on quads any more).  This patch won't
apply any more.  To do this and avoid redundantly setting the min/mag filters
all over the place, I think you'll need to associate filters with resources
(since in GL min/mag filters are properties of the texture object) and lazily
set them.

[Sami, 2012-11-22 17:20:39]

Yeah, we kind of crossed stream there. This version adds a min/mag filter
setting to Resource and makes ResourceManager lazily manage the filter state at
draw time. Currently only TileDrawQuads get the nearest neighbor treatment.

Please let me know if you want this split into smaller pieces.

[jamesr, 2012-11-26 06:54:26]

https://codereview.chromium.org/11358181/diff/12001/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/11358181/diff/12001/cc/gl_renderer.cc#newcode544
cc/gl_renderer.cc:544: context()->bindTexture(GL_TEXTURE_2D,
texture->getTextureHandle());
you're moving a bind call up here but look at line 552

https://codereview.chromium.org/11358181/diff/12001/cc/gl_renderer.cc#newcode552
cc/gl_renderer.cc:552: copyTextureToFramebuffer(frame, lock.textureId(),
quad->rect, quad->quadTransform());
but doesn't this also change the binding?

https://codereview.chromium.org/11358181/diff/12001/cc/gl_renderer.cc#newcode779
cc/gl_renderer.cc:779: GLenum filter = (quad->IsAntialiased() || texToGeomScaleX
!= 1 || texToGeomScaleY != 1 || !quad->quadTransform().isIntegerTranslation()) ?
GL_LINEAR : GL_NEAREST;
this line is crazy long - can you break it up a bit? maybe use some temporaries?

https://codereview.chromium.org/11358181/diff/12001/cc/resource_provider.h
File cc/resource_provider.h (right):

https://codereview.chromium.org/11358181/diff/12001/cc/resource_provider.h#ne...
cc/resource_provider.h:138: void bindForSampling(ResourceProvider::ResourceId,
GLenum target, GLenum minFilter = GL_LINEAR, GLenum magFilter = GL_LINEAR);
chrome style forbids default parameter values. can you figure out a way to do
this that doesn't involve them?

https://codereview.chromium.org/11358181/diff/12001/cc/resource_provider.h#ne...
cc/resource_provider.h:230: // TODO: Use a separate sampler object for filter
state.
not sure what this means.

in chromium style, TODOs have a name i.e. TODO(skyostil)

https://codereview.chromium.org/11358181/diff/12001/cc/resource_provider.h#ne...
cc/resource_provider.h:232: GLenum magFilter;
do we really need to support separate minification and magnification filters? in
this patch they always appear to be a matched set

[Sami, 2012-11-30 17:49:27]

Sorry I let this sit for a bit, email filtering fail. This addresses all James'
comments, PTAL. Also, gfx::Transform didn't have IsIdentityOrIntegerTranslation
so I had to add that.

https://codereview.chromium.org/11358181/diff/12001/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/11358181/diff/12001/cc/gl_renderer.cc#newcode552
cc/gl_renderer.cc:552: copyTextureToFramebuffer(frame, lock.textureId(),
quad->rect, quad->quadTransform());
On 2012/11/26 06:54:26, jamesr wrote:
> but doesn't this also change the binding?

Good catch. Gotta love shared context state. I'll keep both binds on line 580.

https://codereview.chromium.org/11358181/diff/12001/cc/gl_renderer.cc#newcode779
cc/gl_renderer.cc:779: GLenum filter = (quad->IsAntialiased() || texToGeomScaleX
!= 1 || texToGeomScaleY != 1 || !quad->quadTransform().isIntegerTranslation()) ?
GL_LINEAR : GL_NEAREST;
On 2012/11/26 06:54:26, jamesr wrote:
> this line is crazy long - can you break it up a bit? maybe use some
temporaries?

Agreed, this is long even for WebKit standards. I'll split it up.

https://codereview.chromium.org/11358181/diff/12001/cc/resource_provider.h
File cc/resource_provider.h (right):

https://codereview.chromium.org/11358181/diff/12001/cc/resource_provider.h#ne...
cc/resource_provider.h:138: void bindForSampling(ResourceProvider::ResourceId,
GLenum target, GLenum minFilter = GL_LINEAR, GLenum magFilter = GL_LINEAR);
On 2012/11/26 06:54:26, jamesr wrote:
> chrome style forbids default parameter values. can you figure out a way to do
> this that doesn't involve them?

Ah, I wasn't exactly sure what style guide we're following here. Let's have just
one filter instead of min/mag, so always passing the filter will be manageable.

https://codereview.chromium.org/11358181/diff/12001/cc/resource_provider.h#ne...
cc/resource_provider.h:230: // TODO: Use a separate sampler object for filter
state.
On 2012/11/26 06:54:26, jamesr wrote:
> not sure what this means.
> 
> in chromium style, TODOs have a name i.e. TODO(skyostil)

In more recent OpenGL versions the sampling state has been split out of the
texture object. This way you can more easily mix and match different sampling
scenarios and textures without having to poke at the texture object state. At
some point we might want to have the same separation, but for now I wanted to
keep them together.

https://codereview.chromium.org/11358181/diff/12001/cc/resource_provider.h#ne...
cc/resource_provider.h:232: GLenum magFilter;
On 2012/11/26 06:54:26, jamesr wrote:
> do we really need to support separate minification and magnification filters?
in
> this patch they always appear to be a matched set

For now we only need matching filters so I'll go with that.

[danakj, 2012-11-30 18:34:56]

https://codereview.chromium.org/11358181/diff/19001/ui/gfx/transform_unittest.cc
File ui/gfx/transform_unittest.cc (right):

https://codereview.chromium.org/11358181/diff/19001/ui/gfx/transform_unittest...
ui/gfx/transform_unittest.cc:1165: transform.Translate3d(1, 2, 3);
Can you add negatives to this test as well?

[Sami, 2012-11-30 18:45:55]

On 2012/11/30 18:34:56, danakj wrote:
> > ui/gfx/transform_unittest.cc:1165: transform.Translate3d(1, 2, 3);
> Can you add negatives to this test as well?

Good idea, done.

[danakj, 2012-11-30 18:48:36]

LGTM for ui/gfx

I'll leave cc/ to jamesr

[jamesr, 2012-12-01 01:50:22]

cc lgtm

https://codereview.chromium.org/11358181/diff/9002/cc/resource_provider.cc
File cc/resource_provider.cc (right):

https://codereview.chromium.org/11358181/diff/9002/cc/resource_provider.cc#ne...
cc/resource_provider.cc:853: CHECK(it != m_resources.end());
why not DCHECK() ?

[Sami, 2012-12-03 11:59:21]

https://codereview.chromium.org/11358181/diff/9002/cc/resource_provider.cc
File cc/resource_provider.cc (right):

https://codereview.chromium.org/11358181/diff/9002/cc/resource_provider.cc#ne...
cc/resource_provider.cc:853: CHECK(it != m_resources.end());
On 2012/12/01 01:50:22, jamesr wrote:
> why not DCHECK() ?

Most of the other methods here CHECK() for a valid resource id so I did that
too. However it is a little redundant here because we are assuming that the
resource was already locked for reading and hence the CHECK() was already done
in lockForRead(). I'll make this a DCHECK().

[commit-bot: I haz the power, 2012-12-03 11:59:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/11358181/16002

[commit-bot: I haz the power, 2012-12-03 13:06:43]

Retried try job too often on mac_rel for step(s) ash_unittests

[commit-bot: I haz the power, 2012-12-03 15:20:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/11358181/16002

[commit-bot: I haz the power, 2012-12-03 16:14:18]

Retried try job too often on linux_rel for step(s) ash_unittests

[commit-bot: I haz the power, 2012-12-03 17:10:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/11358181/16002

[commit-bot: I haz the power, 2012-12-03 17:51:23]

Retried try job too often on linux_rel for step(s) ash_unittests

[commit-bot: I haz the power, 2012-12-03 18:10:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/11358181/16002

[commit-bot: I haz the power, 2012-12-03 18:49:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/11358181/16002

[commit-bot: I haz the power, 2012-12-03 23:58:44]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-04 10:43:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/11358181/16002

[commit-bot: I haz the power, 2012-12-04 12:18:32]

Change committed as 170944