cc: Changes to use GL API for GpuMemoryBuffers

cc: Changes to use GL API for GpuMemoryBuffers

Corresponding gpu changes are here:
https://codereview.chromium.org/14456004/

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

[kaanb1, 2013-04-25 21:40:15]

Friendly ping!

[jamesr, 2013-04-25 22:28:56]

This patch has zero tests. Why?

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

https://codereview.chromium.org/14409006/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:897: static_cast<Context3DImpl*>(context3d);
you shouldn't be downcasting like this. it means that the use_gpu_memory_buffer
setting will cause crashes for anyone using a different context type. why isn't
this on the context API itself with an appropriate fallback behavior for when
the extension isn't supported?

[kaanb1, 2013-04-25 22:43:29]

I added unit tests to the GPU patch:
https://codereview.chromium.org/14456004/ 

Is there something specific you want tests for?

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

https://codereview.chromium.org/14409006/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:897: static_cast<Context3DImpl*>(context3d);
On 2013/04/25 22:28:56, jamesr wrote:
> you shouldn't be downcasting like this. it means that the
use_gpu_memory_buffer
> setting will cause crashes for anyone using a different context type. why
isn't
> this on the context API itself with an appropriate fallback behavior for when
> the extension isn't supported?

This feature is currently only used for Android WebView which is hardwired to
use this context type so the downcast would be safe. I thought it would be a
reasonable compromise to move it to the interface API after other people start
using this code. If you disagree, let me know and I'll prepare a blink patch.

[jamesr, 2013-04-25 22:47:57]

On 2013/04/25 22:43:29, kaanb1 wrote:
> I added unit tests to the GPU patch:
> https://codereview.chromium.org/14456004/ 
> 
> Is there something specific you want tests for?

Unit tests are not the same as integration tests - unit tests verify that the
individual units of code behave as expected.  Tests in gpu/command_buffer/, for
instance, test that the command buffer code does what you want.

You're adding logic to resource_provider, so I'd expect something in the
resource provider unit tests to exercise the new codepaths and ensure they do
what you expect.

> This feature is currently only used for Android WebView which is hardwired to
> use this context type so the downcast would be safe. I thought it would be a
> reasonable compromise to move it to the interface API after other people start
> using this code. If you disagree, let me know and I'll prepare a blink patch.

I don't think that is a very sustainable assumption. For instance, it'll make it
really hard for you to write good unit tests for the resource provider.  It's
unfortunate that the context interface is in blink but that's all we've got
today.

[jamesr, 2013-04-25 22:50:46]

