Skip to content

Conversation

@N-Dekker
Copy link
Contributor

MINCImageIO::Write should use signed char for the estimation of the min and the max pixel value, when the pixel component type is IOComponentEnum::CHAR. Both IOComponentEnum::CHAR and MI_TYPE_BYTE specify a signed type, whereas the default char might be unsigned (for example, when using GCC compiler option -funsigned-char).

Obviously the estimation of min and max pixel value may go wrong when the pixels are incorrectly assumed to be unsigned.


@blowekamp This little bug fix is still a spinoff from your suggestion to try to build ITK using -funsigned-char 😃

`MINCImageIO::Write` should use `signed char` for the estimation of the min and
the max pixel value, when the pixel component type is `IOComponentEnum::CHAR`.
Both `IOComponentEnum::CHAR` and `MI_TYPE_BYTE` specify a _signed_ type, whereas
the default `char` might be unsigned (for example, when using GCC compiler
option `-funsigned-char`).

Obviously the estimation of min and max pixel value may go wrong when the pixels
are incorrectly assumed to be unsigned.
@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 15, 2025
@N-Dekker N-Dekker marked this pull request as ready for review July 15, 2025 14:35
@blowekamp
Copy link
Member

Glad to hear that compiler flag has been useful to track down issues!

@seanm
Copy link
Contributor

seanm commented Jul 15, 2025

@gdevenyi may interest you

@thewtex thewtex requested a review from Copilot July 15, 2025 18:52
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug in MINCImageIO where the wrong char type was used for min/max pixel value calculations. The issue occurs when IOComponentEnum::CHAR and MI_TYPE_BYTE are used with compilers that default to unsigned char (e.g., GCC with -funsigned-char), leading to incorrect pixel value range estimation.

  • Changes char to signed char in the template parameter for get_buffer_min_max function call
  • Ensures consistent handling of signed byte data regardless of compiler char signedness defaults

Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

💯

@thewtex thewtex merged commit 87a4fff into InsightSoftwareConsortium:main Jul 16, 2025
16 of 17 checks passed
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Jul 16, 2025
Both `VTKPolyDataMeshIO::WritePoints` and `VTKPolyDataMeshIO::WritePointData`
originally casted the `buffer` to plain `char *` for component type `CHAR`
(using `CASE_INVOKE_WITH_COMPONENT_TYPE`). However, `CHAR` indicates that the
component type is _signed_ char, whereas the default `char` might be unsigned.

Follow-up to pull request InsightSoftwareConsortium#5470
commit 70fbb2b "BUG: MINCImageIO should use
`signed char` for `CHAR` and `MI_TYPE_BYTE`"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Jul 17, 2025
`VTKPolyDataMeshIO` originally casted the `buffer` to plain `char *` for
component type `CHAR`. However, `CHAR` indicates that the component type is
_signed_ char, whereas the default `char` might be unsigned.

Follow-up to pull request InsightSoftwareConsortium#5470
commit 70fbb2b "BUG: MINCImageIO should use
`signed char` for `CHAR` and `MI_TYPE_BYTE`"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Jul 25, 2025
`VTKPolyDataMeshIO` originally casted the `buffer` to plain `char *` for
component type `CHAR`. However, `CHAR` indicates that the component type is
_signed_ char, whereas the default `char` might be unsigned.

Follow-up to pull request InsightSoftwareConsortium#5470
commit 70fbb2b "BUG: MINCImageIO should use
`signed char` for `CHAR` and `MI_TYPE_BYTE`"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Aug 27, 2025
`VTKPolyDataMeshIO` originally casted the `buffer` to plain `char *` for
component type `CHAR`. However, `CHAR` indicates that the component type is
_signed_ char, whereas the default `char` might be unsigned.

Follow-up to pull request InsightSoftwareConsortium#5470
commit 70fbb2b "BUG: MINCImageIO should use
`signed char` for `CHAR` and `MI_TYPE_BYTE`"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Oct 10, 2025
`VTKPolyDataMeshIO` originally casted the `buffer` to plain `char *` for
component type `CHAR`. However, `CHAR` indicates that the component type is
_signed_ char, whereas the default `char` might be unsigned.

Follow-up to pull request InsightSoftwareConsortium#5470
commit 70fbb2b "BUG: MINCImageIO should use
`signed char` for `CHAR` and `MI_TYPE_BYTE`"
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Oct 10, 2025
`VTKPolyDataMeshIO` originally casted the `buffer` to plain `char *` for
component type `SCHAR`. However, `SCHAR` indicates that the component type is
_signed_ char.

Follow-up to pull request InsightSoftwareConsortium#5470
commit 70fbb2b "BUG: MINCImageIO should use
`signed char` for `CHAR` and `MI_TYPE_BYTE`"
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.

4 participants