2
\$\begingroup\$

I've written a program that does three things:

Take an equirectangular image and ...

  • ... cut horizontally-overlapping image areas.
  • ... fill the image from the bottom with black so it has a ratio of 2:1.
  • ... create each face-texture of a cubemap from the image.
  • ... do interpolation optionally.

The algorithm is the following: I use the image as a projection on a unit-sphere. I put a cube around that sphere and project each pixel of the faces onto the sphere. I'm doing that using the pixel-coordinates and the corresponding vector in cartesian-coordinates. I only evaluate the vectors that belong to the -Z-direction-face and rotate them to get the corresponding vectors for other directions.

#define _USE_MATH_DEFINES

#include <iostream>
#include <OpenImageIO/imageio.h>
#include <vector>
#include <boost/filesystem.hpp>
namespace bfs = boost::filesystem;


struct Pixel {
    unsigned char R;
    unsigned char G;
    unsigned char B;
};


struct Vector {
    double x;
    double y;
    double z;
};


double dot(const Vector& v1, const Vector& v2) {
    return v1.x*v2.x + v1.y*v2.y + v1.z*v2.z;
}


double len(const Vector& v) {
    return std::sqrt(v.x*v.x + v.y*v.y + v.z*v.z);
}


double angle(const Vector& v1, const Vector& v2) {
    double temp = dot(v1, v2) / (len(v1) * len(v2));

    // acos for values outside [-1.0, 1.0] is a complex number
    if (temp > 1.0) {
        temp = 1.0;
    }
    if (temp < -1.0) {
        temp = -1.0;
    }

    return std::acos(temp);
}


const double radToDegFactor = 180.0 / M_PI;
double radToDeg(double rad) {
    return rad * radToDegFactor;
}


enum class Orientation {
    X_POS,
    X_NEG,
    Y_POS,
    Y_NEG,
    Z_POS,
    Z_NEG
};


// using simple 3d rotation matrices:
// X_POS and X_NEG rotate by -90 and 90 around y.
// Y_POS and Y_NEG rotate by 90 and -90 around x.
// Z_POS rotates by 180 around y and Z_NEG doesn't rotate.
Vector rotate(const Vector& v, const Orientation o) {
    switch (o) {
    case Orientation::X_POS:
        return Vector{ -v.z, v.y, v.x };
    case Orientation::X_NEG:
        return Vector{ v.z, v.y, -v.x };
    case Orientation::Y_POS:
        return Vector{ v.x, v.z, -v.y };
    case Orientation::Y_NEG:
        return Vector{ v.x, -v.z, v.y };
    case Orientation::Z_POS:
        return Vector{ -v.x, v.y, -v.z };
    case Orientation::Z_NEG:
        return Vector{ v.x, v.y, v.z };
    default:
        assert(false);
        return Vector{ 0.0, 0.0, 0.0 };
    }
}


class SphericalImage {

public:

    std::vector<unsigned char> data;
    int width, height, nchannels;


    SphericalImage(std::vector<unsigned char>& data, int width, int height, int nchannels)
            : data{ data.begin(), data.end() }, width{ width }, height{ height }, nchannels{ nchannels } {
        assert(data.size() == width * height * nchannels);
    }


    int index(int x, int y) {
        assert(0 <= x && x < width);
        assert(0 <= y && y < height);
        return y * width * nchannels + x * nchannels;
    }


    // replaces the old image by a new image that discards nCols from the right
    void popCols(int nCols) {
        assert(nCols <= width);
        int newWidth = width - nCols;
        std::vector<unsigned char> newData(newWidth * height * nchannels);
        int destIdx = 0;
        for (int h = 0; h < height; ++h) {
            for (int w = 0; w < newWidth; ++w) {
                int srcIdx = index(w, h);
                for (int c = 0; c < nchannels; ++c) {
                    newData[destIdx++] = data[srcIdx++];
                }
            }
        }
        data = std::move(newData);
        width = newWidth;
    }


    void pushRows(int nRows) {
        height += nRows;
        data.resize(width * height * nchannels);
    }


