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

write: fix/support BigEndian PixelData writes, pad odd byte lengths with extra byte #302

Merged
merged 3 commits into from
Mar 9, 2024

Conversation

ducquangkstn
Copy link
Contributor

When reading the code base, I found there are 2 mismatch between read and write process:

  • If the data length is even, we must append 1 byte to round it up
  • If the transfersynax is BigEndian, we must apply the byte order when writing the pixeldata.

for frame := 0; frame < numFrames; frame++ {
for pixel := 0; pixel < numPixels; pixel++ {
for value := 0; value < numValues; value++ {
pixelValue := image.Frames[frame].NativeData.Data[pixel][value]
switch image.Frames[frame].NativeData.BitsPerSample {
case 8:
if err := binary.Write(buf, binary.LittleEndian, uint8(pixelValue)); err != nil {
if err := binary.Write(buf, bo, uint8(pixelValue)); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ref:

dicom/read.go

Line 477 in 8f1f151

buf[(pixel*samplesPerPixel)+value] = int(bo.Uint32(pixelBuf))

write.go Outdated
@@ -603,7 +604,12 @@ func writePixelData(w dicomio.Writer, t tag.Tag, value Value, vr string, vl uint
}
}
}
if err := w.WriteBytes(buf.Bytes()); err != nil {
// if the byte length is even, append 1 padding byte
rawData := buf.Bytes()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ref:

dicom/read.go

Line 421 in 8f1f151

case uint32(bytesToRead) == vl-1 && vl%2 != 0:

@ducquangkstn ducquangkstn marked this pull request as ready for review March 1, 2024 03:12
Copy link
Owner

@suyashkumar suyashkumar left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @ducquangkstn for the contribution! Indeed I imagine there is a lot of low hanging fruit to shore up the write side of the library.

}
file.Close()

if tc.expectedError != nil {
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's okay to reduce nesting by returning early in this case, but it's worth a comment for future readers to drive home why this is happening, perhaps

Suggested change
if tc.expectedError != nil {
// If we expect an error, we do not need to continue to check the value of the written data, so we continue to the next test case.
if tc.expectedError != nil {

write.go Outdated
Comment on lines 607 to 612
// if the byte length is even, append 1 padding byte
rawData := buf.Bytes()
if len(rawData)%2 != 0 {
rawData = append(rawData, 0)
}
if err := w.WriteBytes(rawData); err != nil {
Copy link
Owner

@suyashkumar suyashkumar Mar 1, 2024

Choose a reason for hiding this comment

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

optional nit: Not a big deal, but can consider using buf.Len() and only calling buf.Bytes() when the buffer is ready to be written.

Suggested change
// if the byte length is even, append 1 padding byte
rawData := buf.Bytes()
if len(rawData)%2 != 0 {
rawData = append(rawData, 0)
}
if err := w.WriteBytes(rawData); err != nil {
// if the byte length is even, append 1 padding byte
if buf.Len() % 2 != 0 {
if err := buf.WriteByte(0); err != nil {
return err
}
}
if err := w.WriteBytes(buf.Bytes()); err != nil {

write_test.go Outdated
expectedError: nil,
},
{
name: "even_PixelData",
Copy link
Owner

Choose a reason for hiding this comment

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

Let's maybe be more descriptive here, perhaps using the odd terminology would be better here since the input test case is PixelData with a odd number of value bytes?

Suggested change
name: "even_PixelData",
name: "native_PixelData_odd_bytes",

Since odd byte length padding is more of a spec compatibility thing instead of a roundtrip test thing, we might want to have a test for writePixelData that tests the padding behavior directly, but we can do that in a future change!

@suyashkumar suyashkumar changed the title Fix: write dicom bug write: fix/support BigEndian PixelData writes, pad odd byte lengths with extra byte Mar 1, 2024
@suyashkumar suyashkumar merged commit 82da819 into suyashkumar:main Mar 9, 2024
2 checks passed
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

Successfully merging this pull request may close these issues.

2 participants