[Vxl-maintainers] Solution to recent vil questions.

[Ian S. <ian...@st...>]

Summary:
My previous emails were wrong - Instead:
1 DO use vil_image_resource_sptr as a pixel-type-agnostic image
  container.
2 Please don't use vil_image_view_base_sptr for any VXL API other
  than as a temporary during assignments.

Details:

I apologise for all the confusion in my previous emails - I went for the
first obvious, simple and wrong solution. I am surprised to find that we did
actually think very hard when we designed vil and already had correct
solutions to these sorts of problems.



The functionality that people are looking for in the recent discussion
appears to be as follows:

An image type where we do not want to know the pixel type, but which we can
still process easily, and convert to a known pixel type image on demand.



In recent emails I had suggested that vil_image_resource was not the
solution to this problem. However looking back over the design notes, and
code, this problem is one that vil_image_resource was specifically designed
to solve.

For example, you can solve Mark's original request as follows
// The image we want to store without worrying about pixel type
vil_image_view<T> input;
// The place we will store the image.
vil_image_resource_sptr container = vil_new_image_resource(
  input.ni(), input.nj(), input.nplanes(), input.pixel_format());
container->put_image(input);
...
// Now we want to retrieve the image to actually do something with it
// for which we have to know the pixel type
switch (output->pixel_format())
{
  case VIL_PIXEL_FORMAT_BYTE:
  {
    vil_image_view<vxl_byte> output =
      container->get_view(0, 0, container->ni(), container->nj());
    ...
  }
  ...
}


It can be argued (as I mistakenly did) that the API of
vil_image_resource_sptr is too heavy weight for the current set of problems.
However it looks as if we wrote the code to avoid most unnecesarry work.
When you are using an in-memory vil_image_resource_sptr (as created by
vil_new_image_resource) get_view returns a reference to the underlying data.
If you do not mess with the view parameters, put_view becomes a
no-operation.

At the monent there is an unecessary malloc/free present in the above code.
I will add a function to avoid this:

vil_image_resource_sptr
  vil_new_image_resource_of_view(vil_image_view_base &input)

For people users who are not interested in the very-large-image support of
vil_image_resource, then the necessity of specifying the view size may be
somewhat annoying. I will add default get_view, get_copy_view and put_view
functions which will assume you want the full image. I don't consider such
functions to really add to the size of the API since they will act like
default parameters. The above code would then become

vil_image_view<T> input;
vil_image_resource_sptr container =
  vil_new_image_resource_of_view(input);
...
switch (output->pixel_format())
{
  case VIL_PIXEL_FORMAT_BYTE: {
    vil_image_view<vxl_byte> output = container->get_view();
    ... }
  ...
}



We can also trivially solve what I understand by Amitha's request for a
clone using vil_smart_ptr's copy
vil_image_resource_sptr cloned_image = container;



Now to the philosophy / design principles:
Peter's warning about letting clients solve the similar problems in
different ways, is almost the opposite of one of TargetJr's problems -
allowing too much API to be added to core classes to solve local client
desires. The original design plans for vil2 took account of both porblems,
and stated that either

1 You know data type, and can directly and immediately access the
  pixel values - in which case you want a vil_image_view<T >
or
2 You may not know the data type, or pixel ordering, or
  immediately access the pixel - in any of which cases you want a
  vil_image_resource_sptr

The problem we are looking at does fit into option 2, so vil expects you to
use a vil_image_resource_sptr. The background overheads when using it are
not large, and consist of
1 things necessary to deal with run-time types (e.g. switch
  statements)
2 extra error checking.


Why not use vil_image_view_base_sptr?
vil_image_view_base_sptr was designed to be a temporary value that was used
only to convert from a vil_image_resource or vil_load to a vil_image_view.
If we encourage the use of vil_image_view_base_sptr as a normal storage type
then

1. We are treating vil_image_view_base_sptr as a container. Providing vsl IO
for it would be expected, but is non-trivial.

2. There will also be an otherwise unnecessary demand for complicated
type-independent image processing functions ,e.g.
vil_image_view_base_sptr vil_corner_detector(vil_image_view_base_sptr &);
which just increases the size of the vil API.

3. Because of the base class relation there will be a demand for several
extra 'useful' methods to be added to vil_image_view, because polymorphism
is prettier and possibly slightly cheaper than lots of switch statements
(although I hope out compilers are optimising the switch statements to table
look-ups, rather than strings of comparison and branch instructions.) Again
this increases the size of the API.



We have no intention of preventing users from doing anything they want with
the code. In that spirit, I offer a suggestion of how Amitha can implement
his clone-like operation on a vil_image_view_base_sptr without having to
modify vil.

static vcl_vector<vil_image_view_base_sptr> view_list;void
dd_view( vil_image_view_base const& v )
{
  switch(vil_pixel_format_component_format(v.pixel_format())
  {
  case VIL_PIXEL_FORMAT_BYTE:
    view_list.push_back(new vil_image_view<vxl_byte>(v));
    break;
  case VIL_PIXEL_FORMAT_SBYTE:
    view_list.push_back(new vil_image_view<vxl_sbyte>(v));
    break;
  case VIL_PIXEL_FORMAT_UINT_32:
    view_list.push_back(new vil_image_view<vxl_uint_32>(v));
    break;
  case VIL_PIXEL_FORMAT_UINT_16:
    view_list.push_back(new vil_image_view<vxl_uint_16>(v));
    break:
  case VIL_PIXEL_FORMAT_INT_32:
    view_list.push_back(new vil_image_view<vxl_uint_32>(v));
    break;
  case VIL_PIXEL_FORMAT_INT_16:
    view_list.push_back(new vil_image_view<vxl_int_16>(v));
    break:
  case VIL_PIXEL_FORMAT_BOOL:
    view_list.push_back(new vil_image_view<bool>(v));
    break:
  case VIL_PIXEL_FORMAT_FLOAT:
    view_list.push_back(new vil_image_view<float>(v));
    break:
  case VIL_PIXEL_FORMAT_DOUBLE:
    view_list.push_back(new vil_image_view<double>(v));
    break:
  }
}

Of course I think you might prefer :-)

static vcl_vector<vil_image_resource_sptr> resource_list;
void add_view( vil_image_view_base const& v )
{
  resouce_list_.push_back(vil_new_image_resource_of_view(v));
}




Tim and I are not totally convinced that we have the right solution, and are
more than happy to debate this further. In particular if anybody feels we
haven't understood the problem/requirements we'd be grateful for any
corrections.

Ian and Tim
ISBE, Manchester