Hi,
I have got a problem and I'm looking for a nice solution (both in terms of code size and source code readibility)

I have got a class for images and images can be of several types (12bpp,16bpp,32bpp,1bpp,8bpp etc)

Now I have dozens of functions that work on those images (like get/putpixel for example)

I would like to use something like image->getpixel(x,y,&color)
or getpixel(x,y,%color,image);
in my code.

The question is, how to implement it nicely?

I could have

void getpixel(...)
{
if image->type=1 getpixel1(..)
if image->type=2 getpixel2(..)
}

etc.
But I think this one pretty much sucks (lots of ifs which take lots of space etc.

Is there a better solution?

Would creating a class of image functions for each image type and then just use pointer to class depending on image type work? Could someone write me how to implement it propertly?

Or maybe something else?

Please share your ideas, thanks :-)

You could create an array of 12 function pointers

getPixel[image->type]( x,y,bla ) ;


pat.

Well I thought someone else would post this already, but what you are talking about is exactly what virtual functions are for.

You would have a base class, call it ImageImpl, with virtual functions, e.g.

class ImageImpl
{
virtual ~ImageImpl() {}
virtual void SetPixel(int x, int y) = 0;
...
}

Then you subclass this for each image type. Hey presto :)

Yes, this problem begs to be solved using Object Oriented Programming.

Originally posted by MirekCz
Hi, I have got a problem and I'm looking for a nice solution (both in terms of code size and source code readibility)
I worked on a lib for this, and to me, all solutions have something wrong with them. First I forced my lib users to use the best-performance solution, calling inlined videomode-specific getpixel and putpixel functions. Some guy complained that he wanted one getpixel/putpixel that would work for all screen modes, so I also made those, and wrote warning comments about how they shouldn't be called inside of loops or places where you are going for speed.

The function pointer solution brought up by patrox is a compromise between readability and performance. You've still got the overhead of pushing parameters onto the stack before the function is called, but it isn't as bad as branching logic inside each call to getpixel/putpixel.

Virtual functions in classes have the same problem. You could have, for example, videomode-specific classes that all derive from a CSurface class with a virtual getpixel/putpixel methods. If you call the virtual methods, the compiler will generate code to push params onto the stack and jump to the function code. This slows down performance in the same way that function pointers do.

-Erik

The fools tried this approach in Java - try and abstract away the details. It might seem elegant but it sure isn't fast until you start getting really esoteric with your usage. In the end they just traded one load of simplistic, well-understood complexity for a completely different, complex object-orientated complexity. We all gave up and most people doing serious stuff in Java use OpenGL instead.

Cas :)

Personally I use a set of derived colour classes
CColour -> CColour15, CColour16, CColour24... etc...

These handle various convertion functions and images are converted into the best format for the current mode at load time.

That way when it comes to the blit, the images are in the correct format at blit time. Chances are you'll be blitting more often than loading so the delay is negligable, especially on todays hardware.

not ideal, but it's a way of doing it!

just my 2p ;)

Sorry, I think I haven't written exactly what I have in mind.

I have got a graphic program which performs many operations on images (like filters - average,gaussian,smooth/sharpen,edge detection), effects (noise,age) etc etc.
The put/getpixel was just a (rather bad) example.

Now I have got a thread in which I do all the image-manipulations.

currently it looks very bad.. like:

if (effect==y)
if (image->type==x)
{.... code goes here....}

etc.

I want to put all effects into functions , for two reasons:
1.thread code and single functions will be easier to find/navigate
2.I can create complex functions by calling two-more "basic" functions

The easiest implementation of function x would be

procedure x(type,data...)
{
if (type==image1bpp) x1bpp(data...)
if (type==image32bpp) x32bpp(data...)
}

which looks stupid, is a lot of work and possibly will eat hard into size of code (I want code to be small, I'm not concerned much about performace as it will be called <100 per second so the procedure call overhead isn't important)

Anyone has got a nice solution to it?

for me the virtual classes idea looks nice, I just need to learn a lot of C++ :-)

