[ot-color] Further improvements on COLR/CPAL implementation (#859)

* Implemented a bsearch on get_base_glyph_record
* Made get_color_record_argb actually work

src/hb-ot-color-colr-table.hh

@@ -80,41 +80,46 @@ struct COLR
   {
     TRACE_SANITIZE (this);
     if (!(c->check_struct (this) &&
-        c->check_array ((const void*) &layerRecordsOffset, sizeof (LayerRecord), numLayerRecords) &&
+        c->check_array ((const void*) &layerRecordsOffsetZ, sizeof (LayerRecord), numLayerRecords) &&
-        c->check_array ((const void*) &baseGlyphRecords, sizeof (BaseGlyphRecord), numBaseGlyphRecords)))
+        c->check_array ((const void*) &baseGlyphRecordsZ, sizeof (BaseGlyphRecord), numBaseGlyphRecords)))
       return_trace (false);
 
-    const BaseGlyphRecord* base_glyph_records = &baseGlyphRecords (this);
+    const BaseGlyphRecord* base_glyph_records = &baseGlyphRecordsZ (this);
     for (unsigned int i = 0; i < numBaseGlyphRecords; ++i)
       if (base_glyph_records[i].firstLayerIndex +
           base_glyph_records[i].numLayers > numLayerRecords)
         return_trace (false);
 
-    /* XXX values of LayerRecord structs should be sanitized */
-
     return_trace (true);
   }
 
   inline const bool get_base_glyph_record (
     hb_codepoint_t glyph_id, unsigned int &first_layer, unsigned int &num_layers) const
   {
-    /* TODO replace with bsearch */
+    const BaseGlyphRecord* base_glyph_records = &baseGlyphRecordsZ (this);
-    const BaseGlyphRecord* base_glyph_records = &baseGlyphRecords (this);
+    unsigned int min = 0, max = numBaseGlyphRecords - 1;
-    unsigned int records = numBaseGlyphRecords;
+    while (min <= max)
-    for (unsigned int i = 0; i < records; ++i)
+    {
-      if (base_glyph_records[i].gID == glyph_id)
+      unsigned int mid = (min + max) / 2;
+      hb_codepoint_t gID = base_glyph_records[mid].gID;
+      if (gID > glyph_id)
+        max = mid - 1;
+      else if (gID < glyph_id)
+        min = mid + 1;
+      else
       {
-        first_layer = base_glyph_records[i].firstLayerIndex;
+        first_layer = base_glyph_records[mid].firstLayerIndex;
-        num_layers = base_glyph_records[i].numLayers;
+        num_layers = base_glyph_records[mid].numLayers;
         return true;
       }
+    }
     return false;
   }
 
   inline void get_layer_record (int layer,
     hb_codepoint_t &glyph_id, unsigned int &palette_index) const
   {
-    const LayerRecord* records = &layerRecordsOffset (this);
+    const LayerRecord* records = &layerRecordsOffsetZ (this);
     glyph_id = records[layer].gID;
     palette_index = records[layer].paletteIndex;
   }
@@ -123,9 +128,9 @@ struct COLR
   HBUINT16	version;		/* Table version number */
   HBUINT16	numBaseGlyphRecords;	/* Number of Base Glyph Records */
   LOffsetTo<BaseGlyphRecord>
-		baseGlyphRecords;	/* Offset to Base Glyph records. */
+		baseGlyphRecordsZ;	/* Offset to Base Glyph records. */
   LOffsetTo<LayerRecord>
-		layerRecordsOffset;	/* Offset to Layer Records */
+		layerRecordsOffsetZ;	/* Offset to Layer Records */
   HBUINT16	numLayerRecords;	/* Number of Layer Records */
   public:
   DEFINE_SIZE_STATIC (14);

src/hb-ot-color-cpal-table.hh

