cc: Add zero-copy texture update support to delegating renderer.

cc: Add zero-copy texture update support to delegating renderer.

This adds necessary BindTexImage/ReleaseTexImage calls to
ResourceProvider::TransferResource.

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

[piman, 2014-01-16 02:29:57]

What would it take to clean this properly? Counting "swaps" just isn't enough
when you can have multiple levels of embedding

[reveman, 2014-01-16 17:46:56]

I removed all swap fence related code from this patch. While we need some
synchronization code to do zero-copy properly with ubercomp I think the missing
code for binding images to textures can land before that. PTAL.

Seems like for proper synchroniozation we'd need to add another Ack or change so
the SwapBuffers Ack is not sent until resources are no longer in use for
sampling. Let's move that discussion to a separate issue though.

[danakj, 2014-01-16 17:55:52]

https://codereview.chromium.org/132873006/diff/140001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/132873006/diff/140001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1259: LazyCreate(source);
Why this? You're transferring a resource that has never been allocated/written
to?

[danakj, 2014-01-16 17:56:22]

Is it possible to add some tests to resource_provider_unittest.cc for these
cases?

[piman, 2014-01-16 21:54:33]

https://codereview.chromium.org/132873006/diff/140001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/132873006/diff/140001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1259: LazyCreate(source);
On 2014/01/16 17:55:52, danakj wrote:
> Why this? You're transferring a resource that has never been allocated/written
> to?

I don't think it's correct here, actually.
This would create a new texture id for an imported or external resource, but not
even Consume the mailbox into it, so we're in a limbo state where we have a
texture id and a mailbox, but they don't correspond to each other.

Maybe this should move into the "else if (!source->mailbox.IsValid())" block
below, gated by source->image_id ?

https://codereview.chromium.org/132873006/diff/140001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1279: if (source->image_id &&
source->dirty_image) {
Add a DCHECK(source->gl_id);
The only reason we'd get here (we have a mailbox and an image_id) is if we
already sent the resource at least once (it can't be imported or external), in
which case, we'd have created the texture in the previous block the first time
around.

[piman, 2014-01-16 21:58:03]

On Thu, Jan 16, 2014 at 9:46 AM, <reveman@chromium.org> wrote:

> I removed all swap fence related code from this patch. While we need some
> synchronization code to do zero-copy properly with ubercomp I think the
> missing
> code for binding images to textures can land before that. PTAL.
>
> Seems like for proper synchroniozation we'd need to add another Ack or
> change so
> the SwapBuffers Ack is not sent until resources are no longer in use for
> sampling. Let's move that discussion to a separate issue though.
>

Sure. I don't think we'd want to delay the Swap Ack, because we still want
the child to produce frames. But maybe we don't want to return those
resources yet? (in which case the parent needs to know what to wait for).
Or, this may be easier to deal entirely in the child where it starts
counting frames (or a more suitable synchronization primitive) after it
received the resource back. That's more conservative (i.e. likely less
efficient) but maybe easier to hook up.


> https://codereview.chromium.org/132873006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[reveman, 2014-01-17 01:38:39]

PTAL

https://codereview.chromium.org/132873006/diff/140001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/132873006/diff/140001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1259: LazyCreate(source);
On 2014/01/16 17:55:52, danakj wrote:
> Why this? You're transferring a resource that has never been allocated/written
> to?

We've created and written to the GLImage but it has not yet been bound to a
texture so there hasn't been a need to create the texture. This is in part to be
nice to android_webview where we need to delay GL commands until draw time.

https://codereview.chromium.org/132873006/diff/140001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1259: LazyCreate(source);
On 2014/01/16 21:54:34, piman wrote:
> On 2014/01/16 17:55:52, danakj wrote:
> > Why this? You're transferring a resource that has never been
allocated/written
> > to?
> 
> I don't think it's correct here, actually.
> This would create a new texture id for an imported or external resource, but
not
> even Consume the mailbox into it, so we're in a limbo state where we have a
> texture id and a mailbox, but they don't correspond to each other.
> 
> Maybe this should move into the "else if (!source->mailbox.IsValid())" block
> below, gated by source->image_id ?

Hm, is texture_pool not 0 in those cases? Makes sense to move it to the "else if
(!source->mailbox.IsValid())" block anyhow but I'd rather not have it gated by
"source->image_id" and instead make sure LazyCreate does the right thing here
and not mess with imported/external resources.

https://codereview.chromium.org/132873006/diff/140001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1279: if (source->image_id &&
source->dirty_image) {
On 2014/01/16 21:54:34, piman wrote:
> Add a DCHECK(source->gl_id);
> The only reason we'd get here (we have a mailbox and an image_id) is if we
> already sent the resource at least once (it can't be imported or external), in
> which case, we'd have created the texture in the previous block the first time
> around.

Done.

[piman, 2014-01-17 02:16:33]

lgtm

[commit-bot: I haz the power, 2014-01-17 03:29:56]

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

[commit-bot: I haz the power, 2014-01-17 03:50:01]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2014-01-17 05:24:27]

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

[commit-bot: I haz the power, 2014-01-17 07:12:12]

Change committed as 245460