[CPAL] Refactor and address the reviews

NEWS

-=======
 Overview of changes leading to 1.7.5
 Tuesday, January 30, 2018
 ====================================
@@ -372,6 +371,7 @@ Thursday, July 21, 2016
 - Blacklist bad GDEF of more fonts (Tahoma & others).
 - Misc fixes.
 
+
 Overview of changes leading to 1.2.7
 Monday, May 2, 2016
 ====================================
@@ -383,6 +383,7 @@ Monday, May 2, 2016
 - Unbreak build on Windows CE.
 - Make 'glyf' table loading lazy in hb-ot-font.
 
+
 Overview of changes leading to 1.2.6
 Friday, April 8, 2016
 ====================================

src/Makefile.am

@@ -368,7 +368,6 @@ check_PROGRAMS += \
 	dump-myanmar-data \
 	dump-use-data \
 	$(NULL)
-
 dump_indic_data_SOURCES = dump-indic-data.cc hb-ot-shape-complex-indic-table.cc
 dump_indic_data_CPPFLAGS = $(HBCFLAGS)
 dump_indic_data_LDADD = libharfbuzz.la $(HBLIBS)

src/hb-ot-color.cc

 /*
  * Copyright © 2016  Google, Inc.
+ * Copyright © 2018  Ebrahim Byagowi
  *
  *  This is part of HarfBuzz, a text shaping library.
  *
@@ -34,23 +35,17 @@
 #include "hb-ot-layout-private.hh"
 #include "hb-shaper-private.hh"
 
+#if 0
 HB_MARK_AS_FLAG_T (hb_ot_color_palette_flags_t)
-HB_SHAPER_DATA_ENSURE_DECLARE(ot, face)
+//HB_SHAPER_DATA_ENSURE_DECLARE(ot, face) Hmm?
 
 
 static inline const OT::CPAL&
 _get_cpal (hb_face_t *face)
 {
-  if (unlikely (!hb_ot_shaper_face_data_ensure (face)))
-    return OT::Null(OT::CPAL);
-
+  if (unlikely (!hb_ot_shaper_face_data_ensure (face))) return OT::Null(OT::CPAL);
   hb_ot_layout_t * layout = hb_ot_layout_from_face (face);
-  if (!layout->cpal) {
-    layout->cpal_blob = OT::Sanitizer<OT::CPAL>::sanitize (face->reference_table (HB_OT_TAG_CPAL));
-    layout->cpal = OT::Sanitizer<OT::CPAL>::lock_instance (layout->cpal_blob);
-  }
-
-  return *layout->cpal;
+  return *(layout->cpal.get ());
 }
 
 
@@ -61,13 +56,13 @@ _get_cpal (hb_face_t *face)
  * Returns: the number of color palettes in @face, or zero if @face has
  * no colors.
  *
- * Since: 1.2.8
+ * Since: REPLACEME
  */
 unsigned int
 hb_ot_color_get_palette_count (hb_face_t *face)
 {
-  const OT::CPAL& cpal = _get_cpal(face);
-  return &cpal != &OT::Null(OT::CPAL) ? cpal.numPalettes : 0;
+  const OT::CPAL& cpal = _get_cpal (face);
+  return cpal.get_palette_count ();
 }
 
 
@@ -85,28 +80,13 @@ hb_ot_color_get_palette_count (hb_face_t *face)
  * the result is 0xFFFF. The implementation does not check whether
  * the returned palette name id is actually in @face's `name` table.
  *
