Get pixel color from screen in C++ on Linux

Question 1

For a long period of time I worked on this code, mostly trying to understand it. That is the main reason for needed review. Second reason is simply that I would like to optimize it in any way you can find.

Side note: I have shortened the code as humanly possible.

Compilation of the below code:

g++-8 -O2 -std=c++17 -Wall -Wextra -Werror -Wpedantic -pedantic-errors getPixelColor.cpp -o getPixelColor $(pkg-config --cflags --libs x11)

#include <iostream>
#include <X11/Xlib.h>
#include <cmath> // round()
// https://stackoverflow.com/a/59663458/1997354
inline __attribute__((always_inline)) \
unsigned long MyXGetPixel(XImage * ximage, const int & x, const int & y)
{
 return (*ximage->f.get_pixel)(ximage, x, y);
}
void MyGetPixelColor(Display * my_display, const int & x_coord, const int & y_coord, XColor * pixel_color)
{
 XImage * screen_image = XGetImage(
 my_display,
 XRootWindow(my_display, XDefaultScreen(my_display)),
 x_coord, y_coord,
 1, 1,
 AllPlanes,
 XYPixmap
 );
 pixel_color->pixel = MyXGetPixel(screen_image, 0, 0);
 XFree(screen_image);
 XQueryColor(my_display, XDefaultColormap(my_display, XDefaultScreen(my_display)), pixel_color);
}
// usage
int main()
{
Display * my_display = XOpenDisplay(NULL);
XColor screen_pixel_color;
// in this example let's take [0,0] coordinates
MyGetPixelColor(my_display, 0, 0, & screen_pixel_color);
XCloseDisplay(my_display);
unsigned short raw_r_value = screen_pixel_color.red;
unsigned short raw_g_value = screen_pixel_color.green;
unsigned short raw_b_value = screen_pixel_color.blue;
std::cout
 << "R = " << round(raw_r_value / 256.0) << std::endl
 << "G = " << round(raw_g_value / 256.0) << std::endl
 << "B = " << round(raw_b_value / 256.0) << std::endl;
}

Unshortened || full code

