Fernando, Wow! You (and other reviewers) spent quite a lot of time reviewing GIL. We really appreciate that!

(*) ValueType concept:

I don't quite get. It seems that its purpose is to draw a distinction between references/pointers and values. That's why there is a ChannelConcept but also a ChannelValueConcept. But it turns that, unless I am too sleep (is 2:20am already), that pointers are "ValueTypes" as far as the concept specification goes. And references are not value types only becasue they are not default constructible. But is that the crux of the distinction? that value types can be default constructed? If so, it must be better documented.

Yes, values have a default constructor and a proper copy constructor. References do not. As stated in the concept definitions.

But even then: why do you need different concepts to draw the distinction between objects (values in this case) and references/pointers to it? I found that quite uncommon and surely GIL is not the only library that uses references as well as values.

Our concept definitions are driven by the minimum requirements needed when the concepts are used in algorithms. For example, consider an algorithm that takes two Pixels and determines, say, if they are equal: template <typename Pixel1, typename Pixel2> bool equal_pixels(const Pixel1& p1, const Pixel2& p2) { ... } In this case, all we need from the arguments is to be able to get the values of their channels. We don't need to be able to default-construct a pixel. This means that Pixel1 and Pixel2 should only model HeterogeneousPixelConcept. This allows us to pass not just pixel values, but also planar references as arguments: bool eq=equal_pixels(planar_view[4], rgb8_pixel_t(3,4,5)); On the other hand, if we have an algorithm that requires us to construct and return a new pixel, then we need more than getting/setting the values of the channels. In that case we would need PixelValueConcept.

In the way it is specified, a model of this concept must be a class specifically designed for that. Yet that is unnecesary. External adaptation (see the BGL) can be used to allow "classic" point classes to be valid models. This comment goes for many of the other concepts.

Yes, we are aware of this and are torn between the desire to allow full external adaptation and to make the code that uses the concept simpler and shorter. "p.x" is much nicer, in my opinion, than "p.x()" and especially "get_x(p)". The latter becomes especially unreadable in longer expressions. Unfortunately the most verbose version is also the one that most allows external adaptation. To ease external adaptation we would have to move everything from a method to a global function taking the object. We have done it for some of the methods, but are hesitating to do it for the rest. Not to mention some operations simply cannot be moved outside the objects, so full external adaptation is compromised anyway... Suggestions/opinions/votes are most welcome.

Also, is there any real compelling reason to allow each dimension to have its own coordinate type? If not then I would say this is being over-specified.

This comes from the fact that every dimension must have a different iterator. The type of the dimension is the difference_type of its iterator.

I believe x and y (and red,green,blue,etc) shouldn't be member variables but "properties", that is, x() and y() (etc) That allows a model to have an array as the internal representation (I know you _can_ have a property interface that uses a named-like syntax as if it were a member variable, but I relly don't like that at all)

This is exactly the same issue regarding external adaptability, as discussed above. Note also that "red", "green", etc. are not part of the pixel concept. Therefore you should not use the channel names directly in generic code. We could provide global functions like get_red(p), set_red(p, x), etc... As of now, you can use p.channel<0>(), or p.semantic_channel<0>(), or, for homogeneous pixels, p[0]

(*) Why is ColorSpaceTypeConcept spelled with "Type" in it?

For no good reason. We will change it. Thanks.

(*) ChannelsCompatibleConcept:

Why is this a concept? Isn't it just a metafunction on channels? (the same goes for the othe "compatible" concepts)

Because it is often the case that algorithms require the types of the two channels they take be compatible. So a nice way to say this is that the channels must model the ChannelsCompatibleConcept. And yes, for each concept and each type, one could make a metafunction determining whether the type satisfies the concept. We have one in cases we need it, channels_are_compatible in this case.

(*) ColorSpaceConcept:

Why isn't it spelled "ColorFormatConcept" instead?

That is a good suggestion. We will think about it some more and may very well change the name accordingly. "Color space" is a well-defined term, but we are also mixing the ordering of the channels in there, so calling it a color_format, or pixel_format might make sense.

(*) Pixel concept:

But then I would rename PixelConcept as ColorConcept.

Well, here the best name is not so evident. As you know, the term Pixel is very widely used in graphics. If we change this to Color, then there will be a similar (justified) criticism that our image library has no notion of a Pixel. People think of images as 2D grids of pixels, not of colors...

(*) color_base<>:

It is introduced as the class that represents the "color-space-specific aspect of a pixel". I have no idea what that means :) I can see that is the base class for the pixel class.

Why is it called "color_base" instead of "pixel_base"? Because it only captures the "color-space-specific" apsect of a pixel? That sentence implies there is another aspect to it, but I can't see it in the definition of the pixel class.

You can classify operations on pixels, pixel references, and pixel iterators, into two categories - ones that depend on the specific color format and ones that are color-format agnostic and treat all the elements (channels) the same way. Examples of the first are return the first semantic channel, or color convert. Examples of the second are assignment and equality. Color base contains the "color-essence" of the pixel/reference/iterator. The color-format-agnostic operations are implemented outside, in pixel_algorithm.hpp. This is briefly mentioned in the design guide - grep for "agnostic"

(*) pixel<> and planar_ref<>:

Why are planar pixels called planar? what is planar about them, or, more precisely, what isn't planar about any other pixel type?

We would rather keep the name "planar" than change it to "layered". This is because "planar" is a well-established terminology for the image format in which color channels are in separate images (planes).

I know what you mean by that, but I found the term totally confusing. I would have called them "layered" pixel.

Also, I wouldn't call "pixel" just pixel given that it represents and interleaved pixel, and there are pixels which are not interleaved, so I would have called them "interleaved_pixel".

"pixel" is the name for the color value of a pixel, whether or not it came from interleaved or planar image. It just so happens that a C reference to this class is also used to represent a reference to a pixel inside an interleaved image. Pixels have one value type "pixel" but may have different reference types - such as "pixel&" and "planar_ref". The value type is only concerned about representing the color, whereas references are concerned with representing the color at a specific location in memory.

What does the "_ref" suffix means? A reference? OK, to what?

To a pixel. We didn't want to add this to the type and thus make it longer. Similarly "_ptr" is an iterator to a pixel.

I know is to the channels.. but why does that have to be put in the name?

planar_ref is a pixel _reference_ whose channels may be located at disjoint places in memory, such as in a planar image.

Can't interleaved pixels provide references to the channels? Of course they can.

We are talking about references to pixels here, not to channels. And yes, interleaved pixels can have references, but usually the built-in one is used by default. Here are four common pixel reference types: "rgb8_ref_t" is "pixel<bits8,rgb_t>&" "rgb8c_ref_t" is "const pixel<bits8,rgb_t>&" "rgb8_planar_ref_t" is "planar_ptr<bits8&, rgb_t>" "rgb8c_planar_ref_t" is "planar_ptr<const bits8&, rgb_t>" They all have the same value type: "rgb8_pixel_t", or "pixel<bits8,rgb_t>"

I can actually go on... but I have to work too :) and I rather post half a review than none.

Thanks for your valuable suggestions. We would like to hear the rest of your comments whenever you find the time. It doesn't have to be part of a formal review, or even in the form of a posted message on the boost list. You could just email them if you prefer, or we could talk.

I found one annoying issue with the documentation that I think is a mistake: It mixes Concept Checking with Concepts. For example:

I read in the design guide that a model for Point2DConcept must have an integral constant named num_dimensions. OK, how does a model provide that? as a nested type? as an "associated" type provided by a particular traits class? Can't tell.

It is a nested constant.

It also requires the type to have an "axis_value()" template function. Is that a member function of the model? A free function in the GIL namespace? Can't tell again.

A nested constant. Some properties are explicitly stated that must be provided in associated traits class, pixel_traits and pixel_iterator_traits. Unless explicitly specified in the design guide, assume nested constants. I know the same argument about adaptability applies here as well. We could have a traits class associated with every GIL concept and move types and constants there. But then refering to the type/constant becomes much longer and uglier expression. This is why, as of now, we use nested typedefs, unless the model can be a type that cannot have nested typedefs. The three cases are: Channels (can be built-in types). So we have channel_traits Pixel (built-in C reference is a model). So we have pixel_traits PixelIterator (a raw pointer can model it). So we have pixel_iterator_traits, which inherit from std::iterator_traits.

So I look into the doxygen docs, and I found that I can't tell either, but I can tell something: PointNDConcept is a concept "checking" class. That's not the same as a Concept even if it comes close. For example, the "public attribute" "P point" listed by doxygen is specific to the "checking" class. It's not part of the Concept.

Yes. What happened there is that we have classes that enforce concepts via boost::concept_check. We Doxygen-tag those classes to belong to the associated concept, which made Doxygen provide "Public Member Functions" and "Public Attributes" section, which could be quite confusing. We should see how to suppress these from the Doxygen documentation.

