Itk: Valgrind defect detected in VectorGradientMagnitudeImageFilter with FixedArray

[blowekamp]

4 valgrind defect detected:

https://open.cdash.org/viewDynamicAnalysis.php?buildid=7011436

Sample:

{

   Memcheck:Cond
   fun:_ZNSt7__equalILb0EE5equalIPKfS3_EEbT_S4_T0_
   fun:_ZSt11__equal_auxIPKfS1_EbT_S2_T0_
   fun:_ZSt5equalIPKfS1_EbT_S2_T0_
   fun:_ZNK3itk10FixedArrayIfLj3EEeqERKS1_
   fun:_ZNK3itk10FixedArrayIfLj3EEneERKS1_
   fun:_ZN3itk34VectorGradientMagnitudeImageFilterINS_23RGBToVectorImageAdaptorINS_5ImageINS_8RGBPixelItEELj2EEEEEfNS2_IfLj2EEEE20SetDerivativeWeightsENS_10FixedArrayIfLj3EEE
   fun:_Z42itkVectorGradientMagnitudeImageFilterTest1iPPc
   fun:main
}

Using demangler.com I see the following is used: itk::VectorGradientMagnitudeImageFilter<itk::RGBToVectorImageAdaptor<itk::Image<itk::RGBPixel<unsigned short>, 2u> >, float, itk::Image<float, 2u> >::SetDerivativeWeights(itk::FixedArray<float, 3u>)

This type of defect is likely due to a value not fully being initialized.

[blowekamp]

@jhlegarreta It looks like you improved the testing recently.

This line does not look right to me:

auto                    componentWeightsValue = static_cast<FilterType::WeightsType::ValueType>(std::stod(argv[5]));

I believe WeightsType is a FixedArray.

[jhlegarreta]

Thanks for the heads up @blowekamp . Will have a look later today.

[jhlegarreta]

@blowekamp Commenting my thoughts in here after some investigation. Will submit a patch set once we agree on a solution.

This line does not look right to me:

auto componentWeightsValue = static_cast(std::stod(argv[5]));

I think the issue goes further than that/it does not stem from that line in reality.

We agree that on that line of the concerned test I am casting the program argument to the ValueType of the WeightsType array, and then I fill the array with this value on the next line, and that there is nothing wrong there (besides the missing typename keyword in the casting type that some compilers might complain about -have not checked in the dashboard, and would deserve a separate PR as it is a COMP issue; will do it once we fix this bug).

This is done frequently for the ITK array types if I'm not wrong, including FixedArray, as shown in here or here.

However, when debugging the code, and reading through the types, one realizes that the m_DerivativeWeights member, which has WeightsType (which has a dimensionality of VectorDimension), is initialized to the ImageDimension dimensionality in the class constructor. Note that the suspect member has the same type as m_ComponentWeights and m_SqrtComponentWeights, which are initialized to the expected VectorDimension dimensionality in the constructor.

So not sure whether:

• m_DerivativeWeights should have the VectorDimension dimensionality, and hence the loops where its values are changed/printed in the implementation file should be changed to accomodate to the new dimensionality. This line looks troublesome, since it loops over the dimensions of the image, which is 2 in this case, to get its spacing, but the pixel dimensionality (and hence VectorDimension) is 3 (RGB) in this case.

• all three mentioned member variables should have the RealVectorType type and we should erase the WeightsType alias.

One could have used the Fill method to initialize all three mentioned member variables, but the bug would still be there/even more hidden if the above reasoning is correct. That being said, once we fix the bug, I think it is worthwhile using it to have a more compact and readable constructor.

Have not gone further than that. Any thoughts?

[blowekamp]

Nice work tracking it down. 👍

My cursory look found that m_DerivativeWeights is access by iterating on ImageDimensions. Perhaps the type is incorrectly defined and it should be:

using WeightsType = FixedArray<TRealType, ImageDimension>;

Looks like you have found a good test case for the filter, and found a bug :)

[jhlegarreta]

:+1:.

I will wait until tomorrow evening in case somebody wants to have a look; otherwise, I'll submit a patch set with the suggested change.

[dzenanz]

I think you don't need to wait, you can go ahead and submit the PR with the above discussed changes.

[blowekamp]

@jhlegarreta Thanks again for fixing this issue so nicely! I checked the dashbroard and valgrind is now green! 🟢

[jhlegarreta]

@blowekamp my pleasure; thanks for all the support and advice as we tried to fix the issue !