Creating OpenGL texture from SDL2 surface - strange pixel values

Question

I'm trying use SDL2 to load a texture for OpenGL rendering of Wavefront Objects (currently I'm testing with the fixed pipeline, but I eventually plan to move to shaders). The problem is that the loaded texture applied to a quad (and a model that uses a small part in the bottom right of the texture) looks like that:

_{(source: image-upload.de)}

This is the texture I used

The image loads fine and looks perfectly normal when drawn with SDL functions, so it's probably the conversion to an OGL texture that's broken. Note that I have alpha blending enabled and the texture is still completely opaque - so the values are not completely random, and probably not uninitialized memory. This is my code for converting the surface (cobbled together from various tutorials and questions on this site here):

GLuint glMakeTexture(bool mipmap = false, int request_size = 0) { // Only works on 32 Bit Surfaces
    GLuint texture = 0;
    if ((bool)_surface) {
        int w,h;
        if (request_size) { // NPOT and rectangular textures are widely supported since at least a decade now; you should never need this...
            w = h = request_size;
            if (w<_surface->w || h<_surface->h) return 0; // No can do.
        } else {
            w = _surface->w;
            h = _surface->h;
        }

        SDL_LockSurface(&*_surface);
        std::cout<<"Bits: "<<(int)_surface->format->BytesPerPixel<<std::endl;

        Uint8 *temp = (Uint8*)malloc(w*h*sizeof(Uint32)); // Yes, I know it's 4...
        if (!temp) return 0;

        // Optimized code
        /*for (int y = 0; y<h; y++) { // Pitch is given in bytes, so we need to cast to 8 bit here!
            memcpy(temp+y*w*sizeof(Uint32),(Uint8*)_surface->pixels+y*_surface->pitch,_surface->w*sizeof(Uint32));
            if (w>_surface->w) memset(temp+y*w*sizeof(Uint32)+_surface->w,0,(w-_surface->w)*sizeof(Uint32));
        }
        for (int y = _surface->h; y<h; y++) memset(temp+y*w*sizeof(Uint32),0,w*sizeof(Uint32));
        GLenum format = (_surface->format->Rmask==0xFF)?GL_RGBA:GL_BGRA;*/

        // Naive code for testing
        for (int y = 0; y<_surface->h; y++)
            for (int x = 0; x<_surface->w; x++) {
                int mempos = (x+y*w)*4;
                SDL_Color pcol = get_pixel(x,y);
                temp[mempos] = pcol.r;
                temp[mempos+1] = pcol.g;
                temp[mempos+2] = pcol.b;
                temp[mempos+3] = pcol.a;
            }
        GLenum format = GL_RGBA;

        SDL_UnlockSurface(&*_surface);

        glGenTextures(1, &texture);
        glBindTexture(GL_TEXTURE_2D, texture);
        if (mipmap) glTexParameteri(texture, GL_GENERATE_MIPMAP, GL_TRUE);
        glTexImage2D(GL_TEXTURE_2D, 0, format, w, h, 0, format, GL_UNSIGNED_BYTE, temp);
        if (mipmap) glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR_MIPMAP_LINEAR);
        else glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
        glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);

        free(temp); // Always clean up...
    }
    return texture;
}

EDIT: _surface is actually a std::shared_ptr to SDL_Surface. Thus, the &* when (un)locking it.

Btw, SDL claims the surface is formatted as 32 Bit RGBA on my machine, I checked that already.

The code that binds the texture and draws the quad is here:

glEnable(GL_TEXTURE_2D);
glTexEnvf(GL_TEXTURE_ENV, GL_TEXTURE_ENV_MODE, GL_MODULATE);
glBindTexture(GL_TEXTURE_2D,_texture[MAP_KD]);

static bool once = true;
if (once) {
    int tex;
    glGetIntegerv(GL_TEXTURE_BINDING_2D, &tex);
    bool valid = glIsTexture(tex);
    std::cout<<tex<<" "<<valid<<std::endl;
    once = false;
}
glBegin(GL_TRIANGLE_STRIP);
    //glColor3f(1.f,1.f,1.f);
    glNormal3f(0,1,0);
    glTexCoord2f(0.f,0.f); glVertex3f(0,0,0);
    glTexCoord2f(0.f,1.f); glVertex3f(0,0,1);
    glTexCoord2f(1.f,0.f); glVertex3f(1,0,0);
    glTexCoord2f(1.f,1.f); glVertex3f(1,0,1);
glEnd();

The axe is drawn later from an index list; the code is way too long to share here (and besides, it works fine apart from the texture).

I also tried the naive method found in many tutorials of passing _surface->pixels to glTexImage2D(), but that doesn't help either (and I heard it's the wrong way to do it anyway, because pitch!=width*BytesPerPixel in general). The outcommented "optimized" code looks exactly the same, btw (as expected). I wrote the lower part for better testing. Setting all pixels to a specific color or making the texture partially transparent works as expected, so I assume OpenGL loads the values in temp correctly. It's likely my understanding of the memory layout in SDL2 Surfaces that's messed up.

FINAL EDIT: Solution (resetting GL_UNPACK_ROW_LENGTH is key, thanks to Peter Clark):

