cc: Use a normal texture for background texture.

cc: Use a normal texture for background texture.

We prefer immutable texture to mutable texture and normal texture to framebuffer target texture.

There are 3 changes;
1. Introduce TextureHint enum to clarify immutable and framebuffer target.
2. The texture for render pass and background filter target is changed from mutable
 framebuffer texture to immutable framebuffer texture.
3. The texture for background filter source is changed from mutable framebuffer
 texture to mutable texture.

TEST=ResourceProviderTest.TextureHint, ResourceProviderTest.TextureAllocationHint
BUG=404986
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291223

[dshwang, 2014-08-18 19:00:56]

Could you review?
I clean-up piggy-back code of ResourceProvider::TextureUsageFramebuffer to avoid
mutable texture. and increase perf in Win for background filter

[dshwang, 2014-08-18 19:28:08]

https://codereview.chromium.org/486853002/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/486853002/diff/1/cc/output/gl_renderer.cc#new...
cc/output/gl_renderer.cc:904: window_rect.size(),
ResourceProvider::TextureHintDefault, RGBA_8888);
Here is only one logic change. Other changes are just due to enum.
Now we don't call following function for this texture.
TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_USAGE_ANGLE,
GL_FRAMEBUFFER_ATTACHMENT_ANGLE);
So ANGLE doesn't make a render target texture on Direct3D

[dshwang, 2014-08-19 08:26:00]

shannonwoods@, this CL makes compositor use normal texture instead of render
target texture on D3D9
What kind of benefit can we get when we uses just texture? Is it related to
performance or vram consumption?

[danakj, 2014-08-20 14:46:17]

https://codereview.chromium.org/486853002/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/486853002/diff/1/cc/output/gl_renderer.cc#new...
cc/output/gl_renderer.cc:904: window_rect.size(),
ResourceProvider::TextureHintDefault, RGBA_8888);
On 2014/08/18 19:28:08, dshwang wrote:
> Here is only one logic change. Other changes are just due to enum.
> Now we don't call following function for this texture.
> TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_USAGE_ANGLE,
> GL_FRAMEBUFFER_ATTACHMENT_ANGLE);
> So ANGLE doesn't make a render target texture on Direct3D

Why not just use TextureUsageAny here, why do we need the new enum values?

[dshwang, 2014-08-20 15:08:35]

https://codereview.chromium.org/486853002/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/486853002/diff/1/cc/output/gl_renderer.cc#new...
cc/output/gl_renderer.cc:904: window_rect.size(),
ResourceProvider::TextureHintDefault, RGBA_8888);
On 2014/08/20 14:46:17, danakj wrote:
> On 2014/08/18 19:28:08, dshwang wrote:
> > Here is only one logic change. Other changes are just due to enum.
> > Now we don't call following function for this texture.
> > TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_USAGE_ANGLE,
> > GL_FRAMEBUFFER_ATTACHMENT_ANGLE);
> > So ANGLE doesn't make a render target texture on Direct3D
> 
> Why not just use TextureUsageAny here, why do we need the new enum values?

That's good question.
Currently, This line wants to alloc texture by glTexImage2D so this uses
TextureUsageFramebuffer although this texture is not attached to framebuffer
object. It uses TextureUsageFramebuffer implementation detail.
TextureUsageAny will alloc texture by glTexStorage2D while
TextureUsageFramebuffer will alloc texture by glTexImage2D and then notify
GL_FRAMEBUFFER_ATTACHMENT_ANGLE to ANGLE.
There are two problem.
1. TextureUsageFramebuffer has two different properties; mutable texture and
framebuffer hint.
2. TextureUsageAny doesn't sounds like immutable texture.

So I split TextureUsageFramebuffer into TextureHintDefault and
TextureHintFramebuffer
- TextureHintDefault: neither immutable texture nor framebuffer target
- TextureHintImmutable: alloc by glTexStorage2D if possible. If you want, I can
change it to TextureUsageAny.
- TextureHintFramebuffer: notify it's framebuffer target

Most cases use TextureHintImmutable. Some cases use TextureHintFramebuffer. Only
this line use TextureHintDefault.

In addition, it's possible for texture to have both TextureHintFramebuffer and
TextureHintImmutable properties, because both are independent on each other.
This CL doesn't handle it, so I'll update the CL to handle this case.

[danakj, 2014-08-20 15:19:28]

