Unify channel index type in nth_channel_view and image_view::num_channels

[mloskot]

Actual behavior

• image_view::num_channels() returns size_t
• nth_channel_view(View, int n) expects int for index

Apart from signed and unsigned mix-up causing the annoying compilation warnings, if no rationale of the mix is provided, it's a sloppy design of the interface.

Expected behavior

Use of common type for indexing of image_view channels.

C++ Minimal Working Example

#include <boost/gil.hpp>
namespace gil = boost::gil;
int main
{
    gil::rgb8_image_t img;
    auto v = gil::view(img);
    for (std::size_t i = 0; i < v.num_channels(); i++)
        auto _ = nth_channel_view(v, i);
}

[sdebionne]

A rationale could be that int is used almost everywhere to identify the channel. I think it helps writing compile-time recursion for per-channel operation but if it' not the case we might want to extend "unification" to color base.

[mloskot]

It has been a bit of a mess. If we think of the image in terms of 3D: X, Y and Z, then currently we define X and Y as ptrdiff_t and the X as int or size_t.

I'm recalling https://github.com/boostorg/gil/pull/305 where some of us, @stefanseefeld, were preferring size_t to express the image-in-memory dimensions. I've been cultivating a more conservative approach to stick to ptrdiff_t as it is what the original authors of GIL (folks at Adobe) decided to use. Looks like it may be a good to review the current practice and decide on the ptrdiff_t vs size_t once for all :)

My suggestion is to

• use ptrdiff_t for sizes of abstract, virtual and physical sizes of image/band dimensions
• use size_t for sizes in memory
• if image dimension is used as size in memory, then static_cast<size_t> it, and vice versa.

Please, have your say @sdebionne @striezel @stefanseefeld @lpranam @chhenning @simmplecoder ...

[striezel]

A rationale could be that int is used almost everywhere to identify the channel.

Everywhere - except where it is not used. While nth_channel_view still uses int (unless #659 is approved, which would change that to std::size_t), many places that are using nth_channel_view are using std::size_t as channel index already. For example:

• image_processing/convolve.hpp
• image_processing/histogram_matching.hpp
• image_processing/morphology.hpp
• image_processing/threshold.hpp

Seems to me like most of the image processing stuff agreed that channels should be a std::size_t.

It has been a bit of a mess.

Yeah, seems like it, but we can fix that - one PR at a time.

I'm recalling #305 where some of us, @stefanseefeld, were preferring size_t to express the image-in-memory dimensions. I've been cultivating a more conservative approach to stick to ptrdiff_t as it is what the original authors of GIL (folks at Adobe) decided to use. Looks like it may be a good to review the current practice and decide on the ptrdiff_t vs size_t once for all :)

Basically I am with Stefan Seefeld on this one. std::ptrdiff_t allows for negative values, but there is no negative channel index. That's why I think std::size_t (or any unsigned type, for that matter) is the better choice here, because it conveys the notion that channel indices start at zero and cannot be negative. As a side effect, any compiler warning about signed / unsigned mixup with regards to channels would then be an indication for potential trouble. After all, passing a possibly negative int or std::ptrdiff_t as channel index is most likely not what people want to do anyway.

[sdebionne]

Everywhere - except where it is not used.

Sorry, I meant "everywhere" in core. Image processing is a recent addition to the library (that I dont know much about TBH).

I think there is a consensus that unsigned integers are error prone and that using unsigned type for size_t was a mistake.

OpenMP does not allow a for loop index to be unsigned (until recent iteration of the standard).

[striezel]

I was not really expecting that people do arithmetic operations on the number of channels, but if that is a concern, then some signed type may indeed be better. In that case it should probably be ptrdiff_t instead, as was suggested earlier (https://github.com/boostorg/gil/issues/373#issuecomment-1118842042). I just do not fancy the name ptrdiff_t for a type that is used to represent the number of channels. The name ptrdiff_t may invoke the idea that this is dealing with pointers, which it is not. So maybe there should be a name like boost::gil::channel_index_t or boost::gil::index_t that then is aliased to ptrdiff_t.

[mloskot]

@striezel

maybe there should be a name like boost::gil::channel_index_t or boost::gil::index_t that then is aliased to ptrdiff_t

I don't like the ptrdiff_t name particularly. I don't say the name is clear across all contexts in GIL. I don't feel comfortable with s/ptrdiff_t/size_t/g across GIL as such seemingly trivial change may turn not as benign as it seems. I assume Lubomir Bourdev and Hailin Jin had reasons to choose ptrdiff_t which, unfortunately, remain undocumented.

I do like the idea of readable and self-descriptive names, so I like the idea of aliasing ptrdiff_t as boost::gil::index_t and use it in all indexing situations in GIL (channel, pixel, x, y, etc.), and fix any existing mismatch to match index_t. Worth it to you too @sdebionne ?