gpu: Run texture upload perf test with different texture formats.

gpu: Run texture upload perf test with different texture formats.

The two formats GL_LUMINANCE and GL_RED_EXT are now been tested in addition to GL_RGBA.
GL_LUMINANCE and GL_RED_EXT are used for yuv resources that are uploaded by VideoLayerImpl.

BUG=423481
Committed: https://crrev.com/ac138e5c000fad73b2e4528615b3fd83f467be94
Cr-Commit-Position: refs/heads/master@{#319788}

[Daniele Castagna, 2015-03-03 20:01:29]

Patchset #1 (id:1) has been deleted

[Daniele Castagna, 2015-03-03 20:02:29]

Patchset #1 (id:20001) has been deleted

[Daniele Castagna, 2015-03-03 20:26:53]

dcastagna@chromium.org changed reviewers:
+ reveman@chromium.org

[Daniele Castagna, 2015-03-03 20:26:54]

No rush on this since I want to commit crrev.com/962863002 first.

Thank you!

[reveman, 2015-03-03 21:59:40]

https://codereview.chromium.org/974913003/diff/60001/gpu/perftests/texture_up...
File gpu/perftests/texture_upload_perftest.cc (right):

https://codereview.chromium.org/974913003/diff/60001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:86: int bytes_per_pixel = format ==
GL_RGBA ? 4 : 1;
If all you use is bytes_per_pixel in this function then maybe it's better to
have that be a function argument than format.

https://codereview.chromium.org/974913003/diff/60001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:105: DCHECK(format == GL_RGBA || format
== GL_LUMINANCE || format == GL_RED_EXT);
how about a default: NOTREACHED() case in the switch statement below instead?

[Daniele Castagna, 2015-03-06 17:29:16]

https://codereview.chromium.org/974913003/diff/60001/gpu/perftests/texture_up...
File gpu/perftests/texture_upload_perftest.cc (right):

https://codereview.chromium.org/974913003/diff/60001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:86: int bytes_per_pixel = format ==
GL_RGBA ? 4 : 1;
On 2015/03/03 at 21:59:40, reveman wrote:
> If all you use is bytes_per_pixel in this function then maybe it's better to
have that be a function argument than format.

Done.

https://codereview.chromium.org/974913003/diff/60001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:105: DCHECK(format == GL_RGBA || format
== GL_LUMINANCE || format == GL_RED_EXT);
On 2015/03/03 at 21:59:40, reveman wrote:
> how about a default: NOTREACHED() case in the switch statement below instead?

Done.

[reveman, 2015-03-06 19:37:54]

lgtm with nits

https://codereview.chromium.org/974913003/diff/80001/gpu/perftests/texture_up...
File gpu/perftests/texture_upload_perftest.cc (right):

https://codereview.chromium.org/974913003/diff/80001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:291: EXPECT_THAT(format,
testing::AnyOf(GL_RGBA, GL_LUMINANCE, GL_RED_EXT));
nit: I don't think this EXPECT statement should be here. If you add a new
format, nothing right here would have to change except the EXPECT statement. I
think it's better to keep the EXPECT statement next to the code that might need
to change.

https://codereview.chromium.org/974913003/diff/80001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:315: GenerateTextureData(size, format
== GL_RGBA ? 4 : 1, i + 1, &pixels);
nit: EXPECT_THAT(format, testing::AnyOf(GL_RGBA, GL_LUMINANCE, GL_RED_EXT))
before this line so it's easier to see that the "format == GL_RGBA ? 4 : 1"
logic depends on that expectation.

[Daniele Castagna, 2015-03-06 20:20:16]

https://codereview.chromium.org/974913003/diff/80001/gpu/perftests/texture_up...
File gpu/perftests/texture_upload_perftest.cc (right):

https://codereview.chromium.org/974913003/diff/80001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:291: EXPECT_THAT(format,
testing::AnyOf(GL_RGBA, GL_LUMINANCE, GL_RED_EXT));
On 2015/03/06 at 19:37:54, reveman wrote:
> nit: I don't think this EXPECT statement should be here. If you add a new
format, nothing right here would have to change except the EXPECT statement. I
think it's better to keep the EXPECT statement next to the code that might need
to change.

Done.

https://codereview.chromium.org/974913003/diff/80001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:291: EXPECT_THAT(format,
testing::AnyOf(GL_RGBA, GL_LUMINANCE, GL_RED_EXT));
On 2015/03/06 at 19:37:54, reveman wrote:
> nit: I don't think this EXPECT statement should be here. If you add a new
format, nothing right here would have to change except the EXPECT statement. I
think it's better to keep the EXPECT statement next to the code that might need
to change.

Done.

https://codereview.chromium.org/974913003/diff/80001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:315: GenerateTextureData(size, format
== GL_RGBA ? 4 : 1, i + 1, &pixels);
On 2015/03/06 at 19:37:54, reveman wrote:
> nit: EXPECT_THAT(format, testing::AnyOf(GL_RGBA, GL_LUMINANCE, GL_RED_EXT))
before this line so it's easier to see that the "format == GL_RGBA ? 4 : 1"
logic depends on that expectation.

I moved it just before the for loop and I changed it to ASSERT_THAT. Let me know
if you don't like it that way.

[Daniele Castagna, 2015-03-06 20:20:22]

The CQ bit was checked by dcastagna@chromium.org

[Daniele Castagna, 2015-03-06 20:20:22]

The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/974913003/#ps100001
(title: "Move format check closer to where the format is used to determine the
number of bits per pixel.")

[commit-bot: I haz the power, 2015-03-06 20:32:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/974913003/100001

[Daniele Castagna, 2015-03-06 20:37:45]

The CQ bit was unchecked by dcastagna@chromium.org

[Daniele Castagna, 2015-03-06 20:39:57]

dcastagna@chromium.org changed reviewers:
+ piman@chromium.org

[Daniele Castagna, 2015-03-06 20:39:58]

+piman for owners review.

[piman, 2015-03-09 22:34:14]

LGTM

[Daniele Castagna, 2015-03-09 22:43:03]

The CQ bit was checked by dcastagna@chromium.org

[commit-bot: I haz the power, 2015-03-09 22:44:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/974913003/100001

[commit-bot: I haz the power, 2015-03-10 00:22:17]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-03-10 00:22:59]

Patchset 4 (id:??) landed as
https://crrev.com/ac138e5c000fad73b2e4528615b3fd83f467be94
Cr-Commit-Position: refs/heads/master@{#319788}