Enable asynchronous glReadPixels on Windows.

Enable asynchronous glReadPixels on Windows.

Chromium is using the GL ES 2.0 spec with ANGLE, which doesn't allow
for async readback on its own.  However, ANGLE also provides two
extensions which, together, provide all the needed extra implementation:
GL_NV_pixel_buffer_object and GL_EXT_map_buffer_range.

This change enables the appropriate feature flags when the two
extensions are present.  It also changes the auto-generated bindings for
the glMapBufferRange function so that glMapBufferRangeEXT is used
instead of glMapBufferRange when ANGLE is initialized below the version
3 spec.

In addition, this change adds propagation of service-side glMapBuffer()
errors back to the glMapBufferCHROMIUM call client-side.  In the process
of writing tests for this, a minor shared memory allocation bug was
revealed and also fixed.

BUG=431420
Committed: https://crrev.com/b70d7859b370731bb54ba32b6e92e8bb573390e5
Cr-Commit-Position: refs/heads/master@{#304340}

[miu, 2014-11-11 21:21:06]

Patchset #1 (id:1) has been deleted

[miu, 2014-11-11 21:21:17]

Patchset #1 (id:20001) has been deleted

[miu, 2014-11-11 22:24:52]

miu@chromium.org changed reviewers:
+ jmadill@chromium.org, piman@chromium.org

[miu, 2014-11-11 22:24:52]

jmadill: PTAL.  This is the solution I discussed w/ you and Shannon on the
e-mail thread.

piman: OWNERS approval please.

[Jamie Madill, 2014-11-11 22:33:27]

Not a command buffer expert, but LGTM given it passes tests & manual testing.

[piman, 2014-11-11 22:54:31]

https://codereview.chromium.org/706173005/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/706173005/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7553: if (data)
In which case should data be NULL?
Should we raise GL_OUT_OF_MEMORY, or lose the context if that's the case?

Silently failing seems bad.

[miu, 2014-11-13 01:02:34]

Patchset #2 (id:60001) has been deleted

[miu, 2014-11-13 01:24:58]

Patchset #3 (id:100001) has been deleted

[miu, 2014-11-13 01:31:46]

piman: PTAL.  Responses inline:

On 2014/11/11 22:54:31, piman (Very slow to review) wrote:
> gpu/command_buffer/service/gles2_cmd_decoder.cc:7553: if (data)
> In which case should data be NULL?

According to the spec, NULL is returned if glMapBuffer() cannot map a region in
the process' address space.  This can fail in both OOM and "32 bits in not
enough" situations.

> Should we raise GL_OUT_OF_MEMORY, or lose the context if that's the case?
> 
> Silently failing seems bad.

Agreed.  I discussed a few approaches to propagating this error status with
hubbe@.  We settled on a simple solution, which is to add a few bytes on to the
end of the transfer buffer to store the GL error code.  It makes sense that the
error that occurs in a glMapBuffer() call service-side should be revealed from
the glMapBufferCHROMIUM call client-side.

I added some new tests and updated existing tests for this new logic.  This also
exposed a minor bug in MappedMemoryManager, which I fixed.  Explained below:

https://codereview.chromium.org/706173005/diff/80001/gpu/command_buffer/clien...
File gpu/command_buffer/client/mapped_memory.cc (right):

https://codereview.chromium.org/706173005/diff/80001/gpu/command_buffer/clien...
gpu/command_buffer/client/mapped_memory.cc:99: DCHECK(mem);
This DCHECK failed with my changes to the BufferTracker unit tests.  I was
adding 4 bytes of metadata to the 64 bytes of data, which meant BufferTracker
would allocate a new gpu::Buffer of size 68 bytes.  The gpu::Buffer and
MemoryChunk would properly account for the 68 bytes, but the FencedAllocator
used by MemoryChunk::Alloc() will always round down to the nearest 16-byte
alignment, causing MemoryChunk::Alloc(68) to fail.

The solution is to ensure the MappedMemoryManager::set_chunk_size_multiple() is
always called with a value >= 16.

[piman, 2014-11-14 19:50:14]

On 2014/11/13 01:31:46, miu wrote:
> piman: PTAL.  Responses inline:
> 
> On 2014/11/11 22:54:31, piman (Very slow to review) wrote:
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:7553: if (data)
> > In which case should data be NULL?
> 
> According to the spec, NULL is returned if glMapBuffer() cannot map a region
in
> the process' address space.  This can fail in both OOM and "32 bits in not
> enough" situations.
> 
> > Should we raise GL_OUT_OF_MEMORY, or lose the context if that's the case?
> > 
> > Silently failing seems bad.
> 
> Agreed.  I discussed a few approaches to propagating this error status with
> hubbe@.  We settled on a simple solution, which is to add a few bytes on to
the
> end of the transfer buffer to store the GL error code.  It makes sense that
the
> error that occurs in a glMapBuffer() call service-side should be revealed from
> the glMapBufferCHROMIUM call client-side.


This seems overkill. It's not like there is anything the client can do if
MapBuffer fails. That's why I suggested raising GL_OUT_OF_MEMORY (service-side).
It's acceptable per spec, and in that case the client can't expect the data to
be correct. It's just a matter of reporting the error as opposed to failing
silently.

> 
> I added some new tests and updated existing tests for this new logic.  This
also
> exposed a minor bug in MappedMemoryManager, which I fixed.  Explained below:
> 
>
https://codereview.chromium.org/706173005/diff/80001/gpu/command_buffer/clien...
> File gpu/command_buffer/client/mapped_memory.cc (right):
> 
>
https://codereview.chromium.org/706173005/diff/80001/gpu/command_buffer/clien...
> gpu/command_buffer/client/mapped_memory.cc:99: DCHECK(mem);
> This DCHECK failed with my changes to the BufferTracker unit tests.  I was
> adding 4 bytes of metadata to the 64 bytes of data, which meant BufferTracker
> would allocate a new gpu::Buffer of size 68 bytes.  The gpu::Buffer and
> MemoryChunk would properly account for the 68 bytes, but the FencedAllocator
> used by MemoryChunk::Alloc() will always round down to the nearest 16-byte
> alignment, causing MemoryChunk::Alloc(68) to fail.
> 
> The solution is to ensure the MappedMemoryManager::set_chunk_size_multiple()
is
> always called with a value >= 16.

[miu, 2014-11-14 23:08:14]

On 2014/11/14 19:50:14, piman (Very slow to review) wrote:
> This seems overkill. It's not like there is anything the client can do if
> MapBuffer fails. That's why I suggested raising GL_OUT_OF_MEMORY
(service-side).

Good point.  Reverted to PS1, then applied your suggestion (and kept the
MappedMemoryManager bug fix).  PTAL.

[piman, 2014-11-15 00:43:43]

lgtm

https://codereview.chromium.org/706173005/diff/80002/gpu/command_buffer/clien...
File gpu/command_buffer/client/mapped_memory.h (right):

https://codereview.chromium.org/706173005/diff/80002/gpu/command_buffer/clien...
gpu/command_buffer/client/mapped_memory.h:138: DCHECK_GE(multiple,
FencedAllocator::kAllocAlignment);
Maybe DCHECK(multiple % FencedAllocator::kAllocAlignment == 0)?

[miu, 2014-11-15 01:04:26]

https://codereview.chromium.org/706173005/diff/80002/gpu/command_buffer/clien...
File gpu/command_buffer/client/mapped_memory.h (right):

https://codereview.chromium.org/706173005/diff/80002/gpu/command_buffer/clien...
gpu/command_buffer/client/mapped_memory.h:138: DCHECK_GE(multiple,
FencedAllocator::kAllocAlignment);
On 2014/11/15 00:43:43, piman (Very slow to review) wrote:
> Maybe DCHECK(multiple % FencedAllocator::kAllocAlignment == 0)?

Done.  Right.  Any time the computed chunk_size == alloc_size (and chunk_size is
not divisible by 16), the rounding down issue can also happen.

[miu, 2014-11-15 01:05:11]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2014-11-15 01:06:22]

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

[commit-bot: I haz the power, 2014-11-15 03:53:59]

Committed patchset #6 (id:170001)

[commit-bot: I haz the power, 2014-11-15 03:55:30]

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