cc/ui: Ensure UI resource bitmaps are correctly aligned for uploads.

The GPU service code assumes that all textures uploads made directly
to shared image interface are 4 byte aligned. While this is true for
RGBA resources, which have 4 bytes per pixel, its not the case for
ALPHA_8 resources. A mismatch in the expected size causes the upload
to fail and the service side tears down the GL context.

This change fixes the above by ensuring that UI resource creation code
allocates and initializes the SkBitmaps with a 4 byte alignment.

Bug:1013992
R=kbr@chromium.org,skyostil@chromium.org

Change-Id: I2bd8e4ce7764573f2cc7b78ac8582346cf9baf7c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2093272Reviewed-by: David Trainor <dtrainor@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Commit-Queue: Khushal <khushalsagar@chromium.org>
Auto-Submit: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749737}

cc/resources/scoped_ui_resource.h

@@ -35,7 +35,7 @@ class CC_EXPORT ScopedUIResource : public UIResourceClient {
   UIResourceId id() { return id_; }
 
   // Returns the memory usage of the bitmap.
-  size_t EstimateMemoryUsage() const { return bitmap_.EstimateMemoryUsage(); }
+  size_t EstimateMemoryUsage() const { return bitmap_.SizeInBytes(); }
 
  protected:
   ScopedUIResource(UIResourceManager* ui_resource_manager,

cc/resources/ui_resource_bitmap.cc

@@ -9,6 +9,7 @@
 #include <memory>
 
 #include "base/logging.h"
+#include "base/numerics/checked_math.h"
 #include "third_party/skia/include/core/SkImageInfo.h"
 #include "third_party/skia/include/core/SkMallocPixelRef.h"
 #include "third_party/skia/include/core/SkPixelRef.h"
@@ -48,6 +49,8 @@ void UIResourceBitmap::Create(sk_sp<SkPixelRef> pixel_ref,
 }
 
 void UIResourceBitmap::DrawToCanvas(SkCanvas* canvas, SkPaint* paint) {
+  DCHECK_NE(info_.colorType(), kUnknown_SkColorType);
+
   SkBitmap bitmap;
   bitmap.setInfo(info_, pixel_ref_.get()->rowBytes());
   bitmap.setPixelRef(pixel_ref_, 0, 0);
@@ -55,8 +58,15 @@ void UIResourceBitmap::DrawToCanvas(SkCanvas* canvas, SkPaint* paint) {
   canvas->flush();
 }
 
+size_t UIResourceBitmap::SizeInBytes() const {
+  if (!pixel_ref_)
+    return 0u;
+  base::CheckedNumeric<size_t> size_in_bytes = pixel_ref_->rowBytes();
+  size_in_bytes *= info_.height();
+  return size_in_bytes.ValueOrDie();
+}
+
 UIResourceBitmap::UIResourceBitmap(const SkBitmap& skbitmap) {
-  DCHECK_EQ(skbitmap.width(), skbitmap.rowBytesAsPixels());
   DCHECK(skbitmap.isImmutable());
 
   sk_sp<SkPixelRef> pixel_ref = sk_ref_sp(skbitmap.pixelRef());
@@ -76,8 +86,10 @@ UIResourceBitmap::UIResourceBitmap(const gfx::Size& size, bool is_opaque) {
 
 UIResourceBitmap::UIResourceBitmap(sk_sp<SkPixelRef> pixel_ref,
                                    const gfx::Size& size) {
-  SkImageInfo info =
-      SkImageInfo::MakeN32(size.width(), size.height(), kOpaque_SkAlphaType);
+  // TODO(khushalsagar): It doesn't make sense to SkPixelRef to pass around
+  // encoded data.
+  SkImageInfo info = SkImageInfo::Make(
+      size.width(), size.height(), kUnknown_SkColorType, kOpaque_SkAlphaType);
   Create(std::move(pixel_ref), info, UIResourceBitmap::ETC1);
 }
 

cc/resources/ui_resource_bitmap.h

@@ -48,14 +48,11 @@ class CC_EXPORT UIResourceBitmap {
   UIResourceBitmap(const UIResourceBitmap& other);
   ~UIResourceBitmap();
 
-  // Returns the memory usage of the bitmap.
-  size_t EstimateMemoryUsage() const {
-    return pixel_ref_ ? pixel_ref_->rowBytes() * info_.height() : 0;
-  }
-
   const uint8_t* GetPixels() const {
     return static_cast<const uint8_t*>(pixel_ref_->pixels());
   }
+  size_t SizeInBytes() const;
+  size_t row_bytes() const { return pixel_ref_ ? pixel_ref_->rowBytes() : 0; }
 
  private:
   friend class AutoLockUIResourceBitmap;

cc/trees/layer_tree_host_impl.cc

@@ -5644,12 +5644,9 @@ void LayerTreeHostImpl::CreateUIResource(UIResourceId uid,
       viz::ContextProvider* context_provider =
           layer_tree_frame_sink_->context_provider();
       auto* sii = context_provider->SharedImageInterface();
-      size_t size_to_send =
-          viz::ResourceSizes::CheckedSizeInBytes<unsigned int>(upload_size,
-                                                               format);
       mailbox = sii->CreateSharedImage(
           format, upload_size, color_space, shared_image_usage,
-          base::span<const uint8_t>(bitmap.GetPixels(), size_to_send));
+          base::span<const uint8_t>(bitmap.GetPixels(), bitmap.SizeInBytes()));
     } else {
       DCHECK_EQ(bitmap.GetFormat(), UIResourceBitmap::RGBA8);
       SkImageInfo src_info =
@@ -5661,7 +5658,7 @@ void LayerTreeHostImpl::CreateUIResource(UIResourceId uid,
           dst_info, shm.mapping.memory(), dst_info.minRowBytes());
       surface->getCanvas()->writePixels(
           src_info, const_cast<uint8_t*>(bitmap.GetPixels()),
-          src_info.minRowBytes(), 0, 0);
+          bitmap.row_bytes(), 0, 0);
     }
   } else {
     // Only support auto-resizing for N32 textures (since this is primarily for
@@ -5679,7 +5676,7 @@ void LayerTreeHostImpl::CreateUIResource(UIResourceId uid,
         SkImageInfo::MakeN32Premul(gfx::SizeToSkISize(source_size));
 
     SkBitmap source_bitmap;
-    source_bitmap.setInfo(info);
+    source_bitmap.setInfo(info, bitmap.row_bytes());
     source_bitmap.setPixels(const_cast<uint8_t*>(bitmap.GetPixels()));
 
     // This applies the scale to draw the |bitmap| into |scaled_surface|. For

chrome/browser/thumbnail/cc/thumbnail_cache.cc

@@ -126,6 +126,14 @@ bool WriteBigEndianFloatToFile(base::File& file, float val) {
   return file.WriteAtCurrentPos(buffer, sizeof(buffer)) == sizeof(buffer);
 }
 
+// TODO(khushalsagar): This is a hack to ensure correct byte size computation
+// for SkPixelRefs wrapping encoded data for ETC1 compressed bitmaps. We ideally
+// shouldn't be using SkPixelRefs to wrap encoded data.
+size_t ETC1RowBytes(int width) {
+  DCHECK_EQ(width & 1, 0);
+  return width / 2;
+}
+
 }  // anonymous namespace
 
 ThumbnailCache::ThumbnailCache(size_t default_cache_size,
@@ -684,8 +692,8 @@ void ThumbnailCache::CompressionTask(
         SkImageInfo::Make(encoded_size.width(), encoded_size.height(),
                           kUnknown_SkColorType, kUnpremul_SkAlphaType);
     sk_sp<SkData> etc1_pixel_data(SkData::MakeUninitialized(encoded_bytes));
-    sk_sp<SkPixelRef> etc1_pixel_ref(
-        SkMallocPixelRef::MakeWithData(info, 0, std::move(etc1_pixel_data)));
+    sk_sp<SkPixelRef> etc1_pixel_ref(SkMallocPixelRef::MakeWithData(
+        info, ETC1RowBytes(encoded_size.width()), std::move(etc1_pixel_data)));
 
     bool success = etc1_encode_image(
         reinterpret_cast<unsigned char*>(raw_data.getPixels()),
@@ -834,8 +842,8 @@ bool ReadFromFile(base::File& file,
   SkImageInfo info = SkImageInfo::Make(
       raw_width, raw_height, kUnknown_SkColorType, kUnpremul_SkAlphaType);
 
-  *out_pixels =
-      SkMallocPixelRef::MakeWithData(info, 0, std::move(etc1_pixel_data));
+  *out_pixels = SkMallocPixelRef::MakeWithData(info, ETC1RowBytes(raw_width),
+                                               std::move(etc1_pixel_data));
 
   int extra_data_version = 0;
   if (!ReadBigEndianFromFile(file, &extra_data_version))

gpu/command_buffer/service/shared_image_backing_factory_gl_texture.cc

@@ -1140,14 +1140,23 @@ SharedImageBackingFactoryGLTexture::CreateSharedImageInternal(
       }
     } else {
       uint32_t bytes_required;
+      uint32_t unpadded_row_size = 0u;
+      uint32_t padded_row_size = 0u;
       if (!gles2::GLES2Util::ComputeImageDataSizes(
               size.width(), size.height(), 1 /* depth */, format_info.gl_format,
-              format_info.gl_type, 4 /* alignment */, &bytes_required, nullptr,
-              nullptr)) {
+              format_info.gl_type, 4 /* alignment */, &bytes_required,
+              &unpadded_row_size, &padded_row_size)) {
         LOG(ERROR) << "CreateSharedImage: Unable to compute required size for "
                       "initial texture upload.";
         return nullptr;
       }
+
+      // The GL spec, used in the computation for required bytes in the function
+      // above, assumes no padding is required for the last row in the image.
+      // But the client data does include this padding, so we add it for the
+      // data validation check here.
+      uint32_t padding = padded_row_size - unpadded_row_size;
+      bytes_required += padding;
       if (pixel_data.size() != bytes_required) {
         LOG(ERROR) << "CreateSharedImage: Initial data does not have expected "
                       "size.";

ui/gfx/android/java_bitmap.cc

@@ -7,6 +7,7 @@
 #include <android/bitmap.h>
 
 #include "base/android/jni_string.h"
+#include "base/bits.h"
 #include "base/logging.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/gfx_jni_headers/BitmapHelper_jni.h"
@@ -90,26 +91,36 @@ SkBitmap CreateSkBitmapFromJavaBitmap(const JavaBitmap& jbitmap) {
   gfx::Size src_size = jbitmap.size();
 
   SkBitmap skbitmap;
+  SkImageInfo image_info;
   switch (jbitmap.format()) {
     case ANDROID_BITMAP_FORMAT_RGBA_8888:
-      skbitmap.allocPixels(SkImageInfo::MakeN32Premul(src_size.width(),
-                                                      src_size.height()),
-                           jbitmap.stride());
+      image_info =
+          SkImageInfo::MakeN32Premul(src_size.width(), src_size.height());
       break;
     case ANDROID_BITMAP_FORMAT_A_8:
-      skbitmap.allocPixels(SkImageInfo::MakeA8(src_size.width(),
-                                               src_size.height()),
-                           jbitmap.stride());
+      image_info =
+          SkImageInfo::SkImageInfo::MakeA8(src_size.width(), src_size.height());
       break;
     default:
       CHECK(false) << "Invalid Java bitmap format: " << jbitmap.format();
       break;
   }
-  CHECK_EQ(jbitmap.byte_count(), static_cast<int>(skbitmap.computeByteSize()));
-  const void* src_pixels = jbitmap.pixels();
-  void* dst_pixels = skbitmap.getPixels();
-  memcpy(dst_pixels, src_pixels, skbitmap.computeByteSize());
 
+  // Ensure 4 byte stride alignment since the texture upload code in the
+  // compositor relies on this.
+  const size_t min_row_bytes = image_info.minRowBytes();
+  DCHECK_GE(jbitmap.stride(), min_row_bytes);
+
+  const size_t row_bytes = base::bits::Align(min_row_bytes, 4u);
+  skbitmap.allocPixels(image_info, row_bytes);
+
+  const char* src_pixels = static_cast<const char*>(jbitmap.pixels());
+  char* dst_pixels = static_cast<char*>(skbitmap.getPixels());
+  for (int i = 0; i < src_size.height(); ++i) {
+    memcpy(dst_pixels, src_pixels, min_row_bytes);
+    src_pixels += jbitmap.stride();
+    dst_pixels += row_bytes;
+  }
   return skbitmap;
 }