4 files changed, 86 insertions, 107 deletions

diff --git a/minui/include/minui/minui.h b/minui/include/minui/minui.h
index e9bd1c4f1..66d992b93 100644
--- a/minui/include/minui/minui.h
+++ b/minui/include/minui/minui.h
@@ -33,10 +33,11 @@ class GRSurface {
   GRSurface() = default;
   virtual ~GRSurface();
 
-  // Creates and returns a GRSurface instance for the given data_size. The starting address of the
-  // surface data is aligned to SURFACE_DATA_ALIGNMENT. Returns the created GRSurface instance (in
-  // std::unique_ptr), or nullptr on error.
-  static std::unique_ptr<GRSurface> Create(size_t data_size);
+  // Creates and returns a GRSurface instance that's sufficient for storing an image of the given
+  // size. The starting address of the surface data is aligned to SURFACE_DATA_ALIGNMENT. Returns
+  // the created GRSurface instance (in std::unique_ptr), or nullptr on error.
+  static std::unique_ptr<GRSurface> Create(int width, int height, int row_bytes, int pixel_bytes,
+                                           size_t data_size);
 
   virtual uint8_t* data() {
     return data_;
@@ -51,6 +52,10 @@ class GRSurface {
   int row_bytes;
   int pixel_bytes;
 
+ protected:
+  GRSurface(int width, int height, int row_bytes, int pixel_bytes)
+      : width(width), height(height), row_bytes(row_bytes), pixel_bytes(pixel_bytes) {}
+
  private:
   uint8_t* data_{ nullptr };
 };
diff --git a/minui/resources.cpp b/minui/resources.cpp
index c01c1868e..9c9af0242 100644
--- a/minui/resources.cpp
+++ b/minui/resources.cpp
@@ -39,9 +39,11 @@
 
 static std::string g_resource_dir{ "/res/images" };
 
-std::unique_ptr<GRSurface> GRSurface::Create(size_t data_size) {
+std::unique_ptr<GRSurface> GRSurface::Create(int width, int height, int row_bytes, int pixel_bytes,
+                                             size_t data_size) {
   static constexpr size_t kSurfaceDataAlignment = 8;
-  std::unique_ptr<GRSurface> result = std::make_unique<GRSurface>();
+  // Cannot use std::make_unique to access non-public ctor.
+  auto result = std::unique_ptr<GRSurface>(new GRSurface(width, height, row_bytes, pixel_bytes));
   size_t aligned_size =
       (data_size + kSurfaceDataAlignment - 1) / kSurfaceDataAlignment * kSurfaceDataAlignment;
   result->data_ = static_cast<uint8_t*>(aligned_alloc(kSurfaceDataAlignment, aligned_size));
@@ -68,7 +70,7 @@ PngHandler::PngHandler(const std::string& name) {
     return;
   }
 
-  unsigned char header[8];
+  uint8_t header[8];
   size_t bytesRead = fread(header, 1, sizeof(header), png_fp_.get());
   if (bytesRead != sizeof(header)) {
     error_code_ = -2;
@@ -131,70 +133,49 @@ PngHandler::~PngHandler() {
   }
 }
 
-// "display" surfaces are transformed into the framebuffer's required
-// pixel format (currently only RGBX is supported) at load time, so
-// gr_blit() can be nothing more than a memcpy() for each row.  The
-// next two functions are the only ones that know anything about the
-// framebuffer pixel format; they need to be modified if the
-// framebuffer format changes (but nothing else should).
-
-// Allocates and returns a GRSurface* sufficient for storing an image of the indicated size in the
-// framebuffer pixel format.
-static std::unique_ptr<GRSurface> init_display_surface(png_uint_32 width, png_uint_32 height) {
-  std::unique_ptr<GRSurface> surface = GRSurface::Create(width * height * 4);
-  if (!surface) return nullptr;
-
-  surface->width = width;
-  surface->height = height;
-  surface->row_bytes = width * 4;
-  surface->pixel_bytes = 4;
-
-  return surface;
-}
+// "display" surfaces are transformed into the framebuffer's required pixel format (currently only
+// RGBX is supported) at load time, so gr_blit() can be nothing more than a memcpy() for each row.
 
-// Copy 'input_row' to 'output_row', transforming it to the
-// framebuffer pixel format.  The input format depends on the value of
-// 'channels':
+// Copies 'input_row' to 'output_row', transforming it to the framebuffer pixel format. The input
+// format depends on the value of 'channels':
 //
 //   1 - input is 8-bit grayscale
 //   3 - input is 24-bit RGB
 //   4 - input is 32-bit RGBA/RGBX
 //
 // 'width' is the number of pixels in the row.
-static void transform_rgb_to_draw(unsigned char* input_row,
-                                  unsigned char* output_row,
-                                  int channels, int width) {
-    int x;
-    unsigned char* ip = input_row;
-    unsigned char* op = output_row;
-
-    switch (channels) {
-        case 1:
-            // expand gray level to RGBX
-            for (x = 0; x < width; ++x) {
-                *op++ = *ip;
-                *op++ = *ip;
-                *op++ = *ip;
-                *op++ = 0xff;
-                ip++;
-            }
-            break;
-
-        case 3:
-            // expand RGBA to RGBX
-            for (x = 0; x < width; ++x) {
-                *op++ = *ip++;
-                *op++ = *ip++;
-                *op++ = *ip++;
-                *op++ = 0xff;
-            }
-            break;
-
-        case 4:
-            // copy RGBA to RGBX
-            memcpy(output_row, input_row, width*4);
-            break;
-    }
+static void TransformRgbToDraw(const uint8_t* input_row, uint8_t* output_row, int channels,
+                               int width) {
+  const uint8_t* ip = input_row;
+  uint8_t* op = output_row;
+
+  switch (channels) {
+    case 1:
+      // expand gray level to RGBX
+      for (int x = 0; x < width; ++x) {
+        *op++ = *ip;
+        *op++ = *ip;
+        *op++ = *ip;
+        *op++ = 0xff;
+        ip++;
+      }
+      break;
+
+    case 3:
+      // expand RGBA to RGBX
+      for (int x = 0; x < width; ++x) {
+        *op++ = *ip++;
+        *op++ = *ip++;
+        *op++ = *ip++;
+        *op++ = 0xff;
+      }
+      break;
+
+    case 4:
+      // copy RGBA to RGBX
+      memcpy(output_row, input_row, width * 4);
+      break;
+  }
 }
 
 int res_create_display_surface(const char* name, GRSurface** pSurface) {
@@ -207,7 +188,7 @@ int res_create_display_surface(const char* name, GRSurface** pSurface) {
   png_uint_32 width = png_handler.width();
   png_uint_32 height = png_handler.height();
 
-  std::unique_ptr<GRSurface> surface = init_display_surface(width, height);
+  auto surface = GRSurface::Create(width, height, width * 4, 4, width * height * 4);
   if (!surface) {
     return -8;
   }
@@ -218,10 +199,10 @@ int res_create_display_surface(const char* name, GRSurface** pSurface) {
   }
 
   for (png_uint_32 y = 0; y < height; ++y) {
-    std::vector<unsigned char> p_row(width * 4);
+    std::vector<uint8_t> p_row(width * 4);
     png_read_row(png_ptr, p_row.data(), nullptr);
-    transform_rgb_to_draw(p_row.data(), surface->data() + y * surface->row_bytes,
-                          png_handler.channels(), width);
+    TransformRgbToDraw(p_row.data(), surface->data() + y * surface->row_bytes,
+                       png_handler.channels(), width);
   }
 
   *pSurface = surface.release();
@@ -277,7 +258,9 @@ int res_create_multi_display_surface(const char* name, int* frames, int* fps,
     goto exit;
   }
   for (int i = 0; i < *frames; ++i) {
-    auto created_surface = init_display_surface(width, height / *frames);
+    auto height_per_frame = height / *frames;
+    auto created_surface =
+        GRSurface::Create(width, height_per_frame, width * 4, 4, width * height_per_frame);
     if (!created_surface) {
       result = -8;
       goto exit;
@@ -290,11 +273,11 @@ int res_create_multi_display_surface(const char* name, int* frames, int* fps,
   }
 
   for (png_uint_32 y = 0; y < height; ++y) {
-    std::vector<unsigned char> p_row(width * 4);
+    std::vector<uint8_t> p_row(width * 4);
     png_read_row(png_ptr, p_row.data(), nullptr);
     int frame = y % *frames;
-    unsigned char* out_row = surface[frame]->data() + (y / *frames) * surface[frame]->row_bytes;
-    transform_rgb_to_draw(p_row.data(), out_row, png_handler.channels(), width);
+    uint8_t* out_row = surface[frame]->data() + (y / *frames) * surface[frame]->row_bytes;
+    TransformRgbToDraw(p_row.data(), out_row, png_handler.channels(), width);
   }
 
   *pSurface = surface;
@@ -325,14 +308,10 @@ int res_create_alpha_surface(const char* name, GRSurface** pSurface) {
   png_uint_32 width = png_handler.width();
   png_uint_32 height = png_handler.height();
 
-  std::unique_ptr<GRSurface> surface = GRSurface::Create(width * height);
+  auto surface = GRSurface::Create(width, height, width, 1, width * height);
   if (!surface) {
     return -8;
   }
-  surface->width = width;
-  surface->height = height;
-  surface->row_bytes = width;
-  surface->pixel_bytes = 1;
 
   PixelFormat pixel_format = gr_pixel_format();
   if (pixel_format == PixelFormat::ABGR || pixel_format == PixelFormat::BGRA) {
@@ -340,7 +319,7 @@ int res_create_alpha_surface(const char* name, GRSurface** pSurface) {
   }
 
   for (png_uint_32 y = 0; y < height; ++y) {
-    unsigned char* p_row = surface->data() + y * surface->row_bytes;
+    uint8_t* p_row = surface->data() + y * surface->row_bytes;
     png_read_row(png_ptr, p_row, nullptr);
   }
 
@@ -389,7 +368,7 @@ std::vector<std::string> get_locales_in_png(const std::string& png_name) {
   }
 
   std::vector<std::string> result;
-  std::vector<unsigned char> row(png_handler.width());
+  std::vector<uint8_t> row(png_handler.width());
   for (png_uint_32 y = 0; y < png_handler.height(); ++y) {
     png_read_row(png_handler.png_ptr(), row.data(), nullptr);
     int h = (row[3] << 8) | row[2];
@@ -425,7 +404,7 @@ int res_create_localized_alpha_surface(const char* name,
   png_uint_32 height = png_handler.height();
 
   for (png_uint_32 y = 0; y < height; ++y) {
-    std::vector<unsigned char> row(width);
+    std::vector<uint8_t> row(width);
     png_read_row(png_ptr, row.data(), nullptr);
     int w = (row[1] << 8) | row[0];
     int h = (row[3] << 8) | row[2];
@@ -435,14 +414,10 @@ int res_create_localized_alpha_surface(const char* name,
     if (y + 1 + h >= height || matches_locale(loc, locale)) {
       printf("  %20s: %s (%d x %d @ %d)\n", name, loc, w, h, y);
 
-      std::unique_ptr<GRSurface> surface = GRSurface::Create(w * h);
+      auto surface = GRSurface::Create(w, h, w, 1, w * h);
       if (!surface) {
         return -8;
       }
-      surface->width = w;
-      surface->height = h;
-      surface->row_bytes = w;
-      surface->pixel_bytes = 1;
 
       for (int i = 0; i < h; ++i, ++y) {
         png_read_row(png_ptr, row.data(), nullptr);
diff --git a/tests/unit/minui_test.cpp b/tests/unit/minui_test.cpp
index cad6a3d79..b188b5992 100644
--- a/tests/unit/minui_test.cpp
+++ b/tests/unit/minui_test.cpp
@@ -25,7 +25,7 @@
 TEST(GRSurfaceTest, Create_aligned) {
   static constexpr size_t kSurfaceDataAlignment = 8;
   for (size_t data_size = 100; data_size < 128; data_size++) {
-    std::unique_ptr<GRSurface> surface(GRSurface::Create(data_size));
+    auto surface = GRSurface::Create(10, 1, 10, 1, data_size);
     ASSERT_TRUE(surface);
     ASSERT_EQ(0, reinterpret_cast<uintptr_t>(surface->data()) % kSurfaceDataAlignment);
   }
diff --git a/tests/unit/screen_ui_test.cpp b/tests/unit/screen_ui_test.cpp
index 0014e45f1..2b76944b1 100644
--- a/tests/unit/screen_ui_test.cpp
+++ b/tests/unit/screen_ui_test.cpp
@@ -69,17 +69,6 @@ class ScreenUITest : public testing::Test {
   MockDrawFunctions draw_funcs_;
 };
 
-// TODO(xunchang) Create a constructor.
-static GRSurface CreateFakeGRSurface(int width, int height, int row_bytes, int pixel_bytes) {
-  GRSurface fake_surface;
-  fake_surface.width = width;
-  fake_surface.height = height;
-  fake_surface.row_bytes = row_bytes;
-  fake_surface.pixel_bytes = pixel_bytes;
-
-  return fake_surface;
-}
-
 TEST_F(ScreenUITest, StartPhoneMenuSmoke) {
   TextMenu menu(false, 10, 20, HEADERS, ITEMS, 0, 20, draw_funcs_);
   ASSERT_FALSE(menu.scrollable());
@@ -241,9 +230,14 @@ TEST_F(ScreenUITest, WearMenuSelectItemsOverflow) {
 }
 
 TEST_F(ScreenUITest, GraphicMenuSelection) {
-  auto fake_surface = CreateFakeGRSurface(50, 50, 50, 1);
-  std::vector<GRSurface*> items = { &fake_surface, &fake_surface, &fake_surface };
-  GraphicMenu menu(&fake_surface, items, 0, draw_funcs_);
+  auto header = GRSurface::Create(50, 50, 50, 1, 50 * 50);
+  auto item = GRSurface::Create(50, 50, 50, 1, 50 * 50);
+  std::vector<GRSurface*> items = {
+    item.get(),
+    item.get(),
+    item.get(),
+  };
+  GraphicMenu menu(header.get(), items, 0, draw_funcs_);
 
   ASSERT_EQ(0, menu.selection());
 
@@ -263,18 +257,23 @@ TEST_F(ScreenUITest, GraphicMenuSelection) {
 }
 
 TEST_F(ScreenUITest, GraphicMenuValidate) {
-  auto fake_surface = CreateFakeGRSurface(50, 50, 50, 1);
-  std::vector<GRSurface*> items = { &fake_surface, &fake_surface, &fake_surface };
+  auto header = GRSurface::Create(50, 50, 50, 1, 50 * 50);
+  auto item = GRSurface::Create(50, 50, 50, 1, 50 * 50);
+  std::vector<GRSurface*> items = {
+    item.get(),
+    item.get(),
+    item.get(),
+  };
 
-  ASSERT_TRUE(GraphicMenu::Validate(200, 200, &fake_surface, items));
+  ASSERT_TRUE(GraphicMenu::Validate(200, 200, header.get(), items));
 
   // Menu exceeds the horizontal boundary.
-  auto wide_surface = CreateFakeGRSurface(300, 50, 300, 1);
-  ASSERT_FALSE(GraphicMenu::Validate(299, 200, &wide_surface, items));
+  auto wide_surface = GRSurface::Create(300, 50, 300, 1, 300 * 50);
+  ASSERT_FALSE(GraphicMenu::Validate(299, 200, wide_surface.get(), items));
 
   // Menu exceeds the vertical boundary.
-  items.push_back(&fake_surface);
-  ASSERT_FALSE(GraphicMenu::Validate(200, 249, &fake_surface, items));
+  items.push_back(item.get());
+  ASSERT_FALSE(GraphicMenu::Validate(200, 249, header.get(), items));
 }
 
 static constexpr int kMagicAction = 101;