IMG-65: Save BMP using structs (#1555)

* Save BMP using structs

.devcontainer/oss/README.md

@@ -19,10 +19,12 @@ lefthook install
 
 You can use [`air`](https://github.com/air-verse/air) for hot-reloading during development. Simply run: `air`.
 
-Port `8080` is forwared to the host.
+Port `8081` is forwared to the host.
 
 # Test images
 
 [test images repo](https://github.com/imgproxy/test-images.git) will be automatically cloned or pulled to `.devcontainer/images` folder before the container starts.
 
-[Try it](http://localhost:8080/insecure/rs:fit:300:200/plain/local:///kitten.jpg@png). -->
+Use `make devcontainer` to attach to the running devcontainer instance.
+
+[Try it](http://localhost:8081/insecure/rs:fit:300:200/plain/local:///kitten.jpg@png). -->

.devcontainer/oss/devcontainer.json

@@ -5,6 +5,7 @@
         8081
     ],
     "containerEnv": {
+        "IMGPROXY_PORT": "8081",
         "IMGPROXY_LOCAL_FILESYSTEM_ROOT": "/images",
         "IMGPROXY_ENABLE_VIDEO_THUMBNAILS": "true",
         "IMGPROXY_MAX_ANIMATION_FRAMES": "999",

vips/bmp.h

@@ -21,6 +21,49 @@
 #define BMP_RLE_EOF 1     // end of file
 #define BMP_RLE_MOVE_TO 2 // move to position
 
+// BMP file header
+typedef struct __attribute__((packed)) _BmpFileHeader {
+  uint8_t sig[2];           // "BM" identifier
+  uint32_t size;            // size of the BMP file
+  uint8_t reserved[4];      // 4 reserved bytes
+  uint32_t offset;          // offset to start of pixel data
+  uint32_t info_header_len; // length of the info header
+} BmpFileHeader;
+
+// BMP DIB header
+typedef struct __attribute__((packed)) _BmpDibHeader {
+  // uint32 info_header_len - already in file header
+
+  // BITMAPINFOHEADER
+  int32_t width;
+  int32_t height;
+  uint16_t planes;
+  uint16_t bpp;
+  uint32_t compression;
+  uint32_t image_size;
+  uint32_t x_ppm;
+  uint32_t y_ppm;
+  uint32_t num_colors;
+  uint32_t num_important_colors;
+
+  // BITMAPV4HEADER
+  uint32_t rmask;
+  uint32_t gmask;
+  uint32_t bmask;
+  uint32_t amask;
+  uint8_t cs_type[4];
+  uint8_t cs[36];
+  uint32_t rgamma;
+  uint32_t ggamma;
+  uint32_t bgamma;
+
+  // BITMAPV5HEADER
+  uint32_t intent;
+  uint32_t profile_data;
+  uint32_t profile_size;
+  uint32_t reserved_5;
+} BmpDibHeader;
+
 // defined in bmpload.c
 VIPS_API
 int vips_bmpload_source(VipsSource *source, VipsImage **out, ...)

vips/bmpload.c

@@ -157,17 +157,14 @@ vips_foreign_load_bmp_header(VipsForeignLoad *load)
   if (vips_source_rewind(bmp->source))
     return -1;
 
-  VipsPel file_header_buf[BMP_FILE_HEADER_LEN + 4];
-
-  // Read the header + the next uint32 after
-  if (vips_foreign_load_read_full(bmp->source, &file_header_buf, BMP_FILE_HEADER_LEN + 4) <= 0) {
+  BmpFileHeader file_header;
+  if (vips_foreign_load_read_full(bmp->source, &file_header, sizeof(file_header)) <= 0) {
     vips_error("vips_foreign_load_bmp_header", "unable to read file header from the source");
     return -1;
   }
 
-  // Check if the info header length is valid
-  uint32_t offset = GUINT32_FROM_LE(*(uint32_t *) (file_header_buf + 10));
-  uint32_t info_header_len = GUINT32_FROM_LE(*(uint32_t *) (file_header_buf + 14));
+  uint32_t offset = GUINT32_FROM_LE(file_header.offset);
+  uint32_t info_header_len = GUINT32_FROM_LE(file_header.info_header_len);
 
   if (
       (info_header_len != BMP_BITMAP_INFO_HEADER_LEN) &&
@@ -177,21 +174,20 @@ vips_foreign_load_bmp_header(VipsForeignLoad *load)
     return -1;
   }
 
-  // Now, read the info header. -4 bytes is because we've already read the first 4 bytes of
-  // the header (info_header_len) at the previous step. Constants include those 4 bytes.
-  VipsPel *info_header = VIPS_ARRAY(load, info_header_len - 4, VipsPel);
+  BmpDibHeader dib_header;
+  memset(&dib_header, 0, sizeof(dib_header));
 
-  if (vips_foreign_load_read_full(bmp->source, info_header, info_header_len - 4) <= 0) {
+  if (vips_foreign_load_read_full(bmp->source, &dib_header, info_header_len - 4) <= 0) {
     vips_error("vips_foreign_load_bmp_header", "unable to read BMP info header");
     return -1;
   }
 
-  int32_t width = GINT32_FROM_LE(*(int32_t *) (info_header));
-  int32_t height = GINT32_FROM_LE(*(int32_t *) (info_header + 4));
-  uint16_t planes = GUINT16_FROM_LE(*(uint16_t *) (info_header + 8));
-  uint16_t bpp = GUINT16_FROM_LE(*(uint16_t *) (info_header + 10));
-  uint32_t compression = GUINT32_FROM_LE(*(uint32_t *) (info_header + 12));
-  uint32_t num_colors = GUINT32_FROM_LE(*(uint32_t *) (info_header + 28));
+  int32_t width = GINT32_FROM_LE(dib_header.width);
+  int32_t height = GINT32_FROM_LE(dib_header.height);
+  uint16_t planes = GUINT16_FROM_LE(dib_header.planes);
+  uint16_t bpp = GUINT16_FROM_LE(dib_header.bpp);
+  uint32_t compression = GUINT32_FROM_LE(dib_header.compression);
+  uint32_t num_colors = GUINT32_FROM_LE(dib_header.num_colors);
   bool top_down = FALSE;
   bool rle = FALSE;
   bool bmp565 = FALSE;
@@ -207,7 +203,7 @@ vips_foreign_load_bmp_header(VipsForeignLoad *load)
   // If the info header is V4 or V5, check for alpha channel mask explicitly.
   // If it's non-zero, then the target image should have an alpha channel.
   if ((has_alpha) && (info_header_len > BMP_BITMAP_INFO_HEADER_LEN)) {
-    has_alpha = GUINT32_FROM_LE(*(uint32_t *) (info_header + 48)) != 0;
+    has_alpha = dib_header.amask != 0;
   }
 
   // Target image should have alpha channel only in case source image has alpha channel
@@ -247,43 +243,38 @@ vips_foreign_load_bmp_header(VipsForeignLoad *load)
   else if (
       (compression == COMPRESSION_BI_BITFIELDS) ||
       (compression == COMPRESSION_BI_BITFIELDS_ALPHA)) {
+
     int color_mask_len = 3;
 
     if (bpp > 24) {
       color_mask_len = 4;
     }
 
-    uint32_t color_mask_buf[4];
-    uint32_t *color_mask;
-
     // for the non-v4/v5 bmp image we need to load color mask separately since
-    // it is not included in the header
+    // it is not included in the header, hence, we had no read it before. Otherwise,
+    // for v4/v5 headers color masks are already read.
     if (info_header_len == BMP_BITMAP_INFO_HEADER_LEN) {
+      memset(&dib_header.rmask, 0, 4 * sizeof(uint32_t));
+
       // let's attach it to load itself so we won't care about conditionally freeing it
-      if (vips_foreign_load_read_full(bmp->source, color_mask_buf, color_mask_len * sizeof(uint32_t)) <= 0) {
+      if (vips_foreign_load_read_full(bmp->source, &dib_header.rmask, color_mask_len * sizeof(uint32_t)) <= 0) {
         vips_error("vips_foreign_load_bmp_header", "unable to read BMP color mask");
         return -1;
       }
-      color_mask = color_mask_buf;
-    }
-    else {
-      // In case of v4/v5 info header, the color mask is already included in the info header,
-      // we just need to read it
-      color_mask = (uint32_t *) ((VipsPel *) info_header + 36);
     }
 
     // Standard says that color masks are in BE order. However, we do all the
     // checks and calculations as like as masks were in LE order.
-    rmask = GUINT32_FROM_LE(color_mask[0]);
-    gmask = GUINT32_FROM_LE(color_mask[1]);
-    bmask = GUINT32_FROM_LE(color_mask[2]);
+    rmask = GUINT32_FROM_LE(dib_header.rmask);
+    gmask = GUINT32_FROM_LE(dib_header.gmask);
+    bmask = GUINT32_FROM_LE(dib_header.bmask);
     amask = 0; // default alpha mask is 0
 
     if (color_mask_len > 3) {
-      amask = GUINT32_FROM_LE(color_mask[3]);
+      amask = GUINT32_FROM_LE(dib_header.amask);
     }
 
-    if ((bpp == 16) && (rmask = 0xF800) && (gmask == 0x7E0) && (bmask == 0x1F)) {
+    if ((bpp == 16) && (rmask == 0xF800) && (gmask == 0x7E0) && (bmask == 0x1F)) {
       bmp565 = TRUE;
     }
     else if ((bpp == 16) && (rmask == 0x7C00) && (gmask == 0x3E0) && (bmask == 0x1F)) {

vips/bmpsave.c

@@ -15,39 +15,6 @@ static VipsBandFormat bandfmt_bmp[10] = {
   /* Promotion: */ UC, UC, UC, UC, UC, UC, UC, UC, UC, UC
 };
 
-// BMP BITMAPINFOHEADERV5 file header struct, ((packed)) since we
-// do not want any compiler-induced padding
-typedef struct __attribute__((packed)) _BmpHeader {
-  uint8_t sig[2];              // Signature 'BM'
-  uint32_t file_size;          // File size in bytes
-  uint16_t reserved[2];        // Reserved fields
-  uint32_t pix_offset;         // Offset to pixel data
-  uint32_t dib_header_size;    // DIB header size
-  int32_t width;               // Image width
-  int32_t height;              // Image height
-  uint16_t color_plane;        // Number of color planes
-  uint16_t bpp;                // Bits per pixel
-  uint32_t compression;        // Compression method
-  uint32_t image_size;         // Image size
-  uint32_t x_pixels_per_meter; // Horizontal resolution
-  uint32_t y_pixels_per_meter; // Vertical resolution
-  uint32_t color_use;          // Number of colors in palette
-  uint32_t color_important;    // Number of important colors
-  uint32_t rmask;              // Red mask
-  uint32_t gmask;              // Green mask
-  uint32_t bmask;              // Blue mask
-  uint32_t amask;              // Alpha mask (optional, only for 32 bpp BMP files)
-  uint8_t cs_type[4];          // Color space type (B G R s)
-  uint8_t cs[36];              // CIEXYZTRIPLE Color Space
-  uint32_t red_gamma;          // Red gamma
-  uint32_t green_gamma;        // Green gamma
-  uint32_t blue_gamma;         // Blue gamma
-  uint32_t intent;
-  uint32_t profile_data; // Profile data (optional, only for 32 bpp BMP files)
-  uint32_t profile_size;
-  uint32_t reserved_5;
-} BmpHeader;
-
 typedef struct _VipsForeignSaveBmp {
   VipsForeignSave parent_object;
 
@@ -139,25 +106,31 @@ vips_foreign_save_bmp_build(VipsObject *object)
   uint32_t pix_offset = BMP_FILE_HEADER_LEN + BMP_V5_INFO_HEADER_LEN;
 
   // Format BMP file header. We write 24/32 bpp BMP files only with no compression.
-  BmpHeader header;
-
-  header.sig[0] = 'B';
-  header.sig[1] = 'M';
-  header.file_size = GUINT32_TO_LE(pix_offset + image_size);
-  header.reserved[0] = 0;
-  header.reserved[1] = 0;
-  header.pix_offset = GUINT32_TO_LE(pix_offset);
-  header.dib_header_size = GUINT32_TO_LE(BMP_V5_INFO_HEADER_LEN);
+  BmpFileHeader file_header;
+  memset(&file_header, 0, sizeof(file_header));
+  file_header.sig[0] = 'B';
+  file_header.sig[1] = 'M';
+  file_header.size = GUINT32_TO_LE(pix_offset + image_size);
+  file_header.offset = GUINT32_TO_LE(pix_offset);
+  file_header.info_header_len = GUINT32_TO_LE(BMP_V5_INFO_HEADER_LEN);
+
+  if (vips_target_write(bmp->target, &file_header, sizeof(file_header)) < 0) {
+    vips_error("vips_foreign_save_bmp_build", "unable to write BMP header to target");
+    return -1;
+  }
+
+  BmpDibHeader header;
+  memset(&header, 0, sizeof(header));
   header.width = GINT32_TO_LE(in->Xsize);
   header.height = GINT32_TO_LE(-in->Ysize);
-  header.color_plane = GUINT16_TO_LE(1);
+  header.planes = GUINT16_TO_LE(1);
   header.bpp = GUINT16_TO_LE(bpp);
   header.compression = COMPRESSION_BI_RGB;
   header.image_size = GUINT32_TO_LE(image_size);
-  header.x_pixels_per_meter = 0; // GUINT32_TO_LE(2835);
-  header.y_pixels_per_meter = 0; // GUINT32_TO_LE(2835);
-  header.color_use = 0;
-  header.color_important = 0;
+  header.x_ppm = 0; // GUINT32_TO_LE(2835);
+  header.y_ppm = 0; // GUINT32_TO_LE(2835);
+  header.num_colors = 0;
+  header.num_important_colors = 0;
   header.rmask = GUINT32_TO_LE(0x00FF0000); // Standard says that masks are in BE order
   header.gmask = GUINT32_TO_LE(0x0000FF00);
   header.bmask = GUINT32_TO_LE(0x000000FF);
@@ -166,10 +139,9 @@ vips_foreign_save_bmp_build(VipsObject *object)
   header.cs_type[1] = 'G';
   header.cs_type[2] = 'R';
   header.cs_type[3] = 's';
-  memset(header.cs, 0, sizeof(header.cs)); // CIEXYZTRIPLE Color Space
-  header.red_gamma = 0;
-  header.green_gamma = 0;
-  header.blue_gamma = 0;
+  header.rgamma = 0;
+  header.ggamma = 0;
+  header.bgamma = 0;
   header.intent = GUINT32_TO_LE(4); // IMAGES intent, must be 4
   header.profile_data = 0;
   header.profile_size = 0;