The same problem happens with all of the other "concepts". Without looking into the code you can't tell how the requirements must be fulfilled by models. traits_classes? nested types? etc

I am still not sure how exactly to represent this using the new concept syntax. We will look into this. Right now to find this you need to read the associated design guide section. Unless it states there is an associated traits class, the types must be nested.

The discussion of compatible color spaces in the design guide mentions that two color spaces (which as I said should be called color formats) are compatible if the have the same base. Maybe is me, but I found that to be an odd way to say that color-space models form inheritance diagrams. If you just say that, it follows by LSP when are they compatible.

No they don't have to use inheritance to be compatible. They just need to have the same base, i.e. boost::is_same<typename CS1::base, typename CS2::base>

Just an example of a problem found through all of the documentation:

It's next to imposible to understand what the "value_type" of a HeterogeneousPixelConcept is. It's documented as: HeterogeneousPixelValueConcept<pixel_value_type>, but: HeterogeneousPixelValueConcept is a refinement of HeterogeneousPixelConcept so the definition is recursive.

Yes indeed. But there are many other places where the definition is recursive. For example, in the definition of a ViewConcept there are types of derived views, which also must model ViewConcept. A pixel iterator must have in its traits a "const_t" which must also model (an immutable) PixelIteratorConcept. In the concept check classes, we simply don't follow the circular branches. Suggestions on how to resolve this are welcome.

pixel_value_type is undefined. Typo - should be "value_type". Sorry.

I could only really uderstood GIL from the source code.

IMO, the documentation needs some serious work.

(*) The following paragraph in the design guide is unparsable to me:

"Interleaved pixels have the built-in reference (pixel<T,C>&)"

have? where? what is *the* built-in reference? (ok, the parethesis makes it just a little but more clear)

"Planar pixels, however, have channels that are not together in memory and therefore a proxy class is required to represent a planar reference."

OK, until I read "a planar reference"... what's that?

"A reference to a planar pixel is defined as a set of references to each corresponding channel"

If 'reference' in that sentence means a C++ reference, then, no, is not defined that way. But I actually know you are not talking about a C++ reference, so, what do you mean by "reference to a planar pixel"? Well, I do know the answer, because eventually I came around to understand your use of the term reference. But I think you should decide on a new term for that to keep it away from a C++ reference and avoid confusion.

Sorry about the confusing paragraph... There are really two interpretations of the term "reference". In a narrow context, reference could mean the built-in C reference to a type T represented as "T&" But in a broader context, "reference" is the return type of the dereference operator of an iterator. As you know, it is a type specified in the iterator traits. This is the interpretation that I used when I say "reference". For the narrower interpretation I use "built-in reference". I am hesitant to come up with a new name here, because STL already uses this general notion of a "reference". Thanks again for your detailed review! Lubomir

"Lubomir Bourdev" <lbourdev@adobe.com> escribió en el mensaje news:B55F4112A7B48C44AF51E442990015C0017CE639@namail1.corp.adobe.com...

Fernando,

Wow! You (and other reviewers) spent quite a lot of time reviewing GIL. We really appreciate that!

You're welcome :) And BTW, I forgot to thank you and Adobe for GIL!

...

(*) ValueType concept:

I don't quite get. It seems that its purpose is to draw a distinction between references/pointers and values. That's why there is a ChannelConcept but also a ChannelValueConcept. But it turns that, unless I am too sleep (is 2:20am already), that pointers are "ValueTypes" as far as the concept specification goes. And references are not value types only becasue they are not default constructible. But is that the crux of the distinction? that value types can be default constructed? If so, it must be better documented.

Yes, values have a default constructor and a proper copy constructor. References do not. As stated in the concept definitions.

Just for the record, references do have copy constructors. Is just that they are shallow. But OK, I can see why a distinction between values and references/pointers at the concept level is usefull.

...

But even then: why do you need different concepts to draw the distinction between objects (values in this case) and references/pointers to it? I found that quite uncommon and surely GIL is not the only library that uses references as well as values.

Our concept definitions are driven by the minimum requirements needed when the concepts are used in algorithms. For example, consider an algorithm that takes two Pixels and determines, say, if they are equal:

template <typename Pixel1, typename Pixel2> bool equal_pixels(const Pixel1& p1, const Pixel2& p2) { ... }

In this case, all we need from the arguments is to be able to get the values of their channels. We don't need to be able to default-construct a pixel. This means that Pixel1 and Pixel2 should only model HeterogeneousPixelConcept. This allows us to pass not just pixel values, but also planar references as arguments:

bool eq=equal_pixels(planar_view[4], rgb8_pixel_t(3,4,5));

On the other hand, if we have an algorithm that requires us to construct and return a new pixel, then we need more than getting/setting the values of the channels. In that case we would need PixelValueConcept.

So a ValueType and the related concepts are there to add the additional requirement of constructing these values, were needed. OK, I can see how those additional requirements need not be enforced in all types. I guess the docs could be more clear on that.

...

In the way it is specified, a model of this concept must be a class specifically designed for that. Yet that is unnecesary. External adaptation (see the BGL) can be used to allow "classic" point classes to be valid models. This comment goes for many of the other concepts.

Yes, we are aware of this and are torn between the desire to allow full external adaptation and to make the code that uses the concept simpler and shorter.

"p.x" is much nicer, in my opinion, than "p.x()" and especially "get_x(p)". The latter becomes especially unreadable in longer expressions. Unfortunately the most verbose version is also the one that most allows external adaptation. To ease external adaptation we would have to move everything from a method to a global function taking the object. We have done it for some of the methods, but are hesitating to do it for the rest. Not to mention some operations simply cannot be moved outside the objects, so full external adaptation is compromised anyway... Suggestions/opinions/votes are most welcome.

OK. I guess I'd like to see external adaptation in the particular case of the Point concept because Point classes are ubiquotous, yet none of them would fit the concept the way it is.

...

Also, is there any real compelling reason to allow each dimension to have its own coordinate type? If not then I would say this is being over-specified.

This comes from the fact that every dimension must have a different iterator. The type of the dimension is the difference_type of its iterator.

OK.

...

I believe x and y (and red,green,blue,etc) shouldn't be member variables but "properties", that is, x() and y() (etc) That allows a model to have an array as the internal representation (I know you _can_ have a property interface that uses a named-like syntax as if it were a member variable, but I relly don't like that at all)

This is exactly the same issue regarding external adaptability, as discussed above.

Note also that "red", "green", etc. are not part of the pixel concept. Therefore you should not use the channel names directly in generic code.

Ha OK. But x,y are part of the Point concept rigtht?

We could provide global functions like get_red(p), set_red(p, x), etc... As of now, you can use p.channel<0>(), or p.semantic_channel<0>(), or, for homogeneous pixels, p[0]

OK. Anyway, Joel explained how Fusion would solve all these.

...

(*) ChannelsCompatibleConcept:

Why is this a concept? Isn't it just a metafunction on channels? (the same goes for the othe "compatible" concepts)

Because it is often the case that algorithms require the types of the two channels they take be compatible. So a nice way to say this is that the channels must model the ChannelsCompatibleConcept. And yes, for each concept and each type, one could make a metafunction determining whether the type satisfies the concept. We have one in cases we need it, channels_are_compatible in this case.

Well, OK, I can see how the concept makes the documentation easier, and even prescribes a proper check.

...

(*) ColorSpaceConcept:

Why isn't it spelled "ColorFormatConcept" instead?

That is a good suggestion. We will think about it some more and may very well change the name accordingly.

Cool. I like it that way better.

...

(*) Pixel concept:

But then I would rename PixelConcept as ColorConcept.

Well, here the best name is not so evident. As you know, the term Pixel is very widely used in graphics. If we change this to Color, then there will be a similar (justified) criticism that our image library has no notion of a Pixel. People think of images as 2D grids of pixels, not of colors...

Well, the imaging libraries that come to mind now all call that color, not pixel. So does the rendering libraries, which share this particular element (they also manipulate pixels) So I would be surprised if the opposite argument were raised.

...

(*) color_base<>:

It is introduced as the class that represents the "color-space-specific aspect of a pixel". I have no idea what that means :) I can see that is the base class for the pixel class.

Why is it called "color_base" instead of "pixel_base"? Because it only captures the "color-space-specific" apsect of a pixel? That sentence implies there is another aspect to it, but I can't see it in the definition of the pixel class.

You can classify operations on pixels, pixel references, and pixel iterators,

<<<< nit-picking on <<<< I can think of operations on pixels. A pixel reference, OTOH, is just an "alias" for the pixel, so I don't think the operation is on the pixel "reference" but on the "pixel" itself, which just happens to be denoted by a reference variable. Similarly, the only operation on pixel iterators I can think of is traversal. Any operation on a dereferenced pixel iterator is on the pixel itslef, not the iterator. <<<< nit-picking off <<<<