Ahh.... suddenly everything becomes clear(~er)!

Well I'd still recomend derived classes, but then that's just me ;)

but if you want to use C (or non-OO) then you could use function pointers and attach the correct pointer to your image class at load time when you know the image format.
Like what pat said. :D

I vote for self-modifying code ;)

Originally posted by MirekCz
Anyone has got a nice solution to it?

for me the virtual classes idea looks nice, I just need to learn a lot of C++ :-)
I think the C++ aspect of this is not so difficult even if you are new to it, so you might consider something like...

class CColor
{
public:
CColor(void) : ucRed(0), ucGreen(0), ucBlue(0), ucAlpha(255) { }
CColor(unsigned char ucSetRed, unsigned char ucSetGreen, unsigned char ucSetBlue, unsigned char ucSetAlpha = 255) ucRed(ucSetRed), ucGreen(ucSetGreen), ucBlue(ucSetBlue), ucAlpha(ucSetAlpha) { }
CColor(const CColor &From) : ucRed(From.ucRed), ucGreen(From.ucGreen), ucBlue(From.ucBlue), ucAlpha(From.ucAlpha) { }
unsigned char ucRed, ucGreen, ucBlue, ucAlpha;
};

class CSurfaceBase
{
protected:
unsigned char *pPixelData; //Pixel data formatted in some video mode.

public:
CSurfaceBase(unsigned char *pSetPixelData) : pPixelData(pSetPixelData) { }
virtual void SetPixel(unsigned int nX, unsigned int nY, CColor Color) = 0; //Must be implemented in derived class.
virtual CColor GetPixel(unsigned int nX, unsigned int nY) const = 0; //Must be implemented in derived class.
};

//16-bit surface where pixels are stored with first 5 bits red, next 6 bits green, next 5 bits blue.
class CSurface16_r5g6b5
{
public:
CSurface16_r5g6b5(unsigned char *pSetPixelData) : CSurfaceBase(pSetPixelData) { }
virtual void SetPixel(unsigned int nX, unsigned int nY, CColor Color);
virtual CColor GetPixel(unsigned int nX, unsigned int nY) const;
}

//24-bit surface with bytes in red/green/blue order.
class CSurface24_rgb
{
public:
CSurface24(unsigned char *pSetPixelData) : CSurfaceBase(pSetPixelData) { }
virtual void SetPixel(unsigned int nX, unsigned int nY, CColor Color);
virtual CColor GetPixel(unsigned int nX, unsigned int nY) const;
}

...And you'd have to make a surface class for each different bit depth and byte-ordering you planned to support. But once you'd done that, then any function that operates on a surface, you just pass a CSurfaceBase pointer, i.e.:

void DrawLittleRedDot(CSurfaceBase *pSurface)
{
CColor Red(255,0,0);
pSurface->SetPixel(100,100,Red);
}

You say that GetPixel/PutPixel is a bad example, but to me it sounds like the first thing to do. The other abstraction you were talking about would be the type of operation. You can do something similar with virtual methods, i.e.:

class CSurfaceOperationBase
{
protected:
CSurfaceBase *pSurface;

public:
CSurfaceOperationBase(CSurfaceBase *pSetSurface) : pSurface(pSetSurface) { }
virtual void PerformOperation(void) = 0; //Must be implemented in derived class. You would probably want to add more params here that are common to every operation, such as a rectangle of the surface upon which to operate.
}

class CGaussianBlurOperation : public CSurfaceOperationBase
{
private:
unsigned int wAmount;

public:
CGaussianBlurOperation(CSurfaceBase *pSetSurface, uint8 wSetAmount) : CSurfaceOperationBase(pSetSurface), wAmount(wSetAmount) { }
virtual void PerformOperation(void); //The code for performing the guassian blur goes in this method. Call GetPixel()/SetPixel() methods of the surface to avoid dealing with surface format.
}

Oh geez, another case of setting out to write a little example code and.... gads, this is lengthy. I should get back to real work. :)

-Erik

