Test gfx::GpuMemoryBuffer::BGRA_8888 GpuMemoryBuffer lifecycle.

Test gfx::GpuMemoryBuffer::BGRA_8888 GpuMemoryBuffer lifecycle.

The test fixture GpuMemoryBufferTest has been parametrized in terms of
gfx::GpuMemoryBuffer::Format so that GpuMemoryBufferTest.Lifecycle can
be run with different gfx::GpuMemoryBuffer::Format.

gfx::GpuMemoryBuffer::BGRA_8888 has been added along with the already
present RGBA_8888.

BUG=

Committed: https://crrev.com/409804aa7155bc497d6be7e17a26f76fad18defb
Cr-Commit-Position: refs/heads/master@{#323948}

[Daniele Castagna, 2015-04-02 20:26:01]

Patchset #1 (id:1) has been deleted

[Daniele Castagna, 2015-04-02 20:27:11]

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

[reveman, 2015-04-03 02:20:16]

https://codereview.chromium.org/1055143003/diff/20001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc (right):

https://codereview.chromium.org/1055143003/diff/20001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:73: namespace {
nit: we use a blank line between namespace and code in most chromium code

https://codereview.chromium.org/1055143003/diff/20001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:89: default:
Please avoid a default case and explicitly list all formats. I'd like to make
sure that when we add a new format we update this code.

https://codereview.chromium.org/1055143003/diff/20001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:106: default:
ditto

https://codereview.chromium.org/1055143003/diff/20001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:111: }  // namespace
nit: we use a blank line between namespace and code in most chromium code

https://codereview.chromium.org/1055143003/diff/20001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:132: for (uint32
x_offset = 0; x_offset < stride; x_offset += pixel.size()) {
I don't think we should be writing past kImageWidth. We should also avoid logic
that prevents a negative stride from working correctly as I think we need to
support that in the future.

[Daniele Castagna, 2015-04-03 02:56:14]

https://codereview.chromium.org/1055143003/diff/20001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc (right):

https://codereview.chromium.org/1055143003/diff/20001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:73: namespace {
On 2015/04/03 at 02:20:16, reveman wrote:
> nit: we use a blank line between namespace and code in most chromium code

Ack. It'll be there at the next iteration.

https://codereview.chromium.org/1055143003/diff/20001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:89: default:
On 2015/04/03 at 02:20:16, reveman wrote:
> Please avoid a default case and explicitly list all formats. I'd like to make
sure that when we add a new format we update this code.

I don't see why if someone adds a new format (to GpuMemoryBuffer), they'd have
to touch this code.

Listing all the formats in all the places where format is used make it so the
number of lines of code we have to write is O(max_enum * places_where_its_used).

Wouldn't it be better, at least in this case where we're in a test, to assert
that the default case is not executed?

If someone adds a new format to GpuMemoryBuffer, but not to this test, I don't
see why this test should change.
On the other side, if someone adds a new format to the test, I'd expect they'd
run the test at least once, and they would easily figure out what to change.

If you're OK with that, I can just change NOTREACHED() with ASSERT_TRUE(false);

https://codereview.chromium.org/1055143003/diff/20001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:106: default:
On 2015/04/03 at 02:20:16, reveman wrote:
> ditto

Ack.

https://codereview.chromium.org/1055143003/diff/20001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:111: }  // namespace
On 2015/04/03 at 02:20:16, reveman wrote:
> nit: we use a blank line between namespace and code in most chromium code

Ack. I'll add it at the next iteration.

https://codereview.chromium.org/1055143003/diff/20001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:132: for (uint32
x_offset = 0; x_offset < stride; x_offset += pixel.size()) {
On 2015/04/03 at 02:20:16, reveman wrote:
> I don't think we should be writing past kImageWidth. We should also avoid
logic that prevents a negative stride from working correctly as I think we need
to support that in the future.

Isn't this similar to what you were suggesting in crrev.com/1051503003 with
memset(mapped_buffer + y * stride, 0xff, ..)?

Do we support negative stride anywhere right now? There is an assert just above,
to make sure that stride is positive. As soon as we add a new format with a
negative stride to this test, it would be pretty clear what to change.

If you're not OK with this, what would you suggest to do instead?

[reveman, 2015-04-03 03:41:02]

https://codereview.chromium.org/1055143003/diff/20001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc (right):

https://codereview.chromium.org/1055143003/diff/20001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:89: default:
On 2015/04/03 at 02:56:14, Daniele Castagna wrote:
> On 2015/04/03 at 02:20:16, reveman wrote:
> > Please avoid a default case and explicitly list all formats. I'd like to
make sure that when we add a new format we update this code.
> 
> I don't see why if someone adds a new format (to GpuMemoryBuffer), they'd have
to touch this code.
> 
> Listing all the formats in all the places where format is used make it so the
number of lines of code we have to write is O(max_enum * places_where_its_used).
> 
> Wouldn't it be better, at least in this case where we're in a test, to assert
that the default case is not executed?
> 
> If someone adds a new format to GpuMemoryBuffer, but not to this test, I don't
see why this test should change.
> On the other side, if someone adds a new format to the test, I'd expect they'd
run the test at least once, and they would easily figure out what to change.
> 
> If you're OK with that, I can just change NOTREACHED() with
ASSERT_TRUE(false);

If someone adds a new format to GpuMemoryBuffer, they might not know or remember
that we have these tests. I'd like the compiler to remind me so please change
this.

Having a default case to save ourselves from typing a few lines of code is the
wrong trade-off imo.

https://codereview.chromium.org/1055143003/diff/20001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:132: for (uint32
x_offset = 0; x_offset < stride; x_offset += pixel.size()) {
On 2015/04/03 at 02:56:14, Daniele Castagna wrote:
> On 2015/04/03 at 02:20:16, reveman wrote:
> > I don't think we should be writing past kImageWidth. We should also avoid
logic that prevents a negative stride from working correctly as I think we need
to support that in the future.
> 
> Isn't this similar to what you were suggesting in crrev.com/1051503003 with
memset(mapped_buffer + y * stride, 0xff, ..)?

memset(mapped_buffer + y * stride, 0xff, ..) works with a negative stride

> 
> Do we support negative stride anywhere right now? There is an assert just
above, to make sure that stride is positive. As soon as we add a new format with
a negative stride to this test, it would be pretty clear what to change.

I'm not aware of any gmb type independent code that doesn't support this right
now. If you know of some let me know as it looks like we'll have to change this
soon.

Let's get it right now instead of having to fix it later.

> 
> If you're not OK with this, what would you suggest to do instead?

for (int y = 0; y < kImageHeight; ++y) {
  for (int x = 0; x < kImageWidth; ++x) {
    std::copy(pixel.begin(), pixel.end(),
              mapped_buffer + y * stride + x * pixel.size());
  }
}

[Daniele Castagna, 2015-04-03 04:26:46]

https://codereview.chromium.org/1055143003/diff/20001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc (right):

https://codereview.chromium.org/1055143003/diff/20001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:73: namespace {
On 2015/04/03 at 02:56:14, Daniele Castagna wrote:
> On 2015/04/03 at 02:20:16, reveman wrote:
> > nit: we use a blank line between namespace and code in most chromium code
> 
> Ack. It'll be there at the next iteration.

Done.

https://codereview.chromium.org/1055143003/diff/20001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:89: default:
On 2015/04/03 at 03:41:02, reveman wrote:
> On 2015/04/03 at 02:56:14, Daniele Castagna wrote:
> > On 2015/04/03 at 02:20:16, reveman wrote:
> > > Please avoid a default case and explicitly list all formats. I'd like to
make sure that when we add a new format we update this code.
> > 
> > I don't see why if someone adds a new format (to GpuMemoryBuffer), they'd
have to touch this code.
> > 
> > Listing all the formats in all the places where format is used make it so
the number of lines of code we have to write is O(max_enum *
places_where_its_used).
> > 
> > Wouldn't it be better, at least in this case where we're in a test, to
assert that the default case is not executed?
> > 
> > If someone adds a new format to GpuMemoryBuffer, but not to this test, I
don't see why this test should change.
> > On the other side, if someone adds a new format to the test, I'd expect
they'd run the test at least once, and they would easily figure out what to
change.
> > 
> > If you're OK with that, I can just change NOTREACHED() with
ASSERT_TRUE(false);
> 
> If someone adds a new format to GpuMemoryBuffer, they might not know or
remember that we have these tests. I'd like the compiler to remind me so please
change this.
> 
> Having a default case to save ourselves from typing a few lines of code is the
wrong trade-off imo.

Done.

https://codereview.chromium.org/1055143003/diff/20001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:106: default:
On 2015/04/03 at 02:56:14, Daniele Castagna wrote:
> On 2015/04/03 at 02:20:16, reveman wrote:
> > ditto
> 
> Ack.

Done.

https://codereview.chromium.org/1055143003/diff/20001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:111: }  // namespace
On 2015/04/03 at 02:56:14, Daniele Castagna wrote:
> On 2015/04/03 at 02:20:16, reveman wrote:
> > nit: we use a blank line between namespace and code in most chromium code
> 
> Ack. I'll add it at the next iteration.

Done.

[Daniele Castagna, 2015-04-03 04:28:43]

Thank you for the review!

It's still going to fail on linux_android_rel_ng. I'll take a look at it
tomorrow.

[reveman, 2015-04-06 16:02:49]

lgtm with nit

https://codereview.chromium.org/1055143003/diff/60001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc (right):

https://codereview.chromium.org/1055143003/diff/60001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:170: for (uint32 x =
0; x < kImageWidth; ++x) {
nit: s/uint32/int/

[Daniele Castagna, 2015-04-06 16:20:48]

Patchset #4 (id:80001) has been deleted

[Daniele Castagna, 2015-04-06 16:41:29]

Patchset #4 (id:100001) has been deleted

[Daniele Castagna, 2015-04-06 16:42:37]

Patchset #4 (id:120001) has been deleted

[Daniele Castagna, 2015-04-06 16:45:12]

Patchset #4 (id:140001) has been deleted

[Daniele Castagna, 2015-04-06 17:57:32]

Patchset #4 (id:160001) has been deleted

[Daniele Castagna, 2015-04-06 17:59:05]

https://codereview.chromium.org/1055143003/diff/60001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc (right):

https://codereview.chromium.org/1055143003/diff/60001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:170: for (uint32 x =
0; x < kImageWidth; ++x) {
On 2015/04/06 at 16:02:49, reveman wrote:
> nit: s/uint32/int/

Done.

[Daniele Castagna, 2015-04-06 17:59:12]

The CQ bit was checked by dcastagna@chromium.org

[Daniele Castagna, 2015-04-06 17:59:12]

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/1055143003/#ps100002
(title: "s/uint32/int.")

[commit-bot: I haz the power, 2015-04-06 17:59:47]

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

[commit-bot: I haz the power, 2015-04-06 18:10:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-06 18:10:15]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Daniele Castagna, 2015-04-06 19:35:33]

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

[Daniele Castagna, 2015-04-06 19:35:34]

+piman for owner approval.

[piman, 2015-04-06 20:18:06]

lgtm

[Daniele Castagna, 2015-04-06 20:21:15]

The CQ bit was checked by dcastagna@chromium.org

[commit-bot: I haz the power, 2015-04-06 20:21:29]

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

[commit-bot: I haz the power, 2015-04-06 21:03:31]

Committed patchset #4 (id:100002)

[commit-bot: I haz the power, 2015-04-06 21:04:52]

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