cc: Support texture rect targets for masks

cc/test/layer_tree_pixel_resource_test.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CC_TEST_LAYER_TREE_PIXEL_RESOURCE_TEST_H_
+#define CC_TEST_LAYER_TREE_PIXEL_RESOURCE_TEST_H_
+
+#include "base/memory/ref_counted.h"
+#include "cc/test/layer_tree_pixel_test.h"
+
+namespace cc {
+
+class LayerTreeHostImpl;
+class RasterWorkerPool;
+class ResourcePool;
+
+// Enumerate the various combinations of renderer, resource pool, staging
+// texture type, and drawing texture types.  Not all of the combinations
+// are possible (or worth testing independently), so this is the minimal
+// list to hit all codepaths.
+enum PixelResourceTestCase {
+  SOFTWARE,
+  GL_GPU_RASTER_2D_DRAW,
+  GL_ONE_COPY_2D_STAGING_2D_DRAW,
+  GL_ONE_COPY_RECT_STAGING_2D_DRAW,
+  GL_ONE_COPY_EXTERNAL_STAGING_2D_DRAW,
+  GL_ZERO_COPY_2D_DRAW,
+  GL_ZERO_COPY_RECT_DRAW,
+  GL_ZERO_COPY_EXTERNAL_DRAW,
+  GL_ASYNC_UPLOAD_2D_DRAW,

[reveman, 2014/10/23 20:39:27]

What's the naming convention here?

GL_(raster-worker-pool-type)_[(staging-target)_STAGING](draw-target)_DRAW?

I'd prefer if it was simply:
GL_(raster-worker-pool-type)_(raster-target)

See my comment related to staging/draw_texture_target_ below.

[enne (OOO), 2014/10/24 22:21:28]

Don't agree.  See comment below.

+  NUM_PIXEL_RESOURCE_TEST_CASES,

[reveman, 2014/10/23 20:39:27]

If you're going to keep this then consider using "PIXEL_RESOURCE_TEST_CASES_MAX
= GL_ASYNC_UPLOAD_2D_DRAW" which I think is the more common pattern at this
point.

[enne (OOO), 2014/10/24 22:21:28]

Removed.

+};
+
+class LayerTreeHostPixelResourceTest : public LayerTreePixelTest {
+ public:
+  explicit LayerTreeHostPixelResourceTest(PixelResourceTestCase test_case);
+
+  void CreateResourceAndRasterWorkerPool(
+      LayerTreeHostImpl* host_impl,
+      scoped_ptr<RasterWorkerPool>* raster_worker_pool,
+      scoped_ptr<ResourcePool>* resource_pool,
+      scoped_ptr<ResourcePool>* staging_resource_pool) override;
+
+  void RunPixelResourceTest(scoped_refptr<Layer> content_root,
+                            base::FilePath file_name);
+
+  enum ResourcePoolOption {

[reveman, 2014/10/23 20:39:27]

ResourcePoolOption, uh? Should it be RasterizeOption or RasterWorkerPoolOption?

[enne (OOO), 2014/10/24 22:21:28]

Sure.

+    RESOURCE_POOL_BITMAP_RASTER_WORKER,
+    RESOURCE_POOL_GPU_RASTER,
+    RESOURCE_POOL_ZERO_COPY,
+    RESOURCE_POOL_ONE_COPY,
+    RESOURCE_POOL_PIXEL_BUFFER_RASTER,

[reveman, 2014/10/23 20:39:27]

Can you make these names better align with RasterWorkerPool implementations?
_BITMAP, _GPU, _ZERO_COPY..

[enne (OOO), 2014/10/24 22:21:28]

I named them identically to the class names, e.g. BITMAP_RASTER_WORKER_POOL.

+  };
+
+ protected:
+  unsigned staging_texture_target_;
+  unsigned draw_texture_target_;

[reveman, 2014/10/23 20:39:27]

I don't think we should have both staging and draw target here. That just allows
this to be set to configurations that can never be supported. Replacing both
with "raster_texture_target" is much better imo.

[enne (OOO), 2014/10/24 22:21:28]

I disagree.  raster_texture_target just hides the fact that if we test both zero
copy and one copy with all the values of raster_texture_target that we'll test
all the staging and draw combinations.  If you happened to remove zero copy,
then you'd be unable to test all the combinations unless you added this back in.

Sure, they can be set to things that don't make sense, but that's why the
variables are controlled by a higher level test case that only permits sane
options.  Being able to set them arbitrarily is why I wrote this code.

[reveman, 2014/10/27 17:19:04]

To pixel test draw target support independent of available RasterWorkerPool
implementations I think we should add some form of pixels tests that doesn't use
a RasterWorkerPool.
I guess I'm failing to see what we're trying to test. If zero-copy didn't exist
or if it didn't need RECT/EXTERNAL support then why would we need to test it?
Wouldn't it be better to remove/DCHECK than keep supporting those draw targets?
Like I mentioned above, if we need to pixel test these draw targets independent
of available raster worker pool implementations, then I think we need some pixel
test that doesn't use an existing raster worker pool implementation.

[enne (OOO), 2014/10/27 17:56:22]

Maybe you should have mentioned that several reviews ago, if that was what you
ultimately wanted.

These are LayerTreeHost pixel tests, not just straight up pixel tests.  So, they
have layers and tile managers and raster worker pools etc.  There seems to be
zero cost to using a real raster worker pool and I see no use in writing a path
to bypass it.

Here was my sequence of thoughts.  I want to test GLRenderer with different draw
targets.  Currently, zero/one copy raster worker pool are the paths that
currently support the whole variety of them.  It made sense to test our actual
raster worker pools rather than write a test raster worker pool, since there's
no cost to using real paths here.  Then, if I was testing one raster worker
pool, why not just test all of them with all combinations.

This sounded useful to me to exercise all of our code paths; it sounds like you
don't agree.

I want to test draw targets.  How about I replace all of these combinations with
just a single texture draw target enum and it'll use zero copy under the hood?

[reveman, 2014/10/27 18:42:44]

Testing all supported draw targets sgtm. Testing all supported raster targets
with all supported raster worker pool implementations sgtm.

That we need test code that make it look like every combination of raster/draw
target is supported is what I'm not a fan of but this lgtm if you insist that
this is the best way to test all this.

+  ResourcePoolOption resource_pool_option_;
+
+ private:
+  PixelResourceTestCase test_case_;
+};
+
+#define INSTANTIATE_PIXEL_RESOURCE_TEST_CASE_P(framework_name) \
+  INSTANTIATE_TEST_CASE_P(                                     \
+      PixelResourceTest,                                       \
+      framework_name,                                          \
+      ::testing::Values(SOFTWARE,                              \
+                        GL_GPU_RASTER_2D_DRAW,                 \
+                        GL_ONE_COPY_2D_STAGING_2D_DRAW,        \
+                        GL_ONE_COPY_RECT_STAGING_2D_DRAW,      \
+                        GL_ONE_COPY_EXTERNAL_STAGING_2D_DRAW,  \
+                        GL_ZERO_COPY_2D_DRAW,                  \
+                        GL_ZERO_COPY_RECT_DRAW,                \
+                        GL_ZERO_COPY_EXTERNAL_DRAW,            \
+                        GL_ASYNC_UPLOAD_2D_DRAW))
+
+class ParameterizedPixelResourceTest
+    : public LayerTreeHostPixelResourceTest,
+      public ::testing::WithParamInterface<PixelResourceTestCase> {
+ public:
+  ParameterizedPixelResourceTest();
+};
+
+}  // namespace cc
+
+#endif  // CC_TEST_LAYER_TREE_PIXEL_RESOURCE_TEST_H_