#include <iostream>
#include <X11/Xlib.h>
#include <cmath> // round()
//#include <cstring>
//#include <cstdlib>
//#include <gdk/gdk.h>
//#include <gtk/gtk.h>
//#include <X11/Xutil.h>
//#include </usr/include/gtk-3.0/gtk/gtk.h>
//#include <ncurses.h>
class BasicCoordinates
{
private:
 long long X;
 long long Y;
public:
 BasicCoordinates(const long long & _X, const long long & _Y)
 {
 X = _X;
 Y = _Y;
 }
 long long getX()
 {
 return X;
 }
 long long getY()
 {
 return Y;
 }
};
bool StringContainsInteger(const std::string & str)
// true : if the string contains an integer number (possibly starting with a sign)
// false: if the string contains some other character(s)
{
 std::string::size_type str_len = str.length();
 if (str_len == 0) return false;
 bool sign_present = (str[0] == '-' || str[0] == '+');
 if (str_len == 1 && sign_present) return false;
 for (std::string::size_type i = 0; i < str_len; i++)
 {
 if (i == 0 && sign_present) continue;
 if (! std::isdigit((unsigned char) str[i])) return false;
 }
 return true;
}
// https://stackoverflow.com/a/59663458/1997354
inline __attribute__((always_inline)) \
unsigned long MyXGetPixel(XImage * ximage, const int & x, const int & y)
{
 return (*ximage->f.get_pixel)(ximage, x, y);
}
void MyGetPixelColor(Display * my_display, const int & x_coord, const int & y_coord, XColor * pixel_color)
{
 XImage * screen_image = XGetImage(
 my_display,
 XRootWindow(my_display, XDefaultScreen(my_display)),
 x_coord, y_coord,
 1, 1,
 AllPlanes,
 XYPixmap
 );
 pixel_color->pixel = MyXGetPixel(screen_image, 0, 0);
 XFree(screen_image);
 XQueryColor(my_display, XDefaultColormap(my_display, XDefaultScreen(my_display)), pixel_color);
}
int main(const int argc, const char * argv[])
{
 const int given_arguments_count = argc - 1;
 if (given_arguments_count != 2)
 {
 std::cerr
 << "Fatal error occurred while checking\n"
 << "the number of given arguments\n"
 << "--------------------------------------\n"
 << "In the function : " << __func__ << std::endl
 << "At the command : " << "given_arguments_count\n"
 << "Given arguments : " << given_arguments_count << std::endl
 << "Error message : " << "This program is expecting exactly 2 arguments\n"
 << " being the X Y coordinates of a pixel on the screen\n";
 return 1;
 }
 const std::string cli_argument_1 = argv[1];
 if (! StringContainsInteger(cli_argument_1))
 {
 std::cerr
 << "Fatal error occurred while checking\n"
 << "if the first argument contains a number\n"
 << "----------------------------------------\n"
 << "In the function : " << __func__ << std::endl
 << "At the command : " << "StringContainsInteger\n"
 << "Input string : " << cli_argument_1 << std::endl
 << "Error message : " << "The first argument is not an integer number\n";
 return 1;
 }
 const std::string cli_argument_2 = argv[2];
 if (! StringContainsInteger(cli_argument_2))
 {
 std::cerr
 << "Fatal error occurred while checking\n"
 << "if the second argument contains a number\n"
 << "----------------------------------------\n"
 << "In the function : " << __func__ << std::endl
 << "At the command : " << "StringContainsInteger\n"
 << "Input string : " << cli_argument_2 << std::endl
 << "Error message : " << "The second argument is not an integer number\n";
 return 1;
 }
 long long x_coord;
 try
 {
 x_coord = std::stoll(cli_argument_1);
 }
 catch (const std::exception & input_exception)
 {
 std::cerr
 << "Fatal error occurred while converting\n"
 << "the first argument to an integer variable\n"
 << "-------------------------------------\n"
 << "In the function : " << __func__ << std::endl
 << "At the command : " << input_exception.what() << std::endl
 << "Input string : " << cli_argument_1 << std::endl
 << "Error message : " << "The first number argument is too big an integer\n";
 return 1;
 }
 long long y_coord;
 try
 {
 y_coord = std::stoll(cli_argument_2);
 }
 catch (const std::exception & input_exception)
 {
 std::cerr
 << "Fatal error occurred while converting\n"
 << "the second argument to an integer variable\n"
 << "--------------------------------------\n"
 << "In the function : " << __func__ << std::endl
 << "At the command : " << input_exception.what() << std::endl
 << "Input string : " << cli_argument_2 << std::endl
 << "Error message : " << "The second number argument is too big an integer\n";
 return 1;
 }
 BasicCoordinates * pixel_coords = new BasicCoordinates(x_coord, y_coord);
 std::cout << "X = " << pixel_coords->getX() << std::endl;
 std::cout << "Y = " << pixel_coords->getY() << std::endl << std::endl;
 Display * my_display = XOpenDisplay(NULL);
 Screen * my_screen = XDefaultScreenOfDisplay(my_display);
 const int screen_width = my_screen->width;
 const int screen_height = my_screen->height;
 if (x_coord >= screen_width)
 {
 std::cerr
 << "X coord bigger than the screen with\n"; // TEMP
 return 1;
 }
 if (y_coord >= screen_height)
 {
 std::cerr
 << "Y coord bigger than the screen height\n"; // TEMP
 return 1;
 }
 XWindowAttributes root_window_attributes;
 XGetWindowAttributes(my_display, DefaultRootWindow(my_display), & root_window_attributes);
 XColor screen_pixel_color;
 MyGetPixelColor(my_display, x_coord, y_coord, & screen_pixel_color);
 XCloseDisplay(my_display);
 unsigned short raw_r_value = screen_pixel_color.red;
 unsigned short raw_g_value = screen_pixel_color.green;
 unsigned short raw_b_value = screen_pixel_color.blue;
/*
 std::cout
 << "Raw Values" << std::endl
 << "R = " << raw_r_value << std::endl
 << "G = " << raw_g_value << std::endl
 << "B = " << raw_b_value << std::endl;
 std::cout << std::endl;
*/
 std::cout
// << "Normalized" << std::endl
 << "R = " << round(raw_r_value / 256.0) << std::endl
 << "G = " << round(raw_g_value / 256.0) << std::endl
 << "B = " << round(raw_b_value / 256.0) << std::endl;
 return 0;
}

Question 2

Have you written this code yourself? There are indications in your question that you dont understand the code, even just to a degree. This would be off topic on this site.

Question 3

@slepic I just partially copied and modified the MyGetPixelColor function.

Question 4

For Code Review, you're not supposed to shorten your code. You're supposed to keep it understandable and readable.

Question 5

@S.S.Anne Full code added..

Question 6

This class is much more complicated than it needs to be:

class BasicCoordinates
{
private:
 long long X;
 long long Y;
public:
 BasicCoordinates(const long long & _X, const long long & _Y)
 {
 X = _X;
 Y = _Y;
 }
 long long getX()
 {
 return X;
 }
 long long getY()
 {
 return Y;
 }
};

A simple aggregate does the job and is more intuitive:

struct Coordinates {
 long long x;
 long long y;
};

This function is also quite convoluted:

bool StringContainsInteger(const std::string & str)
// true : if the string contains an integer number (possibly starting with a sign)
// false: if the string contains some other character(s)
{
 std::string::size_type str_len = str.length();
 if (str_len == 0) return false;
 bool sign_present = (str[0] == '-' || str[0] == '+');
 if (str_len == 1 && sign_present) return false;
 for (std::string::size_type i = 0; i < str_len; i++)
 {
 if (i == 0 && sign_present) continue;
 if (! std::isdigit((unsigned char) str[i])) return false;
 }
 return true;
}

