cc: Add CopyResource function to ResourceProvider API.

cc: Add CopyResource function to ResourceProvider API.

This provides an efficient mechanism for copying a resource.
Source and destination resources must have matching dimensions
and format. Source resource can be backed by an image but
destination cannot. CopyResource creates the appropritate read
lock fence to prevent unsafe access to source image before
the operation has completed.

Also moves the toggle of frame based read lock fences out of
the ResourcePool to not conflict with the read lock fences needed
for CopyResource.

BUG=269808
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263761

[reveman, 2014-04-14 18:24:22]

This is a prerequisite to gpu-to-gpu copy tile resource initialization. See
https://codereview.chromium.org/236313006/ for more context.

[piman, 2014-04-14 18:39:34]

I wonder about the ReadLockFences... Outside of that, just nits.

https://codereview.chromium.org/234403005/diff/1/cc/resources/resource_provid...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/234403005/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.cc:2234:
DCHECK(ReadLockFenceHasPassed(dest_resource));
Do we actually need this?
The ReadLockFence thing was introduced for cpu<->gpu hazards (CPU writing to
memory still in use by GPU), but CopyTextureCHROMIUM is GPU-only, so it should
be well-ordered wrt previous GPU reads.

Actually, similar question for the source...

https://codereview.chromium.org/234403005/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.cc:2238:
DCHECK_EQ(source_resource->size.ToString(), dest_resource->size.ToString());
nit: meh, this isn't a unit test... I'd rather DCHECK(source_resource->size ==
dest_resource->size);

https://codereview.chromium.org/234403005/diff/1/cc/resources/resource_provid...
File cc/resources/resource_provider_unittest.cc (right):

https://codereview.chromium.org/234403005/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider_unittest.cc:3040:
CHECK(output_surface->BindToClient(&output_surface_client));
nit: ASSERT_TRUE

https://codereview.chromium.org/234403005/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider_unittest.cc:3071:
resource_provider->MapImageRasterBuffer(source_id);
nit: it could be useful to add a Mock::VerifyAndClearExpectations(context), to
make sure expectations don't "leak" into the next call.

https://codereview.chromium.org/234403005/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider_unittest.cc:3076:
resource_provider->UnmapImageRasterBuffer(source_id);
and here

https://codereview.chromium.org/234403005/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider_unittest.cc:3096:
resource_provider->CopyResource(source_id, dest_id);
and here

[reveman, 2014-04-14 19:24:09]

PTAL

https://codereview.chromium.org/234403005/diff/1/cc/resources/resource_provid...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/234403005/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.cc:2234:
DCHECK(ReadLockFenceHasPassed(dest_resource));
On 2014/04/14 18:39:34, piman wrote:
> Do we actually need this?
> The ReadLockFence thing was introduced for cpu<->gpu hazards (CPU writing to
> memory still in use by GPU), but CopyTextureCHROMIUM is GPU-only, so it should
> be well-ordered wrt previous GPU reads.
> 
> Actually, similar question for the source...

Right, these DCHECKs are not necessary as it's OK to perform two CopyResource
calls after each other as long as we're not trying to write to the resource
using MapImage in-between. Fixed.

Just to make sure we're on the same page in regards to read lock fences:

We need to create a read lock fence below to prevent us from mapping and
modifying the gpu-memory-buffer associated with the resource before GPU has
completed the texture copy operation. It's enough to make sure the command has
been issued on the gpu side with our current implementations of GLImage but this
will soon change and require me to implement the COMMANDS_COMPLETED query I
mention in the TODO below. I intend to address this asap. My current plan is to
use GL_ARB_sync. This query type can also be used to implement proper frame
based read lock fences instead of relying on the swap count as we currently do.

https://codereview.chromium.org/234403005/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.cc:2238:
DCHECK_EQ(source_resource->size.ToString(), dest_resource->size.ToString());
On 2014/04/14 18:39:34, piman wrote:
> nit: meh, this isn't a unit test... I'd rather DCHECK(source_resource->size ==
> dest_resource->size);

Done.

https://codereview.chromium.org/234403005/diff/1/cc/resources/resource_provid...
File cc/resources/resource_provider_unittest.cc (right):

https://codereview.chromium.org/234403005/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider_unittest.cc:3040:
CHECK(output_surface->BindToClient(&output_surface_client));
On 2014/04/14 18:39:34, piman wrote:
> nit: ASSERT_TRUE

Done.

https://codereview.chromium.org/234403005/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider_unittest.cc:3071:
resource_provider->MapImageRasterBuffer(source_id);
On 2014/04/14 18:39:34, piman wrote:
> nit: it could be useful to add a Mock::VerifyAndClearExpectations(context), to
> make sure expectations don't "leak" into the next call.

Done.

https://codereview.chromium.org/234403005/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider_unittest.cc:3076:
resource_provider->UnmapImageRasterBuffer(source_id);
On 2014/04/14 18:39:34, piman wrote:
> and here

Done.

https://codereview.chromium.org/234403005/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider_unittest.cc:3096:
resource_provider->CopyResource(source_id, dest_id);
On 2014/04/14 18:39:34, piman wrote:
> and here

Done.

[piman, 2014-04-14 19:34:35]

lgtm

https://codereview.chromium.org/234403005/diff/1/cc/resources/resource_provid...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/234403005/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.cc:2234:
DCHECK(ReadLockFenceHasPassed(dest_resource));
On 2014/04/14 19:24:09, reveman wrote:
> On 2014/04/14 18:39:34, piman wrote:
> > Do we actually need this?
> > The ReadLockFence thing was introduced for cpu<->gpu hazards (CPU writing to
> > memory still in use by GPU), but CopyTextureCHROMIUM is GPU-only, so it
should
> > be well-ordered wrt previous GPU reads.
> > 
> > Actually, similar question for the source...
> 
> Right, these DCHECKs are not necessary as it's OK to perform two CopyResource
> calls after each other as long as we're not trying to write to the resource
> using MapImage in-between. Fixed.
> 
> Just to make sure we're on the same page in regards to read lock fences:
> 
> We need to create a read lock fence below to prevent us from mapping and
> modifying the gpu-memory-buffer associated with the resource before GPU has
> completed the texture copy operation.

Yes, understood for that part.

> It's enough to make sure the command has
> been issued on the gpu side with our current implementations of GLImage but
this
> will soon change and require me to implement the COMMANDS_COMPLETED query I
> mention in the TODO below. I intend to address this asap. My current plan is
to
> use GL_ARB_sync. This query type can also be used to implement proper frame
> based read lock fences instead of relying on the swap count as we currently
do.

Great. I really hope we can get rid of the swap count hack, which is actually
meaningless with delegated rendering.

[reveman, 2014-04-14 19:44:46]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-04-14 19:45:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/234403005/20001

[commit-bot: I haz the power, 2014-04-14 21:00:47]

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

[commit-bot: I haz the power, 2014-04-14 21:00:47]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[reveman, 2014-04-14 21:11:19]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-04-14 21:12:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/234403005/20001

[commit-bot: I haz the power, 2014-04-14 22:23:03]

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

[commit-bot: I haz the power, 2014-04-14 22:23:03]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[reveman, 2014-04-14 22:34:57]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-04-14 22:36:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/234403005/20001

[commit-bot: I haz the power, 2014-04-15 00:57:18]

Change committed as 263761