@@ -40,25 +40,6 @@
 namespace OT {
 
 
-struct ColorRecord
-{
-  friend struct CPAL;
-
-  inline bool sanitize (hb_sanitize_context_t *c) const
-  {
-    TRACE_SANITIZE (this);
-    return_trace (true);
-  }
-
-  protected:
-  HBUINT8 blue;
-  HBUINT8 green;
-  HBUINT8 red;
-  HBUINT8 alpha;
-  public:
-  DEFINE_SIZE_STATIC (4);
-};
-
 struct CPALV1Tail
 {
   friend struct CPAL;
@@ -96,6 +77,8 @@ struct CPALV1Tail
   DEFINE_SIZE_STATIC (12);
 };
 
+typedef HBUINT32 BGRAColor;
+
 struct CPAL
 {
   static const hb_tag_t tableTag = HB_OT_TAG_CPAL;
@@ -103,16 +86,13 @@ struct CPAL
   inline bool sanitize (hb_sanitize_context_t *c) const
   {
     TRACE_SANITIZE (this);
-    if (!(c->check_struct (this) &&
+    if (!(c->check_struct (this) && // This checks colorRecordIndicesX sanity also, see #get_size
-		    colorRecords.sanitize (c)))
+        c->check_array ((const void*) &colorRecordsZ, sizeof (BGRAColor), numColorRecords)))
       return_trace (false);
 
-    unsigned int palettes = numPalettes;
+    // Check for indices sanity so no need for doing it runtime
-    if (!c->check_array (colorRecordIndices, sizeof (HBUINT16), palettes))
+    for (unsigned int i = 0; i < numPalettes; ++i)
-      return_trace (false);
+      if (colorRecordIndicesX[i] + numPaletteEntries > numColorRecords)
-
-    for (unsigned int i = 0; i < palettes; ++i)
-      if (colorRecordIndices[i] + numPaletteEntries > colorRecords.get_size ())
         return_trace (false);
 
     // If version is zero, we are done here; otherwise we need to check tail also
@@ -120,12 +100,12 @@ struct CPAL
       return_trace (true);
 
     const CPALV1Tail &v1 = StructAfter<CPALV1Tail> (*this);
-    return_trace (v1.sanitize (c, palettes));
+    return_trace (v1.sanitize (c, numPalettes));
   }
 
   inline unsigned int get_size (void) const
   {
-    return min_size + numPalettes * 2;
+    return min_size + numPalettes * sizeof (HBUINT16);
   }
 
   inline hb_ot_color_palette_flags_t get_palette_flags (unsigned int palette) const
@@ -151,14 +131,14 @@ struct CPAL
     return numPalettes;
   }
 
-  inline void get_color_record (int palette_index, uint8_t &r, uint8_t &g,
+  inline hb_ot_color_t get_color_record_argb (unsigned int color_index, unsigned int palette) const
-                                              uint8_t &b, uint8_t &a) const
   {
-    // We should check if palette_index is in range as it is not done on COLR sanitization
+    if (color_index >= numPaletteEntries || palette >= numPalettes)
-    r = colorRecords[palette_index].red;
+      return 0;
-    g = colorRecords[palette_index].green;
+
-    b = colorRecords[palette_index].blue;
+    const BGRAColor* records = &colorRecordsZ(this);
-    a = colorRecords[palette_index].alpha;
+    // No need for more range check as it is already done on #sanitize
+    return records[colorRecordIndicesX[palette] + color_index];
   }
 
   protected:
@@ -166,11 +146,12 @@ struct CPAL
   /* Version 0 */
   HBUINT16	numPaletteEntries;
   HBUINT16	numPalettes;
-  ArrayOf<ColorRecord>	colorRecords;
+  HBUINT16	numColorRecords;
-  HBUINT16	colorRecordIndices[VAR];  // VAR=numPalettes
+  LOffsetTo<HBUINT32>	colorRecordsZ;
+  HBUINT16	colorRecordIndicesX[VAR];  // VAR=numPalettes
 /*CPALV1Tail	v1[VAR];*/
   public:
-  DEFINE_SIZE_ARRAY (12, colorRecordIndices);
+  DEFINE_SIZE_ARRAY (12, colorRecordIndicesX);
 };
 
 } /* namespace OT */

src/hb-ot-color.h

@@ -39,19 +39,11 @@ HB_BEGIN_DECLS
 
 /**
  * hb_ot_color_t:
- * @red: the intensity of the red channel
+ * ARGB data type for holding color values.
- * @green: the intensity of the green channel
- * @blue: the intensity of the blue channel
- * @alpha: the transparency
- *
- * Structure for holding color values.
  *
  * Since: REPLACEME
  */
-typedef struct
+typedef uint32_t hb_ot_color_t;
-{
-  uint8_t red, green, blue, alpha;
-} hb_ot_color_t;
 
 
 /**