Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issues with using ChangeInformationImageFilter on CudaImage #201

Closed
Hakkk2002 opened this issue Jul 20, 2018 · 5 comments
Closed

Issues with using ChangeInformationImageFilter on CudaImage #201

Hakkk2002 opened this issue Jul 20, 2018 · 5 comments

Comments

@Hakkk2002
Copy link
Contributor

There seems to be a CPU-GPU synchronization issue if an itk::ChangeInformationImageFilter<itk::CudaImage<...>> is inserted before a Cuda filter. It works if the ChangeInformationImageFilter is inserted before a CPU filter.

The issue can be reproduced with the rtkSartCudaTest and the following patch:

File: include/rtkSARTConeBeamReconstructionFilter.hxx

@@ -18,10 +18,11 @@
 
 #ifndef rtkSARTConeBeamReconstructionFilter_hxx
 #define rtkSARTConeBeamReconstructionFilter_hxx
 
 #include "rtkSARTConeBeamReconstructionFilter.h"
+#include <itkChangeInformationImageFilter.h>
 
 #include <algorithm>
 
 namespace rtk
 {
@@ -282,10 +283,15 @@ SARTConeBeamReconstructionFilter<TVolumeImage, TProjectionImage>
 
   // Declare the image used in the main loop
   typename TVolumeImage::Pointer pimg;
   typename TVolumeImage::Pointer norm;
 
+  // test: insert itkChangeInformationImageFilter
+  itk::ChangeInformationImageFilter<VolumeType>::Pointer change = itk::ChangeInformationImageFilter<VolumeType>::New();
+  change->SetInput(this->GetInput(0));
+  m_ForwardProjectionFilter->SetInput(1, change->GetOutput());
+
   // For each iteration, go over each projection
   for(unsigned int iter = 0; iter < m_NumberOfIterations; iter++)
     {
     unsigned int projectionsProcessedInSubset = 0;
     for(unsigned int i = 0; i < nProj; i++)
@@ -326,11 +332,11 @@ SARTConeBeamReconstructionFilter<TVolumeImage, TProjectionImage>
         else
           pimg = m_AddFilter->GetOutput();
         pimg->Update();
         pimg->DisconnectPipeline();
 
-        m_ForwardProjectionFilter->SetInput(1, pimg );
+        change->SetInput(pimg);
         m_AddFilter->SetInput2(pimg);
         m_BackProjectionFilter->SetInput(0, m_ConstantVolumeSource->GetOutput());
         m_BackProjectionNormalizationFilter->SetInput(0, m_ConstantVolumeSource->GetOutput());
 
         projectionsProcessedInSubset = 0;

Results from the rtkSartCudaTest:

****** Case 1: Voxel-Based Backprojector ******

Error per Pixel = 0.0206672
MSE = 0.00282352
PSNR = 31.5127dB
QI = 0.989666


Test PASSED!


****** Case 2: Voxel-Based Backprojector, OS-SART with 2 projections per subset ******

Error per Pixel = 0.0225105
MSE = 0.00346037
PSNR = 30.6294dB
QI = 0.988745


Test PASSED!


****** Case 3: Joseph Backprojector ******

Error per Pixel = 0.0189275
MSE = 0.00259909
PSNR = 31.8724dB
QI = 0.990536


Test PASSED!


****** Case 4: CUDA Voxel-Based Backprojector ******

Error per Pixel = 16.5716
MSE = 525.278
PSNR = -21.1833dB
QI = -7.28582
Test Failed, Error per pixel not valid! 16.5716 instead of 0.032

See also the discussion in the thread here.

@Hakkk2002
Copy link
Contributor Author

What if in itk::CudaImage::SetPixelContainer, besides passing the pixel container, also pass the cuda data manager and retain the CPU/GPU dirty flags of the manager? Maybe this works for the case of itk::ChangeInformationImageFilter but I don't know if it is generic for all cases that calls itk::CudaImage::SetPixelContainer.

@Hakkk2002
Copy link
Contributor Author

Well, this seems more complicated than what I thought... it needs a method to obtain the data manager from the pixel container...

@SimonRit
Copy link
Collaborator

I see two solutions at this stage:

  • modifying itk::ChangeInformationImageFilter to make sure that the CPU buffer is up-to-date. I think the same problem will occur with itk::GPUImage (for OpenCL) so it's worth posting a patch to ITK.
  • create a CudaImage specific ChangeInformationImageFilter.

@Hakkk2002
Copy link
Contributor Author

Hakkk2002 commented Jul 24, 2018

I've done a little bit more investigation and the previous thought may be wrong.
itk::ChangeInformationImageFilter::GenerateData() calls nonConstInput->GetPixelContainer() to get the pixel container of the input, which does a force update of CPU buffer.

I am thinking if it is because although itk::ChangeInformationImageFilter does update the input CPU buffer but it doesn't point the CudaDataManager::m_CPUBuffer of the output to the updated CPU buffer of the input (or, this may be done by SetPixelContainer of CudaImage, like once the pixel container has been reassigned, take care of the m_CPUBuffer as well).

Hakkk2002 added a commit to Hakkk2002/RTK that referenced this issue Jul 25, 2018
…(...)

In CudaImage<...>::SetPixelContainer(...), after setting a new pixel
container, the CPU buffer pointer of the CudaImage's CudaDataManager shall
also be updated. This is a fix for Issue RTKConsortium#201.
@SimonRit
Copy link
Collaborator

Problem fixed by commit 82ba699.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants