[Insight-developers] RE: Adaptor update mechanism

[Miller, James V (Research)]

Karthik, 

Then your modification to UpdateOutputData() is the appropriate fix.

(It might be good to change the method ComputeOffsetTable(), ComputeOffset(), ComputeIndex()
 to something like below just to be safe. This avoids accessing m_BufferedRegion and always calls
GetBufferedRegion(). )


::ComputeOffsetTable()
{
  OffsetValueType num=1;
  const SizeType& bufferSize = this->GetBufferedRegion().GetSize();
  
  m_OffsetTable[0] = num;
  for (unsigned int i=0; i < VImageDimension; i++)
    {
    num *= bufferSize[i];
    m_OffsetTable[i+1] = num;
    }
}

OffsetValueType ComputeOffset(const IndexType &ind) const
  {
    // need to add bounds checking for the region/buffer?
    OffsetValueType offset=0;
    const IndexType &bufferedRegionIndex = this->GetBufferedRegion().GetIndex();
  
    // data is arranged as [][][][slice][row][col]
    // with Index[0] = col, Index[1] = row, Index[2] = slice
    for (int i=VImageDimension-1; i > 0; i--)
      {
      offset += (ind[i] - bufferedRegionIndex[i])*m_OffsetTable[i];
      }
    offset += (ind[0] - bufferedRegionIndex[0]);

    return offset;
  }

IndexType ComputeIndex(OffsetValueType offset) const
  {
    IndexType index;
    const IndexType &bufferedRegionIndex = this->GetBufferedRegion().GetIndex();
    
    for (int i=VImageDimension-1; i > 0; i--)
      {
      index[i] = static_cast<IndexValueType>(offset / m_OffsetTable[i]);
      offset -= (index[i] * m_OffsetTable[i]);
      index[i] += bufferedRegionIndex[i];
      }
    index[0] = bufferedRegionIndex[0] + static_cast<IndexValueType>(offset);

    return index;
  }


-----Original Message-----
From: Karthik Krishnan [mailto:Karthik.Krishnan at kitware.com]
Sent: Wednesday, June 01, 2005 4:18 PM
To: Miller, James V (Research)
Cc: Insight-developers (E-mail); Luis Ibanez
Subject: Re: Adaptor update mechanism


Hi Jim,

Thanks again for looking at this. As you just mentioned, that does solve 
the the problem too.  The ComputeOffset() function in ImageBase.h would 
then need an extra line
    m_BufferedRegion = this->GetBufferedRegion();

With this solution there is an implementation issue though. 
ComputeOffset() is meant to be a const method.

Thanks
Regards
karthik


Miller, James V (Research) wrote:

>Karthik, 
>
>That is probabaly a reasonable fix. I am still not sure why it should
>matter whether the ImageAdapter has a proper m_BufferedRegion.  If all
>access to m_BufferedRegion is via GetBufferedRegion(), the adaptor 
>should have delegated down to the internal image.  The only place
>I saw that m_BufferedRegion was accessed directly was in ComputeOffsetTable(), 
>ComputeOffset() and ComputeIndex() (all in ImageBase).
>
>Did you try changing these methods to call GetBufferedRegion()
>instead of accessing m_BufferedRegion directly?
>
>That being said, it is probably best that the BufferedRegion ivar 
>in the ImageAdaptor be kept consistent with the underlying image.
>So calling this->SetBufferedRegion( m_Image->GetBufferedRegion() )
>is a good idea.
>
>Jim
>
>
>
>-----Original Message-----
>From: Karthik Krishnan [mailto:Karthik.Krishnan at kitware.com]
>Sent: Wednesday, June 01, 2005 1:04 PM
>To: Miller, James V (Research)
>Cc: Insight-developers (E-mail); Luis Ibanez
>Subject: Re: Adaptor update mechanism
>
>
>Hi Jim,
>
>Thank you very much for looking at it.
>
>The changes I was thinking of in the last mail were incorrect. I think 
>I've found it now though.
>
>First, I re-ran the example on Linux and on VS71 and ImageAdaptor2 
>crashes if reader->Update() is not called prior to adaptor->SetImage(). 
>
>As you mentioned, the m_BufferedRegion was not being set correctly, 
>because the ImageAdaptor calls SetBufferedRegion() only in the function 
>SetImage(). So, if a pipeline is set-up prior to execution, I don't see 
>how the buffered region could be propagated correctly.
>
>I did fix it though by adding SetBufferedRegion( 
>m_Image->GetBufferedRegion() ) right after the m_Image updates its 
>output data.
>
>So  ImageAdaptor::UpdateOutputData() would be :
>
>ImageAdaptor<TImage , TAccessor>::UpdateOutputData()
>{
>  // call the superclass' method first, then delegate
>  Superclass::UpdateOutputData();
>
>  // delegation to internal image
>  m_Image->UpdateOutputData();
>  SetBufferedRegion( m_Image->GetBufferedRegion() );
>}
>
>That fixes it, but I am not sure if it is the correct thing to do.
>
>Thanks
>regards
>karthik
>
>PS: Here were the command line params I used to run ImageAdaptor2 (with 
>nightly CVS source)
>~/work/ITK/binaries/Insight/Nightly/bin/ImageAdaptor2 
>~/work/ITK/src/Insight/Nightly/Examples/Data/VisibleWomanEyeSlice.png 
>redEye.png
>
>
>Miller, James V (Research) wrote:
>
>  
>
>>Karthik, 
>>
>>I'd have to look at this in more detail.  Your suggested changes do not seem
>>correct to me.  
>>
>>ImageAdaptor2 works fine for me if I remove the call to reader->Update().
>>(Using .Net 2003).
>>
>>The LargestPossibleRegion and the RequestedRegion should have been propagated
>>through the adaptered already.  If there is a problem, it might be somewhere 
>>where m_BufferedRegion is accessed directly instead of via an access method.
>>
>>The only place I see this happening is in ComputeOffsetTable(). We could have
>>that method call GetBufferedRegion().  However, I cannot duplicate the error 
>>you are getting.
>>
>>
>>Jim
>>
>>
>>-----Original Message-----
>>From: Karthik Krishnan [mailto:Karthik.Krishnan at kitware.com]
>>Sent: Tuesday, May 31, 2005 5:22 PM
>>To: Insight-developers (E-mail); Luis Ibanez; Miller, James V (Research)
>>Subject: Adaptor update mechanism
>>
>>
>>Hi,
>>
>>The adaptors don't seem to propagate information when plugged into the 
>>pipeline. For example ImageAdaptor2 example segfaults if 
>>reader->Update() is commented out.
>>
>>I believe that  the Update() call in ImageAdaptor.txx should be
>>
>>//----------------------------------------------------------------------------
>>template <class TImage, class TAccessor >
>>void
>>ImageAdaptor<TImage , TAccessor>
>>::Update()
>>{
>> Superclass::Update();
>>
>> Superclass::SetLargestPossibleRegion( 
>>m_Image->GetLargestPossibleRegion() );
>> Superclass::SetBufferedRegion( m_Image->GetBufferedRegion() );
>> Superclass::SetRequestedRegion( m_Image->GetRequestedRegion() );
>> m_Image->Update();
>>}
>>
>>instead of what it is now. Please correct me if I am mistaken. I don't 
>>want to commit incorrect code since I am not familiar enough with the 
>>pipeline mechanism.
>>
>>Thanks
>>Regards
>>Karthik
>>
>>
>> 
>>
>>    
>>
>
>  
>