https://codereview.chromium.org/14409006/diff/1/cc/trees/layer_tree_host_impl.cc
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/14409006/diff/1/cc/trees/layer_tree_host_impl...
cc/trees/layer_tree_host_impl.cc:1345: if
(CommandLine::ForCurrentProcess()->HasSwitch(
I don't think this is a good way to plumb this setting in since it's global for
the process but not necessarily something you want set the same way for every
compositor instance in that process.  Instead, what we normally do is add an
entry to cc::LayerTreeSettings and have code outside cc:: (that's aware of the
process model in use) map the command line flag to the setting.

[no sievers, 2013-04-25 23:28:14]

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

https://codereview.chromium.org/14409006/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:897: static_cast<Context3DImpl*>(context3d);
On 2013/04/25 22:43:29, kaanb1 wrote:
> On 2013/04/25 22:28:56, jamesr wrote:
> > you shouldn't be downcasting like this. it means that the
> use_gpu_memory_buffer
> > setting will cause crashes for anyone using a different context type. why
> isn't
> > this on the context API itself with an appropriate fallback behavior for
when
> > the extension isn't supported?
> 
> This feature is currently only used for Android WebView which is hardwired to
> use this context type so the downcast would be safe. I thought it would be a
> reasonable compromise to move it to the interface API after other people start
> using this code. If you disagree, let me know and I'll prepare a blink patch.

Keep in mind that ChromeOS might implement 0-copy uploads too. And so could any
other platforms. If you properly expose it, it will be likelier that this gets
shared like it should.

[kaanb1, 2013-04-25 23:30:00]

On 2013/04/25 23:28:14, Daniel Sievers wrote:
>
https://codereview.chromium.org/14409006/diff/1/cc/resources/resource_provide...
> File cc/resources/resource_provider.cc (right):
> 
>
https://codereview.chromium.org/14409006/diff/1/cc/resources/resource_provide...
> cc/resources/resource_provider.cc:897: static_cast<Context3DImpl*>(context3d);
> On 2013/04/25 22:43:29, kaanb1 wrote:
> > On 2013/04/25 22:28:56, jamesr wrote:
> > > you shouldn't be downcasting like this. it means that the
> > use_gpu_memory_buffer
> > > setting will cause crashes for anyone using a different context type. why
> > isn't
> > > this on the context API itself with an appropriate fallback behavior for
> when
> > > the extension isn't supported?
> > 
> > This feature is currently only used for Android WebView which is hardwired
to
> > use this context type so the downcast would be safe. I thought it would be a
> > reasonable compromise to move it to the interface API after other people
start
> > using this code. If you disagree, let me know and I'll prepare a blink
patch.
> 
> Keep in mind that ChromeOS might implement 0-copy uploads too. And so could
any
> other platforms. If you properly expose it, it will be likelier that this gets
> shared like it should.

The blink patch that adds this API is in the CQ:
https://chromiumcodereview.appspot.com/14502005/

[kaanb1, 2013-04-25 23:55:05]

I'll add unit tests once I address the comments on the gpu/ patch.

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

https://codereview.chromium.org/14409006/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:897: static_cast<Context3DImpl*>(context3d);
On 2013/04/25 23:28:14, Daniel Sievers wrote:
> On 2013/04/25 22:43:29, kaanb1 wrote:
> > On 2013/04/25 22:28:56, jamesr wrote:
> > > you shouldn't be downcasting like this. it means that the
> > use_gpu_memory_buffer
> > > setting will cause crashes for anyone using a different context type. why
> > isn't
> > > this on the context API itself with an appropriate fallback behavior for
> when
> > > the extension isn't supported?
> > 
> > This feature is currently only used for Android WebView which is hardwired
to
> > use this context type so the downcast would be safe. I thought it would be a
> > reasonable compromise to move it to the interface API after other people
start
> > using this code. If you disagree, let me know and I'll prepare a blink
patch.
> 
> Keep in mind that ChromeOS might implement 0-copy uploads too. And so could
any
> other platforms. If you properly expose it, it will be likelier that this gets
> shared like it should.

Done.

https://codereview.chromium.org/14409006/diff/1/cc/trees/layer_tree_host_impl.cc
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/14409006/diff/1/cc/trees/layer_tree_host_impl...
cc/trees/layer_tree_host_impl.cc:1345: if
(CommandLine::ForCurrentProcess()->HasSwitch(
On 2013/04/25 22:50:46, jamesr wrote:
> I don't think this is a good way to plumb this setting in since it's global
for
> the process but not necessarily something you want set the same way for every
> compositor instance in that process.  Instead, what we normally do is add an
> entry to cc::LayerTreeSettings and have code outside cc:: (that's aware of the
> process model in use) map the command line flag to the setting.

Done.

[kaanb, 2013-05-10 01:22:43]

I have patched in 14456004 and made modification to ResourceProvider to use the
new API and added a unittest.

Could I get a review for cc/ files?

[jamesr, 2013-05-10 01:29:08]

Redirecting review for cc/ to reveman.  I can stamp for owners on the rest after
the primary reviewers are happy.

[reveman, 2013-05-10 18:38:51]

I prefer to wait for gpu side changes to land before I start looking at this.

[kaanb, 2013-05-16 19:12:52]

On 2013/05/10 18:38:51, David Reveman wrote:
> I prefer to wait for gpu side changes to land before I start looking at this.

I have rebased past the gpu side changes and uploaded the patch again. I also
tested on an actual device, please take another look.

[reveman, 2013-05-17 01:48:07]

I don't think that masquerading these zero-copy buffers as pixel buffers is the
correct approach.

We need to treat zero-copy differently in the tile manager for ideal performance
(getting this working is blocked on crbug.com/241644) so it's much cleaner to
expose this as a new set of ResourceProvider entry points. My review feedback
below reflects this:

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:926: resource->size.width(),
resource->size.height(), GL_RGBA8_OES);
please put this in a new AcquireImage() function instead.

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:965: resource->image_id = 0;
and this in ReleaseImage()

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1000:
context3d->mapImageCHROMIUM(resource->image_id, GL_READ_WRITE));
and I prefer this in a MapImage() function. We might want to expose
GL_READ_WRITE as a parameter in the future but this initial patch doesn't need
to do that.

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1030:
context3d->unmapImageCHROMIUM(resource->image_id);
UnmapImage.

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1045: DCHECK(!resource->image_id);
I don't think this DCHECK should be here.

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1155:
context3d->bindTexImage2DCHROMIUM(GL_TEXTURE_2D, resource->image_id);
please add a BindImage() function for this instead.

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1235: resource->image_id = 0;
no need for any of these changes.

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1274: // for GpuMemoryBuffers?
I don't think this affects zero-copy. just like it doesn't affect async uploads.

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
cc/resources/resource_provider.h:322: void SetUseGpuMemoryBuffers(bool
use_gpu_memory_buffers);
You don't need this when exposing this using a new set of entry points.

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
cc/resources/resource_provider.h:413: bool use_gpu_memory_buffers_;
remove this

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
File cc/resources/resource_provider_unittest.cc (right):

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
cc/resources/resource_provider_unittest.cc:29: using WebKit::WGC3Dboolean;
what is this for?

https://codereview.chromium.org/14409006/diff/32001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/14409006/diff/32001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:1386: if (settings_.use_gpu_memory_buffers)
Pass settings_.use_gpu_memory_buffers to the tile manager instead, which for now
can just ignore the value.

[kaanb1, 2013-05-17 02:28:55]

Sure, I can change the API but then we'll need to wait when the fix for
crbug.com/241644 lands to be able to use zero-copy buffers in WebView. joth@ are
we fine with that?

On 2013/05/17 01:48:07, David Reveman wrote:
> I don't think that masquerading these zero-copy buffers as pixel buffers is
the
> correct approach.
> 
> We need to treat zero-copy differently in the tile manager for ideal
performance
> (getting this working is blocked on crbug.com/241644) so it's much cleaner to
> expose this as a new set of ResourceProvider entry points. My review feedback
> below reflects this:
> 
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> File cc/resources/resource_provider.cc (right):
> 
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> cc/resources/resource_provider.cc:926: resource->size.width(),
> resource->size.height(), GL_RGBA8_OES);
> please put this in a new AcquireImage() function instead.
> 
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> cc/resources/resource_provider.cc:965: resource->image_id = 0;
> and this in ReleaseImage()
> 
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> cc/resources/resource_provider.cc:1000:
> context3d->mapImageCHROMIUM(resource->image_id, GL_READ_WRITE));
> and I prefer this in a MapImage() function. We might want to expose
> GL_READ_WRITE as a parameter in the future but this initial patch doesn't need
> to do that.
> 
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> cc/resources/resource_provider.cc:1030:
> context3d->unmapImageCHROMIUM(resource->image_id);
> UnmapImage.
> 
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> cc/resources/resource_provider.cc:1045: DCHECK(!resource->image_id);
> I don't think this DCHECK should be here.
> 
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> cc/resources/resource_provider.cc:1155:
> context3d->bindTexImage2DCHROMIUM(GL_TEXTURE_2D, resource->image_id);
> please add a BindImage() function for this instead.
> 
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> cc/resources/resource_provider.cc:1235: resource->image_id = 0;
> no need for any of these changes.
> 
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> cc/resources/resource_provider.cc:1274: // for GpuMemoryBuffers?
> I don't think this affects zero-copy. just like it doesn't affect async
uploads.
> 
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> File cc/resources/resource_provider.h (right):
> 
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> cc/resources/resource_provider.h:322: void SetUseGpuMemoryBuffers(bool
> use_gpu_memory_buffers);
> You don't need this when exposing this using a new set of entry points.
> 
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> cc/resources/resource_provider.h:413: bool use_gpu_memory_buffers_;
> remove this
> 
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> File cc/resources/resource_provider_unittest.cc (right):
> 
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> cc/resources/resource_provider_unittest.cc:29: using WebKit::WGC3Dboolean;
> what is this for?
> 
>
https://codereview.chromium.org/14409006/diff/32001/cc/trees/layer_tree_host_...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/14409006/diff/32001/cc/trees/layer_tree_host_...
> cc/trees/layer_tree_host_impl.cc:1386: if (settings_.use_gpu_memory_buffers)
> Pass settings_.use_gpu_memory_buffers to the tile manager instead, which for
now
> can just ignore the value.

