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.

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

Use a once-allocated buffer for reading native pixel values instead of using binary.Read (which allocates a slice under the hood) #163

Merged
merged 10 commits into from
Dec 22, 2020
Merged

Use a once-allocated buffer for reading native pixel values instead of using binary.Read (which allocates a slice under the hood) #163

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1780bba
Merge pull request #1 from suyashkumar/main
kaxap Dec 20, 2020
7883858
pre-allocate pixel buffer in readNativeFrames; add GetByteOrder() to …
Dec 20, 2020
1e175d1
add GetByteOrder() to the MockReader
Dec 20, 2020
e423725
Update pkg/dicomio/reader.go
kaxap Dec 20, 2020
7f107ef
Update read.go
kaxap Dec 20, 2020
3ef4870
Update pkg/dicomio/reader.go
kaxap Dec 20, 2020
fb1393c
Update mocks/pkg/dicomio/mock_reader.go
kaxap Dec 20, 2020
9e5de7d
Update read.go
kaxap Dec 20, 2020
25288ff
Add sentinel ErrorIncompleteRead for readNativeFrames
Dec 20, 2020
075bb74
Update ReadNativeFrames to use io.ReadFull instead of Read
Dec 22, 2020
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
14 changes: 14 additions & 0 deletions mocks/pkg/dicomio/mock_reader.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions pkg/dicomio/reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ type Reader interface {
// SetCodingSystem sets the charset.CodingSystem to be used when ReadString
// is called.
SetCodingSystem(cs charset.CodingSystem)
ByteOrder() binary.ByteOrder
}

