Skip to content

Avoid manual memory management ImageSeriesReader::m_MetaDataDictionaryArray #5280

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.

Sign up for GitHub

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

Merged
dzenanz merged 2 commits into from
Mar 18, 2025
Merged

Avoid manual memory management ImageSeriesReader::m_MetaDataDictionaryArray #5280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
43eeffd
STYLE: Make m_MetaDataDictionaryArray private, instead of protected
N-Dekker Mar 17, 2025
d7150cd
STYLE: Avoid manual memory management dictionaries of ImageSeriesReader
N-Dekker Mar 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions Modules/IO/ImageBase/include/itkImageSeriesReader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ class ITK_TEMPLATE_EXPORT ImageSeriesReader : public ImageSource<TOutputImage>
: m_ImageIO(nullptr)

{}
~ImageSeriesReader() override;
~ImageSeriesReader() override = default;
void
PrintSelf(std::ostream & os, Indent indent) const override;

Expand All @@ -205,10 +205,6 @@ class ITK_TEMPLATE_EXPORT ImageSeriesReader : public ImageSource<TOutputImage>
*/
unsigned int m_NumberOfDimensionsInImage{ 0 };

/** Array of MetaDataDictionaries. This allows to hold information from the
* ImageIO objects after reading every sub image in the series */
DictionaryArrayType m_MetaDataDictionaryArray{};

bool m_UseStreaming{ true };

bool m_SpacingDefined{ false };
Expand All @@ -221,6 +217,13 @@ class ITK_TEMPLATE_EXPORT ImageSeriesReader : public ImageSource<TOutputImage>
int
ComputeMovingDimensionIndex(ReaderType * reader);

/** Array of MetaDataDictionaries. This allows to hold information from the
* ImageIO objects after reading every sub image in the series */
DictionaryArrayType m_MetaDataDictionaryArray{};

/** The internal storage of MetaDataDictionaries. */
std::vector<MetaDataDictionary> m_InternalMetaDataDictionaries{};

/** Modified time of the MetaDataDictionaryArray */
TimeStamp m_MetaDataDictionaryArrayMTime{};

Expand Down
41 changes: 13 additions & 28 deletions Modules/IO/ImageBase/include/itkImageSeriesReader.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,22 +30,6 @@

namespace itk
{
// Destructor
template <typename TOutputImage>
ImageSeriesReader<TOutputImage>::~ImageSeriesReader()
{
// Clear the eventual previous content of the MetaDictionary array
if (!m_MetaDataDictionaryArray.empty())
{
for (auto & i : m_MetaDataDictionaryArray)
{
// each element is a raw pointer, delete them.
delete i;
}
}
m_MetaDataDictionaryArray.clear();
}

template <typename TOutputImage>
void
ImageSeriesReader<TOutputImage>::PrintSelf(std::ostream & os, Indent indent) const
Expand Down Expand Up @@ -102,15 +86,8 @@ ImageSeriesReader<TOutputImage>::GenerateOutputInformation()
const std::string key("ITK_ImageOrigin");

// Clear the previous content of the MetaDictionary array
if (!m_MetaDataDictionaryArray.empty())
{
for (auto & i : m_MetaDataDictionaryArray)
{
// each element is a raw pointer, delete them.
delete i;
}
}
m_MetaDataDictionaryArray.clear();
m_InternalMetaDataDictionaries.clear();

const auto numberOfFiles = static_cast<int>(m_FileNames.size());
if (numberOfFiles == 0)
Expand Down Expand Up @@ -297,6 +274,8 @@ ImageSeriesReader<TOutputImage>::GenerateData()
double maxSpacingDeviation = 0.0;
bool prevSliceIsValid = false;

m_InternalMetaDataDictionaries.reserve(static_cast<size_t>(numberOfFiles));

for (int i = 0; i != numberOfFiles; ++i)
{
if (TOutputImage::ImageDimension != this->m_NumberOfDimensionsInImage)
Expand Down Expand Up @@ -457,17 +436,23 @@ ImageSeriesReader<TOutputImage>::GenerateData()
// Deep copy the MetaDataDictionary into the array
if (reader->GetImageIO() && needToUpdateMetaDataDictionaryArray)
{
auto newDictionary = new DictionaryType;
*newDictionary = reader->GetImageIO()->GetMetaDataDictionary();
MetaDataDictionary newDictionary = reader->GetImageIO()->GetMetaDataDictionary();
if (nonUniformSampling)
{
// slice-specific information
EncapsulateMetaData<double>(*newDictionary, "ITK_non_uniform_sampling_deviation", spacingDeviation);
EncapsulateMetaData<double>(newDictionary, "ITK_non_uniform_sampling_deviation", spacingDeviation);
}
m_MetaDataDictionaryArray.push_back(newDictionary);
m_InternalMetaDataDictionaries.push_back(std::move(newDictionary));
Comment on lines -467 to +445
Copy link
Copy Markdown
Contributor Author

@N-Dekker N-Dekker Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blowekamp Regarding your suggestion to use "emplace_back": in this particular case I think emplace_back and push_back are basically equivalent, technically. emplace_back(newDictionary) would have copied newDictionary, albeit "in-place" ("emplaced"). emplace_back(std::move(newDictionary)) would have moved newDictionary. But the current push_back(std::move(newDictionary)) also just moves newDictionary.

In general, emplace_back is more interesting as an alternative to push_back(MyClass(a, b, c)), to replace it with emplace_back(a, b, c). (See also clang-tidy use-emplace.)

I don't think the guideline is to always replace push_back with emplace_back. 🤷

Copy link
Copy Markdown
Member

@blowekamp blowekamp Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the usage of emplace_back would have been to remove the local variable... something like:

m_InternalMetaDataDictionaries.emplace_back(reader->GetImageIO()->GetMetaDataDictionary());

My thought had been to use emplace_back to avoid extra copies of the objects being created. For this particular object it doesn't master. And your implementation is nearly equivalent.

Copy link
Copy Markdown
Contributor Author

@N-Dekker N-Dekker Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the usage of emplace_back would have been to remove the local variable... something like:

m_InternalMetaDataDictionaries.emplace_back(reader->GetImageIO()->GetMetaDataDictionary());

OK, but then it still wouldn't be much different from using push_back, as follows:

m_InternalMetaDataDictionaries.push_back(reader->GetImageIO()->GetMetaDataDictionary());

In either way (whether "emplacing" or "pushing"), it would always have to make a copy of the dictionary from the ImageIO object. 🤷 Fortunately copying a dictionary is very fast now, because of Copy On Write (your PR #455).

}
} // end per slice loop

m_MetaDataDictionaryArray.clear();
m_MetaDataDictionaryArray.reserve(m_InternalMetaDataDictionaries.size());

for (MetaDataDictionary & dictionary : m_InternalMetaDataDictionaries)
{
m_MetaDataDictionaryArray.push_back(&dictionary);
}

if (TOutputImage::ImageDimension != this->m_NumberOfDimensionsInImage &&
maxSpacingDeviation > m_SpacingWarningRelThreshold * outputSpacing[this->m_NumberOfDimensionsInImage])
Expand Down