Dynamic method dispatching in C with list of functions

Question

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)

Have a switch - case return a function pointer. You only call this function once at the start - https://stackoverflow.com/questions/840501/how-do-function-pointers-in-c-work and https://stackoverflow.com/questions/9410/how-do-you-pass-a-function-as-a-parameter-in-c — Dave S, Jun 24 '19 at 21:12
So what would be the type of the pointer ? I don't really understand what's going on in this answer. — Aurélien Pierre, Jun 24 '19 at 21:17
Do not use names beginning with `_RGB` in your code. They are *reserved*, as more generally are all names beginning with an underscore and a capital letter, or with two underscores. Don't use `_rgb` at file scope, either. — John Bollinger, Jun 24 '19 at 21:18
maybe something like `struct entry fptrs[] { { dt_iop_toneequalizer_method_t, RGB_mean }, ... }` — Rorschach, Jun 24 '19 at 21:20
I can't make sense of this question. It's like it's trying to ask how to declare the type for `lightness_method` but the actual syntax for doing so exists in the delcaration of the structure `RGB_light`. — Joshua, Jun 24 '19 at 21:21
`method = function that returns a function pointer( enum to choose function );` and that function uses a switch - case to pick one. — Dave S, Jun 24 '19 at 21:30
The structure is the first idea of function packing I found, but I would like an ordered list where functions are indexed by `dt_iop_toneequalizer_method_t` values. It's something I would achieve with a dictionnary of functions in Python, but I have no idea in C. — Aurélien Pierre, Jun 24 '19 at 21:30
**but I did not succeed...** What went wrong? You've explained a lot about what you're hoping to do, but next to nothing about why you couldn't do it. Edit your question to include specifically what you tried and why that didn't work. — Caleb, Jun 24 '19 at 21:51

score 1 · Accepted Answer · answered Jun 24 '19 at 21:58

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.

score 0 · Answer 2 · answered Jun 25 '19 at 00:09

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.

As I wrote in my answer, the unsatisfactory performance likely arises more than anything else from calling your functions indirectly through function pointers. I also suggested a remediation: pull up the indirection to a higher level -- whole image instead of per-pixel. — John Bollinger, Jun 25 '19 at 13:20
but there is no function pointer in the above code, and the switch has been put before the loops. — Aurélien Pierre, Jun 27 '19 at 08:05
Well my bad, then. From context, I took `pixel_rgb_mean`, `pixel_rgb_lightness`, and `pixel_rgb_value` to be identifiers of function *pointers*, not of functions. The only suggestion I have left is along the same lines, though: avoid making a separate function call for each pixel. Function calls are comparatively expensive, and they may also interfere with optimization. — John Bollinger, Jun 27 '19 at 12:07

Dynamic method dispatching in C with list of functions

2 Answers2