On Wed, Aug 20, 2014 at 11:08 AM, <dongseong.hwang@intel.com> wrote:

>
> https://codereview.chromium.org/486853002/diff/1/cc/output/gl_renderer.cc
> File cc/output/gl_renderer.cc (right):
>
> https://codereview.chromium.org/486853002/diff/1/cc/output/gl_renderer.cc#
> newcode904
> cc/output/gl_renderer.cc:904: window_rect.size(),
> ResourceProvider::TextureHintDefault, RGBA_8888);
> On 2014/08/20 14:46:17, danakj wrote:
>
>> On 2014/08/18 19:28:08, dshwang wrote:
>> > Here is only one logic change. Other changes are just due to enum.
>> > Now we don't call following function for this texture.
>> > TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_USAGE_ANGLE,
>> > GL_FRAMEBUFFER_ATTACHMENT_ANGLE);
>> > So ANGLE doesn't make a render target texture on Direct3D
>>
>
>  Why not just use TextureUsageAny here, why do we need the new enum
>>
> values?
>
> That's good question.
> Currently, This line wants to alloc texture by glTexImage2D so this uses
> TextureUsageFramebuffer although this texture is not attached to
> framebuffer object. It uses TextureUsageFramebuffer implementation
> detail.
> TextureUsageAny will alloc texture by glTexStorage2D while
> TextureUsageFramebuffer will alloc texture by glTexImage2D and then
> notify GL_FRAMEBUFFER_ATTACHMENT_ANGLE to ANGLE.
> There are two problem.
> 1. TextureUsageFramebuffer has two different properties; mutable texture
> and framebuffer hint.
> 2. TextureUsageAny doesn't sounds like immutable texture.
>
> So I split TextureUsageFramebuffer into TextureHintDefault and
> TextureHintFramebuffer
> - TextureHintDefault: neither immutable texture nor framebuffer target
> - TextureHintImmutable: alloc by glTexStorage2D if possible. If you
> want, I can change it to TextureUsageAny.
> - TextureHintFramebuffer: notify it's framebuffer target
>

It sounded from other discussion like we should treat all textures as
immutable for good Angle performance. I don't think this texture's size is
being changed anywhere, so why do you want to treat it differently?


>
> Most cases use TextureHintImmutable. Some cases use
> TextureHintFramebuffer. Only this line use TextureHintDefault.
>
> In addition, it's possible for texture to have both
> TextureHintFramebuffer and TextureHintImmutable properties, because both
> are independent on each other. This CL doesn't handle it, so I'll update
> the CL to handle this case.
>

If nothing needs it let's not add it.

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

[dshwang, 2014-08-20 15:38:45]

On 2014/08/20 15:19:28, danakj wrote:
> On Wed, Aug 20, 2014 at 11:08 AM, <mailto:dongseong.hwang@intel.com> wrote:
> 
> >
> > https://codereview.chromium.org/486853002/diff/1/cc/output/gl_renderer.cc
> > File cc/output/gl_renderer.cc (right):
> >
> > https://codereview.chromium.org/486853002/diff/1/cc/output/gl_renderer.cc
> > newcode904
> > cc/output/gl_renderer.cc:904: window_rect.size(),
> > ResourceProvider::TextureHintDefault, RGBA_8888);
> > On 2014/08/20 14:46:17, danakj wrote:
> >
> >> On 2014/08/18 19:28:08, dshwang wrote:
> >> > Here is only one logic change. Other changes are just due to enum.
> >> > Now we don't call following function for this texture.
> >> > TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_USAGE_ANGLE,
> >> > GL_FRAMEBUFFER_ATTACHMENT_ANGLE);
> >> > So ANGLE doesn't make a render target texture on Direct3D
> >>
> >
> >  Why not just use TextureUsageAny here, why do we need the new enum
> >>
> > values?
> >
> > That's good question.
> > Currently, This line wants to alloc texture by glTexImage2D so this uses
> > TextureUsageFramebuffer although this texture is not attached to
> > framebuffer object. It uses TextureUsageFramebuffer implementation
> > detail.
> > TextureUsageAny will alloc texture by glTexStorage2D while
> > TextureUsageFramebuffer will alloc texture by glTexImage2D and then
> > notify GL_FRAMEBUFFER_ATTACHMENT_ANGLE to ANGLE.
> > There are two problem.
> > 1. TextureUsageFramebuffer has two different properties; mutable texture
> > and framebuffer hint.
> > 2. TextureUsageAny doesn't sounds like immutable texture.
> >
> > So I split TextureUsageFramebuffer into TextureHintDefault and
> > TextureHintFramebuffer
> > - TextureHintDefault: neither immutable texture nor framebuffer target
> > - TextureHintImmutable: alloc by glTexStorage2D if possible. If you
> > want, I can change it to TextureUsageAny.
> > - TextureHintFramebuffer: notify it's framebuffer target
> >
> 
> It sounded from other discussion like we should treat all textures as
> immutable for good Angle performance. I don't think this texture's size is
> being changed anywhere, so why do you want to treat it differently?

