Page MenuHomePhabricator
Create Task
Maniphest T13953

Crash on 4D data cropping
Closed, ResolvedPublic
Actions

Description

The application will crash when cropping 4D dataset. The problem comes from the line

unitSpacingImageFilter->SetInput(input->GetVtkImageData(m_TimeStep));

in mitkExtractSliceFilter.cpp. At some point, the volume spacing of the VtkImageData is zero which cause the application to crash. A simple fix is to set the volume spacing before calling GetVtkImageData()

mitk::Image::ImageDataItemPointer volume = input->GetVolumeData(m_TimeStep);
const float* spacing = input->GetSlicedGeometry(m_TimeStep)->GetFloatSpacing();
double dspacing[3] = {spacing[0], spacing[1], spacing[2]};
volume->GetVtkImageData(input)->SetSpacing(dspacing);
unitSpacingImageFilter->SetInput(input->GetVtkImageData(m_TimeStep));

Te code above is also located in the GetVtkImageData() method. I don't know why (yet!) the spacing is not set properly the first time.

Related Objects
Search...

StatusAssignedTask
 DuplicateNoneT14117 Memory leak when using mitk::Image::GetVtkImageData
 ResolvedNoneT13953 Crash on 4D data cropping

Event Timeline

morency added projects: MITK, Bugzilla.Dec 11 2012, 5:04 PM
morency added a subscriber: morency.Dec 12 2012, 11:10 PM

The problem comes from the line

ImageDataItemPointer volume=GetVolumeData(t, n);

in mitkImage.cpp. The smart pointer 'volume' gets deleted at the end of the GetVtkImageData(int, int) method thus setting a null pointer for the volume spacing. I'm not sure as to why the reference count of the pointer falls to zero. If anyone has a hint on this, it is welcome.

The fix is to replace the smart pointer with a standard pointer

ImageDataItem* volume=GetVolumeData(t, n);

and to change the next 'if' condition to

if(volume.IsNull() || volume->GetVtkImageData(this) == NULL)

I would like feedback on this.

morency added a comment.Dec 13 2012, 12:33 AM

Here's some more info...

The line

ImageDataItemPointer volume=GetVolumeData(t, n);

in mitkImage.cpp returns m_Volumes[0] and is manager by the ImageDataItemPointer. The next line

if(volume.GetPointer() == NULL || volume->GetVtkImageData(this) == NULL)

calls volume->GetVtkImageData(this). The methods GetVtkImageData(this) in mitkImageDataItem.cpp looks like

mitk::ImageVtkAccessor* mitk::ImageDataItem::GetVtkImageData(
  mitk::ImagePointer iP) const
{
  if(m_VtkImageData==NULL)
    ConstructVtkImageData(iP);
  return m_VtkImageData;
}

Since m_VtkImageData is 0 at this point, the ConstructVtkImageData(iP) method is called. In this method, the line

mitk::ImageVtkAccessor *inData = ImageVtkAccessor::New(iP);

created a new iP->m_Volumes array. At this point, the only reference to the old m_Volumes is by the 'volume' variable (see above). At the end of the method, the reference count of the 'volume' variable holding an ImageDataItemPointer falls to zero. The problem is that

volume->m_VtkImageData == volume->GetVtkImageData(this)

where 'volume->GetVtkImageData(this)' is the ImageVtkAccessor* returned by the GetVtkImageData(int, int) in mitkImage.cpp. Since the reference count of the variable holding it falls to zero, the m_VtkImageData is Deleted(), thus invalidating the ImageVtkAccessor returned by the GetVtkImageData(int, int) method. The crash comes from the SetInput() call with an invalid ImageVtkAccessor (where GetSpacing() == 0) in mitkExtractSliceFilter.cpp

Any suggestions?

morency added a comment.Dec 13 2012, 5:08 PM

I made a WIP branch on my Github [1]. I'm not sure that the fix is good since the data hold by the ImageDataItemPointer needs to be cleared at some point and I'm not sure it is if we change the data type to a standard pointer. This needs to be investigated but I'm running after time at the moment. Any suggestions are welcome.

[1]: https://github.com/fmorency/MITK/tree/bug-13953-CrashOn4DDataCropping
• goerres added a subscriber: • goerres.Jan 9 2013, 3:41 PM

Thank you for the detailed bug description and the provided fix. Your fix is good and it is possible to change the ImageDataItem smartpointer to a standard pointer because the ImageDataItem is still hold by a smartpointer in the Image class. However, I would call this fix a workaround.

The actual problem is a side effect in the interplay of the classes Image, ImageDataItem and ImageVtkAccessor. Most challenging is the on-demand behavior of ImageDataItems, which are only created and stored in Image if they are needed.

morency added a comment.Jan 9 2013, 3:48 PM

@Joseph, Do you need an official pull request on Github or if you can manage to merge the workaround from the link to my branch?

• goerres added a comment.Jan 9 2013, 5:59 PM

New remote branch pushed: bug-13953-4DdataCroppingCrash-RemoveSideEffectsInImageDataAccess

• goerres added a comment.Jan 9 2013, 6:08 PM

I am working on a more general solution to remove side effects regarding image data access.

However, I am not able to reproduce a crash when cropping 4d image data with the current master (Linux). I tried it with three different 4d images. What operating system do you use and does the crash also occur with the current master?

morency added a comment.Jan 9 2013, 7:11 PM

@Joseph: The crash still occurs with the current master. I attached a screenshot showing the location of the bounding box. I will email you a link to the dataset used for testing.

morency added a comment.Jan 9 2013, 7:12 PM

Crop selection

cropSelection_20130109_01.png (1×1 px, 193 KB)

• goerres added a comment.Jan 16 2013, 4:27 PM

The bug is fixed with the current bug branch.

The side effects are also responsible for T14117. With a second commit, a circular smart pointer dependency is resolved.

morency added a comment.Jan 18 2013, 4:58 PM

I can confirm that the bug is fixed with the current bug branch

• fetzer added a subscriber: • fetzer.Jan 23 2013, 1:33 PM

Updating TargetMilestone

• goerres added a comment.Jan 23 2013, 3:04 PM

[64d68e]: Merge branch 'bug-13953-4DdataCroppingCrash-RemoveSideEffectsInImageDa

Merged commits:

2013-01-16 14:55:03 Joseph Görres [066782]
replaced Image smartpointer to a weakpointer in ImageVtkAccessor

2013-01-09 17:40:01 Joseph Görres [6dfccc]
changed the interplay of Image, ImageDataItem and ImageAccessors in order to remove side effects

graser mentioned this in T14117: Memory leak when using mitk::Image::GetVtkImageData.Jan 22 2013, 9:47 AM
kislinsk merged a task: T14117: Memory leak when using mitk::Image::GetVtkImageData.Aug 1 2016, 10:47 PM
kislinsk edited projects, added MITK (2013-03); removed MITK.
MITK · MIC · IMSY · DKFZ