cc: Remove the ability to specify a GL texture target

cc: Remove the ability to specify a GL texture target

This feature is only used to, in a very roundabout way, specify that we
want to use the GLImage texture target (if available) for the resource.
Make this more clear, by replacing CreateResourceWithTextureTarget with
CreateResourceWithImageTextureTarget, which does not take a target
argument.

Similarly add ResourcePool::CreateForImageTextureTarget as a way to
create resource pools that will have this behavior.

BUG=533677
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/a446856b8c1d867785bc191d3d1e8fff94ea7905
Cr-Commit-Position: refs/heads/master@{#356965}

[ccameron, 2015-10-28 23:24:10]

ccameron@chromium.org changed reviewers:
+ reveman@chromium.org

[ccameron, 2015-10-28 23:24:10]

ptal

[reveman, 2015-10-29 14:11:39]

https://codereview.chromium.org/1422103003/diff/1/cc/resources/resource_pool.h
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/1422103003/diff/1/cc/resources/resource_pool....
cc/resources/resource_pool.h:24: static scoped_ptr<ResourcePool>
CreateForImageTextureTarget(
I think this change will break the ongoing work to support compressed tile
textures as we need a mix of texture targets in that case. Some tiles will be
compressed and some will not. E.g. the driver might only support compression
opaque contents.

https://codereview.chromium.org/1422103003/diff/1/content/browser/gpu/browser...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1422103003/diff/1/content/browser/gpu/browser...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:232: return 0;
I don't think this is correct. This is supposed to return the target used for
non-native buffers, GL_TEXTURE_2D.

https://codereview.chromium.org/1422103003/diff/1/content/browser/gpu/browser...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:245: return 0;
This could be a NOTREACHED case now that the shm factory has been removed. Can
you merge this with the gfx::EMPTY_BUFFER case while here?

[ccameron, 2015-10-29 19:06:21]

Description was changed from

==========
cc: Remove the ability to specify a GL texture target

This feature is only used to, in a very roundabout way, specify that we
want to use the GLImage texture target (if available) for the resource.
Make this more clear, by replacing CreateResourceWithTextureTarget with
CreateResourceWithImageTextureTarget, which does not take a target
argument.

Similarly add ResourcePool::CreateForImageTextureTarget as a way to
create resource pools that will have this behavior.

Also, change BrowserGpuMemoryBufferManager to return 0 as the texture
target for unsupported format or usage (the current default value of
GL_TEXTURE_2D only causes problems on Mac).

BUG=533677
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Remove the ability to specify a GL texture target

This feature is only used to, in a very roundabout way, specify that we
want to use the GLImage texture target (if available) for the resource.
Make this more clear, by replacing CreateResourceWithTextureTarget with
CreateResourceWithImageTextureTarget, which does not take a target
argument.

Similarly add ResourcePool::CreateForImageTextureTarget as a way to
create resource pools that will have this behavior.

BUG=533677
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ccameron, 2015-10-29 19:08:29]

Thanks -- I excised the GMB target part of the patch, because it's wrong and not
the direct target of the work.

I do feel that the ResourceProvider should not be told about the GL texture
target, but should rather should get a higher level intentional signal, and use
that to determine the GL texture target.

PTAL.

https://codereview.chromium.org/1422103003/diff/1/cc/resources/resource_pool.h
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/1422103003/diff/1/cc/resources/resource_pool....
cc/resources/resource_pool.h:24: static scoped_ptr<ResourcePool>
CreateForImageTextureTarget(
On 2015/10/29 14:11:39, reveman wrote:
> I think this change will break the ongoing work to support compressed tile
> textures as we need a mix of texture targets in that case. Some tiles will be
> compressed and some will not. E.g. the driver might only support compression
> opaque contents.

Is there a reason that the texture target selection cannot be pushed down to the
ResourceProvider level?

If we are planning to have the ResourceProvider have a flag saying "make all of
the texture backed by native GMBs", then it will be necessary to never specify a
texture target, because the choice of texture target is already fixed by the
decision to use GMBs.

IMO, the ResourceProvider's interface should be renderer-agnostic, and the GL
texture target is very non-renderer-agnostic.

https://codereview.chromium.org/1422103003/diff/1/content/browser/gpu/browser...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1422103003/diff/1/content/browser/gpu/browser...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:232: return 0;
On 2015/10/29 14:11:39, reveman wrote:
> I don't think this is correct. This is supposed to return the target used for
> non-native buffers, GL_TEXTURE_2D.

Yeah, this was a bad idea and I should have left it out of this CL.

In general, I'd like there to be a way for the ResourceProvider to be able to
tell if a given GMB can become an overlay, but let's solve that problem later.

[reveman, 2015-10-29 19:41:58]

https://codereview.chromium.org/1422103003/diff/1/cc/resources/resource_pool.h
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/1422103003/diff/1/cc/resources/resource_pool....
cc/resources/resource_pool.h:24: static scoped_ptr<ResourcePool>
CreateForImageTextureTarget(
On 2015/10/29 at 19:08:29, ccameron wrote:
> On 2015/10/29 14:11:39, reveman wrote:
> > I think this change will break the ongoing work to support compressed tile
> > textures as we need a mix of texture targets in that case. Some tiles will
be
> > compressed and some will not. E.g. the driver might only support compression
> > opaque contents.
> 
> Is there a reason that the texture target selection cannot be pushed down to
the ResourceProvider level?

There are some plans to allocate resources on worker threads instead of the
compositor thread and that wouldn't really allow us to use the RP. I don't think
there's any reason it couldn't be pushed down to RP in ToT though.

> 
> If we are planning to have the ResourceProvider have a flag saying "make all
of the texture backed by native GMBs", then it will be necessary to never
specify a texture target, because the choice of texture target is already fixed
by the decision to use GMBs.
> 
> IMO, the ResourceProvider's interface should be renderer-agnostic, and the GL
texture target is very non-renderer-agnostic.

I agree. ResourcePool should be render agnostic too.

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

https://codereview.chromium.org/1422103003/diff/20001/cc/resources/resource_p...
cc/resources/resource_provider.cc:407: GLenum target =
GetImageTextureTarget(format);
Can we move this into the RESOURCE_TYPE_GL_TEXTURE case below and simply
s/target/GetImageTextureTarget(format)/ ?

https://codereview.chromium.org/1422103003/diff/20001/cc/resources/resource_p...
cc/resources/resource_provider.cc:408: if (!target)
is this still needed?

[ccameron, 2015-10-29 20:01:29]

Thanks -- updated.

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

https://codereview.chromium.org/1422103003/diff/20001/cc/resources/resource_p...
cc/resources/resource_provider.cc:407: GLenum target =
GetImageTextureTarget(format);
On 2015/10/29 19:41:58, reveman wrote:
> Can we move this into the RESOURCE_TYPE_GL_TEXTURE case below and simply
> s/target/GetImageTextureTarget(format)/ ?

You're right.

https://codereview.chromium.org/1422103003/diff/20001/cc/resources/resource_p...
cc/resources/resource_provider.cc:408: if (!target)
On 2015/10/29 19:41:57, reveman wrote:
> is this still needed?

Nope -- removed.

[reveman, 2015-10-29 20:05:15]

lgtm

[ccameron, 2015-10-29 21:17:00]

Thanks!

[ccameron, 2015-10-29 21:17:04]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-10-29 21:19:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1422103003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1422103003/40001

[commit-bot: I haz the power, 2015-10-29 22:28:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-29 22:29:21]

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