Skip to main content
deleted 393 characters in body; edited tags
Source Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238

C - BMP file writer in C

I recently wrote a BMP writer in C. I still have a few questions regarding code quality and portability left, but first the code:

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

  1. Is the size of the BMPHEADER and BMPINFOHEADER always 14 + 40 = 54 bytes? When does it change?
  2. 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)?
  3. 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)?
  4. 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...
  5. 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)?
  6. 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?)?
  7. 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)?

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

I know this is a lot, but I really do appreciate the work and time spend on a review.

  • 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; (3width + overhead needs to be a multiple of four - overhead should be zero if 3width 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)?

C - BMP file writer

I recently wrote a BMP writer in C. I still have a few questions regarding code quality and portability left, but first the code:

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

  1. Is the size of the BMPHEADER and BMPINFOHEADER always 14 + 40 = 54 bytes? When does it change?
  2. 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)?
  3. 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)?
  4. 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...
  5. 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)?
  6. 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?)?
  7. 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)?

I know this is a lot, but I really do appreciate the work and time spend on a review.

BMP writer in C

I recently wrote a BMP writer in C:

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; (3width + overhead needs to be a multiple of four - overhead should be zero if 3width 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)?

edited title
Link

C - Write BMP file writer

Source Link

C - Write BMP file writer

I recently wrote a BMP writer in C. I still have a few questions regarding code quality and portability left, but first the code:

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).

  1. Is the size of the BMPHEADER and BMPINFOHEADER always 14 + 40 = 54 bytes? When does it change?
  2. 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)?
  3. 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)?
  4. 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...
  5. 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)?
  6. 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?)?
  7. 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)?

I know this is a lot, but I really do appreciate the work and time spend on a review.