cc: Adding ETC1 support to UIResourceBitmap and ResourceProvider

cc: Adding ETC1 support to UIResourceBitmap and ResourceProvider

ETC1 compressed texture format is used by a number of UI elements.

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

[powei, 2013-10-18 01:35:54]

PTAL.  Thanks.

[aelias_OOO_until_Jul13, 2013-10-18 01:59:24]

Looks pretty good.  Please add some tests.

https://codereview.chromium.org/27973002/diff/1/cc/resources/resource.cc
File cc/resources/resource.cc (right):

https://codereview.chromium.org/27973002/diff/1/cc/resources/resource.cc#newc...
cc/resources/resource.cc:18: size_t compressed_width = size.width() / 4 +
(size.width() % 4 ? 1 : 0);
Could we just DCHECK that the resource size is divisible by 4 instead and avoid
this method?

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

https://codereview.chromium.org/27973002/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.h:491: inline unsigned
BytesPerPixel(ResourceFormat format) {
Please rename this method to BitsPerPixel and multiply everything by 8.  ETC1
would be 4.

https://codereview.chromium.org/27973002/diff/1/cc/resources/ui_resource_bitm...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/27973002/diff/1/cc/resources/ui_resource_bitm...
cc/resources/ui_resource_bitmap.h:47: UIResourceWrapMode wrap_mode,
Could you move wrap_mode out of the constructors and add a method SetWrapMode?

[aelias_OOO_until_Jul13, 2013-10-18 03:03:29]

Also, do you need to check for GL_OES_compressed_ETC1_RGB8_texture?  I haven't
been able to find anywhere that says this is mandated in OpenGL ES 2.0.

[kaanb, 2013-10-18 16:54:48]

https://codereview.chromium.org/27973002/diff/1/cc/resources/resource.cc
File cc/resources/resource.cc (right):

https://codereview.chromium.org/27973002/diff/1/cc/resources/resource.cc#newc...
cc/resources/resource.cc:17: size_t block_size = 64;
const size_t kBlockSize = 64;

as this is a compile time constant

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

https://codereview.chromium.org/27973002/diff/1/cc/resources/resource.h#newco...
cc/resources/resource.h:31: else
nit: else is redundant

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

https://codereview.chromium.org/27973002/diff/1/cc/resources/resource_format....
cc/resources/resource_format.h:15: ETC1,
We should keep RGBA_8888 as the first value of the enum, I'd add ETC1 after
RGB_565 and update RESOURCE_FORMAT_MAX as well

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

https://codereview.chromium.org/27973002/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:17: #include "cc/resources/resource.h"
you shouldn't need this include, isn't it already included in
resource_provider.h?

https://codereview.chromium.org/27973002/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:1572: context3d->texStorage2DEXT(
please revert this block, it just changes formatting

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

https://codereview.chromium.org/27973002/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.h:520: GL_ETC1_RGB8_OES,  // ETC1
Is this a GL data format or a GL internal format?

https://codereview.chromium.org/27973002/diff/1/cc/scheduler/texture_uploader.cc
File cc/scheduler/texture_uploader.cc (right):

https://codereview.chromium.org/27973002/diff/1/cc/scheduler/texture_uploader...
cc/scheduler/texture_uploader.cc:189: // ETC1 textures take a separate path
I don't think you need this comment, the code is self-explanatory.

https://codereview.chromium.org/27973002/diff/1/cc/scheduler/texture_uploader...
cc/scheduler/texture_uploader.cc:250: DCHECK_NE(format, ETC1);
nit: DCHECK_NE(ETC1, format);

https://codereview.chromium.org/27973002/diff/1/cc/scheduler/texture_uploader...
cc/scheduler/texture_uploader.cc:306: DCHECK_EQ(dest_offset.x(), 0);
nit: the first parameter to DCHECK is the expected value, hence
DCHECK_EQ(0, des_offset.x())

[powei, 2013-10-23 05:36:15]

PTAL.  Thanks.

GL_OES_compressed_ETC1_RGB8_texture is not officially supported by OES2.  I've
added dchecks accordingly.

https://codereview.chromium.org/27973002/diff/1/cc/resources/resource.cc
File cc/resources/resource.cc (right):

https://codereview.chromium.org/27973002/diff/1/cc/resources/resource.cc#newc...
cc/resources/resource.cc:17: size_t block_size = 64;
On 2013/10/18 16:54:49, kaanb wrote:
> const size_t kBlockSize = 64;
> 
> as this is a compile time constant

Done.

https://codereview.chromium.org/27973002/diff/1/cc/resources/resource.cc#newc...
cc/resources/resource.cc:18: size_t compressed_width = size.width() / 4 +
(size.width() % 4 ? 1 : 0);
On 2013/10/18 01:59:24, aelias wrote:
> Could we just DCHECK that the resource size is divisible by 4 instead and
avoid
> this method?

Done.

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

https://codereview.chromium.org/27973002/diff/1/cc/resources/resource.h#newco...
cc/resources/resource.h:31: else
On 2013/10/18 16:54:49, kaanb wrote:
> nit: else is redundant

Done.

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

https://codereview.chromium.org/27973002/diff/1/cc/resources/resource_format....
cc/resources/resource_format.h:15: ETC1,
On 2013/10/18 16:54:49, kaanb wrote:
> We should keep RGBA_8888 as the first value of the enum, I'd add ETC1 after
> RGB_565 and update RESOURCE_FORMAT_MAX as well

Done.

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

https://codereview.chromium.org/27973002/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:17: #include "cc/resources/resource.h"
On 2013/10/18 16:54:49, kaanb wrote:
> you shouldn't need this include, isn't it already included in
> resource_provider.h?

Done.

https://codereview.chromium.org/27973002/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:1572: context3d->texStorage2DEXT(
On 2013/10/18 16:54:49, kaanb wrote:
> please revert this block, it just changes formatting

Done.

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

https://codereview.chromium.org/27973002/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.h:491: inline unsigned
BytesPerPixel(ResourceFormat format) {
On 2013/10/18 01:59:24, aelias wrote:
> Please rename this method to BitsPerPixel and multiply everything by 8.  ETC1
> would be 4.

Done.

https://codereview.chromium.org/27973002/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.h:520: GL_ETC1_RGB8_OES,  // ETC1
On 2013/10/18 16:54:49, kaanb wrote:
> Is this a GL data format or a GL internal format? 

If I'm reading the spec correctly.  We're suppose to use GL_ETC1_RGB8_OES for
the internal format in glCompressedTexImage2D and for the data format in
glCompressedTexSubImage2D

http://www.khronos.org/registry/gles/extensions/OES/OES_compressed_ETC1_RGB8_...

http://www.khronos.org/opengles/sdk/1.1/docs/man/glCompressedTexSubImage2D.xml

https://codereview.chromium.org/27973002/diff/1/cc/resources/ui_resource_bitm...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/27973002/diff/1/cc/resources/ui_resource_bitm...
cc/resources/ui_resource_bitmap.h:47: UIResourceWrapMode wrap_mode,
On 2013/10/18 01:59:24, aelias wrote:
> Could you move wrap_mode out of the constructors and add a method SetWrapMode?

Done.

https://codereview.chromium.org/27973002/diff/1/cc/scheduler/texture_uploader.cc
File cc/scheduler/texture_uploader.cc (right):

https://codereview.chromium.org/27973002/diff/1/cc/scheduler/texture_uploader...
cc/scheduler/texture_uploader.cc:189: // ETC1 textures take a separate path
On 2013/10/18 16:54:49, kaanb wrote:
> I don't think you need this comment, the code is self-explanatory.

Done.

https://codereview.chromium.org/27973002/diff/1/cc/scheduler/texture_uploader...
cc/scheduler/texture_uploader.cc:250: DCHECK_NE(format, ETC1);
On 2013/10/18 16:54:49, kaanb wrote:
> nit: DCHECK_NE(ETC1, format);

Done.

https://codereview.chromium.org/27973002/diff/1/cc/scheduler/texture_uploader...
cc/scheduler/texture_uploader.cc:306: DCHECK_EQ(dest_offset.x(), 0);
On 2013/10/18 16:54:49, kaanb wrote:
> nit: the first parameter to DCHECK is the expected value, hence
> DCHECK_EQ(0, des_offset.x())

Done.

[aelias_OOO_until_Jul13, 2013-10-23 19:54:06]

lgtm.  Adding sievers@ for OWNERS of content/common/gpu/

https://codereview.chromium.org/27973002/diff/250001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/27973002/diff/250001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1374: gfx::Size& size = resource->size;
Please delete the "&", it doesn't look like you're using this as a non-const
ref.

[David Trainor- moved to gerrit, 2013-10-23 20:09:01]

lgtm pending everyone else.

[kaanb, 2013-10-23 20:30:09]

lg2m with some nits and suggestions

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

https://codereview.chromium.org/27973002/diff/250001/cc/resources/resource_pr...
cc/resources/resource_provider_unittest.cc:17: #include
"cc/resources/resource.h"
do you need this include? Please remove it if IWYU presubmit check doesn't
complain about it.

https://codereview.chromium.org/27973002/diff/250001/cc/resources/resource_pr...
cc/resources/resource_provider_unittest.cc:17: #include
"cc/resources/resource.h"
Please remove this include if IWYU presubmit check doesn't complain about it

https://codereview.chromium.org/27973002/diff/250001/cc/resources/resource_pr...
cc/resources/resource_provider_unittest.cc:2135: WGC3Dsizei imageSize,
nit: s/imageSize/image_size/ also fix the other mock below

https://codereview.chromium.org/27973002/diff/250001/cc/resources/resource_pr...
cc/resources/resource_provider_unittest.cc:2586: ResourceFormat format = ETC1;
consider hardcoding ETC1 while calling ResourceProvider::CreateResource

https://codereview.chromium.org/27973002/diff/250001/cc/resources/resource_pr...
cc/resources/resource_provider_unittest.cc:2593: id =
resource_provider->CreateResource(
you can define id here

https://codereview.chromium.org/27973002/diff/250001/cc/resources/resource_pr...
cc/resources/resource_provider_unittest.cc:2607: id =
resource_provider->CreateResource(
consider moving this part into its own test case

https://codereview.chromium.org/27973002/diff/250001/cc/scheduler/texture_upl...
File cc/scheduler/texture_uploader.cc (right):

https://codereview.chromium.org/27973002/diff/250001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:201: unsigned bytes_per_pixel =
BitsPerPixel(format) / 8;
could you keep a BytesPerPixel method in resource_provider.h that simply returns
BitsPerPixel(format) / 8

[no sievers, 2013-10-23 20:33:15]

https://codereview.chromium.org/27973002/diff/250001/cc/resources/resource.h
File cc/resources/resource.h (right):

https://codereview.chromium.org/27973002/diff/250001/cc/resources/resource.h#...
cc/resources/resource.h:29: return (BitsPerPixel(format) * size.width() *
size.height()) / 8;
nit: maybe dcheck that bpp*w*h is a multiple of 8

https://codereview.chromium.org/27973002/diff/250001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/27973002/diff/250001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1393: if (allocate) {
I think the change in here is wrong. Don't the UI resource uploads go through
ResourceProvider::SetPixels()? Why do you need to change this function?

https://codereview.chromium.org/27973002/diff/250001/cc/scheduler/texture_upl...
File cc/scheduler/texture_uploader.cc (right):

https://codereview.chromium.org/27973002/diff/250001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:87:
use_compressed_texture_etc1_(use_compressed_texture_etc1) {
nit: I'd rename this to 'have_compressed_texture_etc1_' since it's not really
used to chose an alternate codepath like the flags above. You might also just
remove it from this class since you are only using it for a DCHECK(). The the
decoder would already generate a GL error if you tried to use the format where
the extension is not available and the function call is explicit enough
(UploadWithTexSubImageETC1).

https://codereview.chromium.org/27973002/diff/250001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:314: context_->compressedTexSubImage2D(
Wait, I'm confused. Like your comment says above, texSubImage does not work with
ETC1.
The spec does not support it, and there is even a unit test
(GLES2DecoderManualInitTest.CompressedTexImage2DETC1, see 'Test
CompressedTexSubImage not allowed').

[powei, 2013-10-23 22:20:06]

https://codereview.chromium.org/27973002/diff/250001/cc/resources/resource.h
File cc/resources/resource.h (right):

https://codereview.chromium.org/27973002/diff/250001/cc/resources/resource.h#...
cc/resources/resource.h:29: return (BitsPerPixel(format) * size.width() *
size.height()) / 8;
On 2013/10/23 20:33:15, sievers wrote:
> nit: maybe dcheck that bpp*w*h is a multiple of 8

Done.

https://codereview.chromium.org/27973002/diff/250001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/27973002/diff/250001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1374: gfx::Size& size = resource->size;
On 2013/10/23 19:54:07, aelias wrote:
> Please delete the "&", it doesn't look like you're using this as a non-const
> ref.

Done.

https://codereview.chromium.org/27973002/diff/250001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1393: if (allocate) {
On 2013/10/23 20:33:15, sievers wrote:
> I think the change in here is wrong. Don't the UI resource uploads go through
> ResourceProvider::SetPixels()? Why do you need to change this function?

Done.  Removed.  You're right.  I got careless and just added ETC1 path
everywhere I see texImage.

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

https://codereview.chromium.org/27973002/diff/250001/cc/resources/resource_pr...
cc/resources/resource_provider_unittest.cc:17: #include
"cc/resources/resource.h"
On 2013/10/23 20:30:09, kaanb wrote:
> Please remove this include if IWYU presubmit check doesn't complain about it

Done.

https://codereview.chromium.org/27973002/diff/250001/cc/resources/resource_pr...
cc/resources/resource_provider_unittest.cc:2135: WGC3Dsizei imageSize,
On 2013/10/23 20:30:09, kaanb wrote:
> nit: s/imageSize/image_size/ also fix the other mock below

Done.

https://codereview.chromium.org/27973002/diff/250001/cc/resources/resource_pr...
cc/resources/resource_provider_unittest.cc:2586: ResourceFormat format = ETC1;
On 2013/10/23 20:30:09, kaanb wrote:
> consider hardcoding ETC1 while calling ResourceProvider::CreateResource

Done.

https://codereview.chromium.org/27973002/diff/250001/cc/resources/resource_pr...
cc/resources/resource_provider_unittest.cc:2593: id =
resource_provider->CreateResource(
On 2013/10/23 20:30:09, kaanb wrote:
> you can define id here

Done.

https://codereview.chromium.org/27973002/diff/250001/cc/resources/resource_pr...
cc/resources/resource_provider_unittest.cc:2607: id =
resource_provider->CreateResource(
On 2013/10/23 20:30:09, kaanb wrote:
> consider moving this part into its own test case

Done.

https://codereview.chromium.org/27973002/diff/250001/cc/scheduler/texture_upl...
File cc/scheduler/texture_uploader.cc (right):

https://codereview.chromium.org/27973002/diff/250001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:87:
use_compressed_texture_etc1_(use_compressed_texture_etc1) {
On 2013/10/23 20:33:15, sievers wrote:
> nit: I'd rename this to 'have_compressed_texture_etc1_' since it's not really
> used to chose an alternate codepath like the flags above. You might also just
> remove it from this class since you are only using it for a DCHECK(). The the
> decoder would already generate a GL error if you tried to use the format where
> the extension is not available and the function call is explicit enough
> (UploadWithTexSubImageETC1).

Done.  Removed.

https://codereview.chromium.org/27973002/diff/250001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:201: unsigned bytes_per_pixel =
BitsPerPixel(format) / 8;
On 2013/10/23 20:30:09, kaanb wrote:
> could you keep a BytesPerPixel method in resource_provider.h that simply
returns
> BitsPerPixel(format) / 8 
> 

Done.

https://codereview.chromium.org/27973002/diff/250001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:314: context_->compressedTexSubImage2D(
On 2013/10/23 20:33:15, sievers wrote:
> Wait, I'm confused. Like your comment says above, texSubImage does not work
with
> ETC1.
> The spec does not support it, and there is even a unit test
> (GLES2DecoderManualInitTest.CompressedTexImage2DETC1, see 'Test
> CompressedTexSubImage not allowed'). 

Done.  My comment above was to discourage users from using subimage uploads on
an etc1 resource, but I did not know that this is not even supported.  I've
changed it to just compressedTexImage2D.

[kaanb, 2013-10-23 22:25:25]

https://codereview.chromium.org/27973002/diff/600001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/27973002/diff/600001/cc/resources/resource_pr...
cc/resources/resource_provider.h:515: DCHECK_EQ(0u, BitsPerPixel(format) % 8);
wouldn't this be false for ETC1?

[aelias_OOO_until_Jul13, 2013-10-23 22:29:12]

https://codereview.chromium.org/27973002/diff/600001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/27973002/diff/600001/cc/resources/resource_pr...
cc/resources/resource_provider.h:515: DCHECK_EQ(0u, BitsPerPixel(format) % 8);
On 2013/10/23 22:25:25, kaanb wrote:
> wouldn't this be false for ETC1?

Yes, I suggest deleting this method instead of assuming that it will only be
called in non-ETC1 case.  Callers from now on should give a bit of thought about
the case where BitsPerPixel might be <8.  They will typically be multiplying by
the size which should make the post-division by 8 safe.

[kaanb, 2013-10-23 22:34:50]

https://codereview.chromium.org/27973002/diff/600001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/27973002/diff/600001/cc/resources/resource_pr...
cc/resources/resource_provider.h:515: DCHECK_EQ(0u, BitsPerPixel(format) % 8);
On 2013/10/23 22:29:13, aelias wrote:
> On 2013/10/23 22:25:25, kaanb wrote:
> > wouldn't this be false for ETC1?
> 
> Yes, I suggest deleting this method instead of assuming that it will only be
> called in non-ETC1 case.  Callers from now on should give a bit of thought
about
> the case where BitsPerPixel might be <8.  They will typically be multiplying
by
> the size which should make the post-division by 8 safe.

sgtm

[no sievers, 2013-10-23 22:49:59]

https://codereview.chromium.org/27973002/diff/600001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/27973002/diff/600001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1616: NULL));
I don't think glCompressedTexImage2D() supports passing NULL here to preallocate
the storage per the spec. 
The service side seems to handle null pointers (but I did not look at the
cmdbuffer code in the middle). It allocates a buffer set to 0. But even then, if
that doesn't decode properly, this call could still fail (spec: 'If the
compressed image is not encoded according to the defined image format, the
results of the call are
undefined.').

Maybe should just do nothing herefor ETC1?

https://codereview.chromium.org/27973002/diff/600001/cc/scheduler/texture_upl...
File cc/scheduler/texture_uploader.cc (right):

https://codereview.chromium.org/27973002/diff/600001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:296: void
TextureUploader::UploadWithTexSubImageETC1(const uint8* image,
nit: UploadWithTexImageETC1

I also suggest calling into here directly from TextureUploader::Upload() before
checking use_map_tex_sub_image_.

https://codereview.chromium.org/27973002/diff/600001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:302: // ETC1 does not support subimage uploads
yet.
nit: remove the 'yet'.

[powei, 2013-10-23 23:25:58]

https://codereview.chromium.org/27973002/diff/600001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/27973002/diff/600001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1616: NULL));
On 2013/10/23 22:49:59, sievers wrote:
> I don't think glCompressedTexImage2D() supports passing NULL here to
preallocate
> the storage per the spec. 
> The service side seems to handle null pointers (but I did not look at the
> cmdbuffer code in the middle). It allocates a buffer set to 0. But even then,
if
> that doesn't decode properly, this call could still fail (spec: 'If the
> compressed image is not encoded according to the defined image format, the
> results of the call are
> undefined.').
> 
> Maybe should just do nothing herefor ETC1?

Done.  Thanks for catching that.

https://codereview.chromium.org/27973002/diff/600001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/27973002/diff/600001/cc/resources/resource_pr...
cc/resources/resource_provider.h:515: DCHECK_EQ(0u, BitsPerPixel(format) % 8);
On 2013/10/23 22:34:51, kaanb wrote:
> On 2013/10/23 22:29:13, aelias wrote:
> > On 2013/10/23 22:25:25, kaanb wrote:
> > > wouldn't this be false for ETC1?
> > 
> > Yes, I suggest deleting this method instead of assuming that it will only be
> > called in non-ETC1 case.  Callers from now on should give a bit of thought
> about
> > the case where BitsPerPixel might be <8.  They will typically be multiplying
> by
> > the size which should make the post-division by 8 safe.
> 
> sgtm

Done.

https://codereview.chromium.org/27973002/diff/600001/cc/scheduler/texture_upl...
File cc/scheduler/texture_uploader.cc (right):

https://codereview.chromium.org/27973002/diff/600001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:296: void
TextureUploader::UploadWithTexSubImageETC1(const uint8* image,
On 2013/10/23 22:49:59, sievers wrote:
> nit: UploadWithTexImageETC1
> 
> I also suggest calling into here directly from TextureUploader::Upload()
before
> checking use_map_tex_sub_image_.

Done.

https://codereview.chromium.org/27973002/diff/600001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:302: // ETC1 does not support subimage uploads
yet.
On 2013/10/23 22:49:59, sievers wrote:
> nit: remove the 'yet'. 

Done.

[no sievers, 2013-10-24 00:13:37]

lgtm

[powei, 2013-10-24 00:22:04]

Thanks all.

[commit-bot: I haz the power, 2013-10-24 00:25:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/powei@chromium.org/27973002/670001

[commit-bot: I haz the power, 2013-10-24 04:30:32]

Change committed as 230631