[joth, 2013-05-17 18:13:44]

On 2013/05/17 02:28:55, kaanb1 wrote:
> Sure, I can change the API but then we'll need to wait when the fix for
> crbug.com/241644 lands to be able to use zero-copy buffers in WebView. joth@
are
> we fine with that?

It depends on what the timeline here is. We're trying to get switched over to
the new graphics pipeline in the coming 2 weeks, it would be way better if we
can have the gralloc based resources all in place so we are debugging the real
intended final configuration, rather than wasting more time debugging
yet-another temporary graphics architecture.

Assuming crbug.com/241644 is going to take at least few weeks itself, do we have
any options for decoupling this dependency in the meantime? something like
implement what we need with conditional code in tile-manager calling the new
ResourceProvider APIs, and then factor that out to the ResourceInitializer
abstraction as a follow up?

Cheers





> 
> On 2013/05/17 01:48:07, David Reveman wrote:
> > I don't think that masquerading these zero-copy buffers as pixel buffers is
> the
> > correct approach.
> > 
> > We need to treat zero-copy differently in the tile manager for ideal
> performance
> > (getting this working is blocked on crbug.com/241644) so it's much cleaner
to
> > expose this as a new set of ResourceProvider entry points. My review
feedback
> > below reflects this:
> > 
> >
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> > File cc/resources/resource_provider.cc (right):
> > 
> >
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> > cc/resources/resource_provider.cc:926: resource->size.width(),
> > resource->size.height(), GL_RGBA8_OES);
> > please put this in a new AcquireImage() function instead.
> > 
> >
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> > cc/resources/resource_provider.cc:965: resource->image_id = 0;
> > and this in ReleaseImage()
> > 
> >
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> > cc/resources/resource_provider.cc:1000:
> > context3d->mapImageCHROMIUM(resource->image_id, GL_READ_WRITE));
> > and I prefer this in a MapImage() function. We might want to expose
> > GL_READ_WRITE as a parameter in the future but this initial patch doesn't
need
> > to do that.
> > 
> >
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> > cc/resources/resource_provider.cc:1030:
> > context3d->unmapImageCHROMIUM(resource->image_id);
> > UnmapImage.
> > 
> >
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> > cc/resources/resource_provider.cc:1045: DCHECK(!resource->image_id);
> > I don't think this DCHECK should be here.
> > 
> >
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> > cc/resources/resource_provider.cc:1155:
> > context3d->bindTexImage2DCHROMIUM(GL_TEXTURE_2D, resource->image_id);
> > please add a BindImage() function for this instead.
> > 
> >
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> > cc/resources/resource_provider.cc:1235: resource->image_id = 0;
> > no need for any of these changes.
> > 
> >
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> > cc/resources/resource_provider.cc:1274: // for GpuMemoryBuffers?
> > I don't think this affects zero-copy. just like it doesn't affect async
> uploads.
> > 
> >
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> > File cc/resources/resource_provider.h (right):
> > 
> >
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> > cc/resources/resource_provider.h:322: void SetUseGpuMemoryBuffers(bool
> > use_gpu_memory_buffers);
> > You don't need this when exposing this using a new set of entry points.
> > 
> >
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> > cc/resources/resource_provider.h:413: bool use_gpu_memory_buffers_;
> > remove this
> > 
> >
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> > File cc/resources/resource_provider_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
> > cc/resources/resource_provider_unittest.cc:29: using WebKit::WGC3Dboolean;
> > what is this for?
> > 
> >
>
https://codereview.chromium.org/14409006/diff/32001/cc/trees/layer_tree_host_...
> > File cc/trees/layer_tree_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/14409006/diff/32001/cc/trees/layer_tree_host_...
> > cc/trees/layer_tree_host_impl.cc:1386: if (settings_.use_gpu_memory_buffers)
> > Pass settings_.use_gpu_memory_buffers to the tile manager instead, which for
> now
> > can just ignore the value.

