Skip to content

BUG: DCMTK reader wrongly rejects file with preamble#4110

Closed
pieper wants to merge 1 commit intoInsightSoftwareConsortium:masterfrom
ITK-fork:4801-dcmtk-preamble
Closed

BUG: DCMTK reader wrongly rejects file with preamble#4110
pieper wants to merge 1 commit intoInsightSoftwareConsortium:masterfrom
ITK-fork:4801-dcmtk-preamble

Conversation

@pieper
Copy link
Copy Markdown
Contributor

@pieper pieper commented Jul 14, 2023

A previous optimization didn't take into account the premable of a dicom file. This adds a check.

Fixes #4108

A previous optimization didn't take into account the premable
of a dicom file.  This adds a check.

Fixes InsightSoftwareConsortium#4801
@github-actions github-actions Bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances area:IO Issues affecting the IO module labels Jul 14, 2023
@pieper
Copy link
Copy Markdown
Contributor Author

pieper commented Jul 14, 2023

@hjmjohnson could you have a look at this?

It fixes #4108

@issakomi
Copy link
Copy Markdown
Member

issakomi commented Jul 18, 2023

I confirm that DCMTKIO's DCMTKImageIO::CanReadFile function returns false for valid DICOM files with preamble. I have reproduced it just now.

@jhlegarreta
Copy link
Copy Markdown
Member

Thanks for doing this @pieper. I am willing to give this a 👍 when the following are addressed:

In general, commits should only fall into one category, BUG, DOC, STYLE, etc. Helps reviewing, keeping changes of different nature separate, keeping a clean history, etc.

@pieper
Copy link
Copy Markdown
Contributor Author

pieper commented Jul 18, 2023

Thanks for the review @jhlegarreta - I had tried to add just the one function but it looks like my checkout was against an older target. I'll fix it.

@thewtex thewtex added this to the ITK 5.4.0 milestone Jul 19, 2023
@dzenanz dzenanz requested a review from jhlegarreta August 2, 2023 13:42
@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Aug 2, 2023

@pieper did you have time to address Jon's feedback?

@@ -230,7 +259,7 @@ DCMTKImageIO::CanReadFile(const char * filename)
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This could be removed following seek to 0

@thewtex
Copy link
Copy Markdown
Member

thewtex commented Mar 11, 2024

#4501 has updates with @jhlegarreta requests addressed (note that readPreampleDicom is not in the header, so it is moved to an anonymous namespace instead). Also, replaced the file re-open, close with a seek. Replace and with &&.

@thewtex
Copy link
Copy Markdown
Member

thewtex commented Mar 14, 2024

Superceded by #4501

@thewtex thewtex closed this Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:IO Issues affecting the IO module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DCMTK reader fails for valid files

5 participants