map = (byte **)malloc(width);
for (int i=0; i<height; i++)
map = (byte *)malloc(width*3);
fread(map, 1, width*height*3, fp);
fclose(fp);
int y = map[0][0];
Dynamic Multidimentional Array Problem
Author
Trying to load in a RGB raw file that I don't know the size of (though it is square and a power of two). This is what I got to declare the array and read the data.
This seems to be loading my image in (512x512 won't load, but thats another question) as I can save it to disk and it seems to be fine. The problem is with the int y, the program crashes and says memory can't be "read". I'm reading in a height map and trying to smooth it out. Am I not indexing it right? Or am I not allocating the memory for the array right? But then I wouldn't think I would be able to place a copy back to disk.
The errors are:
Why seperate mallocs for reach row ??
Why not just a single malloc width*heigh*3 (if you already known the dimmensions at run time....)
It probably isnt working because malloc may not return contiguous blocks and has some overhead data adjacent to the block it gives you.
You are reading in as if it was one contiguous block (you would want to read it row by row -- put row read inside the for loop....)
y = map[x + (y * width)]; // is how you would access data from a contiguous block
(you might optomize it by using a shift instead of a multiply if the width is always a power of 2 .....)
Why seperate mallocs for reach row ??
Why not just a single malloc width*heigh*3 (if you already known the dimmensions at run time....)
It probably isnt working because malloc may not return contiguous blocks and has some overhead data adjacent to the block it gives you.
You are reading in as if it was one contiguous block (you would want to read it row by row -- put row read inside the for loop....)
y = map[x + (y * width)]; // is how you would access data from a contiguous block
(you might optomize it by using a shift instead of a multiply if the width is always a power of 2 .....)
You can't alloc double-pointers like that and then read in a single block. The allocations all point at separate, non-contiguous locations in memory, and then you go and overwrite your array with the file data - that would be a problem even if you did get consecutive allocations, because you're not reading into those row allocations, but into the base indexing allocation - and trashing all the map pointers by replacing them with raw byte data. Huge kaboom.
In C++, you can avoid a lot of this by using more powerful tools. But since you are writing malloc() calls I assume you are determined to do this in C. I *strongly* suggest that, at minimum, you build a struct to represent your 2D array, and give it an object-like interface. Something like (WARNING not tested):
In C++, you can avoid a lot of this by using more powerful tools. But since you are writing malloc() calls I assume you are determined to do this in C. I *strongly* suggest that, at minimum, you build a struct to represent your 2D array, and give it an object-like interface. Something like (WARNING not tested):
typedef struct raw_data_t { byte* storage; int bytes_per_cell; int bytes_per_row; int size;} raw_data;raw_data* read_raw_data(int bytes_per_cell, int cells_per_row, int rows, FILE * fp) { int size = bytes_per_cell * cells_per_row * rows; // By the way, in modern C, you should NOT cast the result of malloc(). raw_data* result = malloc(sizeof(raw_data)); if (!result) { return NULL; } result->bytes_per_cell = bytes_per_cell; result->bytes_per_row = cells_per_row * bytes_per_cell; result->size = size; if (!result->storage = malloc(size)) { free(result); return NULL; } fread(result->storage, 1, size, fp); // caller will fclose() return result;}void destroy(raw_data* r) { cleanup(r); if (r) { free(r); } }void cleanup(raw_data* r) { if (r && r->storage) { free(r->storage); } }// brutally hacked and prematurely optimized translation of copy_and_swap idiom for C :)void assign(raw_data* lhs, raw_data* rhs) { raw_data* tmp; if (!lhs && !rhs) { return; } // nothing to do here ASSERT(lhs || rhs); // can't have one null and the other not tmp = clone(rhs); if (!tmp) { return NULL; } lhs->bytes_per_cell = tmp->bytes_per_cell; lhs->bytes_per_row = tmp->bytes_per_row; lhs->size = tmp->size; cleanup(lhs); lhs->storage = tmp->storage; free(tmp);}raw_data* clone(raw_data* r) { raw_data* result; if (!r) { return NULL; } result = malloc(sizeof(raw_data)); if (!result) { return NULL; } result->bytes_per_cell = r->bytes_per_cell; result->bytes_per_row = r->bytes_per_row; if (!result->storage = malloc(r->size)) { free(result); return NULL; } memcpy(result->storage, r->storage, r->size); result->size = r->size; return result;}int index(raw_data* r, int row, int cell) { int row_index, col_index, end, i, result = 0; ASSERT(r); row_index = row * r->bytes_per_row; col_index = cell * r->bytes_per_cell; end = row_index + col_index + r->bytes_per_cell; ASSERT(row_index < r->size && col_index < r->bytes_per_row); // Assuming big-endian. Grab bytes_per_cell-many bytes from this location and // construct a value. for (i = row_index + col_index; i < end; ++i) { result <<= 8; result |= r->storage; } return result;}// And your usage becomes something like:map = read_raw_data(3, width, height, fp);fclose(fp);gb = index(map, 0, 0); // first "cell" as a 0x00RRGGBB value.// ...destroy(map);
A duplicate of the OP's code written in C with some C++isms:
(what I like to call C+)
Your "map" struct acts like a 2D array, with the syntaxtic sugar of being able to use map[x][y] on it.
Using typed_malloc gives you some nice type safety to your mallocs, even if you are forced to use malloc to keep your code in sync with other bits of code.
However, if you are writing the project from scratch, I agree with Zahlman. Go further into the C++ world. Use std::vectors and the like.
(what I like to call C+)
// byte_2d// usage:// byte_2d data = byte_2d( raw_byte_pointer, bytes_in_dimension_2// byte* line = data[5];// byte b = line[10];// or:// b = line[5][10];struct byte_2d { int dim2_size; byte* raw; byte_2d(byte* raw_, int dim2_size_):raw(raw_), dim2_size(dim2_size_) {} byte* operator[](int i) {return raw[i*dim1_size];} byte const* operator[](int i) const {return raw[i*dim2_size];}};// --- T* typed_malloc<T>(int count=1) ---// usage:// int* data = typed_malloc<int>(50);// makes a pointer to 50 intstemplate<typename T>T* typed_malloc(int count=1) { return (T*)(malloc(sizeof(T)*count));}//... int data_size = size*size*3; byte* raw_bytes = typed_malloc<byte>(data_size); byte_2d map(raw_bytes, size*3); fread(map.raw, 1, data_size, fp); fclose(fp); byte y = map[0][0];Your "map" struct acts like a 2D array, with the syntaxtic sugar of being able to use map[x][y] on it.
Using typed_malloc gives you some nice type safety to your mallocs, even if you are forced to use malloc to keep your code in sync with other bits of code.
However, if you are writing the project from scratch, I agree with Zahlman. Go further into the C++ world. Use std::vectors and the like.
Author
I know I should move on to the more advanced stuff that C++ can offer. But I self taught C and find it hard to give up things like malloc(). I was pretty sure that new[] just ended up calling malloc() so didn't see the point. To my amazement I did use std::vector awhile back when I was trying to read in a file of unknown length. What I ended up doing was converting the the RGB file into an indexed one. Then it was quite easy to just have one block of memory and index through it as needed.
Quote:Original post by darkchrono4
I know I should move on to the more advanced stuff that C++ can offer. But I self taught C and find it hard to give up things like malloc(). I was pretty sure that new[] just ended up calling malloc() so didn't see the point. To my amazement I did use std::vector awhile back when I was trying to read in a file of unknown length. What I ended up doing was converting the the RGB file into an indexed one. Then it was quite easy to just have one block of memory and index through it as needed.
Here is an example of using "C++isms" in C-style code:
// --- T* typed_malloc<T>(int count=1) ---// usage:// int* data = typed_malloc<int>(50);// makes a pointer to 50 intstemplate<typename T>T* typed_malloc(int count=1) { return (T*)(malloc(sizeof(T)*count));}Your will never forget to multiply your block contents by the size of the data again. The above malloc does it for you.
It is just a potential bug that won't happen.
I understand a leeryness against moving all the way to C++. But coding in C+ (making use of some of the nice features of C++ in C code) is a good idea. :)
Think of the above as a pair of macros:
#define typed_malloc(type, count) (type*)malloc(count*sizeof(type))
#define typed_malloc(type) typed_malloc(type, 1)
except it doesn't have any of the more annoying macro bugs.
As an aside, what new() does that malloc() doesn't do is call the constructors of your data.
So if you have:
struct RGB_pixel { byte red; byte green; byte blue; // default constructor: RGB_pixel(): red(0), green(0), blue(0) {} // channel-wise constructor: RGB_pixel(byte r, byte g, byte b):red(r), green(g), blue(b) {} // copy constructor and operator= left to be generated by compiler};a "new"d version of that pixel will be automatically zeroed.
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