[reveman, 2013-05-17 20:08:06]

On 2013/05/17 18:13:44, joth wrote:
> On 2013/05/17 02:28:55, kaanb1 wrote:
> > Sure, I can change the API but then we'll need to wait when the fix for
> > crbug.com/241644 lands to be able to use zero-copy buffers in WebView. joth@
> are
> > we fine with that?
> 
> It depends on what the timeline here is. We're trying to get switched over to
> the new graphics pipeline in the coming 2 weeks, it would be way better if we
> can have the gralloc based resources all in place so we are debugging the real
> intended final configuration, rather than wasting more time debugging
> yet-another temporary graphics architecture.
> 
> Assuming crbug.com/241644 is going to take at least few weeks itself, do we
have
> any options for decoupling this dependency in the meantime? something like
> implement what we need with conditional code in tile-manager calling the new
> ResourceProvider APIs, and then factor that out to the ResourceInitializer
> abstraction as a follow up?
> 

I don't expect crbug.com/241644 to take much more than a week. It's my top
priority and I already have most of it done. Any temporary code added to the
tile manager will likely slow us down in terms of getting this working correctly
so I would advise against that.

David

[kaanb, 2013-05-17 21:27:36]

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:926: resource->size.width(),
resource->size.height(), GL_RGBA8_OES);
On 2013/05/17 01:48:08, David Reveman wrote:
> please put this in a new AcquireImage() function instead.

Done.

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:965: resource->image_id = 0;
On 2013/05/17 01:48:08, David Reveman wrote:
> and this in ReleaseImage()

Done.

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1000:
context3d->mapImageCHROMIUM(resource->image_id, GL_READ_WRITE));
On 2013/05/17 01:48:08, David Reveman wrote:
> and I prefer this in a MapImage() function. We might want to expose
> GL_READ_WRITE as a parameter in the future but this initial patch doesn't need
> to do that.

Done.

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1030:
context3d->unmapImageCHROMIUM(resource->image_id);
On 2013/05/17 01:48:08, David Reveman wrote:
> UnmapImage.

Done.

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1045: DCHECK(!resource->image_id);
On 2013/05/17 01:48:08, David Reveman wrote:
> I don't think this DCHECK should be here.

Done.

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1155:
context3d->bindTexImage2DCHROMIUM(GL_TEXTURE_2D, resource->image_id);
On 2013/05/17 01:48:08, David Reveman wrote:
> please add a BindImage() function for this instead.

Done.

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1235: resource->image_id = 0;
On 2013/05/17 01:48:08, David Reveman wrote:
> no need for any of these changes.

Done.

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1274: // for GpuMemoryBuffers?
On 2013/05/17 01:48:08, David Reveman wrote:
> I don't think this affects zero-copy. just like it doesn't affect async
uploads.

Acknowledged.

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
cc/resources/resource_provider.h:322: void SetUseGpuMemoryBuffers(bool
use_gpu_memory_buffers);
On 2013/05/17 01:48:08, David Reveman wrote:
> You don't need this when exposing this using a new set of entry points.

Done.

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
cc/resources/resource_provider.h:413: bool use_gpu_memory_buffers_;
On 2013/05/17 01:48:08, David Reveman wrote:
> remove this

Done.

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
File cc/resources/resource_provider_unittest.cc (right):

https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_pro...
cc/resources/resource_provider_unittest.cc:29: using WebKit::WGC3Dboolean;
On 2013/05/17 01:48:08, David Reveman wrote:
> what is this for?

Removed.

https://codereview.chromium.org/14409006/diff/32001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/14409006/diff/32001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:1386: if (settings_.use_gpu_memory_buffers)
On 2013/05/17 01:48:08, David Reveman wrote:
> Pass settings_.use_gpu_memory_buffers to the tile manager instead, which for
now
> can just ignore the value.

Done.

[kaanb, 2013-05-20 22:08:28]

Friendly ping!

[reveman, 2013-05-20 23:16:01]

https://codereview.chromium.org/14409006/diff/44013/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/14409006/diff/44013/cc/base/switches.cc#newco...
cc/base/switches.cc:137: // Use GPU memory buffers within the ResourceProvider.
This is no longer accurate. This is now a tile manager setting.

https://codereview.chromium.org/14409006/diff/44013/cc/debug/fake_web_graphic...
File cc/debug/fake_web_graphics_context_3d.h (right):

https://codereview.chromium.org/14409006/diff/44013/cc/debug/fake_web_graphic...
cc/debug/fake_web_graphics_context_3d.h:602: virtual void
destroyImageCHROMIUM(WebKit::WGC3Duint imageId) {}
nit: image_id

https://codereview.chromium.org/14409006/diff/44013/cc/debug/fake_web_graphic...
cc/debug/fake_web_graphics_context_3d.h:604: WebKit::WGC3Duint imageId,
nit: image_id

https://codereview.chromium.org/14409006/diff/44013/cc/debug/fake_web_graphic...
cc/debug/fake_web_graphics_context_3d.h:608: WebKit::WGC3Duint imageId,
nit: image_id

