views:

257

answers:

3

I realize that what I am trying to do isn't safe. But I am just doing some testing and image processing so my focus here is on speed.

Right now this code gives me the corresponding bytes for a 32-bit pixel value type.

struct Pixel {
    unsigned char b,g,r,a;
};

I wanted to check if I have a pixel that is under a certain value (e.g. r, g, b <= 0x10). I figured I wanted to just conditional-test the bit-and of the bits of the pixel with 0x00E0E0E0 (I could have wrong endianness here) to get the dark pixels.

Rather than using this ugly mess (*((uint32_t*)&pixel)) to get the 32-bit unsigned int value, i figured there should be a way for me to set it up so I can just use pixel.i, while keeping the ability to reference the green byte using pixel.g.

Can I do this? This won't work:

struct Pixel {
    unsigned char b,g,r,a;
};
union Pixel_u {
    Pixel p;
    uint32_t bits;
};

I would need to edit my existing code to say pixel.p.g to get the green color byte. Same happens if I do this:

union Pixel {
    unsigned char c[4];
    uint32_t bits;
};

This would work too but I still need to change everything to index into c, which is a bit ugly but I can make it work with a macro if i really needed to.

+8  A: 

Why not make the ugly mess into an inline routine? Something like:

inline uint32_t pixel32(const Pixel& p)
{
    return *reinterpret_cast<uint32_t*>(&p);
}

You could also provide this routine as a member function for Pixel, called i(), which would allow you to access the value via pixel.i() if you preferred to do it that way. (I'd lean on separating the functionality from the data structure when invariants need not be enforced.)

fbrereto
-1 for even suggesting a macro when any number of other solutions would accomplish the same goal without subverting the compiler.
John Dibling
@John: Fair enough; fixed.
fbrereto
`reinterpret_cast` is a C++-ism. ;)
mipadi
@mipadi: At the time I answered the question it had the `[c++]` tag, but yes you are right.
fbrereto
Yeah, I noticed after my comment that the post had recently been edited to remove the C++ tag.
mipadi
+11  A: 
Joseph Quinsey
In that case it is the `struct` that is anonymous, not the union.
nategoose
@nategoose: Thank you. Correction made.
Joseph Quinsey
+1: Never knew this is possible!
rstevens
Exactly what I thought of when I read the question, but you beat me to it!
A. Levy
Be aware that using anonymous unions and structs restricts the portability of your code. There are many C89 compilers that don't support them.
tomlogic
+5  A: 

The problem with a structure inside a union, is that the compiler is allowed to add padding bytes between members of a structure (or class), except bit fields.

Given:

struct Pixel
{
  unsigned char red;
  unsigned char green;
  unsigned char blue;
  unsigned char alpha;
};

This could be laid out as:

Offset  Field
------  -----
0x00    red
0x04    green
0x08    blue
0x0C    alpha

So the size of the structure would be 16 bytes.

When put in a union, the compiler would take the larger capacity of the two to determine space. Also, as you can see, a 32 bit integer would not align correctly.

I suggest creating functions to combine and extract pixels from a 32-bit quantity. You can declare it inline too:

void Int_To_Pixel(const unsigned int word,
                  Pixel& p)
{
  p.red =   (word & 0xff000000) >> 24;
  p.blue =  (word & 0x00ff0000) >> 16;
  p.green = (word & 0x0000ff00) >> 8;
  p.alpha = (word & 0x000000ff);
  return;
}

This is a lot more reliable than a struct inside a union, including one with bit fields:

struct Pixel_Bit_Fields
{
  unsigned int red::8;
  unsigned int green::8;
  unsigned int blue::8;
  unsigned int alpha::8;
};

There is still some mystery when reading this whether red is the MSB or alpha is the MSB. By using bit manipulation, there is no question when reading the code.

Just my suggestions, YMMV.

Thomas Matthews