1
\$\begingroup\$

I recently wrote a BMP writer in C:

bmpWriter.h

#ifndef BMPWRITER_
#define BMPWRITER_

#include <stdint.h>

int write24BitBMP(const char *outputFileName, uint32_t width, uint32_t height, uint8_t *rgbData);

#endif  /* ifndef BMPWRITER_ */

bmpWriter.c

#include <stdlib.h>
#include <stdio.h>

#include "bmpWriter.h"

typedef struct {
  uint16_t bfType;      // Header field used to identify the BMP. Must be set to 'B','M' (0x42 0x4d).
  uint32_t bfSize;      // Size of the complete file (Header + InfoHeader + PixelData) in bytes.
  uint32_t bfReserved;  // Reserved. Must be set to zero.
  uint32_t bfOffBits;   // Number of bytes until the pixel data starts (counted from the file-start). Always 54?
} BMPHEADER;

typedef struct {
  uint32_t biSize;          // The size of the info header in bytes (40).
  uint32_t biWidth;         // The Bitmap width in pixels.
  uint32_t biHeight;        // The Bitmap height in pixels.
  uint16_t biPlanes;        // The number of color planes. Must be set to one.
  uint16_t biBitCount;      // The number of bits per pixel (color depth). 
  uint32_t biCompression;   // The compression method being used (zero equals no compression).
  uint32_t biSizeImage;     // The size of the raw bitmap data. Zero is fine.
  uint32_t biXPelsPerMeter; // Horizontal resolution of the image. Zero is fine.
  uint32_t biYPelsPerMeter; // Vertical   resolution of the image. Zero is fine.
  uint32_t biClrUsed;       // Number of colors in the color paletter. Zero is fine.
  uint32_t biClrImportant;  // Number of important colors used. Zero equals every color is important.
} BMPINFOHEADER;

// BMP RELATED
static void writeBMPHeader(FILE *outputFile, BMPHEADER *bmph);
static void writeBMPInfoHeader(FILE *outoutFile, BMPINFOHEADER *bmpih);

// HANDLE ENDIANESS
static void reverseBytes(void *data, uint8_t width);
static void reverseBMPHeader(BMPHEADER *bmph);
static void reverseBMPInfoHeader(BMPINFOHEADER *bmpih);
static int  isLittleEndian(void);

int write24BitBMP(const char *outputFileName, uint32_t width, uint32_t height, uint8_t *rgbData) {
  FILE *outputFile = fopen(outputFileName, "wb");

  if (!outputFile) 
    return -1;

  // One data row needs to consist of a multiple of four bytes. This is the needed overhead.
  uint8_t overhead = 3*width % 4 ? 4 - 3*width % 4 : 0;

  // BMPHEADER
  BMPHEADER bmph;
  bmph.bfType = 0x4d42; // 'MB' as HEX, little endian will store this as 'BM'.
  bmph.bfSize = (54 + (3*width+overhead)*height);
  bmph.bfReserved = 0;
  bmph.bfOffBits = 54;

  // BMPINFOHEADER
  BMPINFOHEADER bmpih;
  bmpih.biSize = 40;
  bmpih.biWidth = width;
  bmpih.biHeight = height;
  bmpih.biPlanes = 1;
  bmpih.biBitCount = 24;
  bmpih.biCompression = 0;
  bmpih.biSizeImage = (3*width+overhead) * height;
  bmpih.biXPelsPerMeter = 0;
  bmpih.biYPelsPerMeter = 0;
  bmpih.biClrUsed = 0;
  bmpih.biClrImportant = 0;

  if (!isLittleEndian()) {
    reverseBMPHeader(&bmph);
    reverseBMPInfoHeader(&bmpih);
  }

  // OR WRITE AS A WHOLE, BUT WHAT HAPPENS TO PADDING?
  writeBMPHeader(outputFile, &bmph);
  writeBMPInfoHeader(outputFile, &bmpih);

  // DOES THIS WORK WITH BIG ENDIAN AS WELL?
  uint8_t *rawData = malloc((3*width+overhead)*height);
  uint32_t i, currRow, currColumn, oi;

  if (rawData == NULL)
    return -1;

  for (currRow = height, i = 0; currRow > 0; currRow--) {
    for (currColumn = 0; currColumn < width; currColumn++) {
      rawData[i++] = rgbData[3*width*(currRow-1) + 3*currColumn + 2];
      rawData[i++] = rgbData[3*width*(currRow-1) + 3*currColumn + 1];
      rawData[i++] = rgbData[3*width*(currRow-1) + 3*currColumn];
    }
    for (oi = 0; oi < overhead; oi++) {
      rawData[i++] = 0;
    }
  }

  fwrite(rawData, (3*width+overhead)*height, 1, outputFile);

  // ->
  // RGB1 RGB2
  // RGB3 RGB4
  //
  // ->
  // RGB3 RGB4 OVERHEAD[2]
  // RGB1 RGB2 OVERHEAD[2]

  free(rawData);
  fclose(outputFile);
  return 0;
}