https://codereview.chromium.org/14409006/diff/44013/cc/debug/fake_web_graphic...
cc/debug/fake_web_graphics_context_3d.h:610: virtual void
unmapImageCHROMIUM(WebKit::WGC3Duint imageId) {}
nit: image_id

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
cc/resources/resource_provider.cc:872: DCHECK(!source->image_id);
hm, I think we might need to support this. any specific reason why you're
preventing it?

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
cc/resources/resource_provider.cc:938: DCHECK(!resource->image_id);
technically, there's nothing that stops us from using both pixel buffers and
image, right? I guess it's OK to prevent this for now but please add a DCHECK to
AcquirePixelBuffer too.

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1271: DCHECK_EQ(0u, resource->image_id);
AcquirePixelBuffer can be called twice without an error. I would prefer if
AcquireImage is consistent with that.

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1290: DCHECK(resource->image_id);
Same thing here. It's Ok to call ReleasePixelBuffer without first calling
AcquirePixelBuffer.

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1303: 
It's probably best to have a DCHECK(ReadLockFenceHasPassed(resource)) here.

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
cc/resources/resource_provider.h:295: // Methods for GLImage zero-copy API.
I think these functions could need a bit more of a description. Can you split
them up like the pixel buffer functions above and add a line or to describing
what they do? Btw, the GLImage name is an implementation detail, please avoid
using it here.

https://codereview.chromium.org/14409006/diff/44013/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/14409006/diff/44013/cc/resources/tile_manager...
cc/resources/tile_manager.h:92: void SetUseGpuMemoryBuffers(bool
use_gpu_memory_buffers);
can we pass this to ctor instead?

https://codereview.chromium.org/14409006/diff/44013/cc/resources/tile_manager...
cc/resources/tile_manager.h:215: bool use_gpu_memory_buffers_;
No need to add this. Just pass the setting to the ctor for now.

https://codereview.chromium.org/14409006/diff/44013/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/14409006/diff/44013/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:1394: if (settings_.use_gpu_memory_buffers)
Do we need to filter this setting in
LayerTreeHost::OnCreateAndInitializeOutputSurfaceAttempted and make sure it's
actually supported?

[kaanb, 2013-05-21 00:52:29]

https://codereview.chromium.org/14409006/diff/44013/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/14409006/diff/44013/cc/base/switches.cc#newco...
cc/base/switches.cc:137: // Use GPU memory buffers within the ResourceProvider.
On 2013/05/20 23:16:01, David Reveman wrote:
> This is no longer accurate. This is now a tile manager setting.

Done.

https://codereview.chromium.org/14409006/diff/44013/cc/debug/fake_web_graphic...
File cc/debug/fake_web_graphics_context_3d.h (right):

https://codereview.chromium.org/14409006/diff/44013/cc/debug/fake_web_graphic...
cc/debug/fake_web_graphics_context_3d.h:602: virtual void
destroyImageCHROMIUM(WebKit::WGC3Duint imageId) {}
On 2013/05/20 23:16:01, David Reveman wrote:
> nit: image_id

Done.

https://codereview.chromium.org/14409006/diff/44013/cc/debug/fake_web_graphic...
cc/debug/fake_web_graphics_context_3d.h:604: WebKit::WGC3Duint imageId,
On 2013/05/20 23:16:01, David Reveman wrote:
> nit: image_id

Done.

https://codereview.chromium.org/14409006/diff/44013/cc/debug/fake_web_graphic...
cc/debug/fake_web_graphics_context_3d.h:608: WebKit::WGC3Duint imageId,
On 2013/05/20 23:16:01, David Reveman wrote:
> nit: image_id

Done.

https://codereview.chromium.org/14409006/diff/44013/cc/debug/fake_web_graphic...
cc/debug/fake_web_graphics_context_3d.h:610: virtual void
unmapImageCHROMIUM(WebKit::WGC3Duint imageId) {}
On 2013/05/20 23:16:01, David Reveman wrote:
> nit: image_id

Done.

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
cc/resources/resource_provider.cc:872: DCHECK(!source->image_id);
On 2013/05/20 23:16:01, David Reveman wrote:
> hm, I think we might need to support this. any specific reason why you're
> preventing it?

I'm not sure if this would be relevant for WebView, if so I can open a follow-up
bug to track it. sievers@ what do you think?

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
cc/resources/resource_provider.cc:938: DCHECK(!resource->image_id);
On 2013/05/20 23:16:01, David Reveman wrote:
> technically, there's nothing that stops us from using both pixel buffers and
> image, right? I guess it's OK to prevent this for now but please add a DCHECK
to
> AcquirePixelBuffer too.

Done.

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1271: DCHECK_EQ(0u, resource->image_id);
On 2013/05/20 23:16:01, David Reveman wrote:
> AcquirePixelBuffer can be called twice without an error. I would prefer if
> AcquireImage is consistent with that.

I think we'd be leaking the first buffer if we call AcquirePixelBuffer twice,
right? So rather than inheriting that practice we should start with a clean
plate and do the things the right way?

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1290: DCHECK(resource->image_id);
On 2013/05/20 23:16:01, David Reveman wrote:
> Same thing here. It's Ok to call ReleasePixelBuffer without first calling
> AcquirePixelBuffer.

Again, "double free" is almost always an anti-pattern that causes a plethora of
issues. Given that this is a new code path double free of an image would
probably indicate a bug in some other part of the pipeline. So I'd rather keep
it here.

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1303: 
On 2013/05/20 23:16:01, David Reveman wrote:
> It's probably best to have a DCHECK(ReadLockFenceHasPassed(resource)) here.

Done, should we add the same DCHECK for BindImage as well?

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
cc/resources/resource_provider.h:295: // Methods for GLImage zero-copy API.
On 2013/05/20 23:16:01, David Reveman wrote:
> I think these functions could need a bit more of a description. Can you split
> them up like the pixel buffer functions above and add a line or to describing
> what they do? Btw, the GLImage name is an implementation detail, please avoid
> using it here.

