PDA

View Full Version : [Bug ?] Problem with pixel buffer returned by CL_Surface::get_pixeldata



LiFo2
05-09-2007, 12:02 PM
Hello,

I have a problem with a very simple piece of code. I tried to debug it, but didn't manage to find the problem. The more I search, the more I think that maybe it's not my code that is wrong :rolleyes:
I simplified my function as much as I could, and finally, I tried this :



CL_Surface *test(CL_Surface *surface)
{
CL_PixelBuffer pixbuf = surface->get_pixeldata();

std::cout << "Surface : Width : " << surface->get_width() << ", Height : " << surface->get_height() << std::endl;
std::cout << "PixelBuffer : Width : " << pixbuf.get_width() << ", Height : " << pixbuf.get_height() << std::endl;

// Here is the processing code, that I commented

return new CL_Surface(pixbuf);
}


Sometimes, the surface that is returned is good (same picture as the surface I gave to the function), but sometimes it's not. The size is greater, and there are some additional stuff on the right and on the bottom of the picture. It seems to be other surfaces of my application, but displayed badly.

I'm using ClanLib 0.8.0 with ArchLinux. I tested it on two different computers with different graphic cards, one with DRI and one using Mesa.

If you try to reproduce this bug, but do not manage to do so, here is the picture I have problem with :
http://the3fold.free.fr/surface.tga

If you still can't reproduce the bug with this picture, it would be nice to try using code of trophy from CVS :
http://sourceforge.net/cvs/?group_id=4827

Regards,
Colin

LiFo2
05-09-2007, 02:19 PM
OK, I wrote a little test app. Here it is :



#include <iostream>
#include <ClanLib/core.h>
#include <ClanLib/display.h>
#include <ClanLib/gl.h>
#include <ClanLib/application.h>

class myTestApplication : public CL_ClanApplication
{
virtual int main(int argc, char **argv);
CL_Surface *test_function(CL_Surface *image);
} myApp;

CL_Surface *myTestApplication::test_function(CL_Surface *surface)
{
CL_PixelBuffer pixbuf = surface->get_pixeldata();
std::cout << "Surface : Width : " << surface->get_width() << ", Height : " << surface->get_height() << std::endl;
std::cout << "PixelBuffer : Width : " << pixbuf.get_width() << ", Height : " << pixbuf.get_height() << std::endl;
return new CL_Surface(pixbuf);
}

int myTestApplication::main(int argc, char **argv)
{
const unsigned int border_size = 5;
const unsigned int screen_size_x = 640;
const unsigned int screen_size_y = 480;
const CL_Color black = CL_Color(0, 0, 0, 255);
const CL_Color white = CL_Color(255, 255, 255, 255);
CL_SetupCore::init();
CL_SetupDisplay::init();
CL_SetupGL::init();
CL_DisplayWindow window("My ClanLib Window", screen_size_x, screen_size_y);
CL_InputContext *ic = window.get_ic();
CL_InputDevice keyboard = ic->get_keyboard();

CL_PixelBuffer image = CL_TargaProvider("image.tga");
CL_Surface surface = CL_Surface (image);

CL_Surface *result = test_function(&surface);
std::cout << "Result : Width : " << result->get_width() << ", Height : " << result->get_height() << std::endl;

CL_Display::clear(white);
surface.draw(border_size, border_size);
result->draw(surface.get_width() + 2*border_size, border_size);
CL_Display::flip();
bool space_down = false;
while(!space_down)
{
space_down = keyboard.get_keycode(CL_KEY_SPACE);
}

if(result) delete result;

CL_SetupGL::deinit();
CL_SetupDisplay::deinit();
CL_SetupCore::deinit();
return 0;
}


It gives me the following result on the second computer (using Mesa) :


bash-3.2$ ./test
No hardware acceleration available. I hope you got a really fast machine..
Surface : Width : 95, Height : 93
PixelBuffer : Width : 128, Height : 128
Result : Width : 128, Height : 128


I will test it as soon as possible on my other computer. I'm sure it will give different result for Surface and for the other twos, but I wonder if the numbers will be different from this first computer.

However, there is nothing in the added parts of the picture (so it tends to confirm my intuition that it was some parts of other surfaces of my app), so the result display well.

I joined the image.tga I used, so that anybody could test it. As it is not possible to upload tga files, I renamed it to file.txt.

Regards,
Colin