void writeBMPHeader(FILE *outputFile, BMPHEADER *bmph) {
  fwrite(&bmph->bfType    , sizeof(bmph->bfType)    , 1, outputFile);
  fwrite(&bmph->bfSize    , sizeof(bmph->bfSize)    , 1, outputFile);
  fwrite(&bmph->bfReserved, sizeof(bmph->bfReserved), 1, outputFile);
  fwrite(&bmph->bfOffBits , sizeof(bmph->bfOffBits) , 1, outputFile);
}

void writeBMPInfoHeader(FILE *outputFile, BMPINFOHEADER *bmpih) {
  fwrite(&bmpih->biSize          , sizeof(bmpih->biSize)         , 1, outputFile);
  fwrite(&bmpih->biWidth         , sizeof(bmpih->biWidth)        , 1, outputFile);
  fwrite(&bmpih->biHeight        , sizeof(bmpih->biHeight)       , 1, outputFile);
  fwrite(&bmpih->biPlanes        , sizeof(bmpih->biPlanes)       , 1, outputFile);
  fwrite(&bmpih->biBitCount      , sizeof(bmpih->biBitCount)     , 1, outputFile);
  fwrite(&bmpih->biCompression   , sizeof(bmpih->biCompression)  , 1, outputFile);
  fwrite(&bmpih->biSizeImage     , sizeof(bmpih->biSizeImage)    , 1, outputFile);
  fwrite(&bmpih->biXPelsPerMeter , sizeof(bmpih->biXPelsPerMeter), 1, outputFile);
  fwrite(&bmpih->biYPelsPerMeter , sizeof(bmpih->biYPelsPerMeter), 1, outputFile);
  fwrite(&bmpih->biClrUsed       , sizeof(bmpih->biClrUsed)      , 1, outputFile);
  fwrite(&bmpih->biClrImportant  , sizeof(bmpih->biClrImportant) , 1, outputFile);
}

int isLittleEndian() {
  int tmp = 1;
  return *(char*)&tmp ? 1 : 0;
}

void reverseBytes(void *data, uint8_t width) {
  uint8_t i, tmp;

  for (i =0; i < width/2; i++) {
    tmp = ((uint8_t *)data)[i];
    ((uint8_t *)data)[i] = ((uint8_t *)data)[width-i-1];
    ((uint8_t *)data)[width-i-1] = tmp;
  }
}

void reverseBMPHeader(BMPHEADER *bmph) {
  reverseBytes(&bmph->bfType,    sizeof(bmph->bfType));
  reverseBytes(&bmph->bfSize,    sizeof(bmph->bfSize));
  /* reverseBytes(&bmph->bfReserved, sizeof(bmph->bfReserved)); */
  reverseBytes(&bmph->bfOffBits, sizeof(bmph->bfOffBits));
}