which is basically just:

bool StringContainsInteger(std::string_view str)
{
 // C++20: str.starts_with('+') || str.starts_with('-')
 if (!str.empty() && (str[0] == '+' || str[0] == '-')) {
 str.remove_prefix(1);
 }
 return std::all_of(str.begin(), str.end(),
 [](unsigned char c) { return std::isdigit(c); });
}

(note that std::string_view is preferable to const std::string& if all you want is read the characters in the string).

Question 7

I think StringContainsInteger is redundant anyway, one can use the second argument to std::stoll() to check whether all characters in the string have been processed, and only accept the result if that is the case.

Question 8

@G.Sliepen Yes. It's better to use that directly since OP is calling stoll immediately after validating the string.

Question 9

Use `XGetPixel()` instead of writing your own function

You should use the API provided to you by X libraries whenever possible. The implementation details might change over time, so your own copy of XGetPixel() might no longer be correct in the future. Granted, this is very unlikely in the case of Xlib, but it is good practice in general.

Don't pass by const reference unnecessarily

Don't pass small variables by const reference. You typically only do this for non-trivial types where the cost of passing by value is larger than passing by reference, for example std::string, std::vector and so on. You should never need a const reference for something like an int, float and so on. So:

void MyGetPixelColor(Display *my_display, int x_coord, int y_coord, XColor *pixel_color)
{ ... }

Return by value when appropriate

Functions that return some value should preferably return that value, instead of taking a pointer that will be written to. So I would rewrite your code to:

Xcolor MyGetPixelColor(Display *my_display, int x_coord, int y_coord)
{
 Xcolor pixel_color;
 XImage *screen_image = ...;
 pixel_color->pixel = XGetPixel(screen_image, 0, 0);
 XFree(screen_image);
 XQueryColor(my_display, XDefaultColormap(my_display, XDefaultScreen(my_display)), pixel_color);
 return pixel_color;
}

This doesn't cause extra copies of Xcolor to be made, since C++ compilers will use return value optimization here.

Use `\n` instead of `std::endl`

Avoid using std::endl, it is equivalent to \n but it will also flush the output, which can hurt performance. See this question for more details.

L. F. L. F. 9,6952 gold badges27 silver badges69 bronze badges · Accepted Answer · 2020-03-28 08:51:48Z

This class is much more complicated than it needs to be:

class BasicCoordinates
{
private:
 long long X;
 long long Y;
public:
 BasicCoordinates(const long long & _X, const long long & _Y)
 {
 X = _X;
 Y = _Y;
 }
 long long getX()
 {
 return X;
 }
 long long getY()
 {
 return Y;
 }
};

A simple aggregate does the job and is more intuitive:

struct Coordinates {
 long long x;
 long long y;
};

This function is also quite convoluted:

bool StringContainsInteger(const std::string & str)
// true : if the string contains an integer number (possibly starting with a sign)
// false: if the string contains some other character(s)
{
 std::string::size_type str_len = str.length();
 if (str_len == 0) return false;
 bool sign_present = (str[0] == '-' || str[0] == '+');
 if (str_len == 1 && sign_present) return false;
 for (std::string::size_type i = 0; i < str_len; i++)
 {
 if (i == 0 && sign_present) continue;
 if (! std::isdigit((unsigned char) str[i])) return false;
 }
 return true;
}

which is basically just:

bool StringContainsInteger(std::string_view str)
{
 // C++20: str.starts_with('+') || str.starts_with('-')
 if (!str.empty() && (str[0] == '+' || str[0] == '-')) {
 str.remove_prefix(1);
 }
 return std::all_of(str.begin(), str.end(),
 [](unsigned char c) { return std::isdigit(c); });
}

(note that std::string_view is preferable to const std::string& if all you want is read the characters in the string).

I think StringContainsInteger is redundant anyway, one can use the second argument to std::stoll() to check whether all characters in the string have been processed, and only accept the result if that is the case.
@G.Sliepen Yes. It's better to use that directly since OP is calling stoll immediately after validating the string.

Stack Exchange Network

Get pixel color from screen in C++ on Linux

Unshortened || full code

2 Answers 2

Use `XGetPixel()` instead of writing your own function

Don't pass by const reference unnecessarily

Return by value when appropriate

Use `\n` instead of `std::endl`

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Hot Network Questions

Get pixel color from screen in C++ on Linux

Unshortened || full code

2 Answers 2

Use XGetPixel() instead of writing your own function

Don't pass by const reference unnecessarily

Return by value when appropriate

Use \n instead of std::endl

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Related

Hot Network Questions

Use `XGetPixel()` instead of writing your own function

Use `\n` instead of `std::endl`