Done.

https://codereview.chromium.org/14409006/diff/44013/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/14409006/diff/44013/cc/resources/tile_manager...
cc/resources/tile_manager.h:92: void SetUseGpuMemoryBuffers(bool
use_gpu_memory_buffers);
On 2013/05/20 23:16:01, David Reveman wrote:
> can we pass this to ctor instead?

Done.

https://codereview.chromium.org/14409006/diff/44013/cc/resources/tile_manager...
cc/resources/tile_manager.h:215: bool use_gpu_memory_buffers_;
On 2013/05/20 23:16:01, David Reveman wrote:
> No need to add this. Just pass the setting to the ctor for now.

Done.

https://codereview.chromium.org/14409006/diff/44013/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/14409006/diff/44013/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:1394: if (settings_.use_gpu_memory_buffers)
On 2013/05/20 23:16:01, David Reveman wrote:
> Do we need to filter this setting in
> LayerTreeHost::OnCreateAndInitializeOutputSurfaceAttempted and make sure it's
> actually supported?

Done.

[reveman, 2013-05-21 02:20:19]

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1271: DCHECK_EQ(0u, resource->image_id);
On 2013/05/21 00:52:29, kaanb wrote:
> On 2013/05/20 23:16:01, David Reveman wrote:
> > AcquirePixelBuffer can be called twice without an error. I would prefer if
> > AcquireImage is consistent with that.
> 
> I think we'd be leaking the first buffer if we call AcquirePixelBuffer twice,
> right? So rather than inheriting that practice we should start with a clean
> plate and do the things the right way? 

It's perfectly fine to call glBufferData multiple times in AcquirePixelBuffer.
It just redefines the storage.

The current pooling and reuse of resources in the tile manager will require us
to either be allowed to call AcquireImage multiple times or add some code that
track what resources we have acquired images for. Unless we intend to release
and acquire a new image every time we reuse a buffer. I'm assuming not.

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1290: DCHECK(resource->image_id);
ReleasePixelBuffer handles this properly. Maybe it makes sense to change it.
Either way, I prefer if we're consistent unless there's a good reason why we
shouldn't be.

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1303: 
On 2013/05/21 00:52:29, kaanb wrote:
> On 2013/05/20 23:16:01, David Reveman wrote:
> > It's probably best to have a DCHECK(ReadLockFenceHasPassed(resource)) here.
> 
> Done, should we add the same DCHECK for BindImage as well?

I don't think so. BindImage doesn't modify the image.

https://codereview.chromium.org/14409006/diff/56001/cc/base/switches.h
File cc/base/switches.h (right):

https://codereview.chromium.org/14409006/diff/56001/cc/base/switches.h#newcode43
cc/base/switches.h:43: CC_EXPORT extern const char kUseGpuMemoryBuffers[];
you need to add this to:
chrome/browser/chromeos/login/chrome_restart_request.cc
content/browser/renderer_host/render_process_host_impl.cc

https://codereview.chromium.org/14409006/diff/56001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/14409006/diff/56001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.h:82: bool use_gpu_memory_buffers;
Sorry I didn't think of this earlier but for consistency it would be nice to
name this "using_gpu_memory_buffers" and set it in the same way
using_gpu_memory_manager is set in GLRenderer::Initialize() instead of the code
you currently have in OnCreateAndInitializeOutputSurfaceAttempted.

[kaanb, 2013-05-21 17:53:48]

+piman as reviewer for chrome/browser/chromeos/login

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1271: DCHECK_EQ(0u, resource->image_id);
On 2013/05/21 02:20:19, David Reveman wrote:
> On 2013/05/21 00:52:29, kaanb wrote:
> > On 2013/05/20 23:16:01, David Reveman wrote:
> > > AcquirePixelBuffer can be called twice without an error. I would prefer if
> > > AcquireImage is consistent with that.
> > 
> > I think we'd be leaking the first buffer if we call AcquirePixelBuffer
twice,
> > right? So rather than inheriting that practice we should start with a clean
> > plate and do the things the right way? 
> 
> It's perfectly fine to call glBufferData multiple times in AcquirePixelBuffer.
> It just redefines the storage.
> 
> The current pooling and reuse of resources in the tile manager will require us
> to either be allowed to call AcquireImage multiple times or add some code that
> track what resources we have acquired images for. Unless we intend to release
> and acquire a new image every time we reuse a buffer. I'm assuming not.

I looked at the code and for pixel buffers glBufferData frees the previous
buffer if the bufferData is called successively with the same buffer_id. So I
release the previous image within ResourceProvider to be consistent. createImage
is like createBuffer + bufferData as it generates a new id and allocates the
buffer each time it is called so it is cleaner to free the previous buffer
within ResourceProvider.

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1290: DCHECK(resource->image_id);
On 2013/05/21 02:20:19, David Reveman wrote:
> ReleasePixelBuffer handles this properly. Maybe it makes sense to change it.
> Either way, I prefer if we're consistent unless there's a good reason why we
> shouldn't be.

ReleasePixelBuffer simply returns if pixel_buffer_id is 0, so I decided to
return if the image_id is 0.

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1303: 
On 2013/05/21 02:20:19, David Reveman wrote:
> On 2013/05/21 00:52:29, kaanb wrote:
> > On 2013/05/20 23:16:01, David Reveman wrote:
> > > It's probably best to have a DCHECK(ReadLockFenceHasPassed(resource))
here.
> > 
> > Done, should we add the same DCHECK for BindImage as well?
> 
> I don't think so. BindImage doesn't modify the image.

Acknowledged.