void reverseBMPInfoHeader(BMPINFOHEADER *bmpih) {
  reverseBytes(&bmpih->biSize          , sizeof(bmpih->biSize));
  reverseBytes(&bmpih->biWidth         , sizeof(bmpih->biWidth));
  reverseBytes(&bmpih->biHeight        , sizeof(bmpih->biHeight));
  reverseBytes(&bmpih->biPlanes        , sizeof(bmpih->biPlanes));
  reverseBytes(&bmpih->biBitCount      , sizeof(bmpih->biBitCount));
  /* reverseBytes(&bmpih->biCompression   , sizeof(bmpih->biCompression)); */
  reverseBytes(&bmpih->biSizeImage     , sizeof(bmpih->biSizeImage));
  /* reverseBytes(&bmpih->biXPelsPerMeter , sizeof(bmpih->biXPelsPerMeter)); */
  /* reverseBytes(&bmpih->biYPelsPerMeter , sizeof(bmpih->biYPelsPerMeter)); */
  /* reverseBytes(&bmpih->biClrUsed       , sizeof(bmpih->biClrUsed)); */
  /* reverseBytes(&bmpih->biClrImportant  , sizeof(bmpih->biClrImportant)); */
}

A possible (working) test would be:

#include <stdio.h>
#include "bmpWriter.h"

int main() {
  char *outputFileName = "output.bmp";

  uint8_t rgbData[] = {
    230, 200, 200,   150, 180, 150,   100, 100, 130,
    255, 120,  80,   120, 255, 120,    80, 120, 255,
    130, 100, 100,   150, 180, 150,   200, 200, 230
  };

  write24BitBMP(outputFileName, 3, 3, rgbData);
  return 0;
}

