Re: [boost] [GIL] review

From: Lubomir Bourdev (lbourdev_at_[hidden])
Date: 2006-10-25 20:29:43

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