That's another good question. Short answer is that immutable texture of OpenGL
ES3 doesn't support glCopyTexImage2D.

In detail, immutable texture is also important to OpenGL as well as Direct3D. I
copy&paste Immutable Texture chapter of OpenGL ES3 Programming Guide.

Another feature introduced in OpenGL ES 3.0 to help improve application
performance is immutable textures. As discussed earlier in this chapter, an
application specifies each mipmap level of a texture independently using
functions such as glTexImage2D and glTexImage3D. The problem this creates for
the OpenGL ES driver is that it cannot determine until draw time whether a
texture has been fully specified. That is, it has to check whether each mipmap
level or subimage has matching formats, whether each level has the correct
dimensions, and whether there is sufficient memory. This draw time check can be
costly and can be avoided by using immutable textures.
The idea behind immutable textures is simple: The application specifies the
format and size of a texture before loading it with data. In doing so, the
texture format becomes immutable and the OpenGL ES driver can perform all
consistency and memory checks up-front. Once a texture has become immutable, its
format and dimensions cannot change. However, the application can still load it
with image data by using glTexSubImage2D, glTexSubImage3D, or glGenerateMipMap,
or by rendering to the texture.
To create an immutable texture, an application would bind the texture using
glBindTexture and then allocate its immutable storage using glTexStorage2D or
glTexStorage3D.
Image
Once the immutable texture is created, it is invalid to call glTexImage*,
glCompressedTexImage*, glCopyTexImage*, or glTexStorage* on the texture object.
Doing so will result in a GL_INVALID_OPERATION error being generated. To fill
the immutable texture with image data, the application needs to use
glTexSubImage2D, glTexSubImage3D, or glGenerateMipMap, or else render to the
image as a texture (by using it as an attachment to a framebuffer object).

> >
> > Most cases use TextureHintImmutable. Some cases use
> > TextureHintFramebuffer. Only this line use TextureHintDefault.
> >
> > In addition, it's possible for texture to have both
> > TextureHintFramebuffer and TextureHintImmutable properties, because both
> > are independent on each other. This CL doesn't handle it, so I'll update
> > the CL to handle this case.
> >
> 
> If nothing needs it let's not add it.

We need it in
https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/gl_rende...
It's more efficient to bind immutable texture to framebuffer object.

[danakj, 2014-08-20 15:58:02]

ok cool LGTM thanks for the tests.

We can do the framebuffer/immutable thing separate or you can add it here.. your
preference.

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

https://codereview.chromium.org/486853002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:3301: id =
i assume clang-format did this? i will file a bug, this is weird.

[Shannon Woods, 2014-08-20 16:19:03]

On 2014/08/19 08:26:00, dshwang wrote:
> shannonwoods@, this CL makes compositor use normal texture instead of render
> target texture on D3D9
> What kind of benefit can we get when we uses just texture? Is it related to
> performance or vram consumption?

Both performance and video memory. Essentially, the driver is less free to move
render target textures around and page them out as it needs to. Creating
textures non-renderable whenever possible should allow the driver to manage them
most efficiently. Note that if the texture is created as non-renderable and
needs to become renderable later on, however, this will incur its own
performance penalty, as we have to destroy and recreate the texture.

[dshwang, 2014-08-20 16:33:32]

On 2014/08/20 15:58:02, danakj wrote:
> ok cool LGTM thanks for the tests.

Thank you for reviewing.

> We can do the framebuffer/immutable thing separate or you can add it here..
your
> preference.

I already add new enum value; TextureHintImmutableFramebuffer =
TextureHintImmutable | TextureHintFramebuffer
And all clients for framebuffer use TextureHintImmutableFramebuffer because all
doesn't need to be mutable texture.
Could you review them?