    // checks the different between pixel at (x1, y1) and pixel at (x2, y2)
    // where each absolute distance of each channel is summed up
    int pixelDiff(int x1, int y1, int x2, int y2) {
        int i1 = index(x1, y1);
        int i2 = index(x2, y2);
        int diff = 0;
        for (int c = 0; c < nchannels; ++c) {
            diff += std::abs(data[i1++] - data[i2++]);
        }
        return diff;
    }


    // searches the index of the column that is the most similar to the first one
    // by going backwards starting from the final column and remembering the closest one
    int findOverlap(int range, double threshold, bool centerWeighted) {
        int closestCol = -1;
        double smallestDiff = -1.;
        for (int w = width - 1; w >= width - range; --w) {
            double diff = 0;
            for (int h = 0; h < height; ++h) {
                double currDiff = pixelDiff(0, h, w, h);
                if (centerWeighted) {
                    // we weight the pixels that are vertically in the middle higher
                    currDiff *= (double) std::min(std::abs(h - height), h) / ((double) height / 2);
                }
                diff += currDiff;
            }
            diff /= height;
            if (diff < smallestDiff || smallestDiff == -1) {
                smallestDiff = diff;
                closestCol = w;
            }
        }
        if (smallestDiff > threshold) {
            assert(false);
        }
        return closestCol;
    }


    // interpolate the pixel at the given coordinates with 3 neighbors by considering the fractional part
    // this is a simple bilinear interpolation; we do nothing crazy here
    Pixel interpolate(const double x, const double y) {

        // idx1 is upper left, idx2 is upper right, idx3 is bottom left, idx4 is bottom right
        int idx1 = index((int)x, (int)y);
        int idx2 = index(x == width - 1 ? 0 : (int)x, (int)y);
        int idx3 = index((int)x, y == height - 1 ? (int)y : (int)(y + 1));
        int idx4 = index(x == width - 1 ? 0 : (int)x, y == height - 1 ? (int)y : (int)(y + 1));

        Pixel upperLeft  { data[idx1], data[idx1 + 1], data[idx1 + 2] };
        Pixel upperRight { data[idx2], data[idx2 + 1], data[idx2 + 2] };
        Pixel bottomLeft { data[idx3], data[idx3 + 1], data[idx3 + 2] };
        Pixel bottomRight{ data[idx4], data[idx4 + 1], data[idx4 + 2] };

        double dummy = 42.0;
        double xFrac = std::modf(x, &dummy);
        double yFrac = std::modf(y, &dummy);

        double oneMinusX = 1.0 - xFrac;
        double nulMinusX = std::abs(0.0 - xFrac);
        double oneMinusY = 1.0 - yFrac;
        double nulMinusY = std::abs(0.0 - yFrac);

        // the actual interpolation by combining both rows and combining the results
        Pixel upper{
            oneMinusX * upperLeft.R + nulMinusX * upperRight.R,
            oneMinusX * upperLeft.G + nulMinusX * upperRight.G,
            oneMinusX * upperLeft.B + nulMinusX * upperRight.B,
        };
        Pixel bottom{
            oneMinusX * bottomLeft.R + nulMinusX * bottomRight.R,
            oneMinusX * bottomLeft.G + nulMinusX * bottomRight.G,
            oneMinusX * bottomLeft.B + nulMinusX * bottomRight.B,
        };
        Pixel whole{
            oneMinusY * upper.R + nulMinusY * bottom.R,
            oneMinusY * upper.G + nulMinusY * bottom.G,
            oneMinusY * upper.B + nulMinusY * bottom.B,
        };

        return whole;
    }