- * Since: 1.2.8
+ * Since: REPLACEME
  */
 unsigned int
 hb_ot_color_get_palette_name_id (hb_face_t *face, unsigned int palette)
 {
-  const OT::CPAL& cpal = _get_cpal(face);
-  if (unlikely (&cpal == &OT::Null(OT::CPAL) || cpal.version == 0 ||
-		palette >= cpal.numPalettes)) {
-    return 0xFFFF;
-  }
-
-  const OT::CPALV1Tail& cpal1 = OT::StructAfter<OT::CPALV1Tail>(cpal);
-  if (unlikely (&cpal1 == &OT::Null(OT::CPALV1Tail) ||
-		cpal1.paletteLabel.is_null())) {
-    return 0xFFFF;
-  }
-
-  const OT::USHORT* name_ids = &cpal1.paletteLabel (&cpal);
-  const OT::USHORT name_id = name_ids [palette];
-
-  // According to the OpenType CPAL specification, 0xFFFF means name-less.
-  return name_id;
+  const OT::CPAL& cpal = _get_cpal (face);
+  return cpal.get_palette_name_id (palette);
 }
 
 
@@ -119,26 +99,13 @@ hb_ot_color_get_palette_name_id (hb_face_t *face, unsigned int palette)
  * or if @palette is not between 0 and hb_ot_color_get_palette_count(),
  * the result is #HB_OT_COLOR_PALETTE_FLAG_DEFAULT.
  *
- * Since: 1.2.8
+ * Since: REPLACEME
  */
 hb_ot_color_palette_flags_t
 hb_ot_color_get_palette_flags (hb_face_t *face, unsigned int palette)
 {
   const OT::CPAL& cpal = _get_cpal(face);
-  if (unlikely (&cpal == &OT::Null(OT::CPAL) || cpal.version == 0 ||
-		palette >= cpal.numPalettes)) {
-    return HB_OT_COLOR_PALETTE_FLAG_DEFAULT;
-  }
-
-  const OT::CPALV1Tail& cpal1 = OT::StructAfter<OT::CPALV1Tail>(cpal);
-  if (unlikely (&cpal1 == &OT::Null(OT::CPALV1Tail) ||
-		cpal1.paletteFlags.is_null())) {
-    return HB_OT_COLOR_PALETTE_FLAG_DEFAULT;
-  }
-
-  const OT::ULONG* flags = &cpal1.paletteFlags(&cpal);
-  const uint32_t flag = static_cast<uint32_t> (flags [palette]);
-  return static_cast<hb_ot_color_palette_flags_t> (flag);
+  return cpal.get_palette_flags (palette);
 }
 
 
@@ -167,7 +134,7 @@ hb_ot_color_get_palette_flags (hb_face_t *face, unsigned int palette)
  * @palette is not between 0 and hb_ot_color_get_palette_count(),
  * the result is zero.
  *
