The Fundamentals: Style & Readability
Before we get into optimizing, there are a couple of glaring problems with your coding style—that is, the way the code is written and the way it looks.
The biggest problem is that there is a lot of duplication. You repeat yourself over and over inside the if blocks of the main function. Any time you see repeated code, you should extract it out into a function. Even if there are some subtle differences between the various blocks of code, this can be accommodated by passing parameters to the function to customize its behavior. You want to be DRY—don't repeat yourself. It just makes the code harder to read, harder to maintain, and increases the chances for bugs to slip in.
Variables that never change their values are called constants, and the C++ language has a special way to indicate such variables: the const keyword. Using this helps to prevent bugs, like accidentally writing code that changes a variable's value, when that variable is supposed to be constant.
Thus, the x, y, red, gold, and blue variables declared at the top of the main function should all be const, since these are effectively read-only. Also, I would personally recommend making even local variables whose values do not change after initial initialization (such as hdc and color) explicitly const.
(Also, I'm a little bit obsessive-compulsive, so I like to vertically align all of my assignments. Others may disagree that this is worth the effort, but I use tools in my editor that semi-automate it. I find that it not only makes the code look better overall, but makes inconsistencies—i.e., possible errors—more likely to be caught because they stick out like a sore thumb.)
Speaking of the red, gold, and blue variables, these appear to be magic numbers, the values of which would defy most people's intuition. Why, exactly, would 2039797 be a representation of red?
Instead, you should be declaring these color values using the Win32 RGB macro. This takes individual red, green, and blue color components (a representation with which we are all familiar), and packs them into a 32-bit DWORD. Not only is this more readable, but it also ensures that your code is endianness-independent. The Platform SDK determines whether the encoding is RGB or BGR (in the case of Win32, it is actually BGR).
Furthermore, you should prefer to use the COLORREF type for color values in Windows. While COLORREF is simply a typedef for DWORD, it is clearer and more semantically accurate to use COLORREF. Besides, you used the COLORREF type to store the return value of GetPixel, so you should do it here for the comparison constants in the interest of consistency.
In C++, you should be taking advantage of the RAII paradigm to produce cleaner, more correct code. What I mean by this is that any time you have an unmanaged object (such as a Windows handle), you should wrap that object in a class. Make the class's constructor responsible for allocating the object, and the class's destructor responsible for deallocating/freeing the object. When an object of that class is declared, its unmanaged resources are automatically initialized; when that object goes out of scope, its unmanaged resources are automatically released. This way, the compiler does all of the book-keeping automatically, and you never leak.
Your code as written is actually correct, in that you call ReleaseDC for each call to GetDC, but I bet you had to think about this to make sure that you got it right. Best not to make yourself think so hard. Also, using RAII ensures that your code will be exception safe. (Again, not a problem in this case, since the only code in between GetDC and ReleaseDC is a call to GetPixel and an assignment to a POD type, neither of which can throw, but it's all too easy to add something later and forget to audit carefully for exception safety.)
Most people use a framework like MFC or WTL that provides the wrapper classes for you, but if you're just writing a simple app, you don't necessarily need anything that complicated. It only takes a few lines of code to write a simple wrapper class that does everything you need:
class DC
{
public:
explicit DC(HWND hWnd)
: m_hWnd(hWnd)
, m_hDC (GetDC(hWnd))
{ }
~DC()
{
ReleaseDC(m_hWnd, m_hDC);
}
HDC Handle()
{
return m_hDC;
}
private:
HWND m_hWnd;
HDC m_hDC;
};
You could improve this substantially by adding error checking—at the very least, you should probably have an assert that checks for invalid handles and/or failed function calls in debugging builds.
Instead of using the sizeof operator on the type, prefer to use the sizeof operator on the object. For example, change sizeof(INPUT) to sizeof(i). That way, if i ever changes to, say INPUTEX, there will be fewer places in the code that you have to remember to change.
Don't repeat constant literals throughout the code. To be fair, you've only done it one place—inside the pressW function by hardcoding the value 0x57. The damage is relatively contained, but still, 0x57 is repeated twice and this makes changing it in the future an error-prone task. Declare a constant instead.
Better yet, replace the inscrutable 0x57 with 'W'. In other words, let the compiler do the evaluation and determine what the ASCII code of the W character is. The result is the same, but the code is significantly more readable. (Also do the same thing for 0x31, 0x32, and 0x33.)
In most cases, the pressW function should probably be pressKey, and take the virtual-key code as a parameter. Personally, I would prefer writing it this way from the get-go because it's no harder. But you do have to be careful not to make the code you're writing overly generic, because that wastes development time for things you may not ever need (see also YAGNI).
One thing you are doing well, though, is following a consistent style throughout. While K&R is not my preferred brace style, you are using it consistently, and that is really what's most important.
Taking these points into account, here is how I would revise your current implementation:
#include <windows.h>
void pressW();
void waitForColorChange(DWORD x, DWORD y, COLORREF expectedColor);
class DC
{
public:
explicit DC(HWND hWnd)
: m_hWnd(hWnd)
, m_hDC (GetDC(hWnd))
{ }
~DC() {
ReleaseDC(m_hWnd, m_hDC);
}
HDC Handle() {
return m_hDC;
}
private:
HWND m_hWnd;
HDC m_hDC;
};
int main(int argc, char** argv) {
const DWORD x = 566;
const DWORD y = 967;
const COLORREF red = RGB(245, 31, 31);
const COLORREF gold = RGB(187, 171, 0);
const COLORREF blue = RGB(198, 203, 239);
while (true) {
if (GetAsyncKeyState('1')) {
waitForColorChange(x, y, blue);
}
else if (GetAsyncKeyState('2')) {
waitForColorChange(x, y, red);
}
else if (GetAsyncKeyState('3')) {
waitForColorChange(x, y, gold);
}
}
}
void pressW() {
const WORD vkW = 'W';
INPUT i;
i.type = INPUT_KEYBOARD;
i.ki.wScan = MapVirtualKey(vkW, MAPVK_VK_TO_VSC);
i.ki.time = 0;
i.ki.dwExtraInfo = 0;
i.ki.wVk = vkW;
i.ki.dwFlags = 0;
SendInput(1, &i, sizeof(i));
i.ki.dwFlags = KEYEVENTF_KEYUP;
SendInput(1, &i, sizeof(i));
}
void waitForColorChange(DWORD x, DWORD y, COLORREF expectedColor) {
pressW();
while (true) {
DC dc(NULL);
const COLORREF color = GetPixel(dc.Handle(), x, y);
if (color == expectedColor) {
pressW();
break;
}
}
}
There are, of course, other equally valid ways of refactoring your code, but the important thing is to be DRY! The above example achieves that, and without significantly changing the object code generated by a compiler (in other words, without slowing anything down at run-time).
Going Deeper: Cautions & Potential Optimizations
The GetAsyncKeyState function actually is asynchronous, meaning that if you call it twice in a row, you will get two different results if the keyboard state changes between those calls. If you're not very careful, this can result in glitchy behavior. You are probably okay here, but this is something to watch out for.
The call also isn't free. Spinning in a busy loop, repeatedly calling GetAsyncKeyState is a good way to burn a bunch of CPU cycles and turn your computer into a space heater. A better design would be to listen for notifications of a key press event, rather than polling for a state change.
Since you're not passing either the KEYEVENTF_SCANCODE or KEYEVENTF_UNICODE flags, you can avoid computing the scan code and initializing that field each time you call SendInput. This won't save you a whole lot of time, but it does elide one function call.
Even if you needed to initialize the scan-code field, you could save yourself some time by calling the MapVirtualKey function once and caching its result in a static (or global) variable. The mapping of virtual-key codes to scan codes is not going to change while your application is running, so there's no need to call the function each time.
You can save another function call by collapsing the two calls to SendInput into one. This function allows you to pass an array of INPUT structures, and it will synthesize a separate event for each one. The key-down event goes in the first INPUT structure, and the key-up event goes in the second INPUT structure. The required modifications to the code are trivial, but just in case:
void pressW() {
const WORD vkW = 'W';
INPUT i[2];
i[1].type = INPUT_KEYBOARD;
i[1].ki.wVk = vkW;
i[1].ki.time = 0;
i[1].ki.dwExtraInfo = 0;
i[1].ki.dwFlags = 0;
i[2].type = INPUT_KEYBOARD;
i[2].ki.wVk = vkW;
i[2].ki.time = 0;
i[2].ki.dwExtraInfo = 0;
i[2].ki.dwFlags = KEYEVENTF_KEYUP;
SendInput(ARRAYSIZE(i), i, sizeof(i));
}
Notice that I've used the ARRAYSIZE macro (defined in the Win32 SDK headers) to automatically determine the size of the array so I don't have to hard-code another constant. This looks like more code, but it actually executes more quickly because the cost of initializing that extra INPUT structure is dwarfed by the cost of a function call.
If you really wanted to go all out, you could recognize that the values of these INPUT structures never change and decide to make it statically-initialized. This saves the compiler from having to emit code that performs the initialization before each call to SendInput. It simply pushes a pointer to constant data in the executable's DATA segment and calls SendInput. Micro-optimization at its finest! A different syntax will be required for this static initialization:
void pressW() {
const WORD vkW = 'W';
static INPUT i[2] = {
{ INPUT_KEYBOARD, { vkW, 0, 0, 0 } },
{ INPUT_KEYBOARD, { vkW, 0, KEYEVENTF_KEYUP, 0 } },
};
SendInput(ARRAYSIZE(i), i, sizeof(i));
}
(I really wanted to make i const there, but I can't safely do so because the SendInput function expects a non-const pointer to the array of INPUT structures. I could reasonably assume that the function is not going to modify them, but I cannot prove that without inside knowledge of its implementation, so I cannot rely on it.)
Acquiring and releasing a device context (GetDC/ReleaseDC) are not particularly expensive operations, but they aren't exactly free, either. Therefore, it makes good sense to minimize the number of times you do this cycle. It certainly isn't necessary to do it on each iteration of a tight spin loop! The waitForColorChange function that I wrote could do it at the top, then enter the while loop, and only release the DC after it breaks. Using RAII, this is simple:
void waitForColorChange(DWORD x, DWORD y, COLORREF expectedColor) {
pressW();
DC dc(NULL);
while (true) {
if (GetPixel(dc.Handle(), x, y) == expectedColor) {
pressW();
break;
}
}
}
But actually, this waitForColorChange function is itself called from within a tight spin loop! In fact, your code could be revised such that the DC is acquired at the beginning of the program, used throughout its lifetime, and released when your program exits (which will happen automatically, thanks to RAII). This will require a bit of refactoring—a const reference to the DC object will need to be passed to the waitForColorChange function—but doesn't make the code any less readable.
The only concern is that, generally speaking, device contexts shouldn't be held onto any longer than necessary. It seems justified in this case because you are actively using it the entire time, but that may just speak to a larger design flaw with your application. I've already mentioned my reservations about polling.
The Main Event: Optimizing GetPixel
At the core of your code's logic is a call to GetPixel to retrieve the current color value of a particular pixel on the screen. That is unfortunate if you're concerned about speed, because GetPixel is a very slow function, indeed. Why is it so slow? The above-linked documentation doesn't give much of a hint. In fact, this API has a dirty little secret: individual pixels cannot be directly manipulated by the graphics subsystem, so GetPixel (and its brother, SetPixel) have to simulate it, which makes an ostensibly simple, straightforward function call into an extremely slow operation. In addition to the usual overhead of a function call, parameter validation, and switching to and from kernel mode, the GetPixel function also has to create a temporary bitmap, copy the contents of the DC into that temporary bitmap, perform all necessary color-mapping, map the coordinates and locate the pixel, retrieve its color value, and then destroy the temporary bitmap. A lot of work for a single pixel!
The throughput of GetPixel is on the order of 100,000 pixels per second, which is usually fast enough. If you want to speed up drawing operations that involve the manipulation of multiple pixels, the general strategy is to minimize overhead by copying the DC's contents into a temporary device-independent bitmap (DIB) and then using pointer arithmetic on this DIB to access individual pixels. This gives you direct access to each pixel, and allows a throughput of millions or more pixels per second. The problem for you is that you cannot simply use a cached snapshot of the screen—you really do need to update it each time through the loop, which means that this strategy doesn't save you a whole lot in terms of overhead. But it'll still be slightly faster.
Here's an approximation of the code that would be required:
COLORREF FastGetPixel(DWORD x, DWORD y)
{
// Retrieve a device context for the screen.
const HDC hdcScreen = GetDC(NULL);
// Create a DIB section.
BITMAPINFO bi;
bi.bmiHeader.biSize = sizeof(bi.bmiHeader);
bi.bmiHeader.biWidth = 1;
bi.bmiHeader.biHeight = 1;
bi.bmiHeader.biPlanes = 1;
bi.bmiHeader.biBitCount = 32;
bi.bmiHeader.biCompression = BI_RGB;
bi.bmiHeader.biSizeImage = ((((static_cast<unsigned int>(bi.bmiHeader.biWidth) * 32U + 31U) / 32U) * 4U) * bi.bmiHeader.biHeight);
bi.bmiHeader.biClrUsed = 0;
bi.bmiHeader.biClrImportant = 0;
BYTE* pBits; // points to the DIB section's raw bit data
const HBITMAP hbmpDIB = CreateDIBSection(hdcScreen,
&bi,
DIB_RGB_COLORS,
reinterpret_cast<void**>(&pBits),
NULL,
NULL);
// Create a memory device context that is compatible with the screen.
// We need to select our DIB section into this DC so that we can draw onto it.
// Specifically, we'll copy a 1x1 pixel area from the screen into our DIB section.
const HDC hdcMem = CreateCompatibleDC(hdcScreen);
const HBITMAP hbmpOld = static_cast<HBITMAP>(SelectObject(hdcMem, hbmpDIB));
BitBlt(hdcMem, 0, 0, bi.bmiHeader.biWidth, bi.bmiHeader.biHeight,
hdcScreen, x, y,
SRCCOPY);
SelectObject(hdcMem, hbmpOld);
GdiFlush();
// Read the RGB color value of the top-left pixel in our DIB section
// (which is actually the only pixel that it contains).
// Note that in a bitmap, the byte order is always RGBA!
const COLORREF clr = RGB(pBits[2], pBits[1], pBits[0]);
// Clean up.
DeleteObject(hbmpDIB);
DeleteDC(hdcMem);
ReleaseDC(NULL, hdcScreen);
return clr;
}
The code can be improved by writing simple C++ wrapper classes to implement automatic resource management, as was discussed above. You should also add error-checking.
To really squeeze speed out of it, you'll need to resort to some ugly tactics. As written, this does much the same thing as the Win32 GetPixel function, and like I mentioned above, your setup is such that you need to call this function repeatedly. The obvious way to make code execute faster is to reduce the amount of code that you have to execute. Here, that means factoring out common tasks and doing them only once:
- You don't need to retrieve a screen DC and release it each time through the
FastGetPixel function. You can retrieve one once at the start of your program, and free it at the end once you're done.
- The values of the
BITMAPINFO structure are not changing, so you could make it static const and initialize it only once, instead of each time. This is basically the same as what we did with the INPUT structure.
- You don't need to create a new DIB section and free it each time. You can just reuse the same one. Create it once at the start of your program, use it as needed, and free it at the end. (This actually makes the above bullet unnecessary, since the initialization of
BITMAPINFO will no longer be on the critical path.)
- As with the screen DC, you don't need to create a memory DC and free it each time. Keep the same memory DC, with your DIB section selected into it, and re-use it each time. This saves not only repeated calls to
CreateCompatibleDC and DeleteDC, but also repeated calls to SelectObject.
Although making all of these objects global is pretty much breaking every design guideline in the book, sometimes performance concerns justify ugliness. Address maintenance concerns by wrapping them in C++ classes, following a "singleton" pattern.
Now, your inner loop will simply call the following function:
// Required global objects
// (shown here as simple global variables, but should really be managed by
// singleton classes to ensure correct lifetime management)
HDC hdcScreen;
HDC hdcDIB;
BYTE* pDIBBits;
COLORREF FastIterativeGetPixel(DWORD x, DWORD y)
{
// Validate global objects' state in debug builds using assertions
// so you can easily diagnose problems but don't lose performance
// in release builds.
// ...
// Copy the current contents of the screen into our DIB section.
BitBlt(hdcDIB, 0, 0, 1, 1, hdcScreen, x, y, SRCCOPY);
GdiFlush();
// Retrieve the RGB color value of the top-left pixel in our DIB section
// (which is actually the only pixel that it contains).
// Note that in a bitmap, the byte order is always RGBA!
return RGB(pDIBBits[2], pDIBBits[1], pDIBBits[0]);
}
All initialization and clean-up has been deferred, so that all we need to do is a single BitBlt and, to be on the safe side, a GdiFlush. If you were willing to live a bit dangerously, you could omit the flush. (In fact, you're probably safe omitting it in your case, because all that would happen is you'd get stale color values that didn't accurately reflect the current state of the screen, which would just defer your action until the next time through the loop.)
Applying all of the optimizations mentioned in the last two sections, you should see a significant performance gain—an order of magnitude, if you're lucky. But then you will have hit a wall. Real performance gains will only come—as they always do—with reimagining the fundamental algorithm and/or design of your application. Like finding a way to receive notification from the other application when it has changed the color.
GetPixelis very slow. You could possibly speed things up by copying the screen's contents to a DIB section and then using pointer arithmetic to extract the appropriate per-pixel color data. But this still won't be blazingly fast, and more importantly, it is unclear to me why this design even makes sense. You should know what color the pixel is, because your code is what's making it that color! \$\endgroup\$