cc: Adding support for RGBA_4444 tile textures

cc/scheduler/texture_uploader.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/scheduler/texture_uploader.h"
 
 #include <algorithm>
 #include <vector>
 
 #include "base/debug/alias.h"
@@ skipping 117 matching lines @@
 void TextureUploader::EndQuery() {
   available_queries_.front()->End();
   pending_queries_.push_back(available_queries_.take_front());
   num_blocking_texture_uploads_++;
 }
 
 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));
 
   bool is_full_upload = dest_offset.IsZero() && source_rect.size() == size;
 
   if (is_full_upload)
     BeginQuery();
 
   if (use_map_tex_sub_image_) {
     UploadWithMapTexSubImage(
@@ skipping 22 matching lines @@
 
 void TextureUploader::ReleaseCachedQueries() {
   ProcessQueries();
   available_queries_.clear();
 }
 
 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();
   int source_rect_width = source_rect.width();
   int source_rect_height = source_rect.height();
   int image_rect_x = image_rect.x();
   int image_rect_y = image_rect.y();
   int image_rect_width = image_rect.width();
   int image_rect_height = image_rect.height();
   int dest_offset_x = dest_offset.x();
   int dest_offset_y = dest_offset.y();
   base::debug::Alias(&image);
   base::debug::Alias(&source_rect_x);
   base::debug::Alias(&source_rect_y);
   base::debug::Alias(&source_rect_width);
   base::debug::Alias(&source_rect_height);
   base::debug::Alias(&image_rect_x);
   base::debug::Alias(&image_rect_y);
   base::debug::Alias(&image_rect_width);
   base::debug::Alias(&image_rect_height);
   base::debug::Alias(&dest_offset_x);
   base::debug::Alias(&dest_offset_y);
   TRACE_EVENT0("cc", "TextureUploader::UploadWithTexSubImage");
 
   // Offset from image-rect to source-rect.
   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()) {
     pixel_source = &image[image_rect.width() * bytes_per_pixel * offset.y()];
   } else {
     size_t needed_size = upload_image_stride * source_rect.height();
     if (sub_image_size_ < needed_size) {
       sub_image_.reset(new uint8[needed_size]);
       sub_image_size_ = needed_size;
     }
     // Strides not equal, so do a row-by-row memcpy from the
     // paint results into a temp buffer for uploading.
     for (int row = 0; row < source_rect.height(); ++row)
       memcpy(&sub_image_[upload_image_stride * row],
              &image[bytes_per_pixel *
                  (offset.x() + (offset.y() + row) * image_rect.width())],
              source_rect.width() * bytes_per_pixel);
 
     pixel_source = &sub_image_[0];
   }
 
   context_->texSubImage2D(GL_TEXTURE_2D,
                           0,
                           dest_offset.x(),
                           dest_offset.y(),
                           source_rect.width(),
                           source_rect.height(),
-                          format,
-                          GL_UNSIGNED_BYTE,
+                          ResourceProvider::GetGLDataFormat(format),
+                          ResourceProvider::GetGLDataType(format),
                           pixel_source);
 }
 
 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();
   int source_rect_width = source_rect.width();
   int source_rect_height = source_rect.height();
   int image_rect_x = image_rect.x();
   int image_rect_y = image_rect.y();
   int image_rect_width = image_rect.width();
   int image_rect_height = image_rect.height();
   int dest_offset_x = dest_offset.x();
   int dest_offset_y = dest_offset.y();
   base::debug::Alias(&image);
   base::debug::Alias(&source_rect_x);
   base::debug::Alias(&source_rect_y);
   base::debug::Alias(&source_rect_width);
   base::debug::Alias(&source_rect_height);
   base::debug::Alias(&image_rect_x);
   base::debug::Alias(&image_rect_y);
   base::debug::Alias(&image_rect_width);
   base::debug::Alias(&image_rect_height);
   base::debug::Alias(&dest_offset_x);
   base::debug::Alias(&dest_offset_y);
 
   TRACE_EVENT0("cc", "TextureUploader::UploadWithMapTexSubImage");
 
   // 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*>(
       context_->mapTexSubImage2DCHROMIUM(GL_TEXTURE_2D,
                                          0,
                                          dest_offset.x(),
                                          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) {
     UploadWithTexSubImage(image, image_rect, source_rect, dest_offset, format);
     return;
   }
 
   if (upload_image_stride == image_rect.width() * bytes_per_pixel &&
       !offset.x()) {
     memcpy(pixel_dest,
@@ skipping 35 matching lines @@
       textures_per_second_history_.erase(textures_per_second_history_.begin());
       textures_per_second_history_.erase(--textures_per_second_history_.end());
     }
     textures_per_second_history_.insert(textures_per_second);
 
     available_queries_.push_back(pending_queries_.take_front());
   }
 }
 
 }  // namespace cc