https://codereview.chromium.org/14409006/diff/56001/cc/base/switches.h
File cc/base/switches.h (right):

https://codereview.chromium.org/14409006/diff/56001/cc/base/switches.h#newcode43
cc/base/switches.h:43: CC_EXPORT extern const char kUseGpuMemoryBuffers[];
On 2013/05/21 02:20:19, David Reveman wrote:
> you need to add this to:
> chrome/browser/chromeos/login/chrome_restart_request.cc
> content/browser/renderer_host/render_process_host_impl.cc

Done.

https://codereview.chromium.org/14409006/diff/56001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/14409006/diff/56001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.h:82: bool use_gpu_memory_buffers;
On 2013/05/21 02:20:19, David Reveman wrote:
> Sorry I didn't think of this earlier but for consistency it would be nice to
> name this "using_gpu_memory_buffers" and set it in the same way
> using_gpu_memory_manager is set in GLRenderer::Initialize() instead of the
code
> you currently have in OnCreateAndInitializeOutputSurfaceAttempted.

Done. joth@ and I thought it may be good to disable the use of the zero-copy
buffers from the embedder even though the renderer supports it, especially while
debugging the new WebView path. What would be the best way of doing that?

[piman, 2013-05-21 18:15:14]

LGTM+nits

https://codereview.chromium.org/14409006/diff/61001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/14409006/diff/61001/cc/base/switches.cc#newco...
cc/base/switches.cc:138: const char kUseGpuMemoryBuffers[] =
"use-gpu-memory-buffers";
nit: the flag is about as undescriptive as can be.
How about --cc-use-map-image, named after the extension?

https://codereview.chromium.org/14409006/diff/61001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/14409006/diff/61001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:194: extensions.count("GL_CHROMIUM_map_image") > 0;
nit: can you add this to delegating_renderer.cc too?

[no sievers, 2013-05-21 19:15:46]

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
cc/resources/resource_provider.cc:872: DCHECK(!source->image_id);
On 2013/05/21 00:52:29, kaanb wrote:
> On 2013/05/20 23:16:01, David Reveman wrote:
> > hm, I think we might need to support this. any specific reason why you're
> > preventing it?
> 
> I'm not sure if this would be relevant for WebView, if so I can open a
follow-up
> bug to track it. sievers@ what do you think? 
> 

I'd just remove the DCHECK().

WebView would not use delegated rendering, nor have a browser compositor.

Also, I'm not sure that this does not just work. We might not transfer the
GLImage ref to the other client texture through the mailbox (although Antoine's
texture sharing work might address that). But not sure it matters since it won't
be needed after bindTexImage2D().

[piman, 2013-05-21 19:50:39]

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
cc/resources/resource_provider.cc:872: DCHECK(!source->image_id);
On 2013/05/21 19:15:46, Daniel Sievers wrote:
> On 2013/05/21 00:52:29, kaanb wrote:
> > On 2013/05/20 23:16:01, David Reveman wrote:
> > > hm, I think we might need to support this. any specific reason why you're
> > > preventing it?
> > 
> > I'm not sure if this would be relevant for WebView, if so I can open a
> follow-up
> > bug to track it. sievers@ what do you think? 
> > 
> 
> I'd just remove the DCHECK().
> 
> WebView would not use delegated rendering, nor have a browser compositor.
> 
> Also, I'm not sure that this does not just work. We might not transfer the
> GLImage ref to the other client texture through the mailbox (although
Antoine's
> texture sharing work might address that).

It probably doesn't work right now, but should work as soon as
https://codereview.chromium.org/14188053/ lands. I think it would be good to
just remove the DCHECK. In the future, other Image implementation may work
out-of-process, and it'd be good to leverage this if it's available.

> But not sure it matters since it won't
> be needed after bindTexImage2D().

[kaanb, 2013-05-21 20:25:41]

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_pro...
cc/resources/resource_provider.cc:872: DCHECK(!source->image_id);
On 2013/05/21 19:50:39, piman wrote:
> On 2013/05/21 19:15:46, Daniel Sievers wrote:
> > On 2013/05/21 00:52:29, kaanb wrote:
> > > On 2013/05/20 23:16:01, David Reveman wrote:
> > > > hm, I think we might need to support this. any specific reason why
you're
> > > > preventing it?
> > > 
> > > I'm not sure if this would be relevant for WebView, if so I can open a
> > follow-up
> > > bug to track it. sievers@ what do you think? 
> > > 
> > 
> > I'd just remove the DCHECK().
> > 
> > WebView would not use delegated rendering, nor have a browser compositor.
> > 
> > Also, I'm not sure that this does not just work. We might not transfer the
> > GLImage ref to the other client texture through the mailbox (although
> Antoine's
> > texture sharing work might address that).
> 
> It probably doesn't work right now, but should work as soon as
> https://codereview.chromium.org/14188053/ lands. I think it would be good to
> just remove the DCHECK. In the future, other Image implementation may work
> out-of-process, and it'd be good to leverage this if it's available.
> 
> > But not sure it matters since it won't
> > be needed after bindTexImage2D().

Done.

https://codereview.chromium.org/14409006/diff/61001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/14409006/diff/61001/cc/base/switches.cc#newco...
cc/base/switches.cc:138: const char kUseGpuMemoryBuffers[] =
"use-gpu-memory-buffers";
On 2013/05/21 18:15:14, piman wrote:
> nit: the flag is about as undescriptive as can be.
> How about --cc-use-map-image, named after the extension?

Done.