type reader struct {
Expand Down Expand Up @@ -234,3 +235,7 @@ func (r *reader) SetCodingSystem(cs charset.CodingSystem) {
func (r *reader) Peek(n int) ([]byte, error) {
return r.in.Peek(n)
}

func (r *reader) ByteOrder() binary.ByteOrder {
return r.bo
}
34 changes: 18 additions & 16 deletions read.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ var (
// ErrorUnsupportedVR indicates that this VR is not supported.
ErrorUnsupportedVR = errors.New("unsupported VR")
errorUnableToParseFloat = errors.New("unable to parse float type")
ErrorIncompleteRead = errors.New("unable to read specified number of bytes")
)

func readTag(r dicomio.Reader) (*tag.Tag, error) {
Expand Down Expand Up @@ -212,6 +213,9 @@ func readNativeFrames(d dicomio.Reader, parsedData *Dataset, fc chan<- *frame.Fr

// Parse the pixels:
image.Frames = make([]frame.Frame, nFrames)
bo := d.ByteOrder()
bytesAllocated := bitsAllocated / 8
pixelBuf := make([]byte, bytesAllocated)
for frameIdx := 0; frameIdx < nFrames; frameIdx++ {
// Init current frame
currentFrame := frame.Frame{
Expand All @@ -226,24 +230,22 @@ func readNativeFrames(d dicomio.Reader, parsedData *Dataset, fc chan<- *frame.Fr
buf := make([]int, int(pixelsPerFrame)*samplesPerPixel)
for pixel := 0; pixel < int(pixelsPerFrame); pixel++ {
for value := 0; value < samplesPerPixel; value++ {
if bitsAllocated == 8 {
val, err := d.ReadUInt8()
if err != nil {
return nil, bytesRead, err

Copy link
Owner

Choose a reason for hiding this comment

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

nit: no need for a new line here (but I can also just fix in a cleanup pr later, don't need to block this).

Suggested change

n, err := d.Read(pixelBuf)
Copy link
Owner

@suyashkumar suyashkumar Dec 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Thought about this a little more the second time around (sorry for not bringing this up earlier). Looking at the io.Reader interface I think a single call to read may read "up to len(p)" bytes. Perhaps we should use io.ReadFull to try to read at least the size of the buffer.

In reality for values like 16 or 32 bits, calls to Read may still work no problem, but it may be more "correct" to use something like io.ReadFull which won't be expensive in the case that Read would've worked on the first try. This may also insulate us from the exact implementation of the reader under the hood we're reading from, since there is no garuntee that it must fill the buffer on the first call to Read (but it's possible it may fill the buffer correctly on subsequent calls to Read--see the implementation of ReadAtLeast).

It also looks like based on the implementation, it may return a nice ErrUnexpectedEOF if it reads less than n bytes, saving us an error, but we can leave it in there for not and not depend on that to make it easy and get this merged for now.

WDYT?

Suggested change
n, err := d.Read(pixelBuf)
n, err := io.ReadFull(d, pixelBuf)

Copy link
Contributor Author

@kaxap kaxap Dec 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Good catch. Read(buffer) can in fact return less than n bytes when reading from a network socket, which is expected behaviour.

if err != nil || n < bytesAllocated {
if err == nil {
err = ErrorIncompleteRead
}
buf[(pixel*samplesPerPixel)+value] = int(val)
return nil, bytesRead,
fmt.Errorf("could not read uint%d from input: %w", bitsAllocated, err)
}
suyashkumar marked this conversation as resolved.
Show resolved Hide resolved

if bitsAllocated == 8 {
buf[(pixel*samplesPerPixel)+value] = int(pixelBuf[0])
} else if bitsAllocated == 16 {
val, err := d.ReadUInt16()
if err != nil {
return nil, bytesRead, err
}
buf[(pixel*samplesPerPixel)+value] = int(val)
buf[(pixel*samplesPerPixel)+value] = int(bo.Uint16(pixelBuf))
} else if bitsAllocated == 32 {
val, err := d.ReadUInt32()
if err != nil {
return nil, bytesRead, err
}
buf[(pixel*samplesPerPixel)+value] = int(val)
buf[(pixel*samplesPerPixel)+value] = int(bo.Uint32(pixelBuf))
}
}
currentFrame.NativeData.Data[pixel] = buf[pixel*samplesPerPixel : (pixel+1)*samplesPerPixel]
Expand All @@ -254,7 +256,7 @@ func readNativeFrames(d dicomio.Reader, parsedData *Dataset, fc chan<- *frame.Fr
}
}

bytesRead = (bitsAllocated / 8) * samplesPerPixel * pixelsPerFrame * nFrames
bytesRead = bytesAllocated * samplesPerPixel * pixelsPerFrame * nFrames

return &image, bytesRead, nil
}
Expand Down
16 changes: 15 additions & 1 deletion read_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bufio"
"bytes"
"encoding/binary"
"errors"
"fmt"
"math/rand"
"strconv"
Expand Down Expand Up @@ -249,6 +250,19 @@ func TestReadNativeFrames(t *testing.T) {
},
expectedError: nil,
},
{
Name: "insufficient bytes, uint32",
existingData: Dataset{Elements: []*Element{
mustNewElement(tag.Rows, []int{2}),
mustNewElement(tag.Columns, []int{2}),
mustNewElement(tag.NumberOfFrames, []string{"2"}),
mustNewElement(tag.BitsAllocated, []int{32}),
mustNewElement(tag.SamplesPerPixel, []int{2}),
}},
data: []uint16{1, 2, 3, 2, 1, 2, 3, 2, 1, 2, 3, 2, 1, 2, 3},
expectedPixelData: nil,
expectedError: ErrorIncompleteRead,
},
{
Name: "missing Columns",
existingData: Dataset{Elements: []*Element{
Expand Down Expand Up @@ -278,7 +292,7 @@ func TestReadNativeFrames(t *testing.T) {
}

pixelData, _, err := readNativeFrames(r, &tc.existingData, nil)
if err != tc.expectedError {
if !errors.Is(err, tc.expectedError) {
t.Errorf("TestReadNativeFrames(%v): did not get expected error. got: %v, want: %v", tc.data, err, tc.expectedError)
}

Expand Down