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
