Naive demosaic for bayer images

Question

I have this legacy code and I don't really think this is easy to read/maintain it. Can you suggest a way to make this more readable while still saving the performance?

template<typename T>
    void static ConvertBayerToRgbImageDemosaic(T* BayerChannel, T* RgbChannel, int Width, int Height, ColorSpace ColorSpace)
    {
        //Translates index offset in Bayer image to channel offset in RGB image
        int offsets[4];
        std::vector<int> colorOffsets[4];
        //calculate offsets according to color space
        switch (ColorSpace)
        {
        case ColorSpace::BGGR:
            offsets[0] = 2;
            offsets[1] = 1;
            offsets[2] = 1;
            offsets[3] = 0;
            colorOffsets[0].push_back(0);
            colorOffsets[0].push_back(1);
            colorOffsets[0].push_back(Width);
            colorOffsets[0].push_back(Width + 1);
            colorOffsets[1].push_back(-1);
            colorOffsets[1].push_back(0);
            colorOffsets[2].push_back(0);
            colorOffsets[2].push_back(1);
            colorOffsets[3].push_back(-Width - 1);
            colorOffsets[3].push_back(-Width);
            colorOffsets[3].push_back(-1);
            colorOffsets[3].push_back(0);
            break;
        case ColorSpace::RGGB:
            offsets[0] = 0;
            offsets[1] = 1;
            offsets[2] = 1;
            offsets[3] = 2;
            colorOffsets[0].push_back(0);
            colorOffsets[0].push_back(1);
            colorOffsets[0].push_back(Width);
            colorOffsets[0].push_back(Width + 1);
            colorOffsets[1].push_back(-1);
            colorOffsets[1].push_back(0);
            colorOffsets[2].push_back(0);
            colorOffsets[2].push_back(1);
            colorOffsets[3].push_back(-Width - 1);
            colorOffsets[3].push_back(-Width);
            colorOffsets[3].push_back(-1);
            colorOffsets[3].push_back(0);
            break;
        case ColorSpace::GBRG:
            offsets[0] = 1;
            offsets[1] = 2;
            offsets[2] = 0;
            offsets[3] = 1;
            colorOffsets[0].push_back(0);
            colorOffsets[0].push_back(1);
            colorOffsets[1].push_back(-1);
            colorOffsets[1].push_back(0);
            colorOffsets[1].push_back(Width - 1);
            colorOffsets[1].push_back(Width);
            colorOffsets[2].push_back(-Width);
            colorOffsets[2].push_back(-Width + 1);
            colorOffsets[2].push_back(0);
            colorOffsets[2].push_back(1);
            colorOffsets[3].push_back(-1);
            colorOffsets[3].push_back(0);
            break;
        case ColorSpace::GRBG:
            offsets[0] = 1;
            offsets[1] = 0;
            offsets[2] = 2;
            offsets[3] = 1;
            colorOffsets[0].push_back(0);
            colorOffsets[0].push_back(1);
            colorOffsets[1].push_back(-1);
            colorOffsets[1].push_back(0);
            colorOffsets[1].push_back(Width - 1);
            colorOffsets[1].push_back(Width);
            colorOffsets[2].push_back(-Width);
            colorOffsets[2].push_back(-Width + 1);
            colorOffsets[2].push_back(0);
            colorOffsets[2].push_back(1);
            colorOffsets[3].push_back(-1);
            colorOffsets[3].push_back(0);
            break;
        default:
            throw "Unsupported colorSpace. Only bayer supported.";
        }

        #pragma omp parallel for    
        for (auto row = 0; row < Height; row++)
        {
            for (auto col = 0, bayerIndex = row * Width; col < Width; col++, bayerIndex++)
            {
                auto offset = (row % 2)*2 + (col % 2); //0...3
                std::for_each(colorOffsets[offset].begin(), colorOffsets[offset].end(), 
                    [&](int colorOffset)
                {
                    auto rgbIndex = (bayerIndex + colorOffset) * 3 + offsets[offset];
                    RgbChannel[rgbIndex] = BayerChannel[bayerIndex];
                });
            }
        }
    }

Answer 1

Proposed changes:

To shorten that huge switch statement a little bit you could extract the offsets into a few constant arrays, since those are not dependent on the function inputs.

const int offsets_BGGR[4] = { 2, 1, 1, 0 };
const int offsets_RGGB[4] = { 0, 1, 1, 2 };
const int offsets_GBRG[4] = { 1, 2, 0, 1 };
const int offsets_GRBG[4] = { 1, 0, 2, 1 };

Then just grab a pointer to the appropriate array in the switch cases.

But going further, I would also extract the switch statement into a separate helper function. Something like a GetColorSpaceConversionOffsets().

---

The BayerChannel parameter is only read from, so it should be marked const. This will be a valuable hint to callers of the function, since they can rest assured that the source data will remain unchanged upon function completion (unless there is a bug, of course).

---

Input parameter validation?

At the moment you are not doing any, which means that if a null pointer is passed as one of the parameters, the code would crash without much information about the error. Consider at the very least asserting to validate the inputs and help catching errors early.