>
https://codereview.chromium.org/486853002/diff/1/cc/trees/layer_tree_host_imp...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/486853002/diff/1/cc/trees/layer_tree_host_imp...
> cc/trees/layer_tree_host_impl.cc:3301: id =
> i assume clang-format did this? i will file a bug, this is weird.

'git cl format' did this to avoid 81 line length.


On 2014/08/20 16:19:03, Shannon Woods wrote:
> On 2014/08/19 08:26:00, dshwang wrote:
> > shannonwoods@, this CL makes compositor use normal texture instead of render
> > target texture on D3D9
> > What kind of benefit can we get when we uses just texture? Is it related to
> > performance or vram consumption?
> 
> Both performance and video memory. Essentially, the driver is less free to
move
> render target textures around and page them out as it needs to. Creating
> textures non-renderable whenever possible should allow the driver to manage
them
> most efficiently. Note that if the texture is created as non-renderable and
> needs to become renderable later on, however, this will incur its own
> performance penalty, as we have to destroy and recreate the texture.

That's good to know. Thank you for explaining it!

[danakj, 2014-08-21 14:37:09]

For the future, please try avoiding uploading a patchset that contains both your
own changes and a rebase at the same time. Upload the rebase separately and
label it as such in the title.

[danakj, 2014-08-21 14:41:45]

LGTM w/ comments

https://codereview.chromium.org/486853002/diff/20001/cc/resources/resource_pr...
File cc/resources/resource_provider_unittest.cc (right):

https://codereview.chromium.org/486853002/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider_unittest.cc:2484: for (int texture_id = 1;
texture_id <= 4; ++texture_id) {
s/4/arraysize(hints)/

https://codereview.chromium.org/486853002/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider_unittest.cc:2514: (hints[texture_id - 1] &
ResourceProvider::TextureHintFramebuffer)
can you do this with a temp variable that you set just above the EXPECT_CALL and
then use here inside Times()?

https://codereview.chromium.org/486853002/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider_unittest.cc:2972: for (int texture_id = 1;
texture_id <= 4; ++texture_id) {
s/4/arraysize(hints)/

https://codereview.chromium.org/486853002/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider_unittest.cc:2980: (hints[texture_id - 1] &
ResourceProvider::TextureHintImmutable) ? 1
can you use temp vars for these two values of Times() also. something like
expacted_storage_calls, etc

[dshwang, 2014-08-21 16:30:00]

On 2014/08/21 14:37:09, danakj wrote:
> For the future, please try avoiding uploading a patchset that contains both
your
> own changes and a rebase at the same time. Upload the rebase separately and
> label it as such in the title.

Yes, I see. Sorry for inconvenience.

> LGTM w/ comments

Thank you.

https://codereview.chromium.org/486853002/diff/20001/cc/resources/resource_pr...
File cc/resources/resource_provider_unittest.cc (right):

https://codereview.chromium.org/486853002/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider_unittest.cc:2484: for (int texture_id = 1;
texture_id <= 4; ++texture_id) {
On 2014/08/21 14:41:45, danakj wrote:
> s/4/arraysize(hints)/

Done.

https://codereview.chromium.org/486853002/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider_unittest.cc:2514: (hints[texture_id - 1] &
ResourceProvider::TextureHintFramebuffer)
On 2014/08/21 14:41:45, danakj wrote:
> can you do this with a temp variable that you set just above the EXPECT_CALL
and
> then use here inside Times()?

Done.

https://codereview.chromium.org/486853002/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider_unittest.cc:2972: for (int texture_id = 1;
texture_id <= 4; ++texture_id) {
On 2014/08/21 14:41:45, danakj wrote:
> s/4/arraysize(hints)/

Done.

https://codereview.chromium.org/486853002/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider_unittest.cc:2980: (hints[texture_id - 1] &
ResourceProvider::TextureHintImmutable) ? 1
On 2014/08/21 14:41:45, danakj wrote:
> can you use temp vars for these two values of Times() also. something like
> expacted_storage_calls, etc

Done.

[dshwang, 2014-08-21 16:30:37]

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

[commit-bot: I haz the power, 2014-08-21 16:33:58]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/486853002/40001

[commit-bot: I haz the power, 2014-08-21 22:07:29]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-21 22:59:06]

Committed patchset #3 (40001) as 291223