Allow one-copy task tile worker pool to use compressed textures.

Allow one-copy and zero-copy task tile worker pools to use compressed textures.

BUG=434699
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/7d60ce9a132a88ead407a2a58c91edc431e68259
Cr-Commit-Position: refs/heads/master@{#364326}

Committed: https://crrev.com/10fc39d36c0ba481056ec91a2984e02fd1127cf4
Cr-Commit-Position: refs/heads/master@{#364484}

[christiank, 2015-09-30 07:49:41]

christiank@opera.com changed reviewers:
+ reveman@chromium.org

[christiank, 2015-09-30 07:49:42]

Hi,

This is the final glue necessary for rudimentary tile compression support.

David, this review replaces the old:
https://codereview.chromium.org/1197423003/

Unfortunately it aged to a point where it was very difficult to rebase in a
sensible manner. I have addressed most of your comments. The ones I haven't
addressed I have brought over as I need more feedback.

Sorry for the inconvenience.

https://codereview.chromium.org/1379783002/diff/1/cc/raster/one_copy_tile_tas...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1379783002/diff/1/cc/raster/one_copy_tile_tas...
cc/raster/one_copy_tile_task_worker_pool.cc:506: // TODO(christiank): Should use
CompressedCopySubTextureCHROMIUM.
reveman (cited from 1197423003):
"Let's fix this TODO before landing this patch."

As we discussed in another issue it's not possible to preallocate compressed
textures using glCompressedTexImage2D without specifying a valid data pointer
(see ResourceProvider::LazyAllocate). As a walk around you suggested that we
could call CompressedCopyTextureCHROMIUM for the first copy as it doesn't
require the texture to be defined. I am unsure on how to check if a texture is
defined, do you have any ideas? Or is there another better approach to make sure
CompressedCopyTextureCHROMIUM is used only for the first copy?

https://codereview.chromium.org/1379783002/diff/1/cc/raster/tile_task_worker_...
File cc/raster/tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1379783002/diff/1/cc/raster/tile_task_worker_...
cc/raster/tile_task_worker_pool.cc:229:
TextureCompressor::Create(TextureCompressor::kFormatETC1);
reveman (cited from 1197423003):
"hm, I think we should assume that creating a texture compressor instance is
expensive. can we use a process wide instance?"

Sure. It think singletons would work nicely, having a static
TextureCompressor::Get(<format>) function returning a lazily initialized
singleton compressor. But if I recall correctly Singletons are discouraged?

[reveman, 2015-09-30 09:55:15]

https://codereview.chromium.org/1379783002/diff/1/cc/output/renderer_settings.h
File cc/output/renderer_settings.h (right):

https://codereview.chromium.org/1379783002/diff/1/cc/output/renderer_settings...
cc/output/renderer_settings.h:30: bool use_tile_compression;
Can we merge this with use_rgba_4444_textures somehow? E.g. replace both with a
tile_resource_format setting. Maybe
tile_resource_format+tile_resource_format_with_alpha so the embedder can also
chose the format used when alpha is required.

https://codereview.chromium.org/1379783002/diff/1/cc/raster/one_copy_tile_tas...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1379783002/diff/1/cc/raster/one_copy_tile_tas...
cc/raster/one_copy_tile_task_worker_pool.cc:345: return ETC1;
Can we include zero-copy support in this patch too?

https://codereview.chromium.org/1379783002/diff/1/cc/raster/one_copy_tile_tas...
cc/raster/one_copy_tile_task_worker_pool.cc:506: // TODO(christiank): Should use
CompressedCopySubTextureCHROMIUM.
On 2015/09/30 at 07:49:42, christiank wrote:
> reveman (cited from 1197423003):
> "Let's fix this TODO before landing this patch."
> 
> As we discussed in another issue it's not possible to preallocate compressed
textures using glCompressedTexImage2D without specifying a valid data pointer
(see ResourceProvider::LazyAllocate). As a walk around you suggested that we
could call CompressedCopyTextureCHROMIUM for the first copy as it doesn't
require the texture to be defined. I am unsure on how to check if a texture is
defined, do you have any ideas? Or is there another better approach to make sure
CompressedCopyTextureCHROMIUM is used only for the first copy?

ResourceProvider::LazyAllocate seem wrong. If it can't allocate storage then it
shouldn't set Resource::allocated to true. If we fix that, then we can detect if
storage for the resource has been allocated here (independent of format) and
force a full size copy when that's not the case. I think it would be best to do
this by making the whole "while (y < height)" loop conditional on if storage for
the texture has been allocated or not.

https://codereview.chromium.org/1379783002/diff/1/cc/raster/tile_task_worker_...
File cc/raster/tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1379783002/diff/1/cc/raster/tile_task_worker_...
cc/raster/tile_task_worker_pool.cc:185: bool needs_conversion =
Can we instead include the non-conversion case in the switch statement?
::PlaybackToMemory becomes a large switch statement based on format and we can
remove IsSupportedPlaybackToMemoryFormat as that will be covered by this switch
statement. I think that would also remove the need for a
SkColorTypeToResourceFormat function..

https://codereview.chromium.org/1379783002/diff/1/cc/raster/tile_task_worker_...
cc/raster/tile_task_worker_pool.cc:225: scoped_ptr<TextureCompressor>
texture_compressor;
nit: move to case where it's used

https://codereview.chromium.org/1379783002/diff/1/cc/raster/tile_task_worker_...
cc/raster/tile_task_worker_pool.cc:229:
TextureCompressor::Create(TextureCompressor::kFormatETC1);
On 2015/09/30 at 07:49:42, christiank wrote:
> reveman (cited from 1197423003):
> "hm, I think we should assume that creating a texture compressor instance is
expensive. can we use a process wide instance?"
> 
> Sure. It think singletons would work nicely, having a static
TextureCompressor::Get(<format>) function returning a lazily initialized
singleton compressor. But if I recall correctly Singletons are discouraged?

Creating it here each time is fine for now. Ideally we'd find an owner of these.
Maybe add a TextureCompressorProvider interface that gets passed here and is
implemented by LTHI.

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

https://codereview.chromium.org/1379783002/diff/1/cc/resources/resource_pool....
cc/resources/resource_pool.cc:92: Resource* ResourcePool::AcquireResource(const
gfx::Size& desired_size,
Can we instead just make sure resource dimensions are always multiple of 4 and
have a DCHECK here independent of format? It doesn't make sense for us to ever
use a different resource size as that can easily result in worse performance as
the starting address of each row might not be 8 byte aligned.

[christiank, 2015-11-26 15:32:57]

Description was changed from

==========
Allow one-copy task tile worker pool to use compressed textures.

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

to

==========
Allow one-copy and zero-copy task tile worker pools to use compressed textures.

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

[christiank, 2015-11-26 15:35:36]

Once again sorry for the lack of progress here.

I have addressed some of your comments, a few others I'd like some feedback on.

I'll attempt to rebase this CL as the next step.

https://codereview.chromium.org/1379783002/diff/1/cc/output/renderer_settings.h
File cc/output/renderer_settings.h (right):

https://codereview.chromium.org/1379783002/diff/1/cc/output/renderer_settings...
cc/output/renderer_settings.h:30: bool use_tile_compression;
On 2015/09/30 09:55:15, reveman (OOO Sept 24-29) wrote:
> Can we merge this with use_rgba_4444_textures somehow? E.g. replace both with
a
> tile_resource_format setting. Maybe
> tile_resource_format+tile_resource_format_with_alpha so the embedder can also
> chose the format used when alpha is required.

This would be good, I agree. I think we'd need a new enum for this since we need
an option for (!use_rgba_444_textures && !use_tile_compression). I don't think
using ResourceFormat for this would work as it has no default format. Replacing
ResourceProvider::best_texture_format() with a setting looks tricky. Perhaps
something like this would work?

enum class MemoryEfficientResourceFormat {
  NONE,
  RGBA_4444,
  ETC1,
};

MemoryEfficientResourceFormat memory_efficient_format;
MemoryEfficientResourceFormat memory_efficient_format_with_alpha;

What do you think?

https://codereview.chromium.org/1379783002/diff/1/cc/raster/one_copy_tile_tas...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1379783002/diff/1/cc/raster/one_copy_tile_tas...
cc/raster/one_copy_tile_task_worker_pool.cc:345: return ETC1;
On 2015/09/30 09:55:15, reveman wrote:
> Can we include zero-copy support in this patch too?

Sure!

https://codereview.chromium.org/1379783002/diff/1/cc/raster/one_copy_tile_tas...
cc/raster/one_copy_tile_task_worker_pool.cc:506: // TODO(christiank): Should use
CompressedCopySubTextureCHROMIUM.
On 2015/09/30 09:55:15, reveman (OOO Sept 24-29) wrote:
> On 2015/09/30 at 07:49:42, christiank wrote:
> > reveman (cited from 1197423003):
> > "Let's fix this TODO before landing this patch."
> > 
> > As we discussed in another issue it's not possible to preallocate compressed
> textures using glCompressedTexImage2D without specifying a valid data pointer
> (see ResourceProvider::LazyAllocate). As a walk around you suggested that we
> could call CompressedCopyTextureCHROMIUM for the first copy as it doesn't
> require the texture to be defined. I am unsure on how to check if a texture is
> defined, do you have any ideas? Or is there another better approach to make
sure
> CompressedCopyTextureCHROMIUM is used only for the first copy?
> 
> ResourceProvider::LazyAllocate seem wrong. If it can't allocate storage then
it
> shouldn't set Resource::allocated to true. If we fix that, then we can detect
if
> storage for the resource has been allocated here (independent of format) and
> force a full size copy when that's not the case. I think it would be best to
do
> this by making the whole "while (y < height)" loop conditional on if storage
for
> the texture has been allocated or not.

Actually these are two different Resource classes. There is the
cc::ResourceProvider::Resource which has the allocated member, but the Resource
object here is cc::Resource.

However, cc::ResourceProvider::Resource can be accessed through
ScopedWriteLockGL. I have added accessors to that class, but it doesn't feel
quite right to me though. What do you think?

https://codereview.chromium.org/1379783002/diff/1/cc/raster/tile_task_worker_...
File cc/raster/tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1379783002/diff/1/cc/raster/tile_task_worker_...
cc/raster/tile_task_worker_pool.cc:185: bool needs_conversion =
On 2015/09/30 09:55:15, reveman wrote:
> Can we instead include the non-conversion case in the switch statement?
> ::PlaybackToMemory becomes a large switch statement based on format and we can
> remove IsSupportedPlaybackToMemoryFormat as that will be covered by this
switch
> statement. I think that would also remove the need for a
> SkColorTypeToResourceFormat function..

I can't think of an obvious way to do this. I still think we need some way of
checking if conversion is needed inside the switch cases to support conversion
between RGBA and BGRA depending on the platform format.

I'll give this another shot after rebasing to see if it makes things easier.

https://codereview.chromium.org/1379783002/diff/1/cc/raster/tile_task_worker_...
cc/raster/tile_task_worker_pool.cc:225: scoped_ptr<TextureCompressor>
texture_compressor;
On 2015/09/30 09:55:15, reveman wrote:
> nit: move to case where it's used

Done.

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

https://codereview.chromium.org/1379783002/diff/1/cc/resources/resource_pool....
cc/resources/resource_pool.cc:92: Resource* ResourcePool::AcquireResource(const
gfx::Size& desired_size,
On 2015/09/30 09:55:15, reveman wrote:
> Can we instead just make sure resource dimensions are always multiple of 4 and
> have a DCHECK here independent of format? It doesn't make sense for us to ever
> use a different resource size as that can easily result in worse performance
as
> the starting address of each row might not be 8 byte aligned.

I guess that makes sense. I am not sure if there are specific use cases for
other sizes now that I think about it. I can vaguely recall something about
video elements or WebGL introducing a layer with an odd size.

Do you have any hints on where I can look to restrict the resource size?

[reveman, 2015-11-27 16:46:49]

https://codereview.chromium.org/1379783002/diff/1/cc/output/renderer_settings.h
File cc/output/renderer_settings.h (right):

https://codereview.chromium.org/1379783002/diff/1/cc/output/renderer_settings...
cc/output/renderer_settings.h:30: bool use_tile_compression;
On 2015/11/26 at 15:35:35, christiank wrote:
> On 2015/09/30 09:55:15, reveman (OOO Sept 24-29) wrote:
> > Can we merge this with use_rgba_4444_textures somehow? E.g. replace both
with a
> > tile_resource_format setting. Maybe
> > tile_resource_format+tile_resource_format_with_alpha so the embedder can
also
> > chose the format used when alpha is required.
> 
> This would be good, I agree. I think we'd need a new enum for this since we
need an option for (!use_rgba_444_textures && !use_tile_compression). I don't
think using ResourceFormat for this would work as it has no default format.
Replacing ResourceProvider::best_texture_format() with a setting looks tricky.
Perhaps something like this would work?
> 
> enum class MemoryEfficientResourceFormat {
>   NONE,
>   RGBA_4444,
>   ETC1,
> };
> 
> MemoryEfficientResourceFormat memory_efficient_format;
> MemoryEfficientResourceFormat memory_efficient_format_with_alpha;
> 
> What do you think?

Can we just use ResourceFormat and require it to be set to RGBA_8888 or
equivalent when compressed formats shouldn't be used?

https://codereview.chromium.org/1379783002/diff/1/cc/raster/one_copy_tile_tas...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1379783002/diff/1/cc/raster/one_copy_tile_tas...
cc/raster/one_copy_tile_task_worker_pool.cc:506: // TODO(christiank): Should use
CompressedCopySubTextureCHROMIUM.
On 2015/11/26 at 15:35:35, christiank wrote:
> On 2015/09/30 09:55:15, reveman (OOO Sept 24-29) wrote:
> > On 2015/09/30 at 07:49:42, christiank wrote:
> > > reveman (cited from 1197423003):
> > > "Let's fix this TODO before landing this patch."
> > > 
> > > As we discussed in another issue it's not possible to preallocate
compressed
> > textures using glCompressedTexImage2D without specifying a valid data
pointer
> > (see ResourceProvider::LazyAllocate). As a walk around you suggested that we
> > could call CompressedCopyTextureCHROMIUM for the first copy as it doesn't
> > require the texture to be defined. I am unsure on how to check if a texture
is
> > defined, do you have any ideas? Or is there another better approach to make
sure
> > CompressedCopyTextureCHROMIUM is used only for the first copy?
> > 
> > ResourceProvider::LazyAllocate seem wrong. If it can't allocate storage then
it
> > shouldn't set Resource::allocated to true. If we fix that, then we can
detect if
> > storage for the resource has been allocated here (independent of format) and
> > force a full size copy when that's not the case. I think it would be best to
do
> > this by making the whole "while (y < height)" loop conditional on if storage
for
> > the texture has been allocated or not.
> 
> Actually these are two different Resource classes. There is the
cc::ResourceProvider::Resource which has the allocated member, but the Resource
object here is cc::Resource.
> 
> However, cc::ResourceProvider::Resource can be accessed through
ScopedWriteLockGL. I have added accessors to that class, but it doesn't feel
quite right to me though. What do you think?

Ok, I think it's fine to make the use of SubCopy conditional on if the format is
compressed or not but it shouldn't be inside this loop that is performing
SubCopy calls. Can you just move the compressed format check outside this loop?

https://codereview.chromium.org/1379783002/diff/1/cc/raster/tile_task_worker_...
File cc/raster/tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1379783002/diff/1/cc/raster/tile_task_worker_...
cc/raster/tile_task_worker_pool.cc:185: bool needs_conversion =
On 2015/11/26 at 15:35:35, christiank wrote:
> On 2015/09/30 09:55:15, reveman wrote:
> > Can we instead include the non-conversion case in the switch statement?
> > ::PlaybackToMemory becomes a large switch statement based on format and we
can
> > remove IsSupportedPlaybackToMemoryFormat as that will be covered by this
switch
> > statement. I think that would also remove the need for a
> > SkColorTypeToResourceFormat function..
> 
> I can't think of an obvious way to do this. I still think we need some way of
checking if conversion is needed inside the switch cases to support conversion
between RGBA and BGRA depending on the platform format.
> 
> I'll give this another shot after rebasing to see if it makes things easier.

fyi, I don't we ever convert from rgba to bgra in software. we prefer to flip
the channels in the shader when necessary.

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

https://codereview.chromium.org/1379783002/diff/1/cc/resources/resource_pool....
cc/resources/resource_pool.cc:92: Resource* ResourcePool::AcquireResource(const
gfx::Size& desired_size,
On 2015/11/26 at 15:35:35, christiank wrote:
> On 2015/09/30 09:55:15, reveman wrote:
> > Can we instead just make sure resource dimensions are always multiple of 4
and
> > have a DCHECK here independent of format? It doesn't make sense for us to
ever
> > use a different resource size as that can easily result in worse performance
as
> > the starting address of each row might not be 8 byte aligned.
> 
> I guess that makes sense. I am not sure if there are specific use cases for
other sizes now that I think about it. I can vaguely recall something about
video elements or WebGL introducing a layer with an odd size.
> 
> Do you have any hints on where I can look to restrict the resource size?

https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/picture_...

[christiank, 2015-11-30 15:41:08]

https://codereview.chromium.org/1379783002/diff/1/cc/output/renderer_settings.h
File cc/output/renderer_settings.h (right):

https://codereview.chromium.org/1379783002/diff/1/cc/output/renderer_settings...
cc/output/renderer_settings.h:30: bool use_tile_compression;
On 2015/11/27 16:46:49, reveman wrote:
> On 2015/11/26 at 15:35:35, christiank wrote:
> > On 2015/09/30 09:55:15, reveman (OOO Sept 24-29) wrote:
> > > Can we merge this with use_rgba_4444_textures somehow? E.g. replace both
> with a
> > > tile_resource_format setting. Maybe
> > > tile_resource_format+tile_resource_format_with_alpha so the embedder can
> also
> > > chose the format used when alpha is required.
> > 
> > This would be good, I agree. I think we'd need a new enum for this since we
> need an option for (!use_rgba_444_textures && !use_tile_compression). I don't
> think using ResourceFormat for this would work as it has no default format.
> Replacing ResourceProvider::best_texture_format() with a setting looks tricky.
> Perhaps something like this would work?
> > 
> > enum class MemoryEfficientResourceFormat {
> >   NONE,
> >   RGBA_4444,
> >   ETC1,
> > };
> > 
> > MemoryEfficientResourceFormat memory_efficient_format;
> > MemoryEfficientResourceFormat memory_efficient_format_with_alpha;
> > 
> > What do you think?
> 
> Can we just use ResourceFormat and require it to be set to RGBA_8888 or
> equivalent when compressed formats shouldn't be used? 

Yes, that would work. I have made this change now, not sure if I should document
this behavior somewhere?

https://codereview.chromium.org/1379783002/diff/1/cc/raster/one_copy_tile_tas...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1379783002/diff/1/cc/raster/one_copy_tile_tas...
cc/raster/one_copy_tile_task_worker_pool.cc:506: // TODO(christiank): Should use
CompressedCopySubTextureCHROMIUM.
On 2015/11/27 16:46:49, reveman wrote:
> On 2015/11/26 at 15:35:35, christiank wrote:
> > On 2015/09/30 09:55:15, reveman (OOO Sept 24-29) wrote:
> > > On 2015/09/30 at 07:49:42, christiank wrote:
> > > > reveman (cited from 1197423003):
> > > > "Let's fix this TODO before landing this patch."
> > > > 
> > > > As we discussed in another issue it's not possible to preallocate
> compressed
> > > textures using glCompressedTexImage2D without specifying a valid data
> pointer
> > > (see ResourceProvider::LazyAllocate). As a walk around you suggested that
we
> > > could call CompressedCopyTextureCHROMIUM for the first copy as it doesn't
> > > require the texture to be defined. I am unsure on how to check if a
texture
> is
> > > defined, do you have any ideas? Or is there another better approach to
make
> sure
> > > CompressedCopyTextureCHROMIUM is used only for the first copy?
> > > 
> > > ResourceProvider::LazyAllocate seem wrong. If it can't allocate storage
then
> it
> > > shouldn't set Resource::allocated to true. If we fix that, then we can
> detect if
> > > storage for the resource has been allocated here (independent of format)
and
> > > force a full size copy when that's not the case. I think it would be best
to
> do
> > > this by making the whole "while (y < height)" loop conditional on if
storage
> for
> > > the texture has been allocated or not.
> > 
> > Actually these are two different Resource classes. There is the
> cc::ResourceProvider::Resource which has the allocated member, but the
Resource
> object here is cc::Resource.
> > 
> > However, cc::ResourceProvider::Resource can be accessed through
> ScopedWriteLockGL. I have added accessors to that class, but it doesn't feel
> quite right to me though. What do you think?
> 
> Ok, I think it's fine to make the use of SubCopy conditional on if the format
is
> compressed or not but it shouldn't be inside this loop that is performing
> SubCopy calls. Can you just move the compressed format check outside this
loop?

Ahh good, that'll be better. Fixed now.

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

https://codereview.chromium.org/1379783002/diff/1/cc/resources/resource_pool....
cc/resources/resource_pool.cc:92: Resource* ResourcePool::AcquireResource(const
gfx::Size& desired_size,
On 2015/11/27 16:46:49, reveman wrote:
> On 2015/11/26 at 15:35:35, christiank wrote:
> > On 2015/09/30 09:55:15, reveman wrote:
> > > Can we instead just make sure resource dimensions are always multiple of 4
> and
> > > have a DCHECK here independent of format? It doesn't make sense for us to
> ever
> > > use a different resource size as that can easily result in worse
performance
> as
> > > the starting address of each row might not be 8 byte aligned.
> > 
> > I guess that makes sense. I am not sure if there are specific use cases for
> other sizes now that I think about it. I can vaguely recall something about
> video elements or WebGL introducing a layer with an odd size.
> > 
> > Do you have any hints on where I can look to restrict the resource size?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/picture_...

Perfect, thanks! Will look at this tomorrow.

[reveman, 2015-11-30 16:41:45]

>
https://codereview.chromium.org/1379783002/diff/1/cc/resources/resource_pool....
> cc/resources/resource_pool.cc:92: Resource*
ResourcePool::AcquireResource(const gfx::Size& desired_size,
> On 2015/11/27 16:46:49, reveman wrote:
> > On 2015/11/26 at 15:35:35, christiank wrote:
> > > On 2015/09/30 09:55:15, reveman wrote:
> > > > Can we instead just make sure resource dimensions are always multiple of
4
> > and
> > > > have a DCHECK here independent of format? It doesn't make sense for us
to
> > ever
> > > > use a different resource size as that can easily result in worse
performance
> > as
> > > > the starting address of each row might not be 8 byte aligned.
> > > 
> > > I guess that makes sense. I am not sure if there are specific use cases
for
> > other sizes now that I think about it. I can vaguely recall something about
> > video elements or WebGL introducing a layer with an odd size.
> > > 
> > > Do you have any hints on where I can look to restrict the resource size?
> > 
> >
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/picture_...
> 
> Perfect, thanks! Will look at this tomorrow.

Great! here is some feedback on other parts of the current patch.

https://codereview.chromium.org/1379783002/diff/60001/cc/raster/one_copy_tile...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1379783002/diff/60001/cc/raster/one_copy_tile...
cc/raster/one_copy_tile_task_worker_pool.cc:348: bool must_support_alpha) const
{
Can we adjust this interface to match RendererSettings? That would be
GetMemoryEfficientResourceFormat+GetMemoryEfficientResourceFormatWithAlpha in
current patch.

https://codereview.chromium.org/1379783002/diff/60001/cc/raster/one_copy_tile...
cc/raster/one_copy_tile_task_worker_pool.cc:349: if
(memory_efficient_format_with_alpha_ != RGBA_8888 && must_support_alpha)
I think these RGBA_8888 checks will be confusing for someone who is not familiar
with this code. Can memory_efficient_format_ -> tile_resource_format_ and
reflect the value of best_texture_format()?

https://codereview.chromium.org/1379783002/diff/60001/cc/raster/one_copy_tile...
cc/raster/one_copy_tile_task_worker_pool.cc:519: if
(IsResourceFormatCompressed(resource->format())) {
This is never going to be true thanks to the check above. remove it?

https://codereview.chromium.org/1379783002/diff/60001/cc/raster/tile_task_wor...
File cc/raster/tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1379783002/diff/60001/cc/raster/tile_task_wor...
cc/raster/tile_task_worker_pool.cc:237: case RGBA_8888:
nit: some of these are not valid here (e.g. RGB_565). we we add a NOTREACHED()
to the ones that should not be handled here (not supported by
ResourceFormatToSkColorType?)

[christiank, 2015-12-01 15:01:07]

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

https://codereview.chromium.org/1379783002/diff/1/cc/resources/resource_pool....
cc/resources/resource_pool.cc:92: Resource* ResourcePool::AcquireResource(const
gfx::Size& desired_size,
On 2015/11/30 15:41:08, christiank wrote:
> On 2015/11/27 16:46:49, reveman wrote:
> > On 2015/11/26 at 15:35:35, christiank wrote:
> > > On 2015/09/30 09:55:15, reveman wrote:
> > > > Can we instead just make sure resource dimensions are always multiple of
4
> > and
> > > > have a DCHECK here independent of format? It doesn't make sense for us
to
> > ever
> > > > use a different resource size as that can easily result in worse
> performance
> > as
> > > > the starting address of each row might not be 8 byte aligned.
> > > 
> > > I guess that makes sense. I am not sure if there are specific use cases
for
> > other sizes now that I think about it. I can vaguely recall something about
> > video elements or WebGL introducing a layer with an odd size.
> > > 
> > > Do you have any hints on where I can look to restrict the resource size?
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/picture_...
> 
> Perfect, thanks! Will look at this tomorrow.

Should now be fixed.

https://codereview.chromium.org/1379783002/diff/60001/cc/raster/one_copy_tile...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1379783002/diff/60001/cc/raster/one_copy_tile...
cc/raster/one_copy_tile_task_worker_pool.cc:348: bool must_support_alpha) const
{
On 2015/11/30 16:41:45, reveman wrote:
> Can we adjust this interface to match RendererSettings? That would be
> GetMemoryEfficientResourceFormat+GetMemoryEfficientResourceFormatWithAlpha in
> current patch.

Sure! That will leave it to TileManager::DetermineResourceFormat to determine
whether to use a memory efficient format or not. That should work.

https://codereview.chromium.org/1379783002/diff/60001/cc/raster/one_copy_tile...
cc/raster/one_copy_tile_task_worker_pool.cc:349: if
(memory_efficient_format_with_alpha_ != RGBA_8888 && must_support_alpha)
On 2015/11/30 16:41:45, reveman wrote:
> I think these RGBA_8888 checks will be confusing for someone who is not
familiar
> with this code. Can memory_efficient_format_ -> tile_resource_format_ and
> reflect the value of best_texture_format()?

I agree that it's a bit confusing. I don't quite follow what you mean though,
care to elaborate a little? I don't see how we can avoid checking if we should
use a memory efficient format or not. Unless the default memory efficient format
is initialized to best_texture_format() by default which seems tricky.

For example if adjusting the interface like you suggested above, how would
TileManager::DetermineResourceFormat know whether to call
GetMemoryEfficientResourceFormat* or GetResourceFormat?

https://codereview.chromium.org/1379783002/diff/60001/cc/raster/one_copy_tile...
cc/raster/one_copy_tile_task_worker_pool.cc:519: if
(IsResourceFormatCompressed(resource->format())) {
On 2015/11/30 16:41:45, reveman wrote:
> This is never going to be true thanks to the check above. remove it?

Oh, that's sloppy of me. You're absolutely right. Fixed.

https://codereview.chromium.org/1379783002/diff/60001/cc/raster/tile_task_wor...
File cc/raster/tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1379783002/diff/60001/cc/raster/tile_task_wor...
cc/raster/tile_task_worker_pool.cc:237: case RGBA_8888:
On 2015/11/30 16:41:45, reveman wrote:
> nit: some of these are not valid here (e.g. RGB_565). we we add a NOTREACHED()
> to the ones that should not be handled here (not supported by
> ResourceFormatToSkColorType?)

Done.

[reveman, 2015-12-01 17:27:24]

https://codereview.chromium.org/1379783002/diff/60001/cc/raster/one_copy_tile...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1379783002/diff/60001/cc/raster/one_copy_tile...
cc/raster/one_copy_tile_task_worker_pool.cc:349: if
(memory_efficient_format_with_alpha_ != RGBA_8888 && must_support_alpha)
On 2015/12/01 at 15:01:07, christiank wrote:
> On 2015/11/30 16:41:45, reveman wrote:
> > I think these RGBA_8888 checks will be confusing for someone who is not
familiar
> > with this code. Can memory_efficient_format_ -> tile_resource_format_ and
> > reflect the value of best_texture_format()?
> 
> I agree that it's a bit confusing. I don't quite follow what you mean though,
care to elaborate a little? I don't see how we can avoid checking if we should
use a memory efficient format or not. Unless the default memory efficient format
is initialized to best_texture_format() by default which seems tricky.
> 
> For example if adjusting the interface like you suggested above, how would
TileManager::DetermineResourceFormat know whether to call
GetMemoryEfficientResourceFormat* or GetResourceFormat?

I think it would be much cleaner and easier to understand if the embedder just
had to tell the compositor what tile format to use and that's it. If I
understand this correctly, the problem is that if BGRA (or ETC) can be used
depends on an extension so the embedder is not aware of that.

https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/platf...

what if we allowed the embedder to specific the preferred tile format (BGRA if
SK_B32_SHIFT is set) but we fall-back to RGBA_8888 if it's not supported? and
that would apply to compressed formats too. Basically, there's no such thing as
memory efficient format anymore, there's just the preferred tile format as
specified by the embedder which we'll use if supported and RGBA_8888 if not.
Wdyt?

https://codereview.chromium.org/1379783002/diff/80001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/1379783002/diff/80001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:770: // Ensure that tiles are an even multiple
of 4 in size.
Unconditionally use MathUtil::UncheckedRoundUp(tile_.., 4)?

Also, can you define something like kTileMinimalAlignment with a comment
explaining why we need it instead of just using the magic value of 4?

[christiank, 2015-12-02 16:18:14]

https://codereview.chromium.org/1379783002/diff/60001/cc/raster/one_copy_tile...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1379783002/diff/60001/cc/raster/one_copy_tile...
cc/raster/one_copy_tile_task_worker_pool.cc:349: if
(memory_efficient_format_with_alpha_ != RGBA_8888 && must_support_alpha)
On 2015/12/01 17:27:24, reveman wrote:
> On 2015/12/01 at 15:01:07, christiank wrote:
> > On 2015/11/30 16:41:45, reveman wrote:
> > > I think these RGBA_8888 checks will be confusing for someone who is not
> familiar
> > > with this code. Can memory_efficient_format_ -> tile_resource_format_ and
> > > reflect the value of best_texture_format()?
> > 
> > I agree that it's a bit confusing. I don't quite follow what you mean
though,
> care to elaborate a little? I don't see how we can avoid checking if we should
> use a memory efficient format or not. Unless the default memory efficient
format
> is initialized to best_texture_format() by default which seems tricky.
> > 
> > For example if adjusting the interface like you suggested above, how would
> TileManager::DetermineResourceFormat know whether to call
> GetMemoryEfficientResourceFormat* or GetResourceFormat?
> 
> I think it would be much cleaner and easier to understand if the embedder just
> had to tell the compositor what tile format to use and that's it. If I
> understand this correctly, the problem is that if BGRA (or ETC) can be used
> depends on an extension so the embedder is not aware of that.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/platf...
> 
> what if we allowed the embedder to specific the preferred tile format (BGRA if
> SK_B32_SHIFT is set) but we fall-back to RGBA_8888 if it's not supported? and
> that would apply to compressed formats too. Basically, there's no such thing
as
> memory efficient format anymore, there's just the preferred tile format as
> specified by the embedder which we'll use if supported and RGBA_8888 if not.
> Wdyt?

Now I see what you mean. Sounds good to me, definitely cleaner as you said.

I replaced the memory_efficient_format* settings with a new
preferred_tile_format setting. I decided to keep
PlatformColor::BestTextureFormat in the equation since we need that as a
fallback in case ETC1 is unsuitable (on Android we'd want to fallback to BGRA if
possible).

I hope it's okay with you that I iterate in the review like this rather than
agreeing on a final solution in one go? I think it allows for faster
turn-arounds which is good given the circumstances that I am leaving at the end
of next week :)

https://codereview.chromium.org/1379783002/diff/80001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/1379783002/diff/80001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:770: // Ensure that tiles are an even multiple
of 4 in size.
On 2015/12/01 17:27:24, reveman wrote:
> Unconditionally use MathUtil::UncheckedRoundUp(tile_.., 4)?
> 
> Also, can you define something like kTileMinimalAlignment with a comment
> explaining why we need it instead of just using the magic value of 4?

Done.

[reveman, 2015-12-02 18:27:19]

lgtm with nits

https://codereview.chromium.org/1379783002/diff/100001/cc/layers/picture_laye...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/1379783002/diff/100001/cc/layers/picture_laye...
cc/layers/picture_layer_impl.cc:776: tile_height % kTileMinimalAlignment != 0) {
nit: remove these checks and execute UncheckedRoundUp unconditionally

https://codereview.chromium.org/1379783002/diff/100001/cc/output/renderer_set...
File cc/output/renderer_settings.cc (right):

https://codereview.chromium.org/1379783002/diff/100001/cc/output/renderer_set...
cc/output/renderer_settings.cc:27:
preferred_tile_format(PlatformColor::BestTextureFormat(true)) {}
nit: I find calling BestTextureFormat with supports_bgra8888=true in a bunch of
places a bit awkward. Can you add a new function instead? E.g. remove  the
argument from BestTextureFormat and add a BestSupportedTextureFormat function
that still takes an argument if we even need that now that we have
IsResourceFormatSupported?

Btw, the name should be Best(Supported)ResourceFormat and not TextureFormat. No
need to but feel free to fix that as part of touching this code if you like.

https://codereview.chromium.org/1379783002/diff/100001/cc/output/renderer_set...
File cc/output/renderer_settings.h (right):

https://codereview.chromium.org/1379783002/diff/100001/cc/output/renderer_set...
cc/output/renderer_settings.h:31: ResourceFormat preferred_tile_format;
Great! and we can add preferred_tile_format_with_alpha in a follow up when
needed.

https://codereview.chromium.org/1379783002/diff/100001/cc/resources/resource_...
File cc/resources/resource_format.h (right):

https://codereview.chromium.org/1379783002/diff/100001/cc/resources/resource_...
cc/resources/resource_format.h:41: CC_EXPORT ResourceFormat
BufferFormatToResourceFormat(gfx::BufferFormat format);
nit: is this still needed?

https://codereview.chromium.org/1379783002/diff/100001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/1379783002/diff/100001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:674: if
(!IsResourceFormatCompressed(resource->format())) {
nit: is this conditional still needed? In that case please use:

DCHECK_IMPLIES(!IsResourceFormatCompressed(reso..),
               tile->desired_texture_size().ToString() ==
               resource->size().ToString());

https://codereview.chromium.org/1379783002/diff/100001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1379783002/diff/100001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:36: #include
"cc/resources/platform_color.h"
nit: I don't think you need this with the changes I suggest below

https://codereview.chromium.org/1379783002/diff/100001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:420:
settings.renderer_settings.preferred_tile_format = cc::RGBA_4444;
nit: if (!cmd->HasSwitch(switches::kDisableRGBA4444Textures)) here and remove
that check below so we don't have to call BestTextureFormat in here and can just
leave the setting as default

https://codereview.chromium.org/1379783002/diff/100001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:459: if
(cmd->HasSwitch(switches::kDisableRGBA4444Textures)) {
nit: please make setting referred_tile_format to RGBA_4444 at l.420 conditional
on this instead. see the comment above.

https://codereview.chromium.org/1379783002/diff/100001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1379783002/diff/100001/ui/compositor/composit...
ui/compositor/compositor.cc:145:
settings.renderer_settings.preferred_tile_format = cc::RGBA_4444;
Not needed in this patch but we should probably add a kUIEnableTileCompression
switch in a follow up patch

[reveman, 2015-12-02 18:28:50]

https://codereview.chromium.org/1379783002/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1379783002/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1785: bool
ValidateCompressedCopySubTextureDimensions(
btw, do we still need this? we're not using copysub for compressed textures yet,
right?

[christiank, 2015-12-03 12:57:59]

christiank@opera.com changed reviewers:
+ piman@chromium.org, sievers@chromium.org

[christiank, 2015-12-03 12:58:00]

piman, sievers would you please have a look?

https://codereview.chromium.org/1379783002/diff/100001/cc/layers/picture_laye...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/1379783002/diff/100001/cc/layers/picture_laye...
cc/layers/picture_layer_impl.cc:776: tile_height % kTileMinimalAlignment != 0) {
On 2015/12/02 18:27:18, reveman wrote:
> nit: remove these checks and execute UncheckedRoundUp unconditionally

Done.

https://codereview.chromium.org/1379783002/diff/100001/cc/output/renderer_set...
File cc/output/renderer_settings.cc (right):

https://codereview.chromium.org/1379783002/diff/100001/cc/output/renderer_set...
cc/output/renderer_settings.cc:27:
preferred_tile_format(PlatformColor::BestTextureFormat(true)) {}
On 2015/12/02 18:27:18, reveman wrote:
> nit: I find calling BestTextureFormat with supports_bgra8888=true in a bunch
of
> places a bit awkward. Can you add a new function instead? E.g. remove  the
> argument from BestTextureFormat and add a BestSupportedTextureFormat function
> that still takes an argument if we even need that now that we have
> IsResourceFormatSupported?
> 
> Btw, the name should be Best(Supported)ResourceFormat and not TextureFormat.
No
> need to but feel free to fix that as part of touching this code if you like.

I was thinking along the same lines. Fixed.

I think it makes sense to keep BestSupportedTextureFormat since there is a
different condition for the best render buffer format
(caps.gpu.render_buffer_format_bgra8888).

https://codereview.chromium.org/1379783002/diff/100001/cc/resources/resource_...
File cc/resources/resource_format.h (right):

https://codereview.chromium.org/1379783002/diff/100001/cc/resources/resource_...
cc/resources/resource_format.h:41: CC_EXPORT ResourceFormat
BufferFormatToResourceFormat(gfx::BufferFormat format);
On 2015/12/02 18:27:18, reveman wrote:
> nit: is this still needed?

No it's not, good catch!

https://codereview.chromium.org/1379783002/diff/100001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/1379783002/diff/100001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:674: if
(!IsResourceFormatCompressed(resource->format())) {
On 2015/12/02 18:27:18, reveman wrote:
> nit: is this conditional still needed? In that case please use:
> 
> DCHECK_IMPLIES(!IsResourceFormatCompressed(reso..),
>                tile->desired_texture_size().ToString() ==
>                resource->size().ToString());

No longer need, removed.

https://codereview.chromium.org/1379783002/diff/100001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1379783002/diff/100001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:36: #include
"cc/resources/platform_color.h"
On 2015/12/02 18:27:18, reveman wrote:
> nit: I don't think you need this with the changes I suggest below

Done.

https://codereview.chromium.org/1379783002/diff/100001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:420:
settings.renderer_settings.preferred_tile_format = cc::RGBA_4444;
On 2015/12/02 18:27:18, reveman wrote:
> nit: if (!cmd->HasSwitch(switches::kDisableRGBA4444Textures)) here and remove
> that check below so we don't have to call BestTextureFormat in here and can
just
> leave the setting as default

Done.

https://codereview.chromium.org/1379783002/diff/100001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:459: if
(cmd->HasSwitch(switches::kDisableRGBA4444Textures)) {
On 2015/12/02 18:27:18, reveman wrote:
> nit: please make setting referred_tile_format to RGBA_4444 at l.420
conditional
> on this instead. see the comment above.

Done.

https://codereview.chromium.org/1379783002/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1379783002/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1785: bool
ValidateCompressedCopySubTextureDimensions(
On 2015/12/02 18:28:50, reveman wrote:
> btw, do we still need this? we're not using copysub for compressed textures
yet,
> right?

No we don't need it at the moment. I'll revert the changes for now, they can be
re-added later if needed.

https://codereview.chromium.org/1379783002/diff/100001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1379783002/diff/100001/ui/compositor/composit...
ui/compositor/compositor.cc:145:
settings.renderer_settings.preferred_tile_format = cc::RGBA_4444;
On 2015/12/02 18:27:19, reveman wrote:
> Not needed in this patch but we should probably add a kUIEnableTileCompression
> switch in a follow up patch

Yes, makes sense.

[no sievers, 2015-12-03 19:00:03]

https://codereview.chromium.org/1379783002/diff/120001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1379783002/diff/120001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:457:
settings.renderer_settings.preferred_tile_format = cc::RGBA_4444;
Can we keep the behavior of 'disable' overriding 'enable'?
I *think* that's what we more commonly do.

Or feel free to eliminate one of the cmdlines, or both. It's getting confusing
to know what they'll do now that there's a new cmdline for compression.

[piman, 2015-12-03 23:01:50]

https://codereview.chromium.org/1379783002/diff/120001/cc/resources/resource_...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1379783002/diff/120001/cc/resources/resource_...
cc/resources/resource_provider.cc:854:
resource_provider_->LazyAllocate(resource_);
What happens now if this is ETC1?

Or are we guaranteed never to go through here? If so can we add a DCHECK?

https://codereview.chromium.org/1379783002/diff/120001/cc/resources/resource_...
cc/resources/resource_provider.cc:947:
resource_provider_->LazyAllocate(resource_);
Same here

https://codereview.chromium.org/1379783002/diff/120001/cc/resources/resource_...
cc/resources/resource_provider.cc:1538: .release();
Doesn't resource->allocated need to be true here too? LazyCreateImage DCHECKs
it.

[christiank, 2015-12-04 10:20:40]

https://codereview.chromium.org/1379783002/diff/120001/cc/resources/resource_...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1379783002/diff/120001/cc/resources/resource_...
cc/resources/resource_provider.cc:854:
resource_provider_->LazyAllocate(resource_);
On 2015/12/03 23:01:50, piman (slow to review) wrote:
> What happens now if this is ETC1?
> 
> Or are we guaranteed never to go through here? If so can we add a DCHECK?

It should be fine. As far as I understand we still need to lock the resource
even for ETC1. The only thing that will be different in the ETC1 case is that no
texture memory will be pre-allocated. That shouldn't be a problem. It's
addressed by performing a full upload or copy (which doesn't require texture
memory to be pre-allocated) when updating the resource.

https://codereview.chromium.org/1379783002/diff/120001/cc/resources/resource_...
cc/resources/resource_provider.cc:1538: .release();
On 2015/12/03 23:01:50, piman (slow to review) wrote:
> Doesn't resource->allocated need to be true here too? LazyCreateImage DCHECKs
> it.

You're absolutely right, my bad.

https://codereview.chromium.org/1379783002/diff/120001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1379783002/diff/120001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:457:
settings.renderer_settings.preferred_tile_format = cc::RGBA_4444;
On 2015/12/03 19:00:03, sievers wrote:
> Can we keep the behavior of 'disable' overriding 'enable'?
> I *think* that's what we more commonly do.
> 
> Or feel free to eliminate one of the cmdlines, or both. It's getting confusing
> to know what they'll do now that there's a new cmdline for compression.

Yes, that makes sense. Fixed now.

[no sievers, 2015-12-04 18:23:18]

On 2015/12/04 10:20:40, christiank wrote:
>
https://codereview.chromium.org/1379783002/diff/120001/cc/resources/resource_...
> File cc/resources/resource_provider.cc (right):
> 
>
https://codereview.chromium.org/1379783002/diff/120001/cc/resources/resource_...
> cc/resources/resource_provider.cc:854:
> resource_provider_->LazyAllocate(resource_);
> On 2015/12/03 23:01:50, piman (slow to review) wrote:
> > What happens now if this is ETC1?
> > 
> > Or are we guaranteed never to go through here? If so can we add a DCHECK?
> 
> It should be fine. As far as I understand we still need to lock the resource
> even for ETC1. The only thing that will be different in the ETC1 case is that
no
> texture memory will be pre-allocated. That shouldn't be a problem. It's
> addressed by performing a full upload or copy (which doesn't require texture
> memory to be pre-allocated) when updating the resource.
> 
>
https://codereview.chromium.org/1379783002/diff/120001/cc/resources/resource_...
> cc/resources/resource_provider.cc:1538: .release();
> On 2015/12/03 23:01:50, piman (slow to review) wrote:
> > Doesn't resource->allocated need to be true here too? LazyCreateImage
DCHECKs
> > it.
> 
> You're absolutely right, my bad.
> 
>
https://codereview.chromium.org/1379783002/diff/120001/content/renderer/gpu/r...
> File content/renderer/gpu/render_widget_compositor.cc (right):
> 
>
https://codereview.chromium.org/1379783002/diff/120001/content/renderer/gpu/r...
> content/renderer/gpu/render_widget_compositor.cc:457:
> settings.renderer_settings.preferred_tile_format = cc::RGBA_4444;
> On 2015/12/03 19:00:03, sievers wrote:
> > Can we keep the behavior of 'disable' overriding 'enable'?
> > I *think* that's what we more commonly do.
> > 
> > Or feel free to eliminate one of the cmdlines, or both. It's getting
confusing
> > to know what they'll do now that there's a new cmdline for compression.
> 
> Yes, that makes sense. Fixed now.

content/renderer lgtm (we should consider removing the 4444-related cmdline args
though)

[piman, 2015-12-04 21:30:48]

https://codereview.chromium.org/1379783002/diff/120001/cc/resources/resource_...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1379783002/diff/120001/cc/resources/resource_...
cc/resources/resource_provider.cc:854:
resource_provider_->LazyAllocate(resource_);
On 2015/12/04 10:20:40, christiank wrote:
> On 2015/12/03 23:01:50, piman (slow to review) wrote:
> > What happens now if this is ETC1?
> > 
> > Or are we guaranteed never to go through here? If so can we add a DCHECK?
> 
> It should be fine. As far as I understand we still need to lock the resource
> even for ETC1. The only thing that will be different in the ETC1 case is that
no
> texture memory will be pre-allocated. That shouldn't be a problem. It's
> addressed by performing a full upload or copy (which doesn't require texture
> memory to be pre-allocated) when updating the resource.

So, if I understand correctly, you expect the caller to "allocate" the resource.
However, what sets the allocate flag? There's a bunch of places that expect it
to be set. There's DCHECKs, but there's also some active logic (e.g. memory
tracking).

[christiank, 2015-12-07 08:38:11]

https://codereview.chromium.org/1379783002/diff/120001/cc/resources/resource_...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1379783002/diff/120001/cc/resources/resource_...
cc/resources/resource_provider.cc:854:
resource_provider_->LazyAllocate(resource_);
On 2015/12/04 21:30:48, piman (slow to review) wrote:
> On 2015/12/04 10:20:40, christiank wrote:
> > On 2015/12/03 23:01:50, piman (slow to review) wrote:
> > > What happens now if this is ETC1?
> > > 
> > > Or are we guaranteed never to go through here? If so can we add a DCHECK?
> > 
> > It should be fine. As far as I understand we still need to lock the resource
> > even for ETC1. The only thing that will be different in the ETC1 case is
that
> no
> > texture memory will be pre-allocated. That shouldn't be a problem. It's
> > addressed by performing a full upload or copy (which doesn't require texture
> > memory to be pre-allocated) when updating the resource.
> 
> So, if I understand correctly, you expect the caller to "allocate" the
resource.
> However, what sets the allocate flag? There's a bunch of places that expect it
> to be set. There's DCHECKs, but there's also some active logic (e.g. memory
> tracking).

Oh, now I see what you mean. I experimented with updating the allocated flag
from outside LazyAllocate() in patch set 2 but ended up reverting that. It
turned out to me rather messy, it's difficult to expose a nice clean API for
updating that flag from outside ResourceProvider.

I have reverted back to the original behavior.

[christiank, 2015-12-07 08:52:54]

The CQ bit was checked by christiank@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-07 08:53:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1379783002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1379783002/160001

[commit-bot: I haz the power, 2015-12-07 08:55:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-07 08:55:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[piman, 2015-12-07 19:44:23]

lgtm

[christiank, 2015-12-08 08:22:08]

The CQ bit was checked by christiank@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-08 08:22:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1379783002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1379783002/180001

[commit-bot: I haz the power, 2015-12-08 08:34:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-08 08:34:32]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[christiank, 2015-12-08 09:14:14]

The CQ bit was checked by christiank@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-08 09:14:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1379783002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1379783002/200001

[commit-bot: I haz the power, 2015-12-08 09:48:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-08 09:48:02]

Dry run: Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[christiank, 2015-12-08 11:55:02]

christiank@opera.com changed reviewers:
+ nyquist@chromium.org

[christiank, 2015-12-08 11:55:03]

nyquist, would you please have a look at the small blimp change in patchset 12?

[christiank, 2015-12-08 11:55:41]

The CQ bit was checked by christiank@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-08 11:56:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1379783002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1379783002/220001

[commit-bot: I haz the power, 2015-12-08 12:21:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-08 12:21:21]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[nyquist, 2015-12-08 21:13:50]

blimp lgtm

[christiank, 2015-12-09 08:44:06]

The CQ bit was checked by christiank@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-09 08:45:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1379783002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1379783002/240001

[commit-bot: I haz the power, 2015-12-09 09:26:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-09 09:26:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[christiank, 2015-12-09 16:05:59]

The CQ bit was checked by christiank@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-09 16:07:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1379783002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1379783002/260001

[christiank, 2015-12-09 16:54:37]

reveman,

Would you please have a quick look at the changes in patch set 14.

You suggested to me in an earlier comment to get rid of needs_conversion and
move everything to the switch. I noticed that I really had to because
SkColorTypeToResourceFormat that I added couldn't handle the case of not
converting in software between RGBA and BGRA.

I also moved the modulo 4 DCHECKs from ResourcePool::AcquireResource to the
switch where we perform ETC1 compression. I noticed that there was a ton of test
cases creating non-modulo 4 sized tiles that would need updating. Tests are
using FakePictureLayerImpl to compute tile sizes so they're not bound by the
same restrictions as the normal case.

[reveman, 2015-12-09 17:29:37]

still lgtm

https://codereview.chromium.org/1379783002/diff/260001/cc/raster/tile_task_wo...
File cc/raster/tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1379783002/diff/260001/cc/raster/tile_task_wo...
cc/raster/tile_task_worker_pool.cc:124: canvas_bitmap_rect, scale);
nit: maybe move l.117-124 to a utility function so we don't need both the switch
and the if statement below that both check format

[commit-bot: I haz the power, 2015-12-09 17:36:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-09 17:36:11]

Dry run: This issue passed the CQ dry run.

[christiank, 2015-12-10 09:25:59]

The CQ bit was checked by christiank@opera.com

[christiank, 2015-12-10 09:26:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
sievers@chromium.org, piman@chromium.org, nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/1379783002/#ps260001
(title: "Remove needs_conversion, fix tile size unit test and move modulo 4
DCHECK (for tests)")

[commit-bot: I haz the power, 2015-12-10 09:26:44]

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

[commit-bot: I haz the power, 2015-12-10 09:34:24]

Description was changed from

==========
Allow one-copy and zero-copy task tile worker pools to use compressed textures.

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

to

==========
Allow one-copy and zero-copy task tile worker pools to use compressed textures.

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

[commit-bot: I haz the power, 2015-12-10 09:34:25]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-12-10 09:35:24]

Description was changed from

==========
Allow one-copy and zero-copy task tile worker pools to use compressed textures.

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

to

==========
Allow one-copy and zero-copy task tile worker pools to use compressed textures.

BUG=434699
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/7d60ce9a132a88ead407a2a58c91edc431e68259
Cr-Commit-Position: refs/heads/master@{#364326}
==========

[commit-bot: I haz the power, 2015-12-10 09:35:25]

Patchset 14 (id:??) landed as
https://crrev.com/7d60ce9a132a88ead407a2a58c91edc431e68259
Cr-Commit-Position: refs/heads/master@{#364326}

[engedy, 2015-12-10 12:39:47]

A revert of this CL (patchset #14 id:260001) has been created in
https://codereview.chromium.org/1513733003/ by engedy@chromium.org.

The reason for reverting is: Resulted in
linux-release/sizes/chrome-si/initializers failure:

https://build.chromium.org/p/chromium/builders/Mac/builds/9905
https://build.chromium.org/p/chromium/builders/Linux/builds/69200

26a27
> # texture_compressor_etc1_sse.cc
_GLOBAL__sub_I_texture_compressor_etc1_sse.cc+0xb
29c30
< # Found 28 static initializers in 8 files.
---
> # Found 29 static initializers in 9 files.

If this is intended, please update corresponding performance test expectations..

[christiank, 2015-12-10 15:06:49]

Description was changed from

==========
Allow one-copy and zero-copy task tile worker pools to use compressed textures.

BUG=434699
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/7d60ce9a132a88ead407a2a58c91edc431e68259
Cr-Commit-Position: refs/heads/master@{#364326}
==========

to

==========
Allow one-copy and zero-copy task tile worker pools to use compressed textures.

BUG=434699
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/7d60ce9a132a88ead407a2a58c91edc431e68259
Cr-Commit-Position: refs/heads/master@{#364326}
==========

[christiank, 2015-12-10 18:12:14]

The CQ bit was checked by christiank@opera.com

[commit-bot: I haz the power, 2015-12-10 18:13:56]

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

[commit-bot: I haz the power, 2015-12-10 21:31:39]

Description was changed from

==========
Allow one-copy and zero-copy task tile worker pools to use compressed textures.

BUG=434699
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/7d60ce9a132a88ead407a2a58c91edc431e68259
Cr-Commit-Position: refs/heads/master@{#364326}
==========

to

==========
Allow one-copy and zero-copy task tile worker pools to use compressed textures.

BUG=434699
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/7d60ce9a132a88ead407a2a58c91edc431e68259
Cr-Commit-Position: refs/heads/master@{#364326}
==========

[commit-bot: I haz the power, 2015-12-10 21:31:41]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-12-10 21:33:28]

Description was changed from

==========
Allow one-copy and zero-copy task tile worker pools to use compressed textures.

BUG=434699
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/7d60ce9a132a88ead407a2a58c91edc431e68259
Cr-Commit-Position: refs/heads/master@{#364326}
==========

to

==========
Allow one-copy and zero-copy task tile worker pools to use compressed textures.

BUG=434699
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/7d60ce9a132a88ead407a2a58c91edc431e68259
Cr-Commit-Position: refs/heads/master@{#364326}

Committed: https://crrev.com/10fc39d36c0ba481056ec91a2984e02fd1127cf4
Cr-Commit-Position: refs/heads/master@{#364484}
==========

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

Patchset 14 (id:??) landed as
https://crrev.com/10fc39d36c0ba481056ec91a2984e02fd1127cf4
Cr-Commit-Position: refs/heads/master@{#364484}

[Rick Byers, 2015-12-17 17:04:34]

On 2015/12/10 21:33:29, commit-bot: I haz the power wrote:
> Patchset 14 (id:??) landed as
> https://crrev.com/10fc39d36c0ba481056ec91a2984e02fd1127cf4
> Cr-Commit-Position: refs/heads/master@{#364484}

I believe this commit is triggering some crashes on the perf bots:
https://code.google.com/p/chromium/issues/detail?id=570477

[Rick Byers, 2015-12-18 15:54:56]

A revert of this CL (patchset #14 id:260001) has been created in
https://codereview.chromium.org/1535953003/ by rbyers@chromium.org.

The reason for reverting is: Causing perf bot failures - apparently triggers a
render crash.  http://crbug.com/570477 and possibly http://crbug.com/570809. 
Please run a perf tryjob including these tests before relanding..