sphair
05-09-2007, 09:09 PM
Surfaces on the OpenGL target are always a power of two. So the width and height are one of 1,2,4,8,16,32,64,128,256,512,1024,2048,4096 when the pixelbuffer is loading into a Surface.

LiFo2
05-09-2007, 09:31 PM
OK, so that explains the result ! :confused:

Then, how should I do ? Is there another way to access surface data ? Or is there a way to restore the size of the surface after I modified it ?
Or must I just store this information, and use it each time I draw the surface ?

I thought I could use setup_parameters on the surface to set the src rect, but calling draw override this setting.

I really need the resulting surface to be usable as any other surface, and I can't force it to have a width and a height that are power of 2. Even a transparent area on the right and on the bottom is bad, because I use the size of the surface to place it correctly.

Seth
05-09-2007, 10:45 PM
Hi Colin,

Surface's store the width/height and the actual texture width height separately internally, but a pixel buffer is very simple and is only using the actual texture data, not what the surface pretends it is.

So data is lost in the process.

When you do

surface->get_pixeldata()
what happens behind the scenes is the entire texture is (probably) grabbed off the graphics card however the card decided to store it - which is probably padded to a power of 2.

There is a way around this - create your texture like this:


CL_Surface surface = CL_Surface (image, CL_Surface::flag_keep_pixelbuffer);


This causes the original image data to sit in CPU memory, as well as the optimized copy that is probably sitting on the video card, so getting the pixel buffer is lightning fast and WILL have the dimensions you expect.

An alternate method would be to resize the pixel buffer after it grabs it from the surface to make sure it's right:


CL_Surface *myTestApplication::test_function(CL_Surface *surface)
{
//grab the pixel data from the surface, this might be coming directly from the video card (a flag can be set to change this behavior..)
//so we don't want to keep doing this
CL_PixelBuffer pixBufOriginal(surface->get_pixeldata());

//compute the size per pixel so we can make a new buffer that matches its type
int bytesPerPixel = pixBufOriginal.get_format().get_depth()/8;

//build a new pixebuffer the size of what the original image really was
CL_PixelBuffer pixbuf(surface->get_width(), surface->get_height(), surface->get_width()*bytesPerPixel, pixBufOriginal.get_format());

//copy the old one over the new one, ignoring the padded crap, by default it will just crop the pic which is what we want
pixBufOriginal.convert(pixbuf);

std::cout << "Surface : Width : " << surface->get_width() << ", Height : " << surface->get_height() << std::endl;
std::cout << "Actual texture size: " << pixBufOriginal.get_width() << ", Height : " << pixBufOriginal.get_height() << std::endl;
std::cout << "New PixelBuffer : Width : " << pixbuf.get_width() << ", Height : " << pixbuf.get_height() << std::endl;
CL_Surface *pNewSurf = new CL_Surface(pixbuf);
return pNewSurf;
}

Btw, you probably know this, but just in case you don't, you can use

CL_PixelBuffer image = CL_ProviderFactory::load("image.tga");
to load your image too. (will detect the correct filetype)

LiFo2
05-10-2007, 06:32 AM
I answer to myself :rolleyes:

Sometimes, problems seems very hard whereas the solution is right behind your nose. Here, as I first coded my function by getting the pixel buffer of the surface, modifying it and creating a new surface with it, I was searching in this direction. The solution for me is clearly this :



CL_Surface *myTestApplication::test_function(CL_Surface *surface)
{
CL_Surface *result = new CL_Surface(*surface);
CL_PixelBuffer pixbuf = result->get_pixeldata();

// work on pixbuf ...

return result;
}


Thank you very much Seth for your solution :) And no, I didn't know of CL_ProvideFactory::load(). Is it new with ClanLib-0.8.0 ?
(I'm new to ClanLib, but I'm porting an app from ClanLib-0.6, and this app was using CL_TargaProvider)

Edit : OK, I was sure it would work, and with my test app, it did what I wanted it to do. But when I modify the pixel buffer, the surface (here result) isn't modified. As if get_pixeldata was returning a copy of the pixel buffer of the surface, and not the buffer itself !

Here is what I do :