into two categories - ones that depend on the specific color format and ones that are color-format agnostic and treat all the elements (channels) the same way.

OK.

Examples of the first are return the first semantic channel, or color convert. Examples of the second are assignment and equality.

OK. I get it, you are drawing a distinction between semantics and structure.

Color base contains the "color-essence" of the pixel/reference/iterator.

The color semantics. OK.

The color-format-agnostic operations are implemented outside, in pixel_algorithm.hpp. This is briefly mentioned in the design guide - grep for "agnostic"

The pixel structure. OK.

...

(*) pixel<> and planar_ref<>:

Why are planar pixels called planar? what is planar about them, or, more precisely, what isn't planar about any other pixel type?

We would rather keep the name "planar" than change it to "layered". This is because "planar" is a well-established terminology for the image format in which color channels are in separate images (planes).

I know that images store color components in planes, but I can't relate that to a "planar" pixel. :) But then my main line of work is geometry, not graphics or imaging, so the word planar rings too strong a bell to me.

...

I know what you mean by that, but I found the term totally confusing. I would have called them "layered" pixel.

Also, I wouldn't call "pixel" just pixel given that it represents and interleaved pixel, and there are pixels which are not interleaved, so I would have called them "interleaved_pixel".

"pixel" is the name for the color value of a pixel, whether or not it came from interleaved or planar image. It just so happens that a C reference to this class is also used to represent a reference to a pixel inside an interleaved image.

Pixels have one value type "pixel" but may have different reference types - such as "pixel&" and "planar_ref". The value type is only concerned about representing the color, whereas references are concerned with representing the color at a specific location in memory.

...

What does the "_ref" suffix means? A reference? OK, to what?

To a pixel. We didn't want to add this to the type and thus make it longer. Similarly "_ptr" is an iterator to a pixel.

...

I know is to the channels.. but why does that have to be put in the name?

planar_ref is a pixel _reference_ whose channels may be located at disjoint places in memory, such as in a planar image.

Ha OK. Well, this is what I understood based on your explanations: ----------------------------------------------------------- Images have pixels, and pixels have values, namely, color. A pixel can be interleaved or planar. If it is interleaved, the channels are stored contiguosly in memory so there can be a C++ object *directly* representing it. If is is planar, the channels, being in a separate planes, are not stored contiguously in memory so there cannot be a C++ object *directly* representing it. OTOH, both interleaved and planar pixels have a value, its color, which has some fixed structure which is independent of the disposition of the pixel channels in the image buffer. Hence, there can be a C++ object represeting the color of a pixel, whether interleaved or planar. A natural requirement of an imaging framework is the ability to address pixels in an image. For interleaved images, since its pixels can be directly represented as C++ objects, a C++ pointer can be used for that. However, for planar images, a *single* C++ pointer cannot be used to address the *entire* planar pixel, so the addressing itself must be carried out by an object (specifically, that wraps pointers to each channel). Thus, there is the concept of a pixel pointer, or "iterator" to avoid overloading the term pointer. This concept is modeled by a C++ pointer if the pixel is interleaved (because in that case the pixel is itself a C++ object), or by a wrapper object if the pixel is planar, because in that case the pixel is not a *single* C++ object. Such a wrapper simultaneously points to all of the planar pixel channels. A pointer (or iterator) can be dereferenced, giving value access to the object it points to (through a reference). A pointer to an interleaved pixel, which is a C++ pointer, when dereferenced gives access to *the* image pixel, as physically stored in the buffer. Such a pixel can be an rvalue or an lvalue (it is after all a C++ object) so you to use a pixel pointer to read but also to mutate the color of a pixel right there in the image buffer. However, a pointer to a planar pixel, not being a C++ pointer, cannot offer both rvalue and lvalue access to *the pixel* (a pseudo-object if you like) unless another object with reference semantics is used as well. Thus, there is also the concept of a pixel reference, or "handle" to avoid overloading the term reference. This concept is modeled by a C++ reference if the pixel is interleaved (because in that case the pixel is itself a C++ object), or by a wrapper object if the pixel is planar, because in that case the pixel is not a *single* C++ object. Such a wrapper simultaneously references all the planar pixel channels. A dereferenced pointer gives you a reference which provides the value semantics on the object pointed to. In the case of a pixel, the value in question is its color, which happens to have the same structure for both interleaved and planar pixels. When a pixel reference (or handle, as defined above) is modeled as a C++ reference, the rvalue/lvalue semantics both correspond to the pixel's value, that is, color, since an interleaved pixel and its color are the same object. However, when a pixel is planar, its pixel reference (handle) provides rvalue semantics that correspond to its color but lvalue semantics that corresponds to the pixel's channel as stored in the buffer (through proxy objects). Thus, pixel references (for both types of pixels) yields "pixel<>" objects as a closest match to a "pixel rvalue", even though in the case of a planar pixel such object is really a deep copy of the pixels color. ------------------------------------------------------------------------------ If all that is correct, or mostly correct, then I would suggest: (1) rename the pixel<> class as color<>. The reason is that objects of this class hold the *value* of both kinds of pixels but do not represent the pixels themselves. The fact that a "pixel<>" object from a planar image is really holding a deep copy of the channels might be more evident if the class is spelled "color<>". This goes in hand with the discussion of the Pixel vs Color concept. (2) rename the planar_ptr<> class as planar_pixel_iterator<> The reason for "iterator" is that overloading the term ptr can be confusing. It was for me. The reason for "_pixel_" is that I believe it is generally better to never sacrifice clarity over identifier length unless the length is atually too long. (3) rename the planar_ref<> class as planar_pixel_handle<> The reasons are analogous to those for point (2)

Thanks for your valuable suggestions. We would like to hear the rest of your comments whenever you find the time.

I will :)

It doesn't have to be part of a formal review, or even in the form of a posted message on the boost list. You could just email them if you prefer, or we could talk.

OK. I will post them to the list, but off the review to allow Tom to finally work on it.

...

It also requires the type to have an "axis_value()" template function. Is that a member function of the model? A free function in the GIL namespace? Can't tell again.

A nested constant. Some properties are explicitly stated

OK, I guess there's the problem, some are, but not all, not even in the doxygen docs. Just systematically check all the documentation to see if it is clear how each type/function is to be implemented.

that must be provided in associated traits class, pixel_traits and pixel_iterator_traits. Unless explicitly specified in the design guide, assume nested constants.

I know the same argument about adaptability applies here as well. We could have a traits class associated with every GIL concept and move types and constants there. But then refering to the type/constant becomes much longer and uglier expression. This is why, as of now, we use nested typedefs, unless the model can be a type that cannot have nested typedefs.

That's OK. My complain was about the choice not being clearly stated in the docs in all cases.

...

So I look into the doxygen docs, and I found that I can't tell either, but I can tell something: PointNDConcept is a concept "checking" class. That's not the same as a Concept even if it comes close. For example, the "public attribute" "P point" listed by doxygen is specific to the "checking" class. It's not part of the Concept.

Yes. What happened there is that we have classes that enforce concepts via boost::concept_check. We Doxygen-tag those classes to belong to the associated concept, which made Doxygen provide "Public Member Functions" and "Public Attributes" section, which could be quite confusing. We should see how to suppress these from the Doxygen documentation.

OK, that would help. Or else add a section, somewhere, explaining how to interpret that doxygen documentation.

...

The same problem happens with all of the other "concepts". Without looking into the code you can't tell how the requirements must be fulfilled by models. traits_classes? nested types? etc

I am still not sure how exactly to represent this using the new concept syntax. We will look into this. Right now to find this you need to read the associated design guide section. Unless it states there is an associated traits class, the types must be nested.

The problem is that there is some mismatch bettween the doxygen generated docs + the design guide and the actual code. I found places were I realized that traits classes where used only after looking at the code.

...

The discussion of compatible color spaces in the design guide mentions that two color spaces (which as I said should be called color formats) are compatible if the have the same base. Maybe is me, but I found that to be an odd way to say that color-space models form inheritance diagrams. If you just say that, it follows by LSP when are they compatible.

No they don't have to use inheritance to be compatible. They just need to have the same base, i.e.

boost::is_same<typename CS1::base, typename CS2::base>

Ha OK.

...

Just an example of a problem found through all of the documentation:

It's next to imposible to understand what the "value_type" of a HeterogeneousPixelConcept is. It's documented as: HeterogeneousPixelValueConcept<pixel_value_type>, but: HeterogeneousPixelValueConcept is a refinement of HeterogeneousPixelConcept so the definition is recursive.

Yes indeed. But there are many other places where the definition is recursive. For example, in the definition of a ViewConcept there are types of derived views, which also must model ViewConcept. A pixel iterator must have in its traits a "const_t" which must also model (an immutable) PixelIteratorConcept.

In the concept check classes, we simply don't follow the circular branches. Suggestions on how to resolve this are welcome.