Originally posted by MirekCz
for me the virtual classes idea looks nice, I just need to learn a lot of C++ :-)

It's not so much. Here's some code to get you started:

In the h file you'll have something like:


// Image superclass
class CImage
{
protected:

// All data that you want to prevent direct access to from
// outside the object goes here. protected means data will
// be accessible from derived classes. It could be private if
// you didn't want to share with derived classes, but it
// doesn't make much sense most of the time.

public:

// These are pure virtual functions (identified with '=0'),
// this means they must be redefined in the
// derived classes.
virtual void GausianBlur() = 0;
virtual void Sharpen() = 0;
};

// 1 bpp image
// Inherits from CImage, which means it has all the data an
// functions from the superclass and can redefine all virtual
// functions.
class CImage1 : public CImage
{
public:

// Redefinition of the virtual functions declared in the
// superclass. The code for these functions goes (usually)
// in the cpp file.
virtual void GaussianBlur();
virtual void Sharpen();
}

// 8 bpp image
// Also inherits from CImage and redefines the effect functions.
class CImage8 : public CImage
{
public:

virtual void GaussianBlur();
virtual void Sharpen();
}

// ... And so on


In the cpp file, you'll have something like:


void CImage1::GaussianBlur()
{
// Code for 1 bpp gaussian blur
}

void CImage1::Sharpen()
{
// Code for 1 bpp sharpen
}

void CImage8::GaussianBlur()
{
// Code for 8 bpp gaussian blur
}

void CImage8::Sharpen()
{
// Code for 8 bpp sharpen
}

// ... And so on


Then, in your program code, you would do the following:


#include "Image.h"

void main()
{
CImage8 Image;

Image.GaussianBlur();
Image.Sharpen();
}


I hope this helps you get started with C++ and classes. It's really not so hard and most of the code still is much alike C.

The question is: how much do you want to abstract away, and as a result of that: how much code are you prepared to duplicate? The code for a 16-bit gaussian blur is mostly the same as a 32-bit blur -- only the color resolution is different. In this case it makes sense only to abstract the get/putpixel operations -- and not the entire blur function -- and for that you don't need all those subclasses.

Originally posted by Matthijs Hollemans
The question is: how much do you want to abstract away, and as a result of that: how much code are you prepared to duplicate?
Exactly. MirekCZ is asking for smaller code size, so I think he would benefit by more abstraction, especially for the lowest-level pixel manipulation routines.
The code for a 16-bit gaussian blur is mostly the same as a 32-bit blur -- only the color resolution is different. In this case it makes sense only to abstract the get/putpixel operations -- and not the entire blur function -- and for that you don't need all those subclasses.
I think the biggest advantage for smaller code size and readability is in abstracting getpixel/putpixel, but depending on what MirekCZ is trying to do, it could be quite helpful to also abstract operations. For example, if every operation needed the functionality of masking a rectangle or if every operation needed to be hooked into a progress bar to show percent complete. So to that end, my example code in the previous post abstracts both.

-Erik

Thank you very much for all posts, I will try fwe methods over next days and let you know which path I used... I like the C++ OOP solution atm.

About put/getpixel and gaussian... yes I could do it.. unfortunetly performace-wise it would be a major drawback I'm afraid and I don't have much cpu to burn (this actually works on 104Mhz ARM processor with realtime preview for images 320x240-640x480 - it's a cellphone photo editor)

Currently my installer is 45kb and it has got most of the functionality that is planned (but obviously getting rid of few kb wouldn't be bad at all ;-)
My plan is to have 128kB installer in the end (which will include example truetype fonts, example clipart etc)..

If anyone has a s60/uiq phone and would like to test it I will be more then happy to let you see it and get some feedback :-)

Anyway the main problem was to get rid of dozens of function calls (for different image type) and use a higher layer of abstraction, which OOP seems to do fine.

image->Filter() is so much nicer then
if (image->type==image1bpp) image->filter1bpp(..)
if (image->type==image16bpp) image->filter16bpp(..)
...

:-)