6
\$\begingroup\$

I came up with the following code to convert a rgba color to an argb color:

public static int ToARGB(int rgba){
    return (rgba << (3*8)) | ((rgba>>8) & 0x00FFFFFF);  
}

I used the bitmask 0x00FFFFFF since the rightshift in C# pads with the most significant bit of the number to be shifted. I tested this methode and it seems to work, but I need to be sure I am not missing a case where this is garbage. Any thoughts on my code please?

\$\endgroup\$
  • 2
    \$\begingroup\$ The code looks fine; the more fundamental problem is that you are representing things that are logically not integers using the int class. Make yourself a Color struct if you don't have one already and make constructors for that thing that take rgba and argb values, and accessors that produces ints in that format. \$\endgroup\$ – Eric Lippert May 31 '17 at 16:20
  • 1
    \$\begingroup\$ @EricLippert Please convert your "Your code looks fine" comment to an answer. \$\endgroup\$ – 200_success May 31 '17 at 22:35
  • \$\begingroup\$ I'm using the Unity Engine wich got a Color class but need to convert rgba values from Unity to argb so I can pass them to Android, since the Android Color class can only parse argb. \$\endgroup\$ – Eric Jun 1 '17 at 10:03
4
\$\begingroup\$

When I see this question, I immediately understand what you are trying to do as a bitwise rotation (also known as a circular shift). You need to either rotate the bits in the value left by 24 or right by 8 (they yield equivalent results). As the bits are shifted off one end, they're inserted back in the other end.

Ironically, this is one of those rare cases where the code is conceptually simpler in assembly language (!) than it is in C#. The x86 processor family has two very simple instructions that accomplish precisely this feat—ROL and ROR—which rotate left and right, respectively. That means the code in assembly language is almost as simple as:

rol  value, 24

Whereas it is actually more work—and less obvious what is being done—when writing the code in C or C#. You have to explicitly notate both parts of the circular shift. The canonical form is:

public static UInt32 RotateLeft(UInt32 value, UInt32 amount)
{
    Debug.Assert(amount < 32, "Attempted to rotate by more bits than are in the value.");
    return (value << amount) | (value >> (32 - amount));
}

public static UInt32 RotateRight(UInt32 value, UInt32 amount)
{
    Debug.Assert(amount < 32, "Attempted to rotate by more bits than are in the value.");
    return (value >> amount) | (value << (32 - amount));
}

Note that I've explicitly used the UInt32 type here, instead of the short name uint. This is because I've hard-coded assumptions about the type's width, and I like things to be obvious. UInt32 is obviously always a 32-bit type. At least for me, since I bounce back and forth between languages, that's not always so obvious with uint.

I've also thrown in an assertion to check that we're rotating by a reasonable amount. That's just an assertion, though, not an exception, because the C# language standard guarantees that the shift amount will be masked, such that only the lower 5 bits will be used (resulting in a value from 0 to 31). This means that the language protects you from the undefined behavior you would get in C by trying to shift by too many places. (Again, ironically, so does x86 assembly language.)

So this is basically the generalized form of the code that you have. A call like:

argb = RotateLeft(rgba, 24);

would essentially inline to:

argb = (rgba << 24) | (rgba >> 8);

There is one major difference, though. I've written the code to work with unsigned types, rather than signed types. In fact, bitwise operations should always be done on unsigned types because when you do bitwise operations, you are essentially treating the value as a field of bits, rather than as a number. Using an unsigned type is not only conceptually more correct, but also saves you from bugs.

C# automatically differentiates between signed and unsigned shifts (based on the type of the value you're shifting), which is why you found that your code needed that extra mask. The result was being padded with the most significant bit of the number being shifted because that's how a signed shift works—in effect, it does a signed extension, where the sign bit (the most significant bit) is repeated to preserve the sign of the value. (Note that this distinction is only relevant for right shifts; signed and unsigned left shifts are exactly the same.)

But that's something you don't want to have to think about, and something that you wouldn't have to think about if you just follow the rule that bitwise operations imply unsigned types.

So the correct code is precisely what pgs already suggested:

public static UInt32 ToARGB(UInt32 rgba)
{
    return (rgba << (3*8)) | (rgba >> 8);
}

But you might also want to consider Eric Lippert's advice about logical representations. It is merely an implementation detail that these color values are stored as unsigned 32-bit integers. Therefore, it would make sense to wrap that implementation detail up inside of a structure, so that the client never has to deal with the messiness.


Unfortunately, the C# JIT compiler doesn't seem to be able to produce as efficient of code here as a C compiler would. In C or C++, your function would be transformed directly into that assembly-language instruction I mentioned above. In C#, you get 4 instructions instead of that one:

mov  ecx, eax    // make extra copy of value
shl  ecx, 8      // shift that copy left by 8
shr  eax, 24     // shift original right by 24
or   eax, ecx    // combine the two

Oh well. That's still as efficient as you're going to get.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your detailed answer. I got to know a lot of new things. But I am still wondering why C# JIT compiler doesn't recognize it as a single command? \$\endgroup\$ – pgs Jun 2 '17 at 16:48
  • 1
    \$\begingroup\$ In general, JIT (just-in-time) compilers do not produce the same quality of output as AOT (ahead-of-time) compilers. The reason is pretty simple: JIT compilers have to work very quickly, since the user is sitting there waiting on them to compile the code so it can be executed. An AOT compiler, like C uses, can afford to take its sweet time running multiple optimization passes on the code. There is inherently some more flexibility in a JIT compiler, like being able to tune for the current machine, but most of this just isn't being realized [yet], given the real-world latency constraints. \$\endgroup\$ – Cody Gray Jun 2 '17 at 17:01
  • 1
    \$\begingroup\$ You can see how the JIT compiler has just transformed the C# code very literally into assembly-language instructions. There are two shifts, a left and a right shift, and their results are combined with a bitwise OR. That's exactly what the assembly code does. So basically, it looks like what's happening here is that the JIT compiler is doing register allocation and some peephole optimizations, but isn't recognizing the larger picture, that these two shifts and an OR are actually doing a rotation, and that there's a specialized instruction for that. \$\endgroup\$ – Cody Gray Jun 2 '17 at 17:02
  • 1
    \$\begingroup\$ It is possible that a JIT compiler could recognize that on a subsequent pass through the code, as it further refines code that is on the hot path. I don't know of any way to check that in a debugger. It's also possible that the JIT implementation was just designed very generally, and doesn't utilize special x86 instructions like ROL/ROR. \$\endgroup\$ – Cody Gray Jun 2 '17 at 17:03
2
\$\begingroup\$

The code looks fine.

I would just suggest, if it's possible, to use uint instead int:

    public static uint ToARGB(uint rgba) {
        return (rgba << 24) | (rgba >> 8);
    }

This way, you can avoid using the bitmask 0x00FFFFFF.

\$\endgroup\$
  • \$\begingroup\$ This is a good answer, but it would benefit from a brief explanation of why this makes the bit masking unnecessary. The issue is signed vs. unsigned shifts, of course. \$\endgroup\$ – Cody Gray Jun 2 '17 at 15:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.