CL_Surface *myTestApplication::test_function(CL_Surface *surface)
{
CL_Surface *result = new CL_Surface(*surface);
CL_PixelBuffer pixbuf = result->get_pixeldata();

pixbuf.lock();

unsigned char *data = (unsigned char*)pixbuf.get_data();

bool rgb_order = true;
int bpp = 0; // bytes per pixel
if(pf == CL_PixelFormat::rgba8888)
bpp = 4;
if(pf == CL_PixelFormat::abgr8888)
{
bpp = 4;
rgb_order = false;
}
// ...

int size = pixbuf.get_width() * pixbuf.get_height() * bpp;
for(int i = 0; i < size; i += bpp)
{
data[i] = ...
data[i+1] = ...
data[i+2] = ...
data[i+3] = ...
}

pixbuf.unlock();

return result;
}


Regards,
Colin

sphair
05-10-2007, 08:54 AM
Could you write a little about what you are trying to accomplish? Maybe there are other ways to do it than working on surface data.

Do you have to do your modifications on the surface? Could you for instance just load the pixelbuffer, do the modifications before creating a surface, and then creating the surface?

Or maybe the CL_Canvas class is what you need - it lets you do the normal graphic operations on a canvas (drawing surfaces, lines, etc), and then create a surface from the result.

LiFo2
05-10-2007, 09:23 AM
Hello,

first, thanks for helping me and for your patience :)

What I want to do is change the hue, saturation and value of the picture. This is a generic function, but I mainly use the hue modification. I load the tga file that represents a car in red color in a surface, and then call my function with this surface, and the hue offset I want to apply (I set saturation and value offset to 0).

The function lock the buffer, get a pointer to the data, and get each pixel color component (r, g, b). It converts it to (h, s, v), apply the offset, and converts it back to (r, g, b). Then, it modify the data and unlock the buffer.

Here is the full code of the function (I don't treat all pixel formats for now) :


/** Returns a pointer to a new image, based on 'surface'
but with changed hue. Changing the hue means to "rotate"
the color spectrum. You can read more about the HSV color
model on the Internet.
This method is used to change the color of a sprite
(e.g. a car or another object). I suggest to make the
basic images in a red color-spectrum and create all
other colors from it.
For examples, please visit the Trophy homepage developer
corner (http://trophy.sourceforge.net)
\param hue Changing of hue: 0-360
\param saturation Changing of saturation: -100...100
\param value Changing of value (Color intensity): -100...100
*/
CL_Surface*
CAImageManipulation::changeHSV( CL_Surface* surface,
int hue, int saturation, int value )
{
CL_Surface *result = new CL_Surface(*surface);
CL_PixelBuffer pixbuf = result->get_pixeldata();
CL_PixelFormat pf = pixbuf.get_format();

bool rgb_order = true;
int bpp = 0;
if(pf == CL_PixelFormat::rgba8888)
bpp = 4;
if(pf == CL_PixelFormat::abgr8888)
{
bpp = 4;
rgb_order = false;
}
if(bpp == 0)
{
std::cout << "Unknown pixel format" << std::endl;
return new CL_Surface(*surface);
}

// Calc size in bytes:
int size = pixbuf.get_width() * pixbuf.get_height() * bpp;

pixbuf.lock();

unsigned char *data = (unsigned char*)pixbuf.get_data();

int r, g, b, a;
int h, s, v;

for(int i=0; i<size; i+=bpp )
{
if(rgb_order)
{
r = data[i+3];
g = data[i+2];
b = data[i+1];
//a = data[i];
}
else
{
//a = data[i+3];
b = data[i+2];
g = data[i+1];
r = data[i];
}

if( a!=0 )
{
rgbToHsv( r, g, b, &h, &s, &v );

h += hue;
s += saturation;
v += value;
if( h > 360 ) h -= 360;
if( s > 255 ) s = 255;
if( v > 255 ) v = 255;
if( h < 0 ) h += 360;
if( s < 0 ) s = 0;
if( v < 0 ) v = 0;

hsvToRgb( h, s, v, &r, &g, &b );

if(rgb_order)
{
data[i+3] = (unsigned char)r;
data[i+2] = (unsigned char)g;
data[i+1] = (unsigned char)b;
//data[i] = (unsigned char)a;
}
else
{
//data[i+3] = (unsigned char)a;
data[i+2] = (unsigned char)b;
data[i+1] = (unsigned char)g;
data[i] = (unsigned char)r;
}
}
}
pixbuf.unlock();

return result;
}


Edit :
The alternate method given by Seth works well. I just find it weird that I have to use two pixel buffer to do this. I thought I could just copy my surface, and then modify it.

Regards,
Colin