[Insight-developers] Image memory model
Luis Ibanez
luis.ibanez at kitware.com
Sun Jul 5 17:48:28 EDT 2009
Hi Dan,
I would suggest to keep the code updated in the NAMIC sandbox.
It makes a lot easier for everybody to access the code and
commit improvements.
BTW: This memory model is a great contribution to ITK.
We should keep it in the list of features for ITK 4.0.
Luis
-------------------
Dan Mueller wrote:
> Hi all,
>
> Wow, time flies when your having fun. I started this email thread over
> one year ago, but only now got to finishing everything off.
>
> I have just submitted an article to the Insight Journal with my first
> attempt at implementing some alternative memory models. The URL is not
> yet active, but once the submission is approved, it should live here:
> http://hdl.handle.net/1926/3068
>
> Question: should I update the NAMIC sandbox with my implementation, or
> just keep everything in the journal?
>
> Cheers, Dan
>
> 2008/6/2 Karthik Krishnan <karthik.krishnan at kitware.com>:
>
>>On Sun, Jun 1, 2008 at 7:28 PM, Jim Miller <millerjv at gmail.com> wrote:
>>
>>>Karthik,
>>>
>>>Have you verified that running the NeighborhoodIterators through the
>>>Functors/Accessors did not affect performance? If the extra level of
>>>indirection is not properly optimized out for the default case, then we
>>>could have introduced a performance overhead. Did you running any timing
>>>tests before/after this change?
>>
>>Yes I did. I do not remember noticing any significant reduction in
>>speed after this change (in release optimized mode).
>>
>>All methods are inlined and when unrolled yield the same instructions
>>as before. So we shouldn't be expecting any change either with
>>function inlining.
>>
>>
>>As an example, for the ConstNeighborhoodIterator; What used to be:
>>
>> PixelType GetCenterPixel() const
>> { return *( this->GetCenterPointer() ); }
>> PixelType GetPixel( int n, ...) const
>> {
>> ....
>> return (m_NeighborhoodAccessorFunctor.Get(this->operator[](n)));
>> }
>>
>>
>>was changed to
>>
>> PixelType GetCenterPixel() const
>> { return m_NeighborhoodAccessorFunctor.Get( this->GetCenterPointer() ); }
>> PixelType GetPixel(...) const
>> {
>> ....
>> return (*(this->operator[](n)));
>> }
>>
>>with the NeighborhoodAccessorFunctor reading:
>>
>> inline PixelType NeightborhoodAcecssorFunctor::Get( const
>>InternalPixelType *pixelPointer ) const
>> {
>> return (*pixelPointer);
>> }
>>
>>
>>Same holds true all the several other Set/Get access methods and
>>Boundary Condition accesses in all the iterators.
>>
>>Basically any direct dereferencing is forced to go through the
>>accessors. The default implementation of the accessor just
>>dereferences and returns the value. The converse is done for the Set
>>methods.
>>
>>These tests have been carried only on gcc. I made sure the functions
>>got inlined compiling with -WInline.
>>
>>Thanks
>>--
>>karthik
>>
>>
>>
>>
>>
>>
>>
>>>Jim
>>>
>>>On Sun, Jun 1, 2008 at 1:19 PM, Karthik Krishnan
>>><karthik.krishnan at kitware.com> wrote:
>>>
>>>>On Sun, Jun 1, 2008 at 12:35 PM, Jim Miller <millerjv at gmail.com> wrote:
>>>>
>>>>>I think the issue with this design is that the NeighborhoodIterators do
>>>>>not
>>>>>go through the PixelAccessors (for efficiency). In fact, the
>>>>>NeighborhoodIterators keep a "neighborhood" or pointers to pixels and
>>>>>know
>>>>>how to update those pointers via the iteration.
>>>>
>>>>I had added these a while ago, to support the VectorImage.
>>>>Please see itk::NeighborhoodIteratorFunctor and
>>>>itkVectorImageNeighborhoodAccessorFunctor.
>>>>
>>>>All iterators in ITK go through accessors/accessor functors.
>>>>
>>>>The NeighborhoodAccessorFunctors work just like the PixelAccessors,
>>>>however have a slightly different API, so as to gel with the
>>>>neighborhoodAccessors.
>>>>
>>>> inline PixelType Get( const InternalPixelType * ) const
>>>>
>>>> inline void Set( InternalPixelType* &pixelPointer, const PixelType
>>>>&p ) const
>>>>
>>>> inline PixelType BoundaryCondition(
>>>> const OffsetType& point_index,
>>>> const OffsetType &boundary_offset,
>>>> const NeighborhoodType *data,
>>>> const ImageBoundaryConditionConstPointerType boundaryCondition) const
>>>>
>>>>Dan, you must be familiar with the Set/Get methods, having written the
>>>>PixelAccessor for the SliceContiguousImage. The last method,
>>>>BoundaryCondition method is called whenever the operator() method is
>>>>accessed for the boundary conditions, so you can return the right
>>>>pointer there. In the accessors, we delegate this back to the boundary
>>>>condition (the slower alternative signature is used here). You will
>>>>notice that there are two signatures for each of the boundary
>>>>conditions, One of them takes the accessor itself as an argument.
>>>>
>>>>These accessors are obtained as traits of the image. All iterators are
>>>>tested for itk::VectorImage and itk::Image.
>>>>
>>>>So, I think this should be possible for the SliceContiguousImage as
>>>>well, by writing a itk::SliceContiguousNeighborhoodAccessorFunctor.
>>>>
>>>>Thanks
>>>>--
>>>>karthik
>>>>
>>>>
>>>>>Introducing the GetBufferPointer() method is one of things I regret most
>>>>>in
>>>>>the design of the ITK image. I never should have done that. If all the
>>>>>iterators worked of the concept of an "offset" from the first pixel
>>>>>(i.e.
>>>>>the number of pixels since the beginning of the buffer), then they could
>>>>>have indexed the pixels as PixelContainer[i] to get the ith pixel. The
>>>>>PixelContainer could then hide the memory layout from the Image class
>>>>>and
>>>>>from the Iterators. This would even allow PixelContainers that
>>>>>internally
>>>>>compressed the image data or store part of the image data on disk and
>>>>>part
>>>>>in memory.
>>>>>
>>>>>ImageRegionIterator can support such a referencing scheme easily.
>>>>>ImageRegionIteratorWithIndex would require a little bit of work to
>>>>>support
>>>>>the memory model. NeighborIterator would require a good deal of work to
>>>>>support such a model. These are all doable. It is just a matter of
>>>>>resources and a matter of efficiency (eg. do the neighborhood iterators
>>>>>get
>>>>>too slow in this new model).
>>>>>
>>>>>Outside of the Iterators, the IO mechanisms (including the Import/Export
>>>>>facilities for interfacing to other systems), and few poorly written
>>>>>filters, the use of GetBufferPointer() should be fairly limited and
>>>>>isolated.
>>>>>
>>>>>In practice, when I interface into systems that have a slice contiguous
>>>>>model, I either use ITK just on 2D slices and forgo 3D processing, or I
>>>>>make
>>>>>a volume contiguous image that is a copy of the slice contiguous image.
>>>>>These are not the best solutions but todays 64 bit world is currently
>>>>>much
>>>>>larger than medical image data set sizes.
>>>>>
>>>>>
>>>>>
>>>>>On Sun, May 25, 2008 at 8:28 PM, Karthik Krishnan
>>>>><karthik.krishnan at kitware.com> wrote:
>>>>>
>>>>>>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.
>>>>>>_______________________________________________
>>>>>>Insight-developers mailing list
>>>>>>Insight-developers at itk.org
>>>>>>http://www.itk.org/mailman/listinfo/insight-developers
>>
> _______________________________________________
> Powered by www.kitware.com
>
> Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html
>
> Please keep messages on-topic and check the ITK FAQ at: http://www.itk.org/Wiki/ITK_FAQ
>
> Follow this link to subscribe/unsubscribe:
> http://www.itk.org/mailman/listinfo/insight-developers
>
More information about the Insight-developers
mailing list