Quote:Original post by caldiar
turns out the alpha channel was skewing the byte order creating a pattern of red/green/blue instead of just red.
In the future, feel free to post (small) pictures to illustrate this kind of problem.
Quote:
Wow that's some elegant (ToohrVyk's) code.
Welcome to C++. :)
Quote:
void *safe_malloc( size_t size ){ void *p; p = malloc(size); if(!p) printf("SAFE_MALLOC HAS AN ERROR!"); return p;}
OK. Many problems here.
1) This is C++. Use new. It's idiomatic.
2) This is C++. Use new. It's type-safe. malloc() is not.
3) This is C++. Use new. It doesn't require you to think about the size of the type of thing you want to allocate. malloc() does not.
4) This is C++. Use new and new[]. This keeps scalar and array allocations distinct (which may allow the compiler to make some optimizations in the underlying allocation system, and is useful for documentation purposes). malloc() cannot make such a distinction.
5) You already use new[] elsewhere in the code, so be consistent.
6) Actually, forget all of that, and use std::vector instead. Don't mess around with calling delete yourself. Using a standard library container helps immensely with exception safety.
7) Printing an error message locally doesn't make anything "safe". By writing the code this way, you (a) make the writer of the calling code (you) think that no check for a null return value is necessary (when it is); (b) deny the writer of the calling code (you) the option of *not* printing the message, or of formatting it differently, etc.
Only attempt to handle errors at the point where they can be handled. This is largely the point of exceptions in C++: to provide an easy way to get back to the point in the code where the problem can/should be handled.
Often, failed allocations shouldn't be handled at all, but should just let the program crash - in a controlled manner! - without letting other stuff execute. With new (and implicitly with standard library containers), this is easy to arrange: if there is a failure, a std::bad_alloc exception will be thrown, and all you do is - nothing. When no exception handler is found, std::terminate will be called, and you're done. new and new[] will never (in the normal form) return a NULL pointer (although you can use a special form that will, if you really need it).
As a result, we get:
// Oh, and don't use comments to disable code. Get a proper version control// system; then you can be aggressive about deleting bits, since you can// always get them back from the repository. Alternatively, use something like// #if 0// ...// #endif// for temporary disabling code.void FlipBytes(){ unsigned int printme; // Please use C++ style casts in C++. unsigned char *ptr = reinterpret_cast<unsigned char*>(&printme); int i = 0; // Please, do anything you can to make sure your editor is not allowed // to mix tabs and spaces freely for indentation. int left; // this was previously indented 4 spaces instead of a tab. int right; // In C++, we use std::fill to zero-initialize ranges (arrays, // containers or portions thereof); but std::vector guarantees that the // initial elements - if you request any - will be default-constructed // (which makes them zero for primitive types). std::vector<char> header(18); header[2] = 2; header[12] = 512 & 255; header[13] = 512 >> 8; header[14] = 512 & 255; header[15] = 512 >> 8; header[16] = 24; o_file.write(&header.front(), 18); while (pos < length) { //00 00 00 00 if (buffer[pos] <= '9'){ left = buffer[pos] - '0'; } else { if(buffer[pos] == 'A'){left = toupper(buffer[pos]) - 'A' + 0xA; } if(buffer[pos] == 'B'){left = toupper(buffer[pos]) - 'B' + 0xB; } if(buffer[pos] == 'C'){left = toupper(buffer[pos]) - 'C' + 0xC; } if(buffer[pos] == 'D'){left = toupper(buffer[pos]) - 'D' + 0xD; } if(buffer[pos] == 'E'){left = toupper(buffer[pos]) - 'E' + 0xE; } if(buffer[pos] == 'F'){left = toupper(buffer[pos]) - 'F' + 0xF; } } if(buffer[pos + 1] <= '9'){ right = buffer[pos + 1] - '0'; }else{ if(buffer[pos + 1] == 'A'){right = toupper(buffer[pos + 1]) - 'A' + 0xA; } if(buffer[pos + 1] == 'B'){right = toupper(buffer[pos + 1]) - 'B' + 0xB; } if(buffer[pos + 1] == 'C'){right = toupper(buffer[pos + 1]) - 'C' + 0xC; } if(buffer[pos + 1] == 'D'){right = toupper(buffer[pos + 1]) - 'D' + 0xD; } if(buffer[pos + 1] == 'E'){right = toupper(buffer[pos + 1]) - 'E' + 0xE; } if(buffer[pos + 1] == 'F'){right = toupper(buffer[pos + 1]) - 'F' + 0xF; } } char newbyte = (char)(left * 16 + right); o_file << newbyte; ptr[i++] = newbyte; // similarly here with spaces vs. tabs. // Also notice that array indexing and pointer arithmetic // are equivalent. if(i >= sizeof(unsigned int)){ i = 0; } pos += 3; }}
Of course, if we notice that
(a) Using globals for something like this is really horrible (this is exactly what function parameters are for: to pass information into the function);
(b) There's no point in constructing the "printme" value any more;
(c) Each hex "nibble" can be converted in the same way, and making a function to convert each one would probably be cleaner;
(e) We could probably use a similar approach to write numbers into the header;
(d) 'A' through 'F' are consecutive in ASCII;
(e) Variables should be declared near first use and scoped as tightly as possible;
we get something more like:
template <int size>void writeBigEndian(char* where, int what) { // We make a local copy of 'what', which is modified... for (char* c = where + size; c > where; --c) { // 'c' points one past where we want to write. c[-1] = what & 255; what >>= 8; }}int as_hex(char c) { if (c >= '0' && c <= '9') { return c - '0'; } if (c >= 'A' && c <= 'F') { return c - 'A' + 0xA; } if (c >= 'a' && c <= 'a') { return c - 'a' + 0xA; } throw std::domain_error();}void FlipBytes(std::ofstream& o_file, std::vector<char> buffer, std::vector<char>::iterator pos){ std::vector<char> header(18); header[2] = 2; writeBigEndian<2>(header + 12, 512); writeBigEndian<2>(header + 14, 512); header[16] = 24; o_file.write(&header.front(), 18); while (pos != buffer.end()) { int left = as_hex(*pos); int right = as_hex(*(pos + 1)); // Don't need a temporary for this. o_file << char(left * 16 + right); pos += 3; }}
You know, assuming you still want to do all the work, when it can be done like ToohrVyk showed... :)
