10
\$\begingroup\$

Here's my function that creates a bitmap file from an array of pixels. It can write a bitmap both with transparency and without transparency. Please review my code and give constructive criticism on how to increase the overall quality of it. :)

typedef unsigned char BYTE;

inline int bmpEncoder(const std::string &location,
    const unsigned __int32 &width, const unsigned __int32 &height,
    const std::vector<BYTE> &buffer,
    const bool &hasAlphaChannel = true) {

    std::ofstream fout(location, std::ios::out | std::ios::binary);

    if (fout.fail()) {
        return 0;
    }

    //Padding
    const unsigned __int8 padding = hasAlphaChannel ? 0 : (4 - (width * 3) % 4) % 4;

    //Bitmap file header.
    const char signature[2] = { 'B', 'M' };
    const unsigned __int32 fileSize = buffer.size() * sizeof(BYTE) + padding * (height - 1) + 14 + 124;
    const unsigned __int32 offset = 14 + 124;

    //Bitmap information header file
    const unsigned __int32 DIBSize = 124;
    const __int32 bitmapWidth = width;
    const __int32 bitmapHeight = height;
    const unsigned __int16 numPlanes = 1;
    const unsigned __int16 bitsPerPixel = (hasAlphaChannel) ? 32 : 24;
    const unsigned __int32 compressionMethod = (hasAlphaChannel) ? 3 : 0; //BI_RGB = 0, BI_BITFIELDS = 3
    const unsigned __int32 bitmapSize = buffer.size() * sizeof(BYTE);
    const __int32 horizontalResolution = 2834;
    const __int32 verticalResolution = 2834;
    const unsigned __int32 numColors = 0;
    const unsigned __int32 impColorCount = 0;
    const unsigned __int32 redBitmask = (hasAlphaChannel) ? 0x0000FF00 : 0; //ARGB32 pixel format
    const unsigned __int32 greenBitmask = (hasAlphaChannel) ? 0x00FF0000 : 0;
    const unsigned __int32 blueBitmask = (hasAlphaChannel) ? 0xFF000000 : 0;
    const unsigned __int32 alphaBitmask = (hasAlphaChannel) ? 0x000000FF : 0;

    //Writing the file header and information header to the file 
    std::vector<BYTE> header(offset, 0);
    header[0] = signature[0];
    header[1] = signature[1];

#define HEADERS(i, variableName)    header[i] = variableName; header[i+1] = variableName >> 8; header[i+2] = variableName >> 16; header[i+3] = variableName >> 24;

    HEADERS(2, fileSize);
    HEADERS(6, 0);
    HEADERS(10, offset);
    HEADERS(14, DIBSize);
    HEADERS(18, bitmapWidth);
    HEADERS(22, bitmapHeight);

    header[26] = (BYTE)numPlanes;
    header[27] = (BYTE)(numPlanes >> 8);
    header[28] = (BYTE)bitsPerPixel;
    header[29] = (BYTE)(bitsPerPixel >> 8);

    HEADERS(30, compressionMethod);
    HEADERS(34, bitmapSize);
    HEADERS(38, horizontalResolution);
    HEADERS(42, verticalResolution);
    HEADERS(46, numColors);
    HEADERS(50, impColorCount);
    HEADERS(54, redBitmask);
    HEADERS(58, greenBitmask);
    HEADERS(62, blueBitmask);
    HEADERS(66, alphaBitmask);

#undef HEADERS

    fout.write((char *)header.data(), sizeof(BYTE) * header.size());

    //Writing the pixel array
    const unsigned __int32 bWidth = bitsPerPixel / 8 * width;

    for (int i = height - 1; i >= 0; i--) {
        std::vector<BYTE> row(buffer.begin() + i * bWidth, buffer.begin() + i * bWidth + bWidth);
        fout.write((char *)row.data(), row.size() * sizeof(BYTE));
        fout.seekp(padding * sizeof(BYTE), std::ios::cur);
    }

    fout.close();
    return 1;
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! Could you edit your post to include sample usage of this function? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ May 24 '18 at 21:53
10
\$\begingroup\$

Not bad! Let's go through it line-by-line.

typedef unsigned char BYTE;

This is fine, but just so you know, starting from C++17, there is a std::byte that could replace this. If you're not using C++17 yet, fine, but if this is code is for C++17 or better, that's something you can take advantage of.

inline int bmpEncoder(const std::string &location,
    const unsigned __int32 &width, const unsigned __int32 &height,
    const std::vector<BYTE> &buffer,
    const bool &hasAlphaChannel = true) {

I'm not sure that the inline specifier serves any purpose here. Compilers generally ignore inline suggestions anyway, but in this case I can't see this entire huge function being something you'd want to inline. If you added it to get a header-only library function, that makes sense, but this doesn't really seem like a candidate for a header-only library.

You use __int32 and similar types all over the code, but were you aware there are standard versions? Just include <cstdint> and you get access to the fixed-width integer types. Instead of const unsigned __int32, you'd do std::uint32_t.

I don't really see the benefit of taking 32-bit integers by reference. They're basically cost-free to copy, so you might as well use them by value.

Taking the image data by const std::vector<BYTE>& is a good idea... but you can do even better! You see, if you take your image data by const std::vector<BYTE>&, that means the image data must be in a std::vector<BYTE>. Maybe that's usually the case... but why restrict the options? Ultimately, to use fout.write(), you just need the image data to be in a contiguous array. A std::vector<BYTE> works... but so does a std::unique_ptr<BYTE[]>, or an std::array<BYTE, N>, or third-party container types.

So rather than taking a std::vector<BYTE>, you could just take a const BYTE* and std::size_t pair. Even better, you could use a gsl::span (which is probably going to be in C++20 as std::span).

Finally, there's no reason to take a bool argument by const-ref. Just take it by value.

Oh, also, you only seem to be using the return value as a true/false thing to denote errors. You could do that with a bool, but you should look into better ways to report errors, like std::error_code or exceptions (or the proposed std::expected).

So here's what all that put together looks like:

int bmpEncoder(const std::string &location,
    std::uint32 width, std::uint32_t height,
    const BYTE* buffer, std::size_t buffer_size,
    bool hasAlphaChannel = true) {

Or with span:

int bmpEncoder(const std::string &location,
    std::uint32 width, std::uint32_t height,
    std::span<const BYTE> buffer,
    bool hasAlphaChannel = true) {

The first thing you do in the function is open the output file, then double-check that it's open. But that's the last error-checking you do. If there's any problems writing the output after that, they just get forgotten, so this function could return 1 even though the output never got written.

You have multiple options for how to deal with this. The simplest is to just not bother to do any error checking until the end of the function - just open the file, and write everything to it, then at the end return bool{fout};. That will return 1 if everything worked and all your image data was written to the file (you should probably flush it beforehand), 0 otherwise. If there's an error early in the process, you will waste time spitting bytes into the ether, but no big deal - no harm will be done except time wasted, and the error will be detected.

Another option is to turn exceptions on in your output stream. If any error occurs, output will stop immediately. You can either catch the exception and return 0, or let the exception propagate if you prefer.

So the next bit is the biggest part of the function. What you do here is set up all your data in local variables... then copy all those local variables to a buffer... then write the buffer to the output file. That all seems a bit unnecessary.

Here's what I recommend instead. Create a set of functions that write binary values to the output stream. For example

inline std::ostream& write_binary_8bit(std::ostream& out, char c) {
    return out.put(c);
}

inline std::ostream& write_binary_8bit(std::ostream& out, char const* c, std::size_t n) {
    return out.write(c, n);
}

inline std::ostream& write_binary_8bit(std::ostream& out, BYTE const* c, std::size_t n) {
    return write_binary_8bit(out, reinterpret_cast<char const*>(c), n);
}

inline std::ostream& write_binary_16bit(std::ostream& out, std::uint16_t v) {
    return out.put(v & 0xFFu).put((v >> 8) & 0xFFu);
}

inline std::ostream& write_binary_16bit(std::ostream& out, std::int16_t v) {
    return write_binary_16bit(static_cast<std::uint16_t>(v));
}

// And so on for whatever types you need...

Then your function code just becomes:

write_binary_8bit(fout, "BM", 2);   // signature
write_binary_32bit(fout, buffer.size() + padding * (height - 1) + 14 + 124; // file size
write_binary_32bit(fout, 14 + 124); // offset

write_binary_32bit(fout, 124);      // DIBSize
write_binary_32bit(fout, width);    // width
write_binary_32bit(fout, height);   // height
write_binary_16bit(fout, 1);        // numPlanes
write_binary_16bit(fout, (hasAlphaChannel) ? 32 : 24);    // bitsPerPixel
// and so on

Now you can get rid of the header vector and the macro.

Then it's on to writing the actual image data.

for (int i = height - 1; i >= 0; i--) {
    std::vector<BYTE> row(buffer.begin() + i * bWidth, buffer.begin() + i * bWidth + bWidth);
    fout.write((char *)row.data(), row.size() * sizeof(BYTE));
    fout.seekp(padding * sizeof(BYTE), std::ios::cur);
}

What you're doing here is that for each row, you're creating a new vector, copying the row data into it, then writing the vector. Why the extra steps? All you really want to do is write the row data. So just write it directly. No need for the extra vector.

for (int i = height - 1; i >= 0; i--) {
    write_binary_8bit(buffer.begin() + i * bWidth, bWidth);
    fout.seekp(padding, std::ios::cur);
}

One thing you might want to do here is some error checking, though. You need to be sure that the image data buffer isn't truncated - that it actually does hold width×height pixels, or you might be reading (and writing to a file) random memory data. That's how security disasters like Heartbleed happen.

And finally:

fout.close();

This is unnecessary. The file will close itself automatically.

What you might want to do, though, is:

return bool{fout.flush()};

That will flush everything buffered to disk (hopefully), and check the error status of the stream.

In summary, my major recommendations are:

  • use standard types (instead of __int32)
  • avoid unnecessary buffering by writing data directly (rather than creating a buffer, copying the data to the buffer, then writing the buffer)
  • get rid of the preprocessor macro (and unnecessary buffer) by using helper funcions
  • consider better error checking
\$\endgroup\$
  • 1
    \$\begingroup\$ inline on a function definition means the compiler doesn't have to emit a stand-alone definition of the function at all, if it chooses to inline it into all the callers that appear in this compilation unit. (And to ignore/merge duplicate definitions when linking, instead of making each one private like static does). inline is a linkage specifier like static, not actually a request or guarantee that the function will be inlined. \$\endgroup\$ – Peter Cordes May 25 '18 at 2:02
  • \$\begingroup\$ Creating a buffer for the headers and making out write function call is probably much faster than making a separate function call for every field. But at least each call doesn't have to lock/unlock, because unlike the standard streams (like std::cout), ones you open aren't thread-safe by default. Anyway, creating ~100 byte buffer with no function calls and them copying that to an ofstream buffer with one write will be significantly more efficient, probably copying at least 16 bytes per clock cycle. And also smaller code-size. An array instead of std::vector would be best, though. \$\endgroup\$ – Peter Cordes May 25 '18 at 2:17
  • \$\begingroup\$ inline is not a linkage specifier; inline functions can have extern or static linkage specified (and have the same defaults as normal functions). inline does two things: 1) allows the definition to appear multiple times (it must always be the same, though); and 2) hints to the compiler that the function can be inlined (modern compilers generally ignore the hint, though, and inline things according to their own whims, which is why we have things like attribute((always_inline)) and __forceinline). \$\endgroup\$ – indi May 25 '18 at 2:28
  • \$\begingroup\$ You say I'm not sure that the inline specifier serves any purpose here. But If it's only used once, there's no reason not to inline it, unless you omit inline and thus force the compiler to emit a stand-alone definition. Making it static or static inline would be a much better choice in that case (if you aren't using whole-program optimization), because the compiler could be sure it was only used once. You're probably right that inline alone is pointless; the compiler's heuristics will probably decide not to inline, because it will assume there are callers in other files. \$\endgroup\$ – Peter Cordes May 25 '18 at 2:41
  • \$\begingroup\$ Omitting inline does not force the compiler to create a standalone definition. The compiler is free to inline if it pleases and any decent modern compiler will do so when feasible. Other than for the ability to define something multiple times (such as in a header-only library), inline is pointless. \$\endgroup\$ – indi May 25 '18 at 3:00
5
\$\begingroup\$

This is a great way to learn about image formats and write some practical code that useful in other projects! Here are some thoughts I had on it:

Naming

I think some of your naming is quite good. I like human readable names like hasAlphaChannel. I think some of the names could be improved. location should probably be more explicit. What is it the location of? The output file. Naming it filePath, or imageLocation, or something along those lines would clarify what it is. Likewise, buffer is ambiguous on first read, because maybe the function requires that the caller allocate some sort of memory buffer to pass in for processing. I think if you called it pixelBuffer or imageData, it would be more clear.

Likewise, offset is unclear. Offset of what? It appears to be the offset of the image data from the start of the file, but that's not at all obvious on first read. I would name it imageDataByteOffset. That makes it clear that it's the offset of the image data, and it makes clear that the units are bytes.

Avoid Magic Numbers

You make use of constants which is great. Unfortunately, the values assigned to those constants have no obvious meaning. What are the values 14 + 124? What is 2834? Looking at the constant name isn't that helpful. I can see that 2834 is the resolution of the image. In what units? Pixels per meter? Twips? (Consequently, according to this article the value should be 2835.) At least a comment clarifying would be nice. Or use the above piece of advice and make the names reflect the purpose and/or units - imageDataOffsetBytes and horizontalResolutionInPixelsPerCm.

Use struct and class for Structured Data

Why make header a std::vector? The .BMP header has a well-known format. It would be far more clear to use a struct here. That way you don't have to worry about whether one of the offsets you've typed is incorrect, or accidentally gets changed during another edit in the future. And each field is named, making it easier to understand.

Note that since you're writing the data directly to a file as a whole block you'll need to ensure that the data in a struct is properly aligned before writing out the entire struct, as mentioned in the comments. This isn't too hard. In most cases you can add an annotation like __attribute__((packed)) to the type definition to ensure that the data is packed properly.

Avoid Macros

Your HEADERS macro should be rewritten as a function. It should also be renamed to something like endianSwap. Macros can have nasty side effects when used like a function. If someone were to invoke the macro as HEADERS(someOffset, someValue++);, they would get very strange and hard to debug results. (Consequently, what architecture are you on where you need to swap the endianness of values?) I would see if there already exist such functions for your platform of choice and use those rather than rolling your own. (I know there are functions like ntohl(), etc. in the standard C library, for example.)

Handling Errors

Your function returns an error value which is a good idea. However, it only handles a single error - the inability to open the output stream for writing. But what happens when a failure doesn't occur until you've started writing bytes? (For example, the disk fills up, the drive is unplugged, or some other thing goes wrong.) You probably want to handle those errors as well and return them to the caller.

Use of const references

I really like that you've made the inputs to the function const since they aren't modified. That makes it much clearer to a caller what's going on. For location and buffer it makes sense to use a const reference, since they are large and you want to avoid copies. But for standard 32-bit integers and booleans, there's no reason to make them references. They should still be const in my opinion, but should be passed by value.

Type Names

While it's certainly valid to use types like unsigned __int32, it's a little odd. I recommend using the existing types with more easily readable names like uint32_t, etc. It makes the code less cluttered, in my opinion.

\$\endgroup\$
  • \$\begingroup\$ The ISO C++ standard doesn't guarantee that struct members are packed without padding. Using a struct BMPheader { ... }; would require __attribute__((packed)) for guaranteed portability, as well as native to little-endian function. (And no, ntohl is not part of ISO C or C++.) C++20 provides a std::endian type trait so you can tell what endianness you have, though. (Apparently only big/little/native, ignoring PDP-endian, aka mixed.) \$\endgroup\$ – Peter Cordes May 25 '18 at 2:21
  • \$\begingroup\$ Anyway, yes you'd normally want to use a struct and some endian-conversion functions, but you can't do that without some platform-specific #ifdef noise. You can instead write endian-agnostic code like the OP has done. \$\endgroup\$ – Peter Cordes May 25 '18 at 2:24
  • \$\begingroup\$ I'll update my answer to include information about padding. The post is not tagged for ISO C or C++, so I'm not concerned about that. Just concerned with what's likely to be available in most development environments. \$\endgroup\$ – user1118321 May 25 '18 at 3:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.