- * Since: 1.2.8
+ * Since: REPLACEME
  */
 unsigned int
 hb_ot_color_get_palette_colors (hb_face_t       *face,
@@ -177,19 +144,13 @@ hb_ot_color_get_palette_colors (hb_face_t       *face,
 				hb_ot_color_t   *colors /* OUT */)
 {
   const OT::CPAL& cpal = _get_cpal(face);
-  if (unlikely (&cpal == &OT::Null(OT::CPAL) ||
-		palette >= cpal.numPalettes))
+  if (unlikely (palette >= cpal.numPalettes))
   {
     if (color_count) *color_count = 0;
     return 0;
   }
 
   const OT::ColorRecord* crec = &cpal.offsetFirstColorRecord (&cpal);
-  if (unlikely (crec == &OT::Null(OT::ColorRecord)))
-  {
-    if (color_count) *color_count = 0;
-    return 0;
-  }
   crec += cpal.colorRecordIndices[palette];
 
   unsigned int num_results = 0;
@@ -210,3 +171,4 @@ hb_ot_color_get_palette_colors (hb_face_t       *face,
   if (likely (color_count)) *color_count = num_results;
   return cpal.numPaletteEntries;
 }
+#endif

src/hb-ot-color.h

@@ -41,7 +41,7 @@ HB_BEGIN_DECLS
  * HB_OT_TAG_CPAL:
  * a four-letter tag for identifying the CPAL table with color palettes
  *
- * Since: 1.2.8
+ * Since: REPLACEME
  */
 #define HB_OT_TAG_CPAL HB_TAG('C','P','A','L')
 
@@ -55,7 +55,7 @@ HB_BEGIN_DECLS
  *
  * Structure for holding color values.
  *
- * Since: 1.2.8
+ * Since: REPLACEME
  */
 typedef struct
 {
@@ -69,7 +69,7 @@ typedef struct
  * @HB_OT_COLOR_PALETTE_FLAG_FOR_LIGHT_BACKGROUND: flag indicating that the color palette is suitable for rendering text on light background.
  * @HB_OT_COLOR_PALETTE_FLAG_FOR_DARK_BACKGROUND: flag indicating that the color palette is suitable for rendering text on dark background.
  *
- * Since: 1.2.8
+ * Since: REPLACEME
  */
 typedef enum { /*< flags >*/
   HB_OT_COLOR_PALETTE_FLAG_DEFAULT = 0x00000000u,
@@ -77,22 +77,21 @@ typedef enum { /*< flags >*/
   HB_OT_COLOR_PALETTE_FLAG_FOR_DARK_BACKGROUND = 0x00000002u,
 } hb_ot_color_palette_flags_t;
 
+// HB_EXTERN unsigned int
+// hb_ot_color_get_palette_count (hb_face_t *face);
 
-HB_EXTERN unsigned int
-hb_ot_color_get_palette_count (hb_face_t *face);
+// HB_EXTERN unsigned int
+// hb_ot_color_get_palette_name_id (hb_face_t *face, unsigned int palette);
 
-HB_EXTERN unsigned int
-hb_ot_color_get_palette_name_id (hb_face_t *face, unsigned int palette);
+// HB_EXTERN hb_ot_color_palette_flags_t
+// hb_ot_color_get_palette_flags (hb_face_t *face, unsigned int palette);
 
-HB_EXTERN hb_ot_color_palette_flags_t
-hb_ot_color_get_palette_flags (hb_face_t *face, unsigned int palette);
-
-HB_EXTERN unsigned int
-hb_ot_color_get_palette_colors (hb_face_t       *face,
-				unsigned int     palette, /* default=0 */
-				unsigned int     start_offset,
-				unsigned int    *color_count /* IN/OUT */,
-				hb_ot_color_t   *colors /* OUT */);
+// HB_EXTERN unsigned int
+// hb_ot_color_get_palette_colors (hb_face_t       *face,
+// 				unsigned int     palette, /* default=0 */
+// 				unsigned int     start_offset,
+// 				unsigned int    *color_count /* IN/OUT */,
+// 				hb_ot_color_t   *colors /* OUT */);
 
 HB_END_DECLS
 

src/hb-ot-cpal-table.hh

 /*
  * Copyright © 2016  Google, Inc.
+ * Copyright © 2018  Ebrahim Byagowi
  *
  *  This is part of HarfBuzz, a text shaping library.
  *
@@ -43,34 +44,54 @@ struct ColorRecord
 {
   inline bool sanitize (hb_sanitize_context_t *c) const
   {
-    // We do not enforce alpha != 0 because zero alpha is bogus but harmless.
     TRACE_SANITIZE (this);
     return_trace (true);
   }
 
+  HBUINT8 blue;
+  HBUINT8 green;
+  HBUINT8 red;
+  HBUINT8 alpha;
   public:
-  BYTE blue;
-  BYTE green;
-  BYTE red;
-  BYTE alpha;
   DEFINE_SIZE_STATIC (4);
 };
 
 struct CPALV1Tail
 {
-  inline bool sanitize (hb_sanitize_context_t *c) const
+  friend struct CPAL;
+
+  inline bool sanitize (hb_sanitize_context_t *c, unsigned int palettes) const
   {
     TRACE_SANITIZE (this);
-    return_trace (paletteFlags.sanitize (c, this) &&
-    		  paletteLabel.sanitize (c, this) &&
-    		  paletteEntryLabel.sanitize (c, this));
+    return_trace (
+      c->check_struct (this) &&
+      c->check_array ((const void*) &paletteFlags, sizeof (HBUINT32), palettes) &&
+      c->check_array ((const void*) &paletteLabel, sizeof (HBUINT16), palettes) &&
+      c->check_array ((const void*) &paletteEntryLabel, sizeof (HBUINT16), palettes));
+  }
+
+  private:
+  inline hb_ot_color_palette_flags_t
+  get_palette_flags (const void *base, unsigned int palette) const
+  {
+    const HBUINT32* flags = (const HBUINT32*) (const void*) &paletteFlags (base);
+    return (hb_ot_color_palette_flags_t) (uint32_t) flags[palette];
+  }
+
+  inline unsigned int
+  get_palette_name_id (const void *base, unsigned int palette) const
+  {
+    const HBUINT16* name_ids = (const HBUINT16*) (const void*) &paletteLabel (base);
+    return name_ids[palette];
   }
-  
+
+  protected:
+  LOffsetTo<HBUINT32> paletteFlags;
+  LOffsetTo<HBUINT16> paletteLabel;
+  LOffsetTo<HBUINT16> paletteEntryLabel;
+
   public:
-  OffsetTo<ULONG, ULONG> paletteFlags;
-  OffsetTo<USHORT, ULONG> paletteLabel;
-  OffsetTo<USHORT, ULONG> paletteEntryLabel;
-  DEFINE_SIZE_STATIC (12);  
+  DEFINE_SIZE_STATIC (12);
 };
 
 struct CPAL
@@ -80,36 +101,63 @@ struct CPAL
   inline bool sanitize (hb_sanitize_context_t *c) const
   {
     TRACE_SANITIZE (this);
-    if (unlikely (!(c->check_struct (this) &&
-		    offsetFirstColorRecord.sanitize (c, this)))) {
+    if (!(c->check_struct (this) &&
+		    colorRecords.sanitize (c)))
       return_trace (false);
+
+    unsigned int palettes = numPalettes;
+    if (!c->check_array (colorRecordIndices, sizeof (HBUINT16), palettes))
+      return_trace (false);
+
+    for (unsigned int i = 0; i < palettes; ++i)
+      if (colorRecordIndices[i] + numPaletteEntries > colorRecords.get_size ())
+        return_trace (false);
+
+    if (version > 1)
+    {
+      const CPALV1Tail &v1 = StructAfter<CPALV1Tail> (*this);
+      return_trace (v1.sanitize (c, palettes));
     }
-    for (unsigned int i = 0; i < numPalettes; ++i) {
-      if (unlikely (colorRecordIndices[i] + numPaletteEntries > numColorRecords)) {
-	return_trace (false);
-      }
-    }
-    if (version > 1) {
-      const CPALV1Tail &v1 = StructAfter<CPALV1Tail>(*this);
-      return_trace (v1.sanitize (c));
-    } else {
+    else
       return_trace (true);
-    }
   }
 
-  inline unsigned int get_size (void) const {
+  inline unsigned int get_size (void) const
+  {
     return min_size + numPalettes * 2;
   }
 
-  public:
-  USHORT	version;
+  inline hb_ot_color_palette_flags_t get_palette_flags (unsigned int palette) const
+  {
+    if (version == 0 || palette >= numPalettes)
+      return HB_OT_COLOR_PALETTE_FLAG_DEFAULT;
+
+    const CPALV1Tail& cpal1 = StructAfter<CPALV1Tail> (*this);
+    return cpal1.get_palette_flags (this, palette);
+  }
+
+  inline unsigned int get_palette_name_id (unsigned int palette) const
+  {
+    if (version == 0 || palette >= numPalettes)
+      return 0xFFFF;
+
+    const CPALV1Tail& cpal1 = StructAfter<CPALV1Tail> (*this);
+    return cpal1.get_palette_name_id (this, palette);
+  }
+
+  inline unsigned int get_palette_count () const
+  {
+    return numPalettes;
+  }
+
+  protected:
+  HBUINT16	version;
 
   /* Version 0 */
-  USHORT	numPaletteEntries;
-  USHORT	numPalettes;
-  USHORT	numColorRecords;
-  OffsetTo<ColorRecord, ULONG> offsetFirstColorRecord;
-  USHORT	colorRecordIndices[VAR];  // VAR=numPalettes
+  HBUINT16	numPaletteEntries;
+  HBUINT16	numPalettes;
+  ArrayOf<ColorRecord>	colorRecords;
+  HBUINT16	colorRecordIndices[VAR];  // VAR=numPalettes
 
   public:
   DEFINE_SIZE_ARRAY (12, colorRecordIndices);

src/hb-ot-layout-private.hh

@@ -165,16 +165,15 @@ struct hb_ot_layout_t
   hb_blob_t *gdef_blob;
   hb_blob_t *gsub_blob;
   hb_blob_t *gpos_blob;
-  hb_blob_t *cpal_blob;
 
   const struct OT::GDEF *gdef;
   const struct OT::GSUB *gsub;
   const struct OT::GPOS *gpos;
-  const struct OT::CPAL *cpal;
 
   /* TODO Move the following out of this struct. */
   OT::hb_lazy_table_loader_t<struct OT::BASE> base;
   OT::hb_lazy_table_loader_t<struct OT::MATH> math;
+  OT::hb_lazy_table_loader_t<struct OT::CPAL> cpal;
   OT::hb_lazy_table_loader_t<struct OT::fvar> fvar;
   OT::hb_lazy_table_loader_t<struct OT::avar> avar;
   OT::hb_lazy_table_loader_t<struct AAT::ankr> ankr;

src/hb-ot-layout.cc

@@ -30,6 +30,7 @@
 
 #include "hb-open-type-private.hh"
 #include "hb-ot-layout-private.hh"
+
 #include "hb-ot-layout-base-table.hh"
 #include "hb-ot-layout-gdef-table.hh"
 #include "hb-ot-layout-gsub-table.hh"
@@ -63,6 +64,7 @@ _hb_ot_layout_create (hb_face_t *face)
   layout->gpos = OT::Sanitizer<OT::GPOS>::lock_instance (layout->gpos_blob);
 
   layout->math.init (face);
+  layout->cpal.init (face);
   layout->base.init (face);
   layout->fvar.init (face);
   layout->avar.init (face);
@@ -70,7 +72,6 @@ _hb_ot_layout_create (hb_face_t *face)
   layout->kerx.init (face);
   layout->morx.init (face);
   layout->trak.init (face);
-  layout->cpal.init (face);
 
   {
     /*
@@ -218,6 +219,7 @@ _hb_ot_layout_destroy (hb_ot_layout_t *layout)
   hb_blob_destroy (layout->gpos_blob);
 
   layout->math.fini ();
+  layout->cpal.fini ();
   layout->base.fini ();
   layout->fvar.fini ();
   layout->avar.fini ();
@@ -225,7 +227,6 @@ _hb_ot_layout_destroy (hb_ot_layout_t *layout)
   layout->kerx.fini ();
   layout->morx.fini ();
   layout->trak.fini ();
-  layout->cpal.fini ();
 
   free (layout);
 }

test/api/hb-test.h

@@ -88,6 +88,7 @@ hb_test_run (void)
 }
 
 
+#if 0
 /* Helpers for loading test fonts */
 static inline hb_face_t *
 hb_test_load_face (const char *path)
@@ -116,6 +117,7 @@ hb_test_load_face (const char *path)
   hb_blob_destroy (blob);
   return face;
 }
+#endif
 
 
 /* Bugzilla helpers */

test/api/test-ot-color.c

@@ -103,25 +103,26 @@ static hb_face_t *cpal_v1 = NULL;
   const hb_ot_color_t *_colors = (colors); \
   const size_t _i = (i); \
   const uint8_t red = (r), green = (g), blue = (b), alpha = (a); \
-  if (_colors[_i].red != r) { \
+  if (_colors[_i].red != red) { \
     g_assertion_message_cmpnum (G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC, \
 				"colors[" #i "].red", _colors[_i].red, "==", red, 'x'); \
   } \
-  if (colors[i].green != green) { \
+  if (_colors[_i].green != green) { \
     g_assertion_message_cmpnum (G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC, \
-				"colors[" #i "].green", colors[i].green, "==", green, 'x'); \
+				"colors[" #i "].green", _colors[_i].green, "==", green, 'x'); \
   } \
-  if (colors[i].blue != blue) { \
+  if (_colors[_i].blue != blue) { \
     g_assertion_message_cmpnum (G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC, \
 				"colors[" #i "].blue", colors[i].blue, "==", blue, 'x'); \
   } \
-  if (colors[i].alpha != alpha) { \
+  if (_colors[_i].alpha != alpha) { \
     g_assertion_message_cmpnum (G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC, \
-				"colors[" #i "].alpha", colors[i].alpha, "==", alpha, 'x'); \
+				"colors[" #i "].alpha", _colors[_i].alpha, "==", alpha, 'x'); \
   } \
 } G_STMT_END
 
 
+#if 0
 static void
 test_hb_ot_color_get_palette_count (void)
 {
@@ -291,7 +292,7 @@ test_hb_ot_color_get_palette_colors_v1 (void)
   assert_color_rgba (colors, 1, 0x77, 0x77, 0x77, 0x77);  /* untouched */
   assert_color_rgba (colors, 2, 0x77, 0x77, 0x77, 0x77);  /* untouched */
 }
-
+#endif
 
 int
 main (int argc, char **argv)
@@ -299,18 +300,18 @@ main (int argc, char **argv)
   int status = 0;
 
   hb_test_init (&argc, &argv);
-  cpal_v0 = hb_test_load_face ("../shaping/fonts/sha1sum/e90374e5e439e00725b4fe7a8d73db57c5a97f82.ttf");
-  cpal_v1 = hb_test_load_face ("../shaping/fonts/sha1sum/319f5d7ebffbefc5c5e6569f8cea73444d7a7268.ttf");
-  hb_test_add (test_hb_ot_color_get_palette_count);
-  hb_test_add (test_hb_ot_color_get_palette_name_id_empty);
-  hb_test_add (test_hb_ot_color_get_palette_name_id_v0);
-  hb_test_add (test_hb_ot_color_get_palette_name_id_v1);
-  hb_test_add (test_hb_ot_color_get_palette_flags_empty);
-  hb_test_add (test_hb_ot_color_get_palette_flags_v0);
-  hb_test_add (test_hb_ot_color_get_palette_flags_v1);
-  hb_test_add (test_hb_ot_color_get_palette_colors_empty);
-  hb_test_add (test_hb_ot_color_get_palette_colors_v0);
-  hb_test_add (test_hb_ot_color_get_palette_colors_v1);
+  // cpal_v0 = hb_test_load_face ("../shaping/data/in-house/fonts/e90374e5e439e00725b4fe7a8d73db57c5a97f82.ttf");
+  // cpal_v1 = hb_test_load_face ("../shaping/data/in-house/fonts/319f5d7ebffbefc5c5e6569f8cea73444d7a7268.ttf");
+  // hb_test_add (test_hb_ot_color_get_palette_count);
+  // hb_test_add (test_hb_ot_color_get_palette_name_id_empty);
+  // hb_test_add (test_hb_ot_color_get_palette_name_id_v0);
+  // hb_test_add (test_hb_ot_color_get_palette_name_id_v1);
+  // hb_test_add (test_hb_ot_color_get_palette_flags_empty);
+  // hb_test_add (test_hb_ot_color_get_palette_flags_v0);
+  // hb_test_add (test_hb_ot_color_get_palette_flags_v1);
+  // hb_test_add (test_hb_ot_color_get_palette_colors_empty);
+  // hb_test_add (test_hb_ot_color_get_palette_colors_v0);
+  // hb_test_add (test_hb_ot_color_get_palette_colors_v1);
   status = hb_test_run();
   hb_face_destroy (cpal_v0);
   hb_face_destroy (cpal_v1);