GLuint glTexture(bool mipmap = false) {
    GLuint texture = 0;
    if ((bool)_surface) {
        GLenum texture_format, internal_format, tex_type;
        if (_surface->format->BytesPerPixel == 4) {
            if (_surface->format->Rmask == 0x000000ff) {
                texture_format = GL_RGBA;
                tex_type = GL_UNSIGNED_INT_8_8_8_8_REV;
            } else {
                texture_format = GL_BGRA;
                tex_type = GL_UNSIGNED_INT_8_8_8_8;
            }
            internal_format = GL_RGBA8;
        } else {
            if (_surface->format->Rmask == 0x000000ff) {
                texture_format = GL_RGB;
                tex_type = GL_UNSIGNED_BYTE;
            } else {
                texture_format = GL_BGR;
                tex_type = GL_UNSIGNED_BYTE;
            }
            internal_format = GL_RGB8;
        }

        int alignment = 8;
        while (_surface->pitch%alignment) alignment>>=1; // x%1==0 for any x
        glPixelStorei(GL_UNPACK_ALIGNMENT,alignment);

        int expected_pitch = (_surface->w*_surface->format->BytesPerPixel+alignment-1)/alignment*alignment;
        if (_surface->pitch-expected_pitch>=alignment) // Alignment alone wont't solve it now
            glPixelStorei(GL_UNPACK_ROW_LENGTH,_surface->pitch/_surface->format->BytesPerPixel);
        else glPixelStorei(GL_UNPACK_ROW_LENGTH,0);

        glGenTextures(1, &texture);
        glBindTexture(GL_TEXTURE_2D, texture);

        glTexImage2D(GL_TEXTURE_2D, 0, internal_format, _surface->w, _surface->h, 0, texture_format, tex_type, _surface->pixels);
        if (mipmap) {
            glGenerateMipmap(GL_TEXTURE_2D);
            glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR_MIPMAP_LINEAR);
        } else {
            glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_BASE_LEVEL, 0);
            glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAX_LEVEL, 0);
            glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
        }
        glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);

        glPixelStorei(GL_UNPACK_ALIGNMENT,4);
        glPixelStorei(GL_UNPACK_ROW_LENGTH,0);
    }
    return texture;
}

Answer 1

Pixel Storage Alignment

You need to tell OpenGL what the alignment of the image is with glPixelStorei(GL_UNPACK_ALIGNMENT, [1,2,4,8]). This will be the largest power of 2 that is a divisor of pitch up to 8. If it is not one of the accepted values, you may have to additionally set GL_UNPACK_ROW_LENGTH - see this answer for more details and advice on that topic. One thing to note - GL_UNPACK_ROW_LENGTH is the row length in pixels, where as SDL_Surface::pitch is the row length in bytes. On top of that you have to ensure that internal_format, format, and pixel_type are set to match what the SDL_Surface contains. One more resource on this topic.

"Complete" Textures

You're also not creating a complete texture when not using mipmaps. To create a "complete" texture (one that is ready to be read from or written to) with no mipmaps you must specify that the maximum mipmap level is 0 (the base image) by using glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAX_LEVEL, 0), since the default is 1000.

Just to note: you're using glTexParameteri(texture, GL_GENERATE_MIPMAP, GL_TRUE) to automatically generate mipmaps. While this should work (though I am not familiar with it), be aware that this method is deprecated in favor of glGenerateMipmaps in modern OpenGL.

A Possible Solution

// Load texture into surface...
// Error check..
// Bind GL texture...

// Calculate required align using pitch (largest power of 2 that is a divisor of pitch)

glPixelStorei(GL_UNPACK_ALIGNMENT, align);
//glPixelStorei(GL_UNPACK_ROW_LENGTH, row_length); // row_length = pitch / bytes_per_pixel

glTexImage2D(
  GL_TEXTURE_2D, 
  0, 
  internal_format,
  sdl_surface->w,
  sdl_surface->h,
  0, 
  format,
  pixel_type, 
  sdl_surface->pixels);
// Check for errors

if(use_mipmaps)
{
  glGenerateMipmap(GL_TEXTURE_2D);
  // Check for errors

  glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, /* filter mode */);
  glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, /* filter mode */);
  // Check for errors
}
else
{
  // This makes the texture complete 
  glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_BASE_LEVEL, 0);
  glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAX_LEVEL, 0);

  glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, /* filter mode */);
  glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, /* filter mode */);
  // Check for errors
}

// Potentially reset GL_UNPACK_ALIGNMENT and/or GL_UNPACK_ROW_LENGTH to their default values
// Cleanup

Error Checking

Note that it wouldn't be a bad idea to add some error checking with glGetError() where I've marked Check for errors. You could perhaps print the error if there is one and then place a breakpoint/assert. I generally use a macro for this so I can disable most error checking in a release build - something to the effect of:

#ifdef MYPROJ_GRAPHICS_DEBUG
#define ASSERT_IF_GL_ERROR \
{ \
  GLenum last_error = glGetError(); \
  if(last_error != GL_NO_ERROR); \
  { \
    printf("GL Error: %d", last_error); \
  } \
  __debugbreak(); // Visual Studio intrinsic - other compilers have similar intrinsics
} 
#else
#define ASSERT_IF_GL_ERROR
#endif

It is always a good idea to do error checking, and it may reveal some information about what it going on. Though, since it sounds like the driver is crashing after some undefined behavior, it's possible it won't in this instance.

A Possible Alternative

I think it worth mentioning that I was not aware of this issue before answering this question. I hadn't run into it because I generally use stb_image to load textures. The reason I bring it up is that in the documentation for stb_image it is stated that "There is no padding between image scanlines or between pixels, regardless of format.", meaning stb_image handles it for you. If you can control the images you have to load (say if you're making a game and you control creation of the assets) stb_image is another image loading option.