    // project the point v on the sphere and return the corresponding color from the array data
    // v is initially in the typical -z world coordinates and is reorientated with o before projection
    Pixel project(const Vector& v, const Orientation o, bool interpolated) {
        Vector vec = rotate(v, o);

        Vector longvec{ vec.x, 0.0,   vec.z };
        Vector latvec { vec.x, vec.y, vec.z };
        Vector forward{ 0.0,   0.0,  -1.0 };

        double longitude = radToDeg(angle(forward, longvec));
        double latitude  = radToDeg(angle(longvec, latvec));

        // when v is (0, 0, -1) and o is Y_POS or Y_NEG then |longvec| becomes 0
        // and makes the angle between longvec and latvec undefined
        if (len(longvec) == 0.0) {
            longitude = 0.0;
            latitude = 90.0;
        }

        // the angle between two vectors is positive, therefore we need this hack
        if (vec.x < 0.0) {
            longitude = -longitude;
        }
        if (vec.y < 0.0) {
            latitude = -latitude;
        }

        // the image ranges from 90 to -90 degrees vertically and from -180 to 180 degrees horizontally
        // we map (logitude, latitude) -> (x, y) of the image space and consider the array bounds
        double x = (longitude / 180) * ((double)(width - 1) / 2)  + ((double)(width - 1) / 2);
        double y = (latitude / 90)   * ((double)(height - 1) / 2) + ((double)(height - 1) / 2);

        int idx = index((int)x, (int)y);
        return Pixel{ data[idx], data[idx + 1], data[idx + 2] };
        if (interpolated) {
            return interpolate(x, y);
        }
        else {
            int idx = index((int)x, (int)y);
            return Pixel{ data[idx], data[idx + 1], data[idx + 2] };
        }
    }


    // project the spherical image on the face of the cube that is specified by o
    void projectOnFace(const Orientation o, const int size, const std::string filename) {
        const int width = size;
        const int height = size;
        std::vector<unsigned char> buf(size * size * 3);
        int i = 0;
        for (int y = 0; y < size; y++) {
            for (int x = 0; x < size; x++) {
                // we map (x, y) -> ([-1, 1], [-1, 1]) to stay in range of the face
                Vector v{(double)(x * 2) / size - 1, (double)(y * 2) / size - 1, -1.0};
                Pixel p = project(v, o, false);
                buf[i++] = p.R;
                buf[i++] = p.G;
                buf[i++] = p.B;
            }
        }
        std::cout << filename << '\n';
        std::unique_ptr<OIIO::ImageOutput> testOut = OIIO::ImageOutput::create(filename.c_str());
        if (!testOut) { return assert(false); }
        OIIO::ImageSpec testSpec(width, height, nchannels, OIIO::TypeDesc::UINT8);
        testOut->open(filename.c_str(), testSpec);
        testOut->write_image(OIIO::TypeDesc::UINT8, &buf[0]);
        testOut->close();
    }
    

    void projectOnCube(const int size, const std::string dir) {
        bfs::path path{ dir };
        if (!bfs::exists(path)) {
            bfs::create_directory(path);
        }
        projectOnFace(Orientation::X_POS, size, bfs::path{ path }.append("east.jpg").string());
        projectOnFace(Orientation::X_NEG, size, bfs::path{ path }.append("west.jpg").string());
        projectOnFace(Orientation::Y_POS, size, bfs::path{ path }.append("top.jpg").string());
        projectOnFace(Orientation::Y_NEG, size, bfs::path{ path }.append("bot.jpg").string());
        projectOnFace(Orientation::Z_POS, size, bfs::path{ path }.append("south.jpg").string());
        projectOnFace(Orientation::Z_NEG, size, bfs::path{ path }.append("north.jpg").string());
    }
};


int main(int argc, char* argv[]) {
    std::string inFile(argv[1]);

    std::cout << "input : " << inFile << '\n';

    // Read file.
    std::unique_ptr<OIIO::ImageInput> in = OIIO::ImageInput::open(inFile.c_str());
    if (!in) { return EXIT_FAILURE; }

    const OIIO::ImageSpec& inSpec = in->spec();
    const int inWidth = inSpec.width;
    const int inHeight = inSpec.height;
    const int nchannels = inSpec.nchannels;

    std::cout << "resolution " << inWidth << "x" << inHeight << '\n';

    std::vector<unsigned char> inBuf(inWidth * inHeight * nchannels);
    in->read_image(OIIO::TypeDesc::UINT8, &inBuf[0]);
    in->close();

    // Do the stuff.
    SphericalImage simage(inBuf, inWidth, inHeight, nchannels);
    int chopAt = simage.findOverlap(simage.width / 2, 9., true);
    if (chopAt == -1) { return EXIT_FAILURE; }
    int chopN = simage.width - chopAt;
    if ((simage.width - chopN) % 2 == 1) { ++chopN; }
    simage.popCols(chopN);
    simage.pushRows(simage.width / 2 - simage.height);

    const int outWidth = simage.width;
    const int outHeight = simage.height;

    std::cout << "new resolution " << outWidth << "x" << outHeight << '\n';

    // Write projection.
    simage.projectOnCube(simage.height / 2, 
            bfs::path(inFile).parent_path().append("Cubify_out").string());

    return EXIT_SUCCESS;
}

