Replace usage of std::vector as storage of pixel data with image::Image #1282

p12tic · 2022-10-10T22:48:13Z

This PR is a continuation of #1257 in an attempt to clean up image interfaces.

Currently the codebase has roughly 2 ways to represent an image: image::Image<T> class and std::vector<T> plus width and height (there's also StaticVector<T> but it has std::vector<T> underneath). This is inefficient because this requires twice as many functions to cover the basic image functionality.

This PR replaces usages of std::vector<T> with image::Image<T> at least in cases where readImage() and writeImage() functions are used. This way it was possible to remove the set of functions that operates on std::vector<T>.

writeImage() now accepts configuration data via ImageWriteOptions class. This was required because the function needed even more parameters to support all needed functionality and using class removed this requirement. ImageWriteOptions uses builder pattern to supply options, so it's convenient to use, as common configuration cases can be declared as member functions.

fabiencastan · 2022-10-11T09:15:40Z

src/aliceVision/depthMap/DepthSimMap.cpp

@@ -395,7 +395,8 @@ void DepthSimMap::saveToImage(const std::string& filename, float simThr) const
        metadata.push_back(oiio::ParamValue("AliceVision:storageDataType",
                                            EStorageDataType_enumToString(image::EStorageDataType::Float)));
        image::writeImage(filename, colorBuffer,
-                          image::OutputFileColorSpace(image::EImageColorSpace::NO_CONVERSION), metadata);
+                          image::ImageWriteOptions().toColorSpace(image::EImageColorSpace::LINEAR),


Technical maps like depth, sim, etc have no notion of colorspace. In this case, we usually use NO_CONVERSION to make it explicit.

Previously OutputFileColorSpace was setting both from and to to LINEAR if single-parameter constructor was used with NO_CONVERSION. This code preserves existing behavior.

See 4668fe1#diff-dc4ab367f585ae18a7872e28d2c057e6388eb99b2ff928a3a5295ffc0df14425L66

src/software/pipeline/main_panoramaInit.cpp

fabiencastan · 2022-10-11T09:25:47Z

src/software/export/main_exportMVE2.cpp

@@ -142,7 +142,8 @@ bool exportToMVE2Format(
        // Undistort and save the image
        readImage(srcImage, image, image::EImageColorSpace::NO_CONVERSION);


It's not something introduced in your PR.
But I do not understand why we have so much places where we explicitly specify a target colorspace for reading or writing images. The AUTO should be fine for (almost?) all cases.
It's probably for historical reason, before we introduced the AUTO...
In this particular case, it should clearly be AUTO else it can be completely wrong, as we read any format in input and we output in PNG. So it cannot be statically defined.

fabiencastan · 2022-10-11T09:33:41Z

Thinking again about this colorspace question. It would be great to make a cleanup.

All read should be LINEAR, except in featureExtraction and LdrToHdr (both needs to be in SRGB for now).
All write should be AUTO, except nmodmap (NO_CONVERSION is needed to export some integer values in PNG). All other technical maps are in float and exported in EXR (so linear by default, which means float-to-float without conversion).

src/aliceVision/image/io.hpp

src/aliceVision/depthMap/DepthSimMap.cpp

p12tic · 2022-10-11T18:04:18Z

Thinking again about this colorspace question. It would be great to make a cleanup.

All read should be LINEAR, except in featureExtraction and LdrToHdr (both needs to be in SRGB for now).

All write should be AUTO, except nmodmap (NO_CONVERSION is needed to export some integer values in PNG). All other technical maps are in float and exported in EXR (so linear by default, which means float-to-float without conversion).

@fabiencastan I fully agree. I didn't want to make any changes to behavior in this PR because it's already large and figuring out what exactly broke would be a challenge. It makes sense to implement these recommendation in a next PR.

p12tic · 2022-10-11T18:04:39Z

All comments have been addressed.

p12tic · 2022-10-31T16:35:28Z

@fabiencastan Just a friendly ping :-) I have one resurrected test that depends on this PR being landed.

src/aliceVision/fuseCut/Fuser.cpp

fabiencastan · 2022-11-10T14:13:31Z

Thanks @p12tic for this useful work!

fabiencastan added this to the 2.5.0 milestone Oct 11, 2022