Reputation: 713
I try to compute the lightness of an RGB pixel with various methods, in C99 with GCC 9. I have this enum :
typedef enum dt_iop_toneequalizer_method_t
{
DT_TONEEQ_MEAN = 0,
DT_TONEEQ_LIGHTNESS,
DT_TONEEQ_VALUE
} dt_iop_toneequalizer_method_t;
which is user input from a GUI.
Then, I have several functions corresponding to each case:
typedef float rgb_pixel[4] __attribute__((aligned(16)));
#pragma omp declare simd aligned(pixel:64)
static float _RGB_mean(const rgb_pixel pixel)
{
return (pixel[0] + pixel[1] + pixel[2] + pixel[3]) / 3.0f;
}
#pragma omp declare simd aligned(pixel:16)
static float _RGB_value(const rgb_pixel pixel)
{
return fmaxf(fmaxf(pixel[0], pixel[1]), pixel[2]);
}
#pragma omp declare simd aligned(pixel:16)
static float _RGB_lightness(const rgb_pixel pixel)
{
const float max_rgb = _RGB_value(pixel);
const float min_rgb = fminf(pixel[0], fminf(pixel[1], pixel[2]));
return (max_rgb + min_rgb) / 2.0f;
}
Then, the loop over the image is:
static void exposure_mask(const float *const restrict in,
float *const restrict out,
const size_t width,
const size_t height,
const dt_iop_toneequalizer_method_t method)
{
#pragma omp parallel for simd default(none) schedule(static) aligned(in, out:64)
for(size_t k = 0; k < 4 * width * height; k += 4)
{
const rgb_pixel pixel = { in[k], in[k + 1], in[k + 2], 0.0f };
out[k / 4] = RGB_light(pixel, method);
}
}
My first approach was to use an RGB_light()
function with a switch/case mapping the method
and the functions, but that triggers checks for every pixel, which is quite expensive.
My idea would be to use a list or a struct
of the methods, like that:
typedef struct RGB_light
{
// Pixel intensity (method == DT_TONEEQ_MEAN)
float (*_RGB_mean)(rgb_pixel pixel);
// Pixel HSL lightness (method == DT_TONEEQ_LIGHTNESS)
float (*_RGB_lightness)(rgb_pixel pixel);
// Pixel HSV value (method == DT_TONEEQ_VALUE)
float (*_RGB_value)(rgb_pixel pixel);
} RGB_light;
then initiatize the method once for all before the loop, like
static void exposure_mask(const float *const restrict in,
float *const restrict out,
const size_t width,
const size_t height,
const dt_iop_toneequalizer_method_t method)
{
lightness_method = RGB_light[method]; // obviously wrong syntax
#pragma omp parallel for simd default(none) schedule(static) aligned(in, out:64)
for(size_t k = 0; k < 4 * width * height; k += 4)
{
const rgb_pixel pixel = { in[k], in[k + 1], in[k + 2], 0.0f };
out[k / 4] = lightness_method(pixel);
}
}
but I did not succeed in translating that idea to actual working code.
There is something similar to what I want to do in Python :
def RGB_value(pixel):
return whatever
def RGB_lightness(pixel):
return whatever
methods = { 1: RGB_value, 2: RGB_lightness }
def loop(image, method):
for pixel in image:
lightness = methods[method](pixel)
Upvotes: 2
Views: 106
Reputation: 713
Based on @John Bollinger's answer, I tried this :
#define LOOP(fn) \
{ \
_Pragma ("omp parallel for simd default(none) schedule(static) \
firstprivate(width, height, ch, in, out) \
aligned(in, out:64)" ) \
for(size_t k = 0; k < ch * width * height; k += 4) \
{ \
const rgb_pixel pixel = { in[k], in[k + 1], in[k + 2], 0.0f }; \
out[k / ch] = fn(pixel); \
} \
break; \
}
static void exposure_mask(const float *const restrict in, float *const restrict out,
const size_t width, const size_t height, const size_t ch,
const dt_iop_toneequalizer_method_t method)
{
switch(method)
{
case DT_TONEEQ_MEAN:
LOOP(pixel_rgb_mean);
case DT_TONEEQ_LIGHTNESS:
LOOP(pixel_rgb_lightness);
case DT_TONEEQ_VALUE:
LOOP(pixel_rgb_value);
}
}
However, it turns out to be just as fast (or slow…) as the pixel-wise check I did before (avoiding the macro), probably because I compile with unswitch-loops
parameter.
Upvotes: 0
Reputation: 181459
The crux of the question seems to be this:
My idea would be to use a list or a struct of the methods, like that:
typedef struct RGB_light { // Pixel intensity (method == DT_TONEEQ_MEAN) float (*_RGB_mean)(rgb_pixel pixel); // Pixel HSL lightness (method == DT_TONEEQ_LIGHTNESS) float (*_RGB_lightness)(rgb_pixel pixel); // Pixel HSV value (method == DT_TONEEQ_VALUE) float (*_RGB_value)(rgb_pixel pixel); } RGB_light;
then initiatize the method once for all before the loop, like
static void exposure_mask(const float *const restrict in, float *const restrict out, const size_t width, const size_t height, const dt_iop_toneequalizer_method_t method) { lightness_method = RGB_light[method]; // obviously wrong syntax
, with the actual question being what to use instead of the "obviously wrong syntax". But you clearly know already how to declare a function pointer, and you describe other code that switches based on the method
. The natural way to put those together is
float (*lightness_method)(rgb_pixel pixel);
switch (method) {
case DT_TONEEQ_MEAN:
lightness_method = _RGB_mean;
break;
case DT_TONEEQ_LIGHTNESS:
lightness_method = _RGB_lightness;
break;
case DT_TONEEQ_VALUE:
lightness_method = _RGB_value;
break;
}
... which you would follow up somewhere later with something along the lines of ...
float l = lightness_method(one_pixel);
Similar would apply if you provided the "methods" in any array instead of a struct, in which case you could index the array with the method
variable instead of using a switch
statement.
Do note, however, that inasmuch as you seem to be focusing on performance, you are likely to find that any approach along these lines is a bit lackluster. Calling functions indirectly through pointers denies the compiler opportunities to optimize, and although resolving the specific function pointer outside the loop over the pixels may yield an improvement, the indirection itself is probably more responsible for your performance dissatisfaction than the repeated lookups.
You should consider instead indirecting to multiple versions of exposure_mask()
, each of which calls a specific lightness function directly. At least consider testing such an arrangement. Additionally, since you'll then want a bunch of functions that are mostly identical, you could consider using a macro or programmatic code generator to generate them all, instead of writing and maintaining all those variations by hand.
Upvotes: 1