https://codereview.chromium.org/14409006/diff/61001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/14409006/diff/61001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:194: extensions.count("GL_CHROMIUM_map_image") > 0;
On 2013/05/21 18:15:14, piman wrote:
> nit: can you add this to delegating_renderer.cc too?

Done.

[piman, 2013-05-21 20:27:43]

lgtm

[reveman, 2013-05-22 18:56:41]

https://codereview.chromium.org/14409006/diff/72002/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/14409006/diff/72002/cc/resources/tile_manager...
cc/resources/tile_manager.h:70: bool use_gpu_memory_buffers);
why not use_map_image here as well?

https://codereview.chromium.org/14409006/diff/72002/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/14409006/diff/72002/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:1393: settings_.use_map_image));
settings_.use_map_image doesn't seem to take output surface capabilities() into
account. am I missing something or should you be using
output_surface->capabilities().using_map_image here?

[kaanb, 2013-05-22 19:13:03]

https://codereview.chromium.org/14409006/diff/72002/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/14409006/diff/72002/cc/resources/tile_manager...
cc/resources/tile_manager.h:70: bool use_gpu_memory_buffers);
On 2013/05/22 18:56:41, David Reveman wrote:
> why not use_map_image here as well?

Done.

https://codereview.chromium.org/14409006/diff/72002/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/14409006/diff/72002/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:1393: settings_.use_map_image));
On 2013/05/22 18:56:41, David Reveman wrote:
> settings_.use_map_image doesn't seem to take output surface capabilities()
into
> account. am I missing something or should you be using
> output_surface->capabilities().using_map_image here?

We plan to use a command flag to initially control this feature. After that the
plan is to enable it by default for WebView.

[reveman, 2013-05-22 19:51:19]

https://codereview.chromium.org/14409006/diff/72002/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/14409006/diff/72002/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:1393: settings_.use_map_image));
but shouldn't it be "settings_.use_map_image && capabilities().using_map_image"?
I don't understand what the capabilities part is about otherwise.

[kaanb, 2013-05-22 20:21:01]

https://codereview.chromium.org/14409006/diff/72002/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/14409006/diff/72002/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:1393: settings_.use_map_image));
On 2013/05/22 19:51:19, David Reveman wrote:
> but shouldn't it be "settings_.use_map_image &&
capabilities().using_map_image"?
> I don't understand what the capabilities part is about otherwise.

Okay, I'm using RendererCapabilities, not OutputSurface capabilities.

Please take another look.

[reveman, 2013-05-22 22:53:41]

lgtm, thanks for making that last change!

[jamesr, 2013-05-22 22:56:36]

lgtm once you fix the switch's symbol name

https://codereview.chromium.org/14409006/diff/91001/cc/base/switches.h
File cc/base/switches.h (right):

https://codereview.chromium.org/14409006/diff/91001/cc/base/switches.h#newcode43
cc/base/switches.h:43: CC_EXPORT extern const char kCCUseMapImage[];
no need for 'CC', you're in namespace cc::switches

[kaanb, 2013-05-22 23:01:23]

https://codereview.chromium.org/14409006/diff/91001/cc/base/switches.h
File cc/base/switches.h (right):

https://codereview.chromium.org/14409006/diff/91001/cc/base/switches.h#newcode43
cc/base/switches.h:43: CC_EXPORT extern const char kCCUseMapImage[];
On 2013/05/22 22:56:36, jamesr wrote:
> no need for 'CC', you're in namespace cc::switches

Done.

[reveman, 2013-05-22 23:51:49]

https://codereview.chromium.org/14409006/diff/86002/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/14409006/diff/86002/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:1439: // Renderer may be NULL in unit tests.
is the problem not just that you're creating the renderer below?

[kaanb, 2013-05-22 23:59:27]

https://codereview.chromium.org/14409006/diff/86002/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/14409006/diff/86002/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:1439: // Renderer may be NULL in unit tests.
On 2013/05/22 23:51:49, David Reveman wrote:
> is the problem not just that you're creating the renderer below?

Yes, I moved TileManager creation after the Renderer creation.

[commit-bot: I haz the power, 2013-05-23 00:00:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/14409006/103001

[commit-bot: I haz the power, 2013-05-23 00:42:38]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-23 00:55:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/14409006/84002

[commit-bot: I haz the power, 2013-05-23 06:48:44]

Change committed as 201718

[danakj, 2013-09-03 17:24:55]

Is there a reason this guards the renderer caps "using_map_image" on the layer
tree settings in gl renderer, but not in delegating renderer?

[kaanb1, 2013-09-03 17:40:15]

There's no reason. Good catch. I'll send a fix now.


On Tue, Sep 3, 2013 at 10:24 AM, <danakj@chromium.org> wrote:

> Is there a reason this guards the renderer caps "using_map_image" on the
> layer
> tree settings in gl renderer, but not in delegating renderer?
>
>
https://chromiumcodereview.**appspot.com/14409006/<https://chromiumcodereview...
>



-- 
Thanks,
Kaan

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

[danakj, 2013-09-03 17:45:49]

Ok I have one up.


On Tue, Sep 3, 2013 at 1:40 PM, Kaan Baloglu <kaanb@google.com> wrote:

> There's no reason. Good catch. I'll send a fix now.
>
>
> On Tue, Sep 3, 2013 at 10:24 AM, <danakj@chromium.org> wrote:
>
>> Is there a reason this guards the renderer caps "using_map_image" on the
>> layer
>> tree settings in gl renderer, but not in delegating renderer?
>>
>>
https://chromiumcodereview.**appspot.com/14409006/<https://chromiumcodereview...
>>
>
>
>
> --
> Thanks,
> Kaan
>

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