Hello all! I'm trying to create a Skybox object, which when loaded loads a single image file and splits it into 5 or 6 (depending on if there's a bottom image) sf::Image objects, represented as a std::vector<sf::Image>. I then pass the image data (from image.getPixelsPtr() ) to glTexImage2D and call a number of other OpenGL functions to set up my textures for rendering.
My issue comes at the end of my function: when the vector is cleared off the stack, it results in a crash dump to stderr and then a SIGABRT crash. If I run it in my debugger, it becomes apparent it's resulting from in the sf::Image destructor. Here's my code (with some irrelevant portions removed):
bool Skybox::loadSkybox() {
glGenTextures(6, ids);
//Loads the image file, then splits into 5-6 images using sf::Image::copy, returning vector of images
//returns a list of skybox images, in order: Top, Bottom?, Left, Right, Front, Back
const std::vector<sf::Image> images = Image->getAsSkybox();
if (images.empty() || images.at(0).getSize().x == 0) //Empty list or empty images is an error with loading
return false;
for (unsigned int i = 0; i < images.size(); i++) {
glBindTexture(GL_TEXTURE_2D, ids[i]);
GLuint width = images.at(i).getSize().x, height = images.at(i).getSize().y;
unsigned char* image = const_cast<unsigned char*>(images.at(i).getPixelsPtr());
if (image == NULL || width <= 0 || height <= 0) {
std::err << "Error loading image file." << std::endl;
glDeleteTextures(6, ids);
return false;
}
glTexImage2D(GL_TEXTURE_2D, 0, GL_COMPRESSED_RGBA, width, height, 0, GL_RGBA, GL_UNSIGNED_BYTE, image);
GLenum err;
if ((err = glGetError()) != GL_NO_ERROR) {
std::err << "OpenGL error in loading image number " << i << ": " << gluErrorString(err) << std::endl;
glDeleteTextures(6, ids);
return false;
}
//Call other OpenGL functions to set it up
free(image); //Clear our temporary image data
}
return true;
}
I'm using SFML2, Code::Blocks for my IDE and GCC 4.6 on a 32-bit on an Ubuntu 12.04 LTS environment.
unsigned char* image = const_cast<unsigned char*>(images.at(i).getPixelsPtr());
...
free(image); //Clear our temporary image data
What the... never ever use free in your own C++ code. Here you kind of asked for it by doing something really dumb. The pointer returned from getPixelsPtr() is const for a reason. If you cast away the constness, and free the memory allocated by the underlying object yourself, you can't complain you get all sorts of undefined behaviour. In this case, it results in a double free when the sf::Image destructors are called.
Rule #1 of C dynamic memory management: For every malloc() there must be a free(), and vice versa.
Fix:
// free(image); //Clear our temporary image data
What the... never ever use free in your own C++ code.
What if I'm biding C to c++ and things might get allocated or freed in libs with cstdlib functions? :'( :'( :'( :'(
What the... never ever use free in your own C++ code.
/*
* File: main.cpp
* Author: superprogrammer
*
* Created on October 24, 2013, 2:08 PM
*/
#include <iostream>
#include <cstdlib>
#include <cstring>
int main()
{
char * buffer=std::malloc(100);
std::strcpy(buffer,"Hello Modern C++");
std::cout<<buffer<<std::endl;
//std::free(buffer) // - commented cus binary1248 said to never use free in C++
}
;D
glTexImage2D requires an unsigned char*
Not for me on my Linux and not in docs of GL. Both take const void * and const pointer of any kind should be implicitly convertible to that(= what is the error message and how is that function declared for you??). Even if it wasn't const, casting away const doesn't mean that you can free pointers you didn't malloc...
glTexImage2D requires an unsigned char*
No, it does not. (https://www.opengl.org/sdk/docs/man/xhtml/glTexImage2D.xml) It expects const GLvoid*:
void glTexImage2D( GLenum target,
GLint level,
GLint internalFormat,
GLsizei width,
GLsizei height,
GLint border,
GLenum format,
GLenum type,
const GLvoid * data);
binary1248's message concerning malloc/free was pretty clear: Don't use it in C++. Of course there are exceptional cases, but they are extremely rare, and completely irrelevant for this thread.