Hmm, I see. Well, if I can come up with something I'll tell you. Best -- Fernando Cacciola SciSoft http://fcacciola.50webs.com/

Lubomir Bourdev wrote:

Fernando Cacciola wrote:

...

(1) rename the pixel<> class as color<>. (2) rename the planar_ptr<> class as planar_pixel_iterator<> (3) rename the planar_ref<> class as planar_pixel_handle<>

I am hesitating to do (1). Using color is very strange when inside the image. Color iterators? Looking at several other libraries I see the terms pixel, pixel iterators, used frequently...

I agree with Lubomir. Although they might not be images in the traditional meaning, normal maps and bump maps can be handled similar to color maps at the level GIL addresses. However, the pixel of a normal map represents a direction, not a color. Malte

Hi Ulli, Thanks for your review! Ulli wrote:

I vote to accept GIL into boost after revision. In addition, I consider addition of an algorithm library in the not-too-distant future mandatory in order to realize the full potential of the image classes.

As I already mentioned, we also would like very much to see image processing algorithms added to GIL, whether or not it is accepted in Boost. In particular, the Vigra algorithms provide a good starting point. Although they could be invoked through a compatibility layer, as you and I demonstrated, it would be great if they are rewritten in native GIL, because we don't believe a compatibility layer can handle all possible image abstrations. If you and/or others would like to volunteer to do that, you can expect and extra level of support from us. We also are looking for volunteers to implement other algorithms, such as the ones in OpenCV. It would be great to also have a GIL-layer over Intel's IPP library. Also, providing interfaces to AGG would be terrific. Providing parallel versions of image processing algorithms would be great. GPU support too. More I/O formats too.

...

- What is your evaluation of the design?

I like the design of the image classes (including the dynamic_image) and the heavy use of views as a major algorithm interface. The various iteration abstractions are also quite powerful. The proof-of-concept implementation of the VIGRA interoperability layer was quite encouraging.

I'd like to see the following additions to the image class: - constructors that allow copy construction of the pixels values

You mean deep-copy the images? The image class has a copy constructor which deep-copy's the images.

- resize() functions with default construction and copy construction of the newly allocated pixel values (without resize(), the default constructor image() is pointless).

There is a function called resize_clobber_image. Is that what you are looking for? If the images are of different dimensions, it constructs an image of the target size and swaps them. It does not copy the pixels. This could be done manually with copy_pixels if needed. We are looking for a better name, but resize_image implies the pixels are copied (and truncated) as with vector resize. We didn't add copying of the pixels because doing so is often not necessary, and it is unclear what the copy policy should be exactly. (copy into top left? Truncate?)

I'm less convinced of the pixel classes (which, as I argued in another post, should be renamed into pixel_value or just value). IMHO, a pixel class without a set of operations specific to this particular pixel type is not very useful. As far as I can tell, there are not even conversion functions between RGB, LAB and HSB (and, unlike the RGB-CMYK conversion, these are too expensive to be implemented as pixel adapters).

I recommend to either - drop the specific color classes for the time being and use a general static_vector<3, T> together with accordingly changed pixel_value_tag concepts, or - add a set of functions justifying the existence of the color classes.

Why does VIGRA have a separate class for RGB Value then? What is special about RGB? There are lots of functions that take the color information into account. Color information goes down to the very basics. For example, any per-channel operation that takes two or more pixels operates on semantic channels, not physical channels. These will do the right thing: bool eq=equal_pixels(rgb8_view, bgr8_view); copy_pixels(rgb8_view, bgr8_view); rgb8_pix = bgr8_pix; rgb8_image_t img(bgr8_img); Color information also is useful for compile-time type safety. GIL constructs that don't have the same base color space are incompatible and cannot be copy-constructed, assigned and compared to each other. These will generate a compile error: copy_pixels(lab8_view, rgb8_view); // error rgb8_pix = lab8_pix; // error This level of compile-time safety is not possible if you only consider the type and number of channels. Of course, there are a variety of color conversion functions: copy_and_convert_pixels(rgb8_view, cmyk16_view); cmyk16_pixel_t c16; color_convert(rgb8_pixel_t(255,0,0), c16); You are right that some color conversion combinations are not yet provided. This has nothing to do with the performance to do color conversion, but more with the fact that there is a combinatorial explosion in the number of conversions - every color space to every other. Color conversion between RGB and LAB could be implemented just like it is done between, say, RGB and CMYK. Is there any performance advantage to implementing it differently? I agree that sometimes it doesn't make sense to have a specific color space. Sometimes you really want to think of the data as image of pixel with N channels of type T. GIL provides models of anonymous N-channel homogeneous pixels, which have "device-N" color space. Here are the types of 5-channel anonymous 16-bit unsigned integral pixels and image views: dev5n16_pixel_t p(0,1,3,2,3); dev5n16_view_t view5; Their color space is devicen_t<N> where N is the number of channels. (We are open to suggestions for a better name)

Regarding pixel type conversion and mixed-valued functions: It was already pointed out that the resulting code bloat can easily get out of control, especially in the context of the dynamic_image. I read Lubomir's OOPSLA

Which, by the way, I noticed is posted here: http://sms.cs.chalmers.se/bibliography/proceedings/2006-LCSD.pdf

paper and think that it only offers a partial solution - it only covers the case were different type combinations actually create the same binary code. IMHO, this is not the most problematic case.

Consider just the scalar pixel types 'unsigned byte', 'signed short', 'unsigned short', 'int', and 'float'. All of them are regularly occuring in VIGRA algorithms, and no combination will be binary equal to any other.

As described in my paper, the types can be equivalent in the context of the algorithm. For example, for bitwise-copy, copying "signed short to signed short" and copying "unsigned short to unsigned short" can be treated as equivalent.

What is needed is a consistent set of coercion rules. For example, 'unsigned char' x 'unsigned char' might be implemented natively (for speed), while 'unsigned char' x 'float' will be implemented by a coercion of the first pixel to 'float', followed by a native 'float' x 'float' operation.

These rules, together with appropriate result_of types, can be easily formulated in a set a traits classes, and should also be customizable on a per-function-call basis. Coercion may not be a prerequisite for the usefulness of the core part of GIL, but it is critical for the dynamic_image and the future image algorithm library (including the color conversion function suggested above).

I can see how automatically converting one type to another can be useful, but I am not sure how this relates to the topic of dynamic_image. Whether or not the type of the image is specified at run time, and whether or not you want to convert one type to another are two orthogonal things... unless I am misunderstanding you.

...

- What is your evaluation of the implementation?

It is mostly OK. I would like the following improvements:

- there are several comments saying 'potential performance problem' (or equivalent) in the code. The code should be changed to eliminate these problems because the affected functions are performance critical.

I agree, on a case by case basis. We are trying to get what we believe are the bottlenecks as efficient as possible. It would be quite a job to make all parts of GIL completely optimal. Very often extra efficiency comes at the cost of code size, extra stuff in data structures, or decreasing the performance in other places of the code... Sometimes the tradeoff is not worth it. For example, currently view.end() is not very efficient as it is implemented as "return begin()+size()". If you say: for (it=view.begin(); it!=view.end(); ++it) {...} You may suffer a performance hit there. But how do we fix this? Store the end iterator inside the view? Then the view class will grow and contain less than minimum info... Unlike std::vector implementations, we need to keep the dimensions explicitly as they cannot be inferred simply by end() - begin(), which is 1D.

- C-style casts and reinterpret_casts should be eliminated (if this is impossible in the latter case, the casts should be guarded by static_assert to do the right thing).

I would love to eliminate them but I don't see how. Static asserts are always a good idea.

- proper initialization of the pixel values should be added (e.g. by uninitialized_fill).

Already on our todo list.

- I didn't check in detail whether all functions (especially constructors and destructors) are fully exception safe. The authors should make a statement about this.

Unless there are bugs, everything should be exception-safe.

- code readability should be improved (more line-breaks).

- the test suite should be made part of the distribution.

It is posted on our download page. Thanks again for your review. Lubomir

Hi Ulli,

No, I was unclear. I mean that it should be possible to provide an initial pixel value (which is copied into all pixels), like

some_image_t img(a_size, an_initial_pixel_value);

That's a good idea. Will do.

...

...

- resize() functions with default construction and copy

construction

...

...

of the newly allocated pixel values (without resize(), the default constructor image() is pointless).

There is a function called resize_clobber_image. Is that what you are looking for?

Probably. But not being a native English speaker, I have no idea what 'clobber' stands for. In addition, I'd expect resize() to be a member function of the image class, similar to std::vector. I agree that the old pixel values need not be preserved. The above comment about an initial pixel value applies to resize() as well:

sime_image.resize(new_size, an_initial_pixel_value);

The policy should be: the member function image::resize() just resizes the memory without copying pixel values, whereas the free functions resize...() interpolate (or drop) pixel values, but assume the right amount of memory to be already allocated.