Example input:

enter image description here

Example output (stitched together to a single image):

enter image description here

I wonder if there is anything strange in the code I've written. I'm especially interested in readability of my code. I feel like I'm writing hard-to-understand-code but I'm not sure how to simplify it or improve documentation. I'm using BOOST for IO, OpenImageIO for image-IO and nothing else.

Previous version: Program for chopping overlapping image areas and filling up to a specific ratio

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Make sure you add #include for everything you use directly

Your code happens to compile without errors because some other header file #includes the necessary header files for you to be able to use functions like std::acos() (from <cmath>), std::abs() (from <cstdlib>), std::min() (from <algorithm>), std::unique_ptr (from <memory>), and so on, you should not rely on this. Go through all the functions from the standard library you use, and ensure the corresponding #include is present.

Consider making dot(), len(), etc. member functions of Vector

These functions clearly only work on instances of Vector, so it makes sense to make them member functions of Vector. This avoids polluting the global namespace. Do this for all the functions that operate purely on vectors: dot(), len(), angle(), rotate().

Use radians everywhere

I have found that a major source of bugs is using degrees, when all the standard library functions work in radians. It is easy to make a mistake in converting between them, and it can also result in less efficient code. The only place I would use degrees in code is when displaying angles or reading angles as input. So for example:

double longitude = forward.angle(longvec);
...
double x = (longitude / M_PI) * (width - 1) / 2.0 + (width - 1) / 2.0;

Make helper functions private

Everything in class SphericalImage is public, however only a few functions should actually part of the public API. Right away, you can make index(), pixelDiff(), interpolate() and project() private, since they are only used internally by other member functions.

Then there is "the stuff" that you do in main(). Can you make a single member function that performs the task of finding the overlap, popping columns and pushing rows, with a clear and descriptive name? Then, findOverlap(), popCols() and pushRows() can also be hidden.

Use size_t for sizes and counts

Use size_t for variables such as width, height, nchannels, srcIdx and so on. This type is guaranteed to be able to hold integers large enough for everything that can be held in memory. Furthermore, it is unsigned, so you don't have to worry about negative numbers. Lastly, it will avoid compiler warnings about comparing integers of different signedness in expressings such as data.size() == width * height * nchannels.

Optimize popCols()

When you are removing columns, you first allocate space for the new image, build the new image, and then copy it back into data. But this is unnecessary, you can update data in-place:

void popCols(size_t nCols) {
    assert(nCols <= width);
    size_t newWidth = width - nCols;
    size_t destIdx = 0;

    for (int h = 0; h < height; ++h) {
        for (int w = 0; w < newWidth; ++w) {
            size_t srcIdx = index(w, h);
            for (int c = 0; c < nchannels; ++c) {
                data[destIdx++] = data[srcIdx++];
            }
        }
    }

    width = newWidth;
    data.resize(width * height * nchannels);
}

Don't assert(false)

The whole point of the function assert() is that you provide it with a condition to check, and if the condition is false, it will print an error message that contains the condition. So just write:

assert(smallestDiff > threshold);

This way, when the assertion triggers, a more helpful error message is displayed.

Avoid unnecessary casts

C and C++ will implicitly cast variables for you in some cases. While that is sometimes a problem, it usually avoids you having to write explicit casts. For example, when calling index(), you don't need to explictly cast double values to an integer type. For example, you can just write:

