cc: Use CHROMIUM_copy_image for one-copy tile updates.

cc: Use CHROMIUM_copy_image for one-copy tile updates.

This reduces complexitiy and improves tile update performance by
using the new CHROMIUM_copy_image extension.

BUG=653908
TEST=cc_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

[reveman, 2016-10-23 23:19:54]

Description was changed from

==========
cc: Use CHROMIUM_copy_image for one-copy tile updates.

This reduces complexitiy and improves tile update performance by
using the new CHROMIUM_copy_image extension.

BUG=653908
TEST=cc_unittests
==========

to

==========
cc: Use CHROMIUM_copy_image for one-copy tile updates.

This reduces complexitiy and improves tile update performance by
using the new CHROMIUM_copy_image extension.

BUG=653908
TEST=cc_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[reveman, 2017-01-06 08:24:10]

reveman@chromium.org changed reviewers:
+ ericrk@chromium.org, sonnyps@chromium.org

[sunnyps, 2017-01-06 20:36:52]

Description was changed from

==========
cc: Use CHROMIUM_copy_image for one-copy tile updates.

This reduces complexitiy and improves tile update performance by
using the new CHROMIUM_copy_image extension.

BUG=653908
TEST=cc_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Use CHROMIUM_copy_image for one-copy tile updates.

This reduces complexitiy and improves tile update performance by
using the new CHROMIUM_copy_image extension.

BUG=653908
TEST=cc_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[sunnyps, 2017-01-06 20:36:52]

sunnyps@chromium.org changed reviewers:
+ sunnyps@chromium.org
- sonnyps@chromium.org

[ericrk, 2017-01-10 03:50:31]

Looks much nicer! a few small questions.

https://codereview.chromium.org/2446523002/diff/20001/cc/raster/one_copy_rast...
File cc/raster/one_copy_raster_buffer_provider.cc (right):

https://codereview.chromium.org/2446523002/diff/20001/cc/raster/one_copy_rast...
cc/raster/one_copy_raster_buffer_provider.cc:318:
gl->CopyImageSubDataCHROMIUM(staging_buffer->image_id, resource_texture_id,
Internally (if I traced through the code correctly), it looks like we end up
issuing a glCompressedTexSubImage2D rather than the previous
glCompressedTexImage2D. I assume this is fine, but after reading the above
comment I wanted to double check that calling SubImage with the full image rect
is fine wrt/ dealing with an unallocated resource.

https://codereview.chromium.org/2446523002/diff/20001/cc/raster/staging_buffe...
File cc/raster/staging_buffer_pool.cc (right):

https://codereview.chromium.org/2446523002/diff/20001/cc/raster/staging_buffe...
cc/raster/staging_buffer_pool.cc:37: if (gpu_fence->Wait(end - now))
nit: I understand that things like this sometimes return early, but I wonder if
it's nicer for the GpuFence::Wait implementation to handle this logic? Not sure
if the "may return early" is desirable in other use cases.

https://codereview.chromium.org/2446523002/diff/20001/cc/raster/staging_buffe...
cc/raster/staging_buffer_pool.cc:323: ContextProvider::ScopedContextLock
scoped_context(
not sure what the overhead of locking/re-locking, but I wonder if it's worth
making scoped_context a unique_ptr or base::Optional scoped outside of the while
loop. It could be initialized the first time it would be used - so it's only
taken once, if necessary.

I guess that brings the MarkStagingBuffersAsBusy and RemoveStagingBuffers calls
into the lock scope, but they seem pretty lightweight?

[reveman, 2017-01-10 19:37:56]

https://codereview.chromium.org/2446523002/diff/20001/cc/raster/one_copy_rast...
File cc/raster/one_copy_raster_buffer_provider.cc (right):

https://codereview.chromium.org/2446523002/diff/20001/cc/raster/one_copy_rast...
cc/raster/one_copy_raster_buffer_provider.cc:318:
gl->CopyImageSubDataCHROMIUM(staging_buffer->image_id, resource_texture_id,
On 2017/01/10 at 03:50:31, ericrk wrote:
> Internally (if I traced through the code correctly), it looks like we end up
issuing a glCompressedTexSubImage2D rather than the previous
glCompressedTexImage2D. I assume this is fine, but after reading the above
comment I wanted to double check that calling SubImage with the full image rect
is fine wrt/ dealing with an unallocated resource.

We might need to add some special code for this on the service side or do an
initial CompressedTexImage2D call here to make this work. I'll make sure this
works correctly before we get to a point where this can land.

https://codereview.chromium.org/2446523002/diff/20001/cc/raster/staging_buffe...
File cc/raster/staging_buffer_pool.cc (right):

https://codereview.chromium.org/2446523002/diff/20001/cc/raster/staging_buffe...
cc/raster/staging_buffer_pool.cc:37: if (gpu_fence->Wait(end - now))
On 2017/01/10 at 03:50:31, ericrk wrote:
> nit: I understand that things like this sometimes return early, but I wonder
if it's nicer for the GpuFence::Wait implementation to handle this logic? Not
sure if the "may return early" is desirable in other use cases.

That was my initial approach but I decided it was better to just push this up to
the caller where it can in some cases be handled more efficiently and it's also
consistent with base::ConditionVariable
(https://cs.chromium.org/chromium/src/base/synchronization/condition_variable....)

https://codereview.chromium.org/2446523002/diff/20001/cc/raster/staging_buffe...
cc/raster/staging_buffer_pool.cc:323: ContextProvider::ScopedContextLock
scoped_context(
On 2017/01/10 at 03:50:31, ericrk wrote:
> not sure what the overhead of locking/re-locking, but I wonder if it's worth
making scoped_context a unique_ptr or base::Optional scoped outside of the while
loop. It could be initialized the first time it would be used - so it's only
taken once, if necessary.
> 
> I guess that brings the MarkStagingBuffersAsBusy and RemoveStagingBuffers
calls into the lock scope, but they seem pretty lightweight?

Makes sense. I'll address this as I rebase this patch.