cc: Adding support for RGBA_4444 tile textures

cc/scheduler/texture_uploader.cc

Index: cc/scheduler/texture_uploader.cc
diff --git a/cc/scheduler/texture_uploader.cc b/cc/scheduler/texture_uploader.cc
index 920c38e669275d0992d6b5166a1683464aecaad4..422244d1bb5cd6cbad615708ca9200295582337e 100644
--- a/cc/scheduler/texture_uploader.cc
+++ b/cc/scheduler/texture_uploader.cc
@@ -135,7 +135,7 @@ void TextureUploader::Upload(const uint8* image,
                              gfx::Rect image_rect,
                              gfx::Rect source_rect,
                              gfx::Vector2d dest_offset,
-                             GLenum format,
+                             ResourceFormat format,
                              gfx::Size size) {
   CHECK(image_rect.Contains(source_rect));
 
@@ -178,7 +178,7 @@ void TextureUploader::UploadWithTexSubImage(const uint8* image,
                                             gfx::Rect image_rect,
                                             gfx::Rect source_rect,
                                             gfx::Vector2d dest_offset,
-                                            GLenum format) {
+                                            ResourceFormat format) {
   // Instrumentation to debug issue 156107
   int source_rect_x = source_rect.x();
   int source_rect_y = source_rect.y();
@@ -207,11 +207,10 @@ void TextureUploader::UploadWithTexSubImage(const uint8* image,
   gfx::Vector2d offset(source_rect.origin() - image_rect.origin());
 
   const uint8* pixel_source;
-  unsigned int bytes_per_pixel = Resource::BytesPerPixel(format);
-  // Use 4-byte row alignment (OpenGL default) for upload performance.
-  // Assuming that GL_UNPACK_ALIGNMENT has not changed from default.

[reveman, 2013/09/17 17:44:16]

I don't think you should be removing this comment. It's still valid if I
understand this correctly.

[kaanb, 2013/09/17 18:44:26]

Done.

+  size_t bytes_per_pixel = ResourceProvider::BytesPerPixel(format);
+  DCHECK(format != RGBA_4444 || (source_rect.width() % 2) == 0);

[reveman, 2013/09/17 13:52:02]

I'm still confused by this. Why does width need to be a multiple of 2 when
format is RGBA_4444?

[kaanb, 2013/09/17 16:41:59]

Because we're not setting GL_UNPACK_ALIGNMENT to 2 when the format is 4444. See
skyostil's comments on Sep 6 and Sep 9.

[reveman, 2013/09/17 17:44:16]

But this breaks support for uneven resource dimensions. Why not let the code
below that handles this do its job?

[kaanb, 2013/09/17 18:44:26]

It turns out we don't have uneven resource dimensions in the compositor except
for unittests, so epenner and skyostil were fine with putting the DCHECK for
now. 

AFAIK, to remove the DCHECK we'd need to set UNPACK_ALIGNMENT to 2 before
issuing the texSubImage2D and set it back to 4 after that to keep Skia happy.
Furthermore async upload code needs to be changed in a similar fashion as well.

I don't mind removing the DCHECK if we all agree on what needs to be done to set
UNPACK_ALIGNMENT correctly for all codepaths.

[reveman, 2013/09/17 19:13:13]

Hm, I think you can set it to an uneven size using --default-tile-width/height.
The code here is also not specific to tiles. It's general texture upload code
that shouldn't make assumptions about texture size.
Or you perform a copy into a properly aligned buffer, which is exactly what the
code below does afaict.
Doesn't skia support 4-byte alignment for 16bpp? I thought we could use whatever
stride we want.
I don't think we should use a non-default value for UNPACK_ALIGNMENT but instead
always produce contents that is 4-byte aligned.

   unsigned int upload_image_stride =
-      RoundUp(bytes_per_pixel * source_rect.width(), 4u);
+      RoundUp(bytes_per_pixel * source_rect.width(), static_cast<size_t>(4));

[reveman, 2013/09/17 13:52:02]

why change this? 4u would be preferred.

[kaanb, 2013/09/17 16:41:59]

I get a compiler error. I guess I could leave 4u and then static_cast the LHS to
unsigned, if that's somehow more preferred.

[reveman, 2013/09/17 17:44:16]

I guess you get this error because you changed the type of bytes_per_pixel to
size_t. maybe cleanest to change that back to 'unsigned'.

Btw, while touching this code, can you also s/unsigned int/unsigned/ here?

[kaanb, 2013/09/17 18:44:26]

Done.

 
   if (upload_image_stride == image_rect.width() * bytes_per_pixel &&
       !offset.x()) {
@@ -239,8 +238,8 @@ void TextureUploader::UploadWithTexSubImage(const uint8* image,
                           dest_offset.y(),
                           source_rect.width(),
                           source_rect.height(),
-                          format,
-                          GL_UNSIGNED_BYTE,
+                          ResourceProvider::GetGLDataFormat(format),
+                          ResourceProvider::GetGLDataType(format),
                           pixel_source);
 }
 
@@ -248,7 +247,7 @@ void TextureUploader::UploadWithMapTexSubImage(const uint8* image,
                                                gfx::Rect image_rect,
                                                gfx::Rect source_rect,
                                                gfx::Vector2d dest_offset,
-                                               GLenum format) {
+                                               ResourceFormat format) {
   // Instrumentation to debug issue 156107
   int source_rect_x = source_rect.x();
   int source_rect_y = source_rect.y();
@@ -277,11 +276,10 @@ void TextureUploader::UploadWithMapTexSubImage(const uint8* image,
   // Offset from image-rect to source-rect.
   gfx::Vector2d offset(source_rect.origin() - image_rect.origin());
 
-  unsigned int bytes_per_pixel = Resource::BytesPerPixel(format);
-  // Use 4-byte row alignment (OpenGL default) for upload performance.
-  // Assuming that GL_UNPACK_ALIGNMENT has not changed from default.
+  size_t bytes_per_pixel = ResourceProvider::BytesPerPixel(format);
+  DCHECK(format != RGBA_4444 || (source_rect.width() % 2) == 0);
   unsigned int upload_image_stride =
-      RoundUp(bytes_per_pixel * source_rect.width(), 4u);
+      RoundUp(bytes_per_pixel * source_rect.width(), static_cast<size_t>(4));

[reveman, 2013/09/17 13:52:02]

same here. why not 4u?

[kaanb, 2013/09/17 16:41:59]

same answer.

[reveman, 2013/09/17 17:44:16]

same suggestions as above.

[kaanb, 2013/09/17 18:44:26]

Done.

 
   // Upload tile data via a mapped transfer buffer
   uint8* pixel_dest = static_cast<uint8*>(
@@ -291,8 +289,10 @@ void TextureUploader::UploadWithMapTexSubImage(const uint8* image,
                                          dest_offset.y(),
                                          source_rect.width(),
                                          source_rect.height(),
-                                         format,
-                                         GL_UNSIGNED_BYTE,
+                                         ResourceProvider::GetGLDataFormat(
+                                             format),
+                                         ResourceProvider::GetGLDataType(
+                                             format),
                                          GL_WRITE_ONLY));
 
   if (!pixel_dest) {