Here are the questions about my code (I marked a few of them in the code already):

  • Is the size of the BMPHEADER and BMPINFOHEADER always 14 + 40 = 54 bytes? When does it change?
  • Would it be an improvement to store the bfType in an uint8_t [2] array instead of an uint16_t (I wouldn't have to reverse the chars 'B' and 'M' then)?

  • I'm current compiling with the clang flag -Wno-padded otherwise I'm getting the warning:

    warning: padding struct BMPHEADER with 2 bytes to align bfSize

    Should I change the order of members in the struct for better aligning (the members wouldn't be in the right order, but the struct isn't exposed and it doesn't matter for writing, since I write member per member individually)?

  • As you can see I'm currently writing struct-member per struct-member separately to disk. Would it be an improvement to write the whole struct at once? I'm currently not sure if the file will get destroyed caused by alignment.
  • Is there a more elegant way to calculate the overhead for one data-row than overhead = 3*width % 4? 4 - 3*width%4 : 0; (3*width + overhead needs to be a multiple of four - overhead should be zero if 3*width is already a multiple of four)?
  • Does my code handle endianness correctly? Is the code portable (especially the rgbData isn't reversed on big endian (only the BMPHEADER and BIMPINFOHEADER data. Is this necessary as well?)?
  • As you can see I'm currently creating one array of rawData and write it to disk (only one fwrite-call). Is it better to write the file pixel-row by pixel-row (it wouldn't take a allocation of big memory chunk, but multiple fwrite-calls)?
\$\endgroup\$

2 Answers 2

Reset to default
3
\$\begingroup\$

  • Seems like you allocating too much. There is no much performance penalty (if any) to write one row at a time. OTOH allocating the whole image may present a problem if the image is large enough. I recommend to allocate 3 * width + overhead, and reuse it. As a side benefit, you would initialize the overhead bytes just once.

  • You never use overhead per se, but only in the 3*width + overhead expression, which is the actual row width. I recommend to compute it directly:

    uint32_t row_width = ((3 * width) + 3) & ~0x03;

  • Pixel offset is 54 as long as you adhere to BITMAPINFOHEADER and do not supply the palette info (at 24 bpp you are fine).

  • To be completely portable you may want to address middle-endian platforms.

\$\endgroup\$
5
  • \$\begingroup\$ Thank you really much! Could you explain the uint32_t row_width = ((3 * width) + 3) & ~0x03; line a little bit more? \$\endgroup\$ Commented Feb 8, 2016 at 21:13
  • \$\begingroup\$ @LastSecondsToLive Adding 3 and masking off 2 least significant bits gives the desired multiple of 4. Best way to understand it is to go through 4 possible cases (0, 1, 2, and 3) with pencil and paper. \$\endgroup\$ Commented Feb 8, 2016 at 21:22
  • \$\begingroup\$ I will and come back to you, if any should questions occur. Thank you so much! \$\endgroup\$ Commented Feb 8, 2016 at 21:24
  • \$\begingroup\$ This is great! How did you get the idea behind it? \$\endgroup\$ Commented Feb 8, 2016 at 21:52
  • \$\begingroup\$ @LastSecondsToLive I have no idea. It is pretty standard technique. \$\endgroup\$ Commented Feb 8, 2016 at 21:53
2
\$\begingroup\$

File handling

At the beginning of write24BitBPM, you create a file and immediately return if file creation is unsuccessful. This is not very flexible for the code that calls this function. Instead, you should have the caller provide the open file stream themselves.

Single Responsibility Principle

Again, in your write24BitBPM function, you aren't following the single responsibility principle. Near the beginning, you are populating a BMPHEADER and a BMPINFOHEADER. These two operations should be extracted into separate functions to increase managing ability.

Use... ALL THE FWRITES!

In your writeBMPInfoHeader and writeBPMHeader functions, you are using a lot of fwrite calls. Generally, you want to keep IO function calls to a minimum because they can be expensive.

Instead of having a bunch of fwrite calls, you should just use one call. This will help speed up your code.

Questions

I'm going to do my best to answer these.

  1. Yes that will always be 54 bytes in total because each header has a specific size that was defined when the struct was being defined with the various types you used for each field. Therefore, the size will not change. However, (1) the compiler might optimize some structs by removing unnecessary data (I am unsure about that, actually), and (2) sometimes structs have fields that need to have memory allocated still and that could make it any size.

  2. I don't think it would be necessarily advantageous. If the edianness changed, you would still have to change the order in which the characters are written.

  3. I am unsure about this one. I'm not sure if rearranging the fields would actually change anything, but you could try. As for writing, if you follow the above tip with fwrite, you wouldn't just be able to change the order of your writing.

  4. I somewhat answered this above. It would be a lot better to just write the whole thing at once, and the docs don't say that any padding problems would arise. You should at least try to see if it works.

  5. I am unsure about this.

  6. If the machine edianness needs to be switched, everything needs to be switched so it can be properly read. A big edian machine isn't going to read the header big edian and then read the rest of it little edian; it's going to read everything big edian.

  7. I am also unsure about this one, but I think it is always best to keep the IO calls to a minimum. It is perfectly fine if you have to allocate a large amount of memory because then you can just send the whole chunk over to be written. Your other idea of writing out each pixel could be used as a fall back if not enough memory is available.

\$\endgroup\$
8
  • \$\begingroup\$ How could I fwrite the whole BMPHEADER and BMPINFOHEADER at once? As the compiler warning mentions: The struct contains extra padding and I would write this padding to disk as well, if I'd write the struct at once. \$\endgroup\$ Commented Feb 8, 2016 at 21:20
  • \$\begingroup\$ 6. I mean the pixel data is stored in single bytes each, so endianness wouldn't matter - nothing needs to be switched here, I guess. \$\endgroup\$ Commented Feb 8, 2016 at 21:23
  • \$\begingroup\$ @LastSecondsToLive Is there a compiler option for removing that padding? \$\endgroup\$ Commented Feb 8, 2016 at 21:25
  • \$\begingroup\$ There is something like __attribute__((packed)) which you can add in front of your struct, but compiler dependent. \$\endgroup\$ Commented Feb 8, 2016 at 21:28
  • \$\begingroup\$ @LastSecondsToLive Does writing with the padding harm the output? \$\endgroup\$ Commented Feb 8, 2016 at 21:29

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.