Re: [boost] [gil] Ideas to make my extension more generic?

From: Christian Henning (chhenning_at_[hidden])
Date: 2007-04-26 15:33:46

Hi Lubomir, thanks a lot for the tips. It took me a couple of days to
implement them. Please read my comments below.

> This term is used for a different operation. You are scaling up the
> histogram to the full range, so maybe scale_histogram or
> spread_histogram is better.
I cannot quite follow you here. Could you elaborate on what you mean
with scaling up the histogram to the full range?

> While this algorithm is useful for increasing the contrast of the image,
> I wouldn't use it as the default when saving higher bit depth images to
> 8-bit formats. If you want to preserve as much as possible the original
> look of the image I suggest using color_converted_view to a form
> supported by the I/O format.
Again, useful for increasing the contrast? Seems my limited knowledge
is getting back on me, here. Could you also please describe what you
mean.

>
> Other suggestions regarding your code:
>
> 1. A better histogram spread computes the image histogram and uses the
> top/bottom N% value as the min/max value, instead of the absolute
> minimum and maximum. This is more robust to noise.

How would you implement it? I started my own implementation that would
create a vector<channel_t> for each channel. But I was running into
problems all over.

I tried creating a mpl::vector that contains a std::vector<channel_t>
for each channel using the mpl::transform1 algorithm. That works
flawlessly. For a rgb8_view_t. It would create something like:
mpl::vector< std::vector<uint8>, std::vector<uint8>,
std::vector<uint8> > .

template <typename T>
struct add_vector
{
   typedef std::vector<T> type;
};

template< class PIXEL
            , class VIEW >
inline PIXEL min_channel_values( const VIEW&  view
                        , const size_t percent = 1 )
{
   typedef mpl::transform1< color_space_type<VIEW>::type,
add_vector<mpl::_1> >::type         channels_t;

   return PIXEL();
}

Right now, I don't understand how to create such an object where all
vectors are resized to the correct max values. Do you have any ideas
on how to do that?

Having this structure in order it's easy to accumulate the channel
values for the whole view and to find the N% values.

Can you follow my design?

>
> 2. Consider taking out the "min pixel value" and "max pixel value" as
> separate stand-alone algorithms. They could be used in other contexts.
Done. I have one for the min, max and both values. Same will be done
for the N% ones.

>
> 3. You might be able to use std::min_element, std::max_element,
> boost::minmax_element coupled with nth_channel_view to find the min/max
> value for each channel in the image. It is simpler though it might be
> slower for interleaved images because we will be revisiting the pixels
> of the image K times (K == number of channels).
I like my current implementation and it works for planar and
interleaved images. For example for the min channel value I have:

struct set_to_max
{
   template <typename CHANNEL > void operator()(CHANNEL& c) const
   {
      c = channel_traits< CHANNEL >::max_value();
   }
};

template< class PIXEL
        , class VIEW >
inline
PIXEL min_channel_values( const VIEW& view )
{
   PIXEL min;

   // initialize the min pixel with the max values
   static_for_each( min, set_to_max() );

   for( int y=0; y < view.height(); ++y )
   {
      VIEW::x_iterator x_it = view.row_begin( y );

      for( int x = 0; x < view.width(); ++x )
      {
         typename PIXEL::const_reference p = x_it[x];

         for( int i = 0; i < num_channels<PIXEL>::type::value; ++i )
         {
            if( dynamic_at_c( p, i ) < dynamic_at_c( min, i ))
            {
               dynamic_at_c( min, i ) = dynamic_at_c( p, i );
            }
         }
      }
   }

   return min;
}

>
> 4. Never take a non-constant reference to a view. Image views are always
> constant. Mutability is part of their type.(line 65)
done

>
> 5. Don't assume that the reference type is value_type& (lines
> 91,120,121). In fact, this doesn't hold true for planar images. Use
> View::reference or View::const_reference as the return type of
> dereferencing the view iterators.
done

>
> 6. I would suggest making the transformation dst_max * (src - min) /
> (max - min) as a per-channel function object and using static_for_each
> to apply it to each channel. This is not only faster but will work for
> heterogeneous channels. Another advantage is that performance
> specializations can be provided for pairs of source/destination
> channels.
>
> You can then wrap it into a function object per pixel and use
> gil::for_each_pixel

I tried it but static_for_each only supports up to three pixels as
parameters. But I think I need four. One for the current src pixel,
current dst, src_min, and src_diff.

Here is what I have:

// channel_wise calculation.

// @todo need better name
template< typename DST_MAX >
struct foo
{
   foo( const DST_MAX& dst_max )
   : _dst_max( dst_max ){}

   template < typename DST_CHANNEL
            , typename SRC_CHANNEL >
   void operator()( DST_CHANNEL&  dst
                  , SRC_CHANNEL&  src
                  , SRC_CHANNEL&  min
                  , SRC_CHANNEL&  diff )
   const
   {
      if( diff == 0 )
      {
         dst = 0;

         return;
      }

      float d = ( static_cast<float>( _dst_max )
                * ( ( static_cast<float>( src ) - static_cast<float>( min ))
                  / static_cast<float>( diff ))));

      dst_channel = static_cast<DST_CHANNEL>( dst );
   }

   DST_MAX _dst_max;
};

// pixel_wise calculation.

// @todo need better name
template< typename SRC_VIEW
        , typename SRC_PIXEL
        , typename DST_VIEW
        , typename DST_MAX
        >
struct do_it
{
   do_it( const DST_VIEW&  dst_view
        , const DST_MAX&   dst_max
        , const SRC_PIXEL& min
        , const SRC_PIXEL& diff      )
   : _dst_view( dst_view )
   , _min( min )
   , _diff( diff )
   , _op( dst_max )
   {
      _dst_it = dst_view.begin();
   }

   void operator()( SRC_PIXEL& src )
   {
      static_for_each( *_dst_it
                     , src
                     , _min
                     , _diff
                     , _op    );
      ++_dst_it;
   }

   typename DST_VIEW::iterator _dst_it;

   foo<DST_MAX> _op;

   DST_VIEW _dst_view;

   SRC_PIXEL _min;
   SRC_PIXEL _diff;
};

int main(int argc, char* argv[])
{
      bits8 max = 255;
      rgb16_pixel_t min;
      rgb16_pixel_t diff;

      rgb16_image_t src( 640, 480 );
      rgb8_image_t dst( 640, 480 );

      do_it< rgb16_view_t
           , rgb16_pixel_t
           , rgb8_view_t
           , bits8
           > d( view( dst ), max, min, diff );

   for_each_pixel( view( src ), d );

   bmp_write_view( ".\\red.bmp", view( dst ));
}

Thank you such much for your time.

Christian