We on purpose did not make the interface the same as vector::resize. This is because the behavior is different - the vector copies the values on resize, whereas gil::image doesn't. This is why we use the name "resize_clobber_image". It is also a global function to make external adaptation easier. For example, dynamic images already provide an overload. There are many functions that have the same interface for dynamic images and templated ones, so you can write some code that can be instantiated with either: template <typename MetaImage> void flip_image(const char* file_name) { MetaImage img; jpeg_read_image(file_name, img); jpeg_write_view(file_name, flip_left_right_view(const_view(img))); resize_clobber_image(img, 100,100); ... } Of course, we could have done the same by keeping resize_clobber inside the image class and provided one for the dynamic_image too... I don't have a strong opinion. Do people think we should move all global functions like this to be part of the image class? Currently they are: resize_clobber_image(img) view(img) const_view(img) get_height(img or view) get_width(img or view) get_dimensions(img or view) get_num_channels(img or view) Except for the first one, all these global functions have member method equivalents. We could: 1. Make image::resize_clobber a method to be consistent 2. or get rid of the global functions 3. or make the member functions private, so people only use the global ones Also any suggestions for a better name than resize_clobber_image (just not resize, not to be confused with vector resize behavior)

...

Why does VIGRA have a separate class for RGB Value then?

What is special

...

about RGB?

RGB has some specific functions (e.g. red(), luminance()), and I'm not arguing against an RGB value class. I'm just saying that

1. It may not be appropropriate to represent each and every color model by their own class.

Color models are an open class, so we cannot represent all of them even if we wanted. This applies to just about any other part of GIL. Our goal is to provide a comprehensive set of the most frequently used models.

2. If a color model does not have specific functions (in the current version of the code base), a specific class is definitely not warranted.

This applies especially to the lab and hsb classes in the current GIL version.

We could get rid of them. Or we could provide color conversion for them.

...

There are lots of functions that take the color information into account. Color information goes down to the very basics.

Yes, but there are other solutions than providing a new class, e.g. a string tag in the image class. It is not as type safe, but simpler and more flexible. Type safety may not be the main consideration here, because type errors in color space conversions are easily visible during testing.

It is not just type safety. My rule is to push complexity down and resolve it down at the building blocks. In my experience that results in simpler and more flexible high level design. For example, GIL pixels are smart enough to properly handle channel permutations: rgb8_pix = bgr8_pix; By resolving the channel permutation once, down at the pixel level, we no longer need to worry about this every time we deal with pixels, in algorithms like copy_pixels, equal_pixels, etc. A similar case is for dealing with the planar vs interleaved organization. We could have treated planar images as separate 1-channel images and leave the burden on each higher level algorithm to handle them explicitly (or, what is worse, not support them at all!). Instead, GIL's approach is to resolve this down at the fundamental pixel and pixel iterator models. As a result, you can write an open set of higher level GIL algorithms without having to deal with planar/interleaved complexity. (If this reminds you of the iterator adaptor vs data accessor discussion, it is not an accident - this is the same discussion) The same principles could apply down to the channel levels. The channel could be made smart enough to know its range, which allows us to write channel-level algorithms that can convert one channel to another. Having smart channels allows us to write the color conversion routines that just deal with color, and delegate the channel conversion down to the channels: template <typename T1, typename T2> struct color_converter_default_impl<T1,gray_t,T2,rgb_t> { template <typename P1, typename P2> void operator()(const P1& src, P2& dst) const { dst.red=dst.green=dst.blue=channel_convert<T2>(src.gray); } }; Unfortunately, propagating the same principles to the channels brings us to a dilemma: 1. We want to use simple built-in types to represent channels. Wrapping the channel in a class may result in potential abstraction penalty (although we haven't verified it does) 2. At the same time, we want the channels to be smart, i.e. to associate traits types, such as min value and max value, to the channel type, so that we can implement channel-level operations like channel_convert. 3. The same built-in types may be used for semantically different channels. For example, float often has an operating range of 0..1, but if you want to use high dynamic range images you may want to use floats with different ranges. I don't know of a way in C++ to make an "alias" type to a given type that allows us to associate different traits. For example, I'd like to say something like: alias float_dyn_range = float; alias float_01 = float; template <> struct channel_traits<float_01> {...}; template <> struct channel_traits<float_dyn_range> {...}; Right now GIL hard-codes the commonly used types to their commonly used ranges (unsigned char is 0..255, float is 0..1, etc). If you want a different range, you can make a wrapper class and define the traits for it. We are open to alternative suggestions.

...

You are right that some color conversion combinations are not yet provided. This has nothing to do with the performance to do color conversion, but more with the fact that there is a combinatorial explosion in the number of conversions - every color space to every other.

That's another area were coercion is needed. By providing rules for multistep conversion, the actual conversion code can be generated automatically from a few basic building blocks.

You mean, writing conversion to/from a common color space? Although this is appealing, the two problems I see with this are performance (it is faster to convert A to B than A to C and C to B) and loss of precision (there is no common color space that includes the gamut of all others)

...

Color conversion between RGB and LAB could be implemented just like it is done between, say, RGB and CMYK. Is there any performance advantage to implementing it differently?

No, but the RGB-LAB conversion is so expensive that the only practical relevant use of an lab_view is

copy_pixels(lab_view(some_rgb_image), view(lab_image));

Naive users may not be aware of the cost, and may use an lab_view as an argument to convole(), say. Therefore, I'd prefer a functor-based solution in that case, such as

transform_image(view(rgb_image), view(lab_image), rgb2lab());

This cannot be accidentaly misused.

True, but at the same time you can write a view pipeline that ends up executing faster than if you were to copy to an intermediate buffer. Consider this: jpeg_write_view("out.jpg", color_converted_view<lab8_pixel_t>(subsampled_view(my_view, 2,2))); This performs color conversion only for the even pixels. And it is faster than having to copy to an intermediate buffer. I am in favor of letting the programmer choose what is best rather than imposing one strategy, which in my opinion is the style of C++ and STL as compared to other languages.

...

As described in my paper, the types can be equivalent in

the context of

...

the algorithm. For example, for bitwise-copy, copying "signed short to signed short" and copying "unsigned short to unsigned short" can be treated as equivalent.

Yes, but a copy between uniform types is about the only example were this works.

It works for any bitwise-identical operation - copy construction, assignment, equality comparison. But I agree these are fairly limited.

...

I can see how automatically converting one type to another can be useful, but I am not sure how this relates to the topic of dynamic_image.

Suppose you want to support something like:

transform(dynamic_image1, dynamic_image2, dynamic_image3, _1 + _2);

for a large set of possible pixel types. Then your method of code bloat reduction by binary compatibility only applies to very few instances. One also needs coercion, i.e. one splits transform() into two parts:

- converting all input into a one among a few supported forms (e.g. leave uniform type input untouched, but convert mixed type input to the highest input type)

- doing the actual computation with only the few supported type combinations.