Pixel interpolate(const double x, const double y) {
    size_t idx1 = index(x, y);
    size_t idx2 = index(x == width - 1 ? 0 : x, y);
    size_t idx3 = index(x, y == height - 1 ? y : y + 1);
    size_t idx4 = index(x == width - 1 ? 0 : x, y == height - 1 ? y : y + 1);
    ...

Also, when performing arithmetic operations involving constants, you can make the constants doubles, and then they can automatically cause integers to be promoted to double, like so:

Vector v{x * 2.0 / size - 1, y * 2.0 / size - 1, -1.0};

Split responsibilities

The function projectOnFace() not only performs an image projection, it also writes out the image. In general, it is best to split up such a function in two parts, one that does the projection, and another that writes it to a file. Consider that you might want to do something else with the projects face before writing it out, or perhaps you don't want to write it to a file, but rather display it on the screen. Ideally, projectOnFace() returns an image object of some kind. Since you are using OpenImageIO, consider using OIIO::ImageBuf for this.

The function projectOnCube() has similar issues, although it doesn't do any projection of its own. Since this is the one called from main() to write out the images, maybe it should just call projectOnFace() six times to get image buffers, and then it write those to disk itself. The function should be renamed to something more descriptive, like writeCubeFaces().

Only use assert() to catch programming errors

You should only use assert() to check for possible programming errors, not use them as a generic error handling function for things that can go wrong even if the program itself is written correctly. Take for example:

std::unique_ptr<OIIO::ImageOutput> testOut = ...;
if (!testOut) { return assert(false); }`

Apart from the fact that the last like should just have been assert(testOut), the issue here is that not being able to create a file is not a programming error, but rather something like the program being called inside a directory that is not writable, or having run out of disk space, and so on. The user of your program is not helped by a core dump and the message "assertion 'false' is false". Even worse, assert() is a macro that is typically disabled in release builds, so then there would be no error message at all.

The manual of OpenImageIO shows the correct way to handle errors:

#include <stdexcept>
...
std::unique_ptr<OIIO::ImageOutput> testOut = ...;
if (!testOut) {
    std::cerr << "Could not create an ImageOutput for "
              << filename << ", error = "
              << OpenImageIO::geterror() << "\n";
    return;
}

Now the user gets a detailed error message, which should explain why it couldn't write the file. The user then hopefully has enough information to correct the situation. However, just returning from the function makes an error condition indistinguishable from success for the caller of this function. Therefore, I would replace the return statement with:

throw std::runtime_error("Error creating output image");

Check for all possible errors

Just checking whether a file could be opened or created is not enough. You also have to check whether the whole file was successfully read, or if all image data has been fully written to disk. So check the return value of read_image(), write_image() and close(), ensure you print a helpful error message in each case, and throw an exception if necessary to signal any callers of an error.

Consider whether it is necessary to create a class SphericalImage at all

The only thing you can do with a class SphericalImage is to project an image onto cube faces. The few functions it has to manipulate the stored image are just there to help with the projection. Instead of using a class, perhaps it is better to just have a single function that takes an image, and splits it into six cube faces. It could look like this:

std::array<OIIO::ImageBuf, 6> projectOnCube(const OIIO:ImageBuf &image);

Basically, you give it an ImageBuf, and you get six ImageBufs back. You can add additional parameters for your algorithm, like threshold, centerWeighted, possibly with default values. Your main() should then be able to look like:

int main(int argc, char* argv[]) {
    if (argc <= 1) {
        // report usage error and exit
    }

    OIIO::ImageBuf image(argv[1]);

    if (!image.read(0, 0, true, OIIO::TypeDesc::UINT8)) {
        // report read error and exit
    }

    auto faces = projectOnCube(image);

    const char *filenames[6] = {"east.jpg", "west.jpg", ...};

    for (size_t i = 0; i < 6; ++i) {
        if (!faces[i].write(filenames[i])) {
            // report write error and exit
        }
    }
}

Note that this doesn't mean you have to put all the functionality inside that function, you can still have helper functions. These should then be made static.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you much for all the tipps! \$\endgroup\$
    – akuzminykh
    Commented Sep 6, 2020 at 17:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.