cc: zero/one-copy based on shared memory on IOS uses GL_TEXTURE_2D, not GL_TEXTURE_RECTANGLE_ARB.

cc: zero/one-copy based on shared memory on IOS uses GL_TEXTURE_2D, not GL_TEXTURE_RECTANGLE_ARB.

GL_TEXTURE_RECTANGLE_ARB is needed for only IOSurface backed images like GL_TEXTURE_EXTERNAL_OES
is needed for only surface texture backed images on Android. Replace switches::kUseImageExternal
with switches::kUseImageTextureTarget in order to let cc know what is proper texture target backed image.

Committed: https://crrev.com/72183b04964ecf5d8925dcf63335c4b5dc0e47ae
Cr-Commit-Position: refs/heads/master@{#307214}

[dshwang, 2014-11-28 08:52:01]

dongseong.hwang@intel.com changed reviewers:
+ reveman@chromium.org

[dshwang, 2014-11-28 08:52:02]

It's kind of follow-up CL of your https://codereview.chromium.org/699073004
I'm not sure the purpose of kUseImageExternal command line. As I understand, it
means zero/one-copy uses hardware backed image, while I feel it's duplicated to
kEnableNativeGpuMemoryBuffers. Can I remove kUseImageExternal and make
LayerTreeHostImpl check kEnableNativeGpuMemoryBuffers directly?
Anyway, I think shared memory backed image should always use GL_TEXTURE_2D.
Could you give me right direction?

[reveman, 2014-12-01 17:03:14]

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

https://codereview.chromium.org/766663005/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:2021: if (settings_.use_image_external) {
use_image_external means that we should be using TEXTURE_EXTERNAL_OES not
something else. We shouldn't be using this setting for something that it's not
meant to be used for.

https://codereview.chromium.org/766663005/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/766663005/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host_impl.cc:1041: // or
GL_TEXTURE_RECTANGLE_ARB.
This comment is now incorrect and telling the renderer to use ImageExternal for
IO surfaces is incorrect. IO surfaces require TEXTURE_RECTANGLE_ARB.

[reveman, 2014-12-01 17:12:13]

On 2014/11/28 08:52:02, dshwang wrote:
> It's kind of follow-up CL of your https://codereview.chromium.org/699073004
> I'm not sure the purpose of kUseImageExternal command line. As I understand,
it
> means zero/one-copy uses hardware backed image, while I feel it's duplicated
to
> kEnableNativeGpuMemoryBuffers. Can I remove kUseImageExternal and make
> LayerTreeHostImpl check kEnableNativeGpuMemoryBuffers directly?
> Anyway, I think shared memory backed image should always use GL_TEXTURE_2D.
> Could you give me right direction?

kUseImageExternal tells the renderer to use EXTERNAL_OES target.

kEnableNativeGpuMemoryBuffers is a browser process command line argument. It's
not forwarded to the renderer. I think we need to add another argument in
addition to kUseImageExternal if we need to be able to choose between TEXTURE_2D
and TEXTURE_RECTANGLE at runtime. Or we turn that argument and setting into an
enum.

What's the problem with always using TEXTURE_RECTANGLE on Mac? The TEXTURE_2D
optimizations in glCopyTextureCHROMIUM doesn't apply to shared memory backed
buffers anyhow.

[dshwang, 2014-12-01 17:58:56]

On 2014/12/01 17:12:13, reveman wrote:
> On 2014/11/28 08:52:02, dshwang wrote:
> > It's kind of follow-up CL of your https://codereview.chromium.org/699073004
> > I'm not sure the purpose of kUseImageExternal command line. As I understand,
> it
> > means zero/one-copy uses hardware backed image, while I feel it's duplicated
> to
> > kEnableNativeGpuMemoryBuffers. Can I remove kUseImageExternal and make
> > LayerTreeHostImpl check kEnableNativeGpuMemoryBuffers directly?
> > Anyway, I think shared memory backed image should always use GL_TEXTURE_2D.
> > Could you give me right direction?
> 
> kUseImageExternal tells the renderer to use EXTERNAL_OES target.
> 
> kEnableNativeGpuMemoryBuffers is a browser process command line argument. It's
> not forwarded to the renderer. I think we need to add another argument in
> addition to kUseImageExternal if we need to be able to choose between
TEXTURE_2D
> and TEXTURE_RECTANGLE at runtime. Or we turn that argument and setting into an
> enum.

Thank you for explaining kUseImageExternal.

> What's the problem with always using TEXTURE_RECTANGLE on Mac? The TEXTURE_2D
> optimizations in glCopyTextureCHROMIUM doesn't apply to shared memory backed
> buffers anyhow.

glCopyTextureCHROMIUM can copy TEXTURE_2D texture using glCopyTexImage2D, while
TEXTURE_RECTANGLE texture is copied by GL drawing API.
However, after re-reading code, shared memory backed buffers doesn't use
glCopyTextureCHROMIUM because GLImageMemory::CopyTexImage() performs texture
copy.
So this CL is not quite necessary.

However, it's not bad to change mac to use TEXTURE_2D when using shared memory
backed buffers, because there is not reason to use TEXTURE_RECTANGLE.
WDYT?
If you think yes, I'll forward kEnableNativeGpuMemoryBuffers to the renderer.

[reveman, 2014-12-01 18:20:05]

On 2014/12/01 17:58:56, dshwang wrote:
> On 2014/12/01 17:12:13, reveman wrote:
> > On 2014/11/28 08:52:02, dshwang wrote:
> > > It's kind of follow-up CL of your
https://codereview.chromium.org/699073004
> > > I'm not sure the purpose of kUseImageExternal command line. As I
understand,
> > it
> > > means zero/one-copy uses hardware backed image, while I feel it's
duplicated
> > to
> > > kEnableNativeGpuMemoryBuffers. Can I remove kUseImageExternal and make
> > > LayerTreeHostImpl check kEnableNativeGpuMemoryBuffers directly?
> > > Anyway, I think shared memory backed image should always use
GL_TEXTURE_2D.
> > > Could you give me right direction?
> > 
> > kUseImageExternal tells the renderer to use EXTERNAL_OES target.
> > 
> > kEnableNativeGpuMemoryBuffers is a browser process command line argument.
It's
> > not forwarded to the renderer. I think we need to add another argument in
> > addition to kUseImageExternal if we need to be able to choose between
> TEXTURE_2D
> > and TEXTURE_RECTANGLE at runtime. Or we turn that argument and setting into
an
> > enum.
> 
> Thank you for explaining kUseImageExternal.
> 
> > What's the problem with always using TEXTURE_RECTANGLE on Mac? The
TEXTURE_2D
> > optimizations in glCopyTextureCHROMIUM doesn't apply to shared memory backed
> > buffers anyhow.
> 
> glCopyTextureCHROMIUM can copy TEXTURE_2D texture using glCopyTexImage2D,
while
> TEXTURE_RECTANGLE texture is copied by GL drawing API.
> However, after re-reading code, shared memory backed buffers doesn't use
> glCopyTextureCHROMIUM because GLImageMemory::CopyTexImage() performs texture
> copy.
> So this CL is not quite necessary.
> 
> However, it's not bad to change mac to use TEXTURE_2D when using shared memory
> backed buffers, because there is not reason to use TEXTURE_RECTANGLE.
> WDYT?
> If you think yes, I'll forward kEnableNativeGpuMemoryBuffers to the renderer.

Using TEXTURE_2D instead of TEXTURE_RECTANGLE on mac with shared memory backed
buffers would be cleaner but I don't think forwarding
kEnableNativeGpuMemoryBuffers to the renderer and using that to determine what
target to use is the best approach. I prefer to keep
kEnableNativeGpuMemoryBuffers as a browser side setting and there might be other
parameters that determine what buffer type that a renderer is allowed to use in
the future. For example, one renderer might be allowed to use native buffers but
another one might not. I think it's better to add a different
chromium-image-texture-target setting for the renderer, which doesn't have to be
the same as kEnableNativeGpuMemoryBuffers passed to the browser.

[dshwang, 2014-12-02 20:15:42]

On 2014/12/01 18:20:05, reveman wrote:
> Using TEXTURE_2D instead of TEXTURE_RECTANGLE on mac with shared memory backed
> buffers would be cleaner but I don't think forwarding
> kEnableNativeGpuMemoryBuffers to the renderer and using that to determine what
> target to use is the best approach. I prefer to keep
> kEnableNativeGpuMemoryBuffers as a browser side setting and there might be
other
> parameters that determine what buffer type that a renderer is allowed to use
in
> the future. For example, one renderer might be allowed to use native buffers
but
> another one might not. I think it's better to add a different
> chromium-image-texture-target setting for the renderer, which doesn't have to
be
> the same as kEnableNativeGpuMemoryBuffers passed to the browser.

Thank you for explaining the proper direction. New patch set introduces
switches::kUseImageIOSurface for IOS like switches::kUseImageExternal for
Android.
WDYT?

[reveman, 2014-12-03 02:19:42]

https://codereview.chromium.org/766663005/diff/20001/cc/trees/layer_tree_sett...
File cc/trees/layer_tree_settings.h (right):

https://codereview.chromium.org/766663005/diff/20001/cc/trees/layer_tree_sett...
cc/trees/layer_tree_settings.h:77: bool use_image_io_surface;
I can see that this might have been confusing but "use_image_external" does not
refer to CHROMIUM_image. It's "image" as in OES_EGL_image_external. This code
should preferably not be aware of the existence of IOSurface backed GPU memory
buffers.

This should be "use_texture_rectangle" instead of "use_image_io_surface". Or
maybe better, we replace use_image_external+use_texture_rectangle with a texture
target enum.

[dshwang, 2014-12-03 10:56:44]

On 2014/12/03 02:19:42, reveman wrote:
>
https://codereview.chromium.org/766663005/diff/20001/cc/trees/layer_tree_sett...
> File cc/trees/layer_tree_settings.h (right):
> 
>
https://codereview.chromium.org/766663005/diff/20001/cc/trees/layer_tree_sett...
> cc/trees/layer_tree_settings.h:77: bool use_image_io_surface;
> I can see that this might have been confusing but "use_image_external" does
not
> refer to CHROMIUM_image. It's "image" as in OES_EGL_image_external. This code
> should preferably not be aware of the existence of IOSurface backed GPU memory
> buffers.
> 
> This should be "use_texture_rectangle" instead of "use_image_io_surface". Or
> maybe better, we replace use_image_external+use_texture_rectangle with a
texture
> target enum.

I agree. It's why my gut felt hesitating to introduce new command line.
I buy "we replace use_image_external+use_texture_rectangle with a texture target
enum."
I replace Replace switches::kUseImageExternal with
switches::kImageTextureTarget.
render_process_host_impl.cc append command line like
--image-texture-target=GL_TEXTURE_RECTANGLE_ARB

[dshwang, 2014-12-03 11:00:36]

Patchset #3 (id:40001) has been deleted

[reveman, 2014-12-03 15:57:19]

https://codereview.chromium.org/766663005/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/766663005/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2021: if (image_target ==
GL_TEXTURE_RECTANGLE_ARB) {
Please remove if/else statements and use DCHECK_IMPLIES instead. ie.
DCHECK_IMPLIES(image_target == GL_TEXTURE_RECTANGLE_ARB,
context_provider->ContextCapabilities().gpu.texture_rectangle)

https://codereview.chromium.org/766663005/diff/60001/cc/trees/layer_tree_sett...
File cc/trees/layer_tree_settings.cc (right):

https://codereview.chromium.org/766663005/diff/60001/cc/trees/layer_tree_sett...
cc/trees/layer_tree_settings.cc:12: #include "gpu/GLES2/gl2extchromium.h"
is gl2extchromium.h needed? all you need is TEXTURE_2D, which is not a chromium
extension. gl.h enough?

https://codereview.chromium.org/766663005/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/766663005/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1040: if (type ==
gfx::SURFACE_TEXTURE_BUFFER) {
please use a switch statement instead.

https://codereview.chromium.org/766663005/diff/60001/content/public/common/co...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/766663005/diff/60001/content/public/common/co...
content/public/common/content_switches.cc:852: // Set target of texture backed
image.
I don't understand this comment. Maybe something like:
"Overrides the default texture target used with CHROMIUM_image extension."

https://codereview.chromium.org/766663005/diff/60001/content/public/common/co...
content/public/common/content_switches.cc:853: const char kImageTextureTarget[]
= "image-texture-target";
nit: kUseImageTextureTarget and "use-image-texture-target" as that would be a
bit more consistent with other multiple choice command line flags.

https://codereview.chromium.org/766663005/diff/60001/content/renderer/render_...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/766663005/diff/60001/content/renderer/render_...
content/renderer/render_thread_impl.cc:524: image_texture_target_ =
GL_TEXTURE_2D;
do you need to set this here?

https://codereview.chromium.org/766663005/diff/60001/content/renderer/render_...
content/renderer/render_thread_impl.cc:528: if (texture_target ==
"GL_TEXTURE_RECTANGLE_ARB") {
I prefer "rectangle" instead of "GL_TEXTURE_RECTANGLE_ARB".

https://codereview.chromium.org/766663005/diff/60001/content/renderer/render_...
content/renderer/render_thread_impl.cc:530: } else if (texture_target ==
"GL_TEXTURE_EXTERNAL_OES") {
"external"

[dshwang, 2014-12-03 18:37:31]

On 2014/12/03 15:57:19, reveman wrote:

Thank you for review!

>
https://codereview.chromium.org/766663005/diff/60001/cc/trees/layer_tree_host...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/766663005/diff/60001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host_impl.cc:2021: if (image_target ==
> GL_TEXTURE_RECTANGLE_ARB) {
> Please remove if/else statements and use DCHECK_IMPLIES instead. ie.
> DCHECK_IMPLIES(image_target == GL_TEXTURE_RECTANGLE_ARB,
> context_provider->ContextCapabilities().gpu.texture_rectangle)

DCHECK_IMPLIES is cool. Done.

>
https://codereview.chromium.org/766663005/diff/60001/cc/trees/layer_tree_sett...
> File cc/trees/layer_tree_settings.cc (right):
> 
>
https://codereview.chromium.org/766663005/diff/60001/cc/trees/layer_tree_sett...
> cc/trees/layer_tree_settings.cc:12: #include "gpu/GLES2/gl2extchromium.h"
> is gl2extchromium.h needed? all you need is TEXTURE_2D, which is not a
chromium
> extension. gl.h enough?

Done.
 
>
https://codereview.chromium.org/766663005/diff/60001/content/browser/renderer...
> File content/browser/renderer_host/render_process_host_impl.cc (right):
> 
>
https://codereview.chromium.org/766663005/diff/60001/content/browser/renderer...
> content/browser/renderer_host/render_process_host_impl.cc:1040: if (type ==
> gfx::SURFACE_TEXTURE_BUFFER) {
> please use a switch statement instead.

Done.

>
https://codereview.chromium.org/766663005/diff/60001/content/public/common/co...
> File content/public/common/content_switches.cc (right):
> 
>
https://codereview.chromium.org/766663005/diff/60001/content/public/common/co...
> content/public/common/content_switches.cc:852: // Set target of texture backed
> image.
> I don't understand this comment. Maybe something like:
> "Overrides the default texture target used with CHROMIUM_image extension."

Nice sentence. Done.

>
https://codereview.chromium.org/766663005/diff/60001/content/public/common/co...
> content/public/common/content_switches.cc:853: const char
kImageTextureTarget[]
> = "image-texture-target";
> nit: kUseImageTextureTarget and "use-image-texture-target" as that would be a
> bit more consistent with other multiple choice command line flags.

Yep, done.
 
>
https://codereview.chromium.org/766663005/diff/60001/content/renderer/render_...
> File content/renderer/render_thread_impl.cc (right):
> 
>
https://codereview.chromium.org/766663005/diff/60001/content/renderer/render_...
> content/renderer/render_thread_impl.cc:524: image_texture_target_ =
> GL_TEXTURE_2D;
> do you need to set this here?

Two reasons;
1. ctor doesn't initialize the member
2. LayerTreeHostImpl just uses it as `unsigned image_target =
settings_.use_image_texture_target;`

>
https://codereview.chromium.org/766663005/diff/60001/content/renderer/render_...
> content/renderer/render_thread_impl.cc:528: if (texture_target ==
> "GL_TEXTURE_RECTANGLE_ARB") {
> I prefer "rectangle" instead of "GL_TEXTURE_RECTANGLE_ARB".

Done.

>
https://codereview.chromium.org/766663005/diff/60001/content/renderer/render_...
> content/renderer/render_thread_impl.cc:530: } else if (texture_target ==
> "GL_TEXTURE_EXTERNAL_OES") {
> "external"

Done.

[reveman, 2014-12-03 19:22:36]

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

[reveman, 2014-12-03 19:22:37]

lgtm

+piman for content/

[dshwang, 2014-12-03 19:57:21]

On 2014/12/03 19:22:37, reveman wrote:
> lgtm
> 
> +piman for content/

Thank you for reviewing and appointing to content owner.

[piman, 2014-12-03 21:16:50]

https://codereview.chromium.org/766663005/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/766663005/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1045: "external");
nit: can you make "external" and "rectangle" a global constant (maybe in the
content_switches.cc/h files)? Since they're used in 2 places, that gives us a
compile-time guarantee against typos.

[dshwang, 2014-12-04 09:21:14]

Thank you for review. I address nits

https://codereview.chromium.org/766663005/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/766663005/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1045: "external");
On 2014/12/03 21:16:50, piman (Very slow to review) wrote:
> nit: can you make "external" and "rectangle" a global constant (maybe in the
> content_switches.cc/h files)? Since they're used in 2 places, that gives us a
> compile-time guarantee against typos.

That's good idea. Done.
in content_switches.cc
// Texture backed image requires GL_TEXTURE_RECTANGLE_ARB target.
const char kImageTextureRectangle[] = "image-texture-rectangle";

// Texture backed image requires GL_TEXTURE_EXTERNAL_OES target.
const char kImageTextureExternal[] = "image-texture-external";

[reveman, 2014-12-04 17:03:25]

https://codereview.chromium.org/766663005/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/766663005/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1045:
switches::kImageTextureExternal);
can we use GLES2Util::GetStringEnum(GL_TEXTURE_EXTERNAL_OES) here instead of
adding kImageTextureExternal?

[dshwang, 2014-12-04 18:42:11]

On 2014/12/04 17:03:25, reveman wrote:
>
https://codereview.chromium.org/766663005/diff/100001/content/browser/rendere...
> File content/browser/renderer_host/render_process_host_impl.cc (right):
> 
>
https://codereview.chromium.org/766663005/diff/100001/content/browser/rendere...
> content/browser/renderer_host/render_process_host_impl.cc:1045:
> switches::kImageTextureExternal);
> can we use GLES2Util::GetStringEnum(GL_TEXTURE_EXTERNAL_OES) here instead of
> adding kImageTextureExternal?

I think it's good idea. Done.

[reveman, 2014-12-04 20:10:38]

lgtm still

https://codereview.chromium.org/766663005/diff/120001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/766663005/diff/120001/content/renderer/render...
content/renderer/render_thread_impl.cc:536: }
Current code is fine but you might want to consider something like this:

const unsigned targets[] = {
  GL_TEXTURE_RECTANGLE_ARB,
  GL_TEXTURE_EXTERNAL_OES
};
for (unsigned& target : targets) {
  if (target_string == gpu::gles2::GLES2Util::GetStringEnum(target))
    use_image_texture_target_ = target;
}

[dshwang, 2014-12-04 20:55:59]

https://codereview.chromium.org/766663005/diff/120001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/766663005/diff/120001/content/renderer/render...
content/renderer/render_thread_impl.cc:536: }
On 2014/12/04 20:10:38, reveman wrote:
> Current code is fine but you might want to consider something like this:
> 
> const unsigned targets[] = {
>   GL_TEXTURE_RECTANGLE_ARB,
>   GL_TEXTURE_EXTERNAL_OES
> };
> for (unsigned& target : targets) {
>   if (target_string == gpu::gles2::GLES2Util::GetStringEnum(target))
>     use_image_texture_target_ = target;
> }

nice code stub. I buy it :)
Done.

[dshwang, 2014-12-05 12:53:54]

code might settle down.
@piman, could you review again?

[piman, 2014-12-05 20:55:31]

LGTM after compile errors fixed.

[dshwang, 2014-12-08 08:45:34]

Patchset #8 (id:160001) has been deleted

[dshwang, 2014-12-08 09:57:04]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-12-08 09:57:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/766663005/180001

[dshwang, 2014-12-08 10:05:04]

On 2014/12/05 20:55:31, piman (Very slow to review) wrote:
> LGTM after compile errors fixed.

Thank you for reviewing. I fixed compile errors.

[commit-bot: I haz the power, 2014-12-08 10:42:03]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2014-12-08 10:43:51]

Patchset 8 (id:??) landed as
https://crrev.com/72183b04964ecf5d8925dcf63335c4b5dc0e47ae
Cr-Commit-Position: refs/heads/master@{#307214}