So, only a lot of conversion functions are needed (which are simple and don't bloat the code too much), whereas all complicated functions are instantiated only for a few types.

Now I understand what you mean. But I still think whether you convert one type to another, and whether you apply that operation on statically-specified or dynamically-specified types are two orthogonal things. First of all, in GIL we rarely allow direct mixing images of incompatible types together. They must have the same base color space and the same set of channels to be used in most binary algorithms, such as copy, equality comparison, etc. For compatible images there is no need for type coercion and no loss of precision. For example copy_pixels(rgb8, cmyk16) will not compile. If you use runtime images and call copy_pixels on two incompatible images you will get an exception. If you want to use incompatible views in algorithms like copy_pixels, you have to state explicitly how you want the mapping to be done: copy_pixels(rgb8_v, color_converted_view<rgb8_pixel_t>(cmyk16)); copy_pixels(nth_channel_view(rgb8_v,2), gray8_v); Now, it is possible that a generic GIL algorithm needs to take views that are incompatible. In this case, I agree that type coercion may be an option, depending on the specifics of the algorithm. But whether or not the algorithm inside uses type coercion is orthogonal to whether we happen to instantiate it for static types or for dynamic types, right? Lubomir

Hi Ulli,

I like to think of images in terms of Abstract Data Types. My rules for member functions vs. free functions are therefore:

- functions that retrieve the internal state of a single image or change this state belong to the class (i.e. resize(), get_height() etc.)

- functions that invlove several images and create new images (or rather write the values of a pre-allocated destination) belong outside the class, e.g. transform() and convolve()

- functions that have potentially open-ended semantics belong outside the class (e.g. view creation).

I have no problem with following these rules.

...

Also any suggestions for a better name than resize_clobber_image (just not resize, not to be confused with vector resize behavior)

Yes, please. IMHO image::resize() is ok, the semantic difference to std::vector is no problem for me, but others' opinions may be different.

My rule of thumb: when I provide functionality that is similar but not identical to existing functionality, I go out of my way to indicate that they are different. This is why GIL calls 2D 'iterators' locators, although they are almost identical to traditional iterators. Similarly, I prefer names like resize_and_clobber(), resize_and_invalidate(), create_with_new_dimensions(), recreate(), or reset() than resize(). What do other people think?

...

...

2. If a color model does not have specific functions (in the current version of the code base), a specific class is definitely not warranted.

This applies especially to the lab and hsb classes in the current GIL version.

We could get rid of them. Or we could provide color conversion for them.

The first possibility migth be ok for the first release, but eventually the second will be required. Still, the question is: is a color conversion function sufficient to warrant an own class? (This is a general question, not targeted against GIL).

I think it depends on your application. In some contexts specific color models are widely used and others are not needed. But each color model was invented because it was needed somewhere. I see little harm in providing an extensive set of color models. You don't have to use them if you don't want to - you could just use the anonymous model.

This is a good thing, and VIGRA tries the same (e.g. interleaved vs. planar images can be abstracted by means of accessors). But the problem is also open-ended. For example, what about tiled images (necessary for very large files that don't fit into memory, and for better chache locality)?

In my opinion tiled images are a different story, they cannot be just abstracted out and hidden under the rug the way planar/intereaved images can. If you want your algorithm to work on a tile-by-tile, you have to redesign it with this requirement in mind. And some algorithms are inherently less tile-friendly than others. It may be possible to create a virtual image that tries to predict and adapt to the access pattern of a generic algorithm, but my feeling is that it won't do much better than simply letting the virtual memory of your OS handle it.

In my experience, it doesn't cost much (if anything), at least for integral underlying types. As an analogous example: a pure pointer and an iterator class which contains just a pure pointer behave identically on all systems I tried.

We have some internal performance benchmarks showing that for complex algorithms (like sort), representing the vector iterator as a pointer wrapped in a class results in suboptimal performance on some compilers. Compilers are good enough for simple algorithms, but in more fancy contexts may fail to keep the wrapped pointer in a register. But I haven't looked into specifics.

...

Right now GIL hard-codes the commonly used types to their commonly used ranges - float is 0..1.

Really? I didn't realize this and don't like it at all as a default setting. IMHO, the pleasant property of floats is that one (mostly) doesn't need to take care of ranges and loss of precision.

Think of these as part of the channel traits specifying the legal range of the channel, which is often smaller than its physical range. You are, of course, free to ignore them (and it may make sense to do so in intermediate computations), but they are implicitly always defined. Having them explicitly defined makes certain operations much easier, such as converting from one channel type to another. Not having them defined would force you to propagate them to every function that might need them. For example channel_convert will need to be provided the minimum and maximum value for both source and destination, and so will the color conversion functions, the color converted view, etc.

Some color conversions are defined by means of the rgb or xyz color space as intermediate spaces anyway. Others are concatenations of linear transforms, which are easily optimized at compile-time or run-time. Loss of precision is a non-issue if the right temporary type is used for intermediate results.

I was talking about loss of precision due to out-of-gamut representations. Those could result in large errors, as the result of color conversion can only lie in the intersection of the gamuts of all color spaces.

...

But I still think whether you convert one type to another, and whether you apply that operation on statically-specified or dynamically-specified types are two orthogonal things.

Yes, but when types are statically specified, you usually know what to expect, so the number of supported types is fairly limited. In contrast, the dynamic type must (statically) support any type combination that could happen. Thus, the two things are linked in practice.

GIL's dynamic image doesn't do anything fancy. It simply instantiates the algorithm with all possible types and selects the right one at run time. If your source runtime image can be of type 1 or 2, and your destination can be A,B or C, when you invoke copy_pixels, it will instantiate: copy_pixels(1,A) copy_pixels(1,B) copy_pixels(1,C) copy_pixels(2,A) copy_pixels(2,B) copy_pixels(2,C) and switch to the correct one at run-time. It has some optional mode to reduce code bloat by collapsing identical instantiations and representing binary algorithms using a single switch statement (instead of two nested ones.

...

First of all, in GIL we rarely allow direct mixing images of incompatible types together.

What are the criteria for 'incompatible'?

Compatible images must allow for lossless conversion back and forth. All others are incompatible.

...

For compatible images there is no need for type coercion and no loss of precision.

Loss of precision and out-of-range values become important issues as soon as arithmetic operations enter the picture -- you only want to round once, at the end of the computation, and not in every intermediate step.

I agree - in cases you need arithmentic operations you need to worry about specifying intermediate types and there is loss of precision.

...

For example copy_pixels(rgb8, cmyk16) will not compile.

What's incompatible: rgb vs cmyk or 8 bit vs 16 bit?

Both the color space and the channel type.

...

If you want to use incompatible views in algorithms like copy_pixels, you have to state explicitly how you want the mapping to be done:

copy_pixels(rgb8_v, color_converted_view<rgb8_pixel_t>(cmyk16)); copy_pixels(nth_channel_view(rgb8_v,2), gray8_v);

Fair enough, but why not make the most common case the default? It worked well for us.

There are some fundamental operations that can be performed at the level of channels, pixels, image views and images. They are: - copy construction and copy - assignment - equality comparison We would like to have them defined only between compatible channels/pixels/views/images. Otherwise some simple operations are not mathematically consistent. For example, equality comparison should be an equivalence relation, but if you define it for incompatible types, it is no longer transitive... Lubomir

...

Similarly, I prefer names like resize_and_clobber(), resize_and_invalidate(), create_with_new_dimensions(), recreate(), or reset() than resize(). What do other people think? What about reshape()?

I'm German, too, and so I'm not familiar with term like clobber. Looking at the dictionary doesn't bring much more sense to me. Clobber in real life means something like beating up someone. ;-) Taken from dict.leo.org I think resize() is the name to go for. Christian

Hi Lubomir! On Tuesday, 14. November 2006 00:40, Lubomir Bourdev wrote:

When searching for appropriate names, we thought about Traverser too, and we like it. It emphasizes more the navigation aspect, whereas Locator emphasizes the access aspect. Both Locator and Traverser have precedence. We ended up choosing Locator because it is a bit shorter. Thanks for the explanation.

Another term we considered is Position. To me, a position is the difference_type of a traverser and a specific 'origin traverser'.

Someone also brought up the term Cursor. I am not sure if there are strong precendents within the image analysis domain, but that term probably already has too many associations.

I don't have a strong opinion about sticking with Locator; other than the work to change the name everywhere. If people overwhelmingly prefer one term over another we can change it. I prefer Traverser, but I can understand that you don't want to change all code just because of me. ;-) (OTOH, that's a killer argument - it would be sad if good design changes would not be made just because it's a lot of work.)

...

...

Similarly, I prefer names like resize_and_clobber(), resize_and_invalidate(), create_with_new_dimensions(), recreate(), or reset() than resize(). What do other people think?

What about reshape()?

Reshape is used in Matlab to change the dimensions of an array, but it does not invalidate the elements of the array. So I wouldn't recommend using it for the same reason I don't like resize() - because there are precedents with the same name and different semantics. Still, I would say that there's a big difference between a Matlab predecessor and an STL one. But I agree that Matlab is well-known in the image analysis domain, so..

How about image::recreate(width,height)? ..I very much prefer this one! :-) (Ulli seems to like it, too.)

For non-compatible channels you must use copy_and_convert_pixels, not copy_pixels, to emphasize that the copy may be lossy. And your channel types must be convertible. That is, there must be a function:

DstChannel channel_convert<DstChannel>(SrcChannel c);

It will define how to convert one channel type to another (scale, shift by 8 bits, etc.). The default simply transforms the range of one to the other and rounds to the nearest element. If I may mention the principle of least surprise again - the default should be copy-and-round, not scale IMO, but..

If you don't like this transformation, you may choose to create a custom channel type, you may provide custom conversion function object, ..it should be easy to do so, to provide a custom converter, and the current default converters (changing the range) should be available as such functors. I would not like to create custom "channel type"s just for this I think, but maybe I am not thinking GILish enough yet.

or you may choose to use transform_pixels instead of copy_and_convert_pixels, depending on your design. In VIGRA, copyImage is simply copying the image data (using traits to do an explicit type conversion to make the compiler silent if necessary), and you use transformImage with e.g. a LinearIntensityTransform functor to change the range while doing so:

// transform 0..255 byte image to 0.f..1.f float image: transformImage(srcImageRange(byteImage), destImage(floatImage), linearIntensityTransform(1.0/255)); This is very close to the STL, isn't it? Now that I think of it, what's the difference between transform_pixels and copy_and_convert_pixels (when using function objects)?

...

When discussing the stacking of views, your argument was:

...

I am in favor of letting the programmer choose what is best rather than imposing one strategy, which in my opinion is the style of C++ and STL as compared to other languages.

Does this argument not hold for the above case?

The two contexts are not the same. I was arguing that we shouldn't prevent creating color_converted_view with certain color spaces simply because color conversion with them is expensive. We should let the programmer decide whether it makes sense to use color converted view or explicitly color convert.

But the semantics of color_converted_view with LAB is very clear - it is the same as with any other color space, whereas how to copy between two incompatible images is not always obvious.

That's a good argument. However, how clear are the semantics of copy_and_convert_pixels with the default converters? To me, "convert" was simply "change type" (which already has clear semantics in C++, even if they may not always be what you need), not "change type, scale, and round".

It comes down to some basic principles:

1. We want equality comparison to be an equivalence relation. 2. We also want equality comparison and copying to be defined on the same set of types. 3. The two operations should work as expected. In particular, after a=b, you can assert that a==b.

I hope you will agree that these are fundamental rules that hold in mathematics and violating them can lead to unintuitive and bug-prone systems.

Yes, I agree to that and the consequences, and I thus agree that it's good design to forbid copy_pixels on non-compatible types. However, that's inconvenient, as you will probably agree. That's why you introduced the default channel conversions: for convenience.

Regardless of what we do, there is always a notion of the operating range of a channel, whether it is implicit or explicit. You are essentially suggesting that the range of a channel be defined at run time, rather than at compile time, right?

You are right, this seems to be the main difference between the VIGRA and GIL approaches in this respect: In VIGRA, short is simply a (pixel/channel) type, but does not imply any operating range (that's up to the application), whereas in GIL, it has an operating range associated with it (BTW: -2^7..2^7 or starting from zero?) and you need to define new channel types in order to change that operating range, right? -- Ciao, / / /--/ / / ANS

Hi again, On Tuesday, 14. November 2006 03:01, Lubomir Bourdev wrote:

I should add to my earlier reply that I have no objection to having the above transformation per se. You are free to define it if you think it is useful. You can call it copy_and_cast_pixels, for example.

Indeed, that would be a good name, which makes the semantics clear. Ciao, / / /--/ / / ANS

Hi Ulli:

Lubomir Bourdev wrote:

...

...

...

First of all, in GIL we rarely allow direct mixing images of incompatible types together.

What are the criteria for 'incompatible'?

Compatible images must allow for lossless conversion back and forth. All others are incompatible.

That's a nice rule, but it's not the rule of the underlying C/C++ language, where compatible means more or less that an implicit conversion between two types is defined. Our default behavior in VIGRA resembles this as far as possible, with the exception that the default conversions include clamping and rounding where appropriate. In practice, both definitions of compatibility will probably work equally well as long as conversions can be customized. I still think that a default implicit conversion makes life easier in many situations, without causing unpleasent surprises.

I don't see the big convenience of having copy_pixels do implicit conversion. If you want to convert between incompatible types, then instead of copy_pixels, simply use something else, like copy_and_cast_pixels, or copy_and_round_pixels, or whatever. You are free to define such operations.

I like this name -- it says exactly what's happening. Please remember that

image::recreate(width,height, initial_pixel_value) image::recreate(size_object) image::recreate(size_object, initial_pixel_value)

should also be defined.

Yes, good idea.

...

It comes down to some basic principles:

1. We want equality comparison to be an equivalence relation. 2. We also want equality comparison and copying to be defined on the same set of types. 3. The two operations should work as expected. In particular, after a=b, you can assert that a==b.

I hope you will agree that these are fundamental rules that hold in mathematics and violating them can lead to unintuitive and bug-prone systems.

Unfortunately, the CPU itself violates rule 3: a double in a register and the same double written to memory and read back need no longer compare equal - a behavior that is really hard to debug :-(

Well, the world will never be perfect, but that doesn't mean we shouldn't strive for perfection :-) (That seems quite a serious problem though! Can you point me at a document describing this? Which CPUs are affected?)

...

Regardless of what we do, there is always a notion of the operating range of a channel, whether it is implicit or explicit.

I don't agree. For example, when you use Gaussian filters to compute derivatives or the structure tensor etc. the result has no obvious range - it depends on the image data and operator in a non-trivial way. For example, the minimal and maximal possible values of a Gaussian first derivative are proportional to 1/sigma, but sigma (the scale of the Gaussian filter) is usually only known at runtime. It is the whole point of floating point (as opposed to fixed point) to get rid of these range considerations.

So in this case the range is -infinity to infinity. It is still defined. But I would argue that most of the time the range is finite.

...

You are essentially suggesting that the range of a channel be defined at run time, rather than at compile time, right?

More precisely, we avoid explicitly defined ranges, until some operation (e.g. display) requires an explicit range. Remember that it is no longer slow to do all image processing in float.

My experience, and that of my colleagues, is that there is often significant speed difference between integer and floating point computations. Floating point operations have higher latency and lower throughput because of fewer functional units available to process them. Another issue is their size and ability to fit in the cache, since they are typically four to eight times larger than a char. A third issue is the performance of floating point to integer conversion on many common architectures. The differences are especially large on non-desktop devices, such as PDAs. I was able to speed up my face detector by more than 25% just by making certain operations integer. This is why providing generic algorithms that can work natively on integral types (unsigned char, short, int) is very important for GIL. This necessitates providing a suite of atomic channel-level operations (like channel_invert, channel_multiply, channel_convert) that have performance specializations for various channel types. Many of these operations require knowing the range of the channel, which is why GIL channels have ranges.

...

But you still need to know the range for many operations. For example, converting between additive and subtractive

color spaces > requires inverting the channel, which requires knowing its range.

Not necessarily. In many cases, one can simply negate the channel value(s) and worry about range remapping later, when the required output format is known.

I am not arguing that there are contexts in which knowing the range is not important - of course there are! All I am saying is that the ranges matter at least for _some_ operations. GIL's principles, as I stated before, are to push the complexity down to the elements and resolve it there. Having smart channels makes writing higher-level code easier. If the channel knows its operational range, we can define a set of atomic channel-level operations (such as channel_invert, channel_multiply, channel_convert) and then use them when writing higher level algorithms.

Likewise, if you don't require fixed ranges, you may perform out-of-gamut computations without loss of precision. You can't display these values, but they are mostly intermediate results anyway.

It is not against GIL principles to have intermediate values outside the range when it makes sense, as long as you know what you are doing.

Thus, I strongly argue that the range 0...1 for floating point values should be dropped as the default behavior.

We don't like hard-coding 0..1 range for float either, so we agree with you and are willing to look into alternatives. Here is what we have come up with: 1. Provide a metafunction to construct a channel type from a (built-in) type and range. For example, here is how we could wrap a float into a class and associate the range [0..1] with it: typedef channel_type<float,0,1>::type bits32f; 2. Define a RangedChannelConcept (a channel that has associated range). All atomic channel-level functions that need a range will require a model of RangedChannelConcept Now we have several alternatives: A. Define the range of any channel T by default to use numeric_limits<T>::min() and max() The advantage is that all built-in types will be valid models of RangedChannelConcept, and it so happens that the range 0..255 for uchar and 0..65535 for ushort are the ones people typically use. The disadvantage is that float will have a range of -inf to inf, so using it with range-requiring algorithms will be totally bizarre. We could mitigate this by typedef-ing bits32f to be the float0..1 type instead of native float. And we can provide typedefs for other useful float/double ranges. B. Don't define a default range. As a result, built-in types will not model RangedChannelConcept. So GIL's channel typedefs bits8, bits16, bits32f will all be wrapped and not native. The advantage is that using a range algorithm like color conversion with a built-in type like float or double will not compile at all (rather than produce undesired results). The disadvantage is that built-in types will rarely be used as GIL channels, which could have performance implications due to abstraction penalty (and I do have data to demonstrate such performance penalty exists) In both cases, the client is free to define a range for the built-in types, which will make them model RangedChannelConcept. As long that such definition is on the client side and not in generic code, I believe it is OK. C. Like A, but associate ranges with certain built-in types (like 0..1 with float) This is essentially what GIL does currently. The advantage is that in the vast majority of cases you can use built-in types as channels (no abstraction penalty) and they will do what you want. You can always use a float and you can be outside range, but any range-requiring algorithms will assume 0..1 range for float. If you do want to use range-requiring algorithms, but don't want the 0..1 range, you can still create a custom wrapper channel. (For example, if you want to display the gradient image on screen). Alternatively, you can scale your float image to 0..1 range before color-converting it. The obvious disadvantage is that we are hard-coding 0..1 for float which is somewhat arbitrary and non-generic. My inclination is to go with option A, as it provides a reasonable tradeoff between performance and genericity. Thoughts?

...

In my opinion tiled images are a different story, they cannot be just > abstracted out and hidden under the rug the way planar/interleaved images > can.

I'm not so pessimistic. I have some ideas about how algorithms could be easily prepared for handling tiled storage formats.

We would be very interested in hearing more about this. But I must be misunderstanding you because I can't imagine how this could possibly be. How could you have a scheme for taking any inherently global algorithm (like flood-fill) and making it tile-friendly. Some algorithms require to be rewritten from scratch completely, and for others no reasonable tile-friendly solution exists... Lubomir

Lubomir Bourdev wrote:

I don't see the big convenience of having copy_pixels do implicit conversion.

I agree that there is no advantage at all in a direct call of copy_pixels. But I'm thinking about conversions happening in nested function calls, where the intermediate types are _deduced_ (by means of traits and little template metaprograms). Consequentially, the appropriate conversions must also be deduced, and a default conversion is just the simplest form of deduction. Type deduction is central to VIGRA. For example, gaussianSmoothing(byte_image_src, byte_image_dest); is actually executed as a separable convolution gaussianSmoothingX(byte_image_src, temp_image); gaussianSmoothingY(temp_image, byte_image_dest); where the type of temp_image is automatically determined, and both calls involve an automatic conversion. (I admit that the customization options of this behavior could be improved.) With deeper nesting, customization of this behavior can become somewhat complicated, and defaults will be useful (or even required).

...

Unfortunately, the CPU itself violates rule 3

(That seems quite a serious problem though! Can you point me at a document describing this? Which CPUs are affected?)

We learned it the hard way. AFAIK, it affects Intel CPUs and compatible. Registers have more than 64 bits for higher accuracy, but this is not appropriately handled in the comparisons. One can switch off the extra bits, but this throws out the baby with the bath water. I'm sure, someone at Adobe knows everything about this problem and its optiomal solution. Please keep me informed.

So in this case the range is -infinity to infinity. It is still defined. But I would argue that most of the time the range is finite.

Yes, but often it is not necessary to specify the range explicitly.

Floating point operations have higher latency and lower throughput because of fewer functional units available to process them.

When you time 32-bit integers against 32-bit floats (so that memory throughput is the same) on a modern desktop machine, the difference is small (if it exists at all). Small computers (e.g. PDAs and cell phones) are a different story, where I don't have much experience.

Another issue is their size and ability to fit in the cache, since they are typically four to eight times larger than a char.

Well, to do image processing with any kind of accuracy, you will need at least 16-bit integers. Then the difference to 32-bit float shouldn't be that big.

A third issue is the performance of floating point to integer conversion on many common architectures.

Indeed, these are real performance killers. That's why we tend to work in floating point throughout, when we don't need the last bit of speed. After all, the 25% speed-up of your face detector is not that impressive, given that it was probably a lot of work. We made a similar experience with replacing floating point by fixed point in some application -- it was faster, but hardly that much faster to justify the effort and loss in genericity.

This is why providing generic algorithms that can work natively on integral types (unsigned char, short, int) is very important for GIL. This necessitates providing a suite of atomic channel-level operations (like channel_invert, channel_multiply, channel_convert) that have performance specializations for various channel types.

What I often do is to specialize the functors. For example, a LinearRangeMappingFunctor computes a linear transformation at each pixel by default, but for uint8, it computes a look-up table in its constructor. The specialized functor can be created automatically.

I am not arguing that there are contexts in which knowing the range is not important - of course there are! All I am saying is that the ranges matter at least for _some_ operations.

No doubt about that. Perhaps, the notion of a range is just too general? It might be better to study the semantics of various uses of ranges and provide the appropriate specializations on this basis. For example, one specialization I was thinking about is a 'fraction' which maps an arbitray range onto the semantic interval 0...1. For example, Fraction<unsigned char> is the type of the standard 8-bit color channel, but Fraction<unsigned char, 0, 200> Fraction<unsigned short, 1000, 16000> would be possible as well, and the lower and upper bounds represent 0 and 1 respectively. The default bounds would be numeric_limits::min and numeric_limits::max. Fraction<float, 0, 1> would be a float restricted to the interval 0...1 (which could be mapped to a native float, depending on the out-of-bounds policy). A traits class can specify how out-of-bounds values are handled (e.g. by clamping, or by simply allowing them) and how mixed-type expressions are to be coerced. I suppose you have benchmarked the abstraction penalty of ideas similar to this -- can you send me some of the data? What other semantic interpretations of ranges are required?

It is not against GIL principles to have intermediate values outside the range when it makes sense, as long as you know what you are doing.

OK, that makes sense.

1. Provide a metafunction to construct a channel type from a (built-in) type and range. For example, here is how we could wrap a float into a class and associate the range [0..1] with it:

typedef channel_type<float,0,1>::type bits32f;

That's very similar to my Fraction proposal above. You would then just write channel_type<Fraction<float,0,1> >::type which also assigns a meaning to the range. And if out-of-bounds handling was 'ALLOW_OUT_OF_BOUNDS', that type could be a native float.

C. Like A, but associate ranges with certain built-in types (like 0..1 with float)

This is essentially what GIL does currently. The advantage is that in the vast majority of cases you can use built-in types as channels (no abstraction penalty) and they will do what you want.

Well, I prefer clamping over modulo arithmetic as a default, which is not quite built-in for the integral types.

...

...

In my opinion tiled images are a different story, they cannot be just > abstracted out and hidden under the rug the way planar/interleaved images > can.

I'm not so pessimistic. I have some ideas about how algorithms could be easily prepared for handling tiled storage formats.

We would be very interested in hearing more about this. But I must be misunderstanding you because I can't imagine how this could possibly be. How could you have a scheme for taking any inherently global algorithm (like flood-fill) and making it tile-friendly.

This is certainly a difficult one, but I guess there exists some parallel version written in the Golden Age of Parallel Image Processing (which ended because the serial computers improved faster than people were able to write parallel algorithms). But for a general solution, I was thinking mainly about the simpler functions, like pixel transformations, filters, morphology, local edge detectors, perhaps geometric transformations and warping. Ulli -- ________________________________________________________________ | | | Ullrich Koethe Universitaet Hamburg / University of Hamburg | | FB Informatik / Dept. of Informatics | | AB Kognitive Systeme / Cognitive Systems Group | | | | Phone: +49 (0)40 42883-2573 Vogt-Koelln-Str. 30 | | Fax: +49 (0)40 42883-2572 D - 22527 Hamburg | | Email: u.koethe@computer.org Germany | | koethe@informatik.uni-hamburg.de | | WWW: http://kogs-www.informatik.uni-hamburg.de/~koethe/ | |________________________________________________________________|

Hi Lubomir, let me throw in some more thoughts about a future imaging solution from another VIGRA developer, being out of scope of the review though. (Sorry if I am repeating topics from previous discussions; I was too busy in the last weeks to take part in them or even to read all messages carefully.) On Thursday, 02. November 2006 03:48, Lubomir Bourdev wrote:

As I already mentioned, we also would like very much to see image processing algorithms added to GIL, whether or not it is accepted in Boost. In particular, the Vigra algorithms provide a good starting point.

Actually, this sounds like a good idea from a users' POV, but as a VIGRA developer, it brings the bitter taste of submitting precious work results to a rival project... ;-} In the process of creating an improved imaging library solution from all the mentioned projects, would you be willing to consider a name change or sth. like that? E.g. boost::imaging or similar? I could imagine that more developers would be attracted to such a "neutral" successor, compared to "giving up" their own libraries and working on GIL.

We also are looking for volunteers to implement other algorithms, such as the ones in OpenCV. It would be great to also have a GIL-layer over Intel's IPP library. Also, providing interfaces to AGG would be terrific. Maybe a look at the ITK can be inspiring, too.

OTOH, one wants to have compatibility with linear algebra libraries, geometry/drawing libraries (AGG / cairo / freetype / ...) and scripting languages. In the last years, I have put quite some work into our VIGRA python bindings (using boost::python), which already exist in their second main incarnation (alas, not officially released yet) and I would like to work on boost::imaging python bindings, too then. (Our latest idea for a major overhaul would be to use NumPy arrays behind the scenes, providing even more compatibility and re-usability of existing, clever solutions.) Just to mention yet another important component of an imaging system.

More I/O formats too.

Do you plan to bring the GIL io layer into boost, too? While I do not see our "impex" (import-export) library as being perfect at all, I really think it is far superior to the GIL one. One major advantage (besides supporting more filetypes) is that we have a system for pluggable readers/writers which are automatically chosen depending on the file type (file header for reading, filename extension on writing).

There is a function called resize_clobber_image. [...] It does not copy the pixels. This could be done manually with copy_pixels if needed. We are looking for a better name, but resize_image implies the pixels are copied (and truncated) as with vector resize. I would prefer the simple, STL-compatible name resize(), accepting the different^H^H^H^H^H^H^H^H^Hadapted semantics.

We didn't add copying of the pixels because doing so is often not necessary, and it is unclear what the copy policy should be exactly. (copy into top left? Truncate?) Agreed.

I will try to catch up with the previous discussions and participate more often during the next weeks. Greetings, Hans