---

A couple other minor points:

• Avoid throwing native types, such as raw strings and integers. The C++ Standard provides a set of throwable classes in the <stdexcept> standard header, that you can use instead. Most commonly you will throw a runtime_error for generic runtime problems, but you can also define a custom exception class to make error reporting more detailed.

• Use camelCase with the first letter lowercase for variables and function parameters, reserving PascalCase for types. For instance, in your function the ColorSpace type name confuses itself with the variable instance name:

  void static ConvertBayerToRgbImageDemosaic(T* BayerChannel, T* RgbChannel, 
               int Width, int Height, ColorSpace ColorSpace) { ...
                                      ^^^^^^^^^^ ^^^^^^^^^^
  

---

Updated code:

#include <vector>
#include <algorithm>
#include <stdexcept>

enum class ColorSpace
{
    BGGR, RGGB, GBRG, GRBG
};

// Local data and helpers "hidden" inside an anonymous namespace.
namespace
{

const int offsets_BGGR[4] = { 2, 1, 1, 0 };
const int offsets_RGGB[4] = { 0, 1, 1, 2 };
const int offsets_GBRG[4] = { 1, 2, 0, 1 };
const int offsets_GRBG[4] = { 1, 0, 2, 1 };

void GetColorSpaceConversionOffsets(ColorSpace colorSpace, int width, std::vector<int> colorOffsets[4], const int *& offsets)
{
    // Calculate offsets according to color space
    switch (colorSpace)
    {
    case ColorSpace::BGGR:
        offsets = offsets_BGGR;
        colorOffsets[0].push_back(0);
        colorOffsets[0].push_back(1);
        colorOffsets[0].push_back(width);
        colorOffsets[0].push_back(width + 1);
        colorOffsets[1].push_back(-1);
        colorOffsets[1].push_back(0);
        colorOffsets[2].push_back(0);
        colorOffsets[2].push_back(1);
        colorOffsets[3].push_back(-width - 1);
        colorOffsets[3].push_back(-width);
        colorOffsets[3].push_back(-1);
        colorOffsets[3].push_back(0);
        break;
    case ColorSpace::RGGB:
        offsets = offsets_RGGB;
        colorOffsets[0].push_back(0);
        colorOffsets[0].push_back(1);
        colorOffsets[0].push_back(width);
        colorOffsets[0].push_back(width + 1);
        colorOffsets[1].push_back(-1);
        colorOffsets[1].push_back(0);
        colorOffsets[2].push_back(0);
        colorOffsets[2].push_back(1);
        colorOffsets[3].push_back(-width - 1);
        colorOffsets[3].push_back(-width);
        colorOffsets[3].push_back(-1);
        colorOffsets[3].push_back(0);
        break;
    case ColorSpace::GBRG:
        offsets = offsets_GBRG;
        colorOffsets[0].push_back(0);
        colorOffsets[0].push_back(1);
        colorOffsets[1].push_back(-1);
        colorOffsets[1].push_back(0);
        colorOffsets[1].push_back(width - 1);
        colorOffsets[1].push_back(width);
        colorOffsets[2].push_back(-width);
        colorOffsets[2].push_back(-width + 1);
        colorOffsets[2].push_back(0);
        colorOffsets[2].push_back(1);
        colorOffsets[3].push_back(-1);
        colorOffsets[3].push_back(0);
        break;
    case ColorSpace::GRBG:
        offsets = offsets_GRBG;
        colorOffsets[0].push_back(0);
        colorOffsets[0].push_back(1);
        colorOffsets[1].push_back(-1);
        colorOffsets[1].push_back(0);
        colorOffsets[1].push_back(width - 1);
        colorOffsets[1].push_back(width);
        colorOffsets[2].push_back(-width);
        colorOffsets[2].push_back(-width + 1);
        colorOffsets[2].push_back(0);
        colorOffsets[2].push_back(1);
        colorOffsets[3].push_back(-1);
        colorOffsets[3].push_back(0);
        break;
    default:
        throw std::runtime_error("Unsupported ColorSpace. Only bayer supported.");
    }
}

} // namespace {}

template<typename T>
void ConvertBayerToRgbImageDemosaic(const T* bayerChannel, T* rgbChannel, int width, int height, ColorSpace colorSpace)
{
    // Translates index offset in Bayer image to channel offset in RGB image
    std::vector<int> colorOffsets[4];
    const int * offsets = nullptr;
    GetColorSpaceConversionOffsets(colorSpace, width, colorOffsets, offsets);

    #pragma omp parallel for
    for (auto row = 0; row < height; row++)
    {
        for (auto col = 0, bayerIndex = row * width; col < width; col++, bayerIndex++)
        {
            auto offset = (row % 2) * 2 + (col % 2); // 0...3

            std::for_each(colorOffsets[offset].begin(), colorOffsets[offset].end(),
                [&](int colorOffset)
                {
                    auto rgbIndex = (bayerIndex + colorOffset) * 3 + offsets[offset];
                    rgbChannel[rgbIndex] = bayerChannel[bayerIndex];
                });
        }
    }
}