[Insight-developers] Image memory model

[Dan Mueller]

Hi Karthik (and developers),

Thanks for your detailed response regarding the implementation of a
slice contiguous image. Sorry I have taken so long to write back, but
it has been hard to find the time to work on this issue.

Regarding the design of the pixel accessors: you're right, it's not
"too good to be true", it is just true! What a wonderful design!

I have used your ideas and now have a working
itk::SliceContiguousImage into which I can import slices from
non-contiguous memory for use with linear iterators (and filters which
only use linear iterators). I still need to polish up the API and
think about the neighborhood accessor, but things are looking good!

A few further questions:

1. What should SliceContiguousImage do with GetBufferPointer()?
In my current design, GetBufferPointer() simply returns 0. I guess
this is logical because there is no "buffer", rather an array of
pointers to slices -- currently stored as std::vector< PixelType * >.
With this design the new SliceContiguousPixelAccessor must be given
access to both the array of slices and the buffered region size. I
think this could be improved if GetBufferPointer() were to return the
pointer to the list of slices rather than 0. Do you think this is too
misleading (the name of the method no longer describes fully what is
being returned)? If it is not too misleading, then the only extra
information required by the pixel accessor is the buffered region
size; it could recover the slices from the input pointer. I like this
design, but it somewhat involves hijacking the GetBufferPointer()
method...

2. Is it misleading if SliceContiguousPixelContainer actually
allocates slices in contiguous memory?
The new container will allow users to import memory from
non-contiguous slices, but what should it do if allocating the memory
itself? I guess it is easier to allocate contiguous memory (ie. a
single buffer in contiguous memory), as long as the user is aware
there is no guarantee SliceContiguousImage has a contiguous memory
footprint...

3. Not all filters will work with the new SliceContiguousImage. How
should this be indicated?
In the case of VectorImage, new vector-based filters had to be added
because the image type did not work with some of the existing filters.
This is (thankfully) not the case for SliceContiguousImage -- it
*should* work with most existing filters/functions/etc. Some examples
of potential non-interoperable filters include:
RayCastInterpolateImageFunction, WatershedImageFilter,
itkVTKImageExport, itkImageFileReader, itkImageFileWriter. Obviously
it would be good to document and/or inform the user of incorrect
usage. Perhaps an "\ingroup SliceContiguousImageFilter" tag can be
added to supported filters? Could concept checking also be used to
help guide the user?

That's all for now, although I might have a few more questions when I
take a look at the neighborhood accessor.

Thanks again, Dan

2008/5/26 Karthik Krishnan <karthik.krishnan at kitware.com>:
> Dan:
>
> On Sun, May 25, 2008 at 2:41 AM, Dan Mueller <dan.muel at gmail.com> wrote:
>>
>> Image::GetBufferPointer() is used extensively within the toolkit. Take
>> a look at the constructors for ImageConstIterator,
>> ConstNeighborhoodIterator, ImageReverseConstIterator,
>> ImageConstIteratorWithIndex, etc. I have added a list to
>>    http://www.itk.org/Wiki/Proposals:Slice_contiguous_images
>> however I realise now that some of these are
>> ImportImageContainer::GetBufferPointer() (I'll fix this up when time
>> permits).
>
> That should not be a cause of concern
>   image->GetBufferPointer() is just a pointer to the first pixel in
> the image. It is the dereferencing (m_BufferPointer + offset) that's
> of concern.
>
>>
>> Unfortunately, while your suggestion of using the PixelAccessor and
>> PixelAccessorFunctor would have been easy, I think it is too good to
>> be true.
>
> It is true.
>
>> The assumptions regarding contiguous memory allocation are
>> deeply entrenched.
>
> These classes were introduced a year or two ago, precisely because we
> had to support an alternate memory model image for NAMIC.
>
>>
>> Take for example the very first step you discussed, the act of passing
>> the input pixel value from the iterator.Get() function to the
>> PixelAccessorFunctor:
>>>   PixelType iterator.Get()   reads
>>>    { return pixelAccessorFunctor.Get(*(m_Buffer+m_Offset)); }
>
> The variable is passed by reference, however it is later dereferenced
> by the functor to the corrent pointer. The VectorPixelAccessor reads:
>
>  inline ExternalType Get( const InternalType & input, const unsigned
> long offset ) const
>    {
>    ExternalType output( (&input)+(offset*m_OffsetMultiplier) ,
> m_VectorLength );
>    return output;
>    }
>
> ie the offset is passed in and based on the vector length instead of
> returning *(m_Buffer+m_Offset) that an itk::Image would do, the
> itk::VectorImage returns *(m_Buffer + m_Offset * m_VectorLength ) .
>
>>
>> This deference *(m_Buffer+m_Offset) will access potentially invalid
>> memory in the slice-contiguous case (m_Buffer is the result of
>> m_Image->GetBufferPointer(), the start of the contiguous array, or the
>> start of the first slice for my case).
>
> I realize that it would have been better to pass in the offset and the
> buffer_start_pointer in, to avoid invalid de-references. (I think
> passing them by reference avoids the de-reference). So we could change
> the signature of the the accesssor to
>
>  inline ExternalType Get( const InternalType & begin, const unsigned
> long offset ) const
>
> and the iterator would read:
>
>  PixelType Get(void) const
>    { return m_PixelAccessorFunctor.Get(m_Buffer, m_Offset); }
>
> instead of
>
>  PixelType Get(void) const
>    { return m_PixelAccessorFunctor.Get(*(m_Buffer+m_Offset)); }
>
>
>> There are other places this assumption is also made: for example
>> ImageConstIteratorWithIndex::SetIndex() uses the buffer pointer to
>> compute the index.
>
> Again the starting pointer should not matter. All attempts to
> dereference the pointers, via Set or Get go through the accessors.
>
>>
>> Have I misinterpreted your meaning? I think either m_Buffer and/or
>> m_Offset would need to altered from their original meaning to work for
>> slice-contiguous memory...
>
> No. I don't think so. In your case,
>
> m_Buffer would be any arbitrary value say 0.
> m_Offset would be the Nth pixel into the image.
>
> Any attempt to access the Nth pixel would go through the accessor,
> which would have start pointers to all the slices in your image and
> would read something like:
>
>  inline ExternalType itk::SlicePixelAccessor::Get( const InternalType
> & begin, const unsigned long offset ) const
>    {
>    const unsigned long slice = offset/(x_size*y_size);
>    const unsigned long offset_into_slice = offset % (x_size*y_size);
>    return *(m_SlicePointer[slice] + offset_into_slice);
>    }
>
>
> --
> Hope this helps.
>
> Thanks
> --
> Karthik Krishnan
> R&D Engineer,
> Kitware Inc.
>