From 65259e55c2dd96a811e752aff2864ef16b903351 Mon Sep 17 00:00:00 2001 From: Suyash Kumar Date: Sat, 1 Jun 2024 18:24:22 -0400 Subject: [PATCH] Remove Reader interface in favor of using the struct directly, use writer struct pointer for consistency (#324) Making this refactor, along with other API-breaking changes being made in the next release. If users need an interface, they can create one client-side as needed for their use cases! This also matches the Writer to use a pointer struct for consistency. This closes #319. --- parse.go | 2 +- pkg/dicomio/reader.go | 139 ++++++++++++++++-------------------------- pkg/dicomio/writer.go | 4 +- read.go | 10 +-- read_test.go | 2 +- write.go | 38 ++++++------ 6 files changed, 81 insertions(+), 114 deletions(-) diff --git a/parse.go b/parse.go index 7206072d..d4ab5e30 100644 --- a/parse.go +++ b/parse.go @@ -219,7 +219,7 @@ func (p *Parser) GetMetadata() Dataset { return p.metadata } -// SetTransferSyntax sets the transfer syntax for the underlying dicomio.Reader. +// SetTransferSyntax sets the transfer syntax for the underlying *dicomio.Reader. func (p *Parser) SetTransferSyntax(bo binary.ByteOrder, implicit bool) { p.reader.rawReader.SetTransferSyntax(bo, implicit) } diff --git a/pkg/dicomio/reader.go b/pkg/dicomio/reader.go index 605b1057..a927c5dd 100644 --- a/pkg/dicomio/reader.go +++ b/pkg/dicomio/reader.go @@ -24,67 +24,7 @@ var ( // Reader should read until EOF. const LimitReadUntilEOF = -9999 -// Reader provides common functionality for reading underlying DICOM data. -type Reader interface { - io.Reader - // ReadUInt8 reads a uint16 from the underlying reader. - ReadUInt8() (uint8, error) - // ReadUInt16 reads a uint16 from the underlying reader. - ReadUInt16() (uint16, error) - // ReadUInt32 reads a uint32 from the underlying reader. - ReadUInt32() (uint32, error) - // ReadInt16 reads a int16 from the underlying reader. - ReadInt16() (int16, error) - // ReadInt32 reads a int32 from the underlying reader. - ReadInt32() (int32, error) - // ReadFloat32 reads a float32 from the underlying reader. - ReadFloat32() (float32, error) - // ReadFloat64 reads a float32 from the underlying reader. - ReadFloat64() (float64, error) - // ReadString reads an n byte string from the underlying reader. - // Uses the charset.CodingSystem encoding decoders to read the string, if - // set. - ReadString(n uint32) (string, error) - // Skip skips the reader ahead by n bytes. - Skip(n int64) error - // Peek returns the next n bytes without advancing the reader. This will - // return bufio.ErrBufferFull if the buffer is full. - Peek(n int) ([]byte, error) - // PushLimit sets a read limit of n bytes from the current position of the - // reader. Once the limit is reached, IsLimitExhausted will return true, and - // other attempts to read data from dicomio.Reader will return io.EOF. - PushLimit(n int64) error - // PopLimit removes the most recent limit set, and restores the limit before - // that one. - PopLimit() - // IsLimitExhausted indicates whether or not we have read up to the - // currently set limit position. - IsLimitExhausted() bool - // BytesLeftUntilLimit returns the number of bytes remaining until we reach - // the currently set limit position. - BytesLeftUntilLimit() int64 - // SetTransferSyntax sets the byte order and whether the current transfer - // syntax is implicit or not. - SetTransferSyntax(bo binary.ByteOrder, implicit bool) - // IsImplicit returns if the currently set transfer syntax on this Reader is - // implicit or not. - IsImplicit() bool - // SetCodingSystem sets the charset.CodingSystem to be used when ReadString - // is called. - SetCodingSystem(cs charset.CodingSystem) - // ByteOrder returns the current byte order. - ByteOrder() binary.ByteOrder - // SetDeflate applies deflate decompression to the underlying reader for all - // subsequent reads. This should be set when working with a deflated - // transfer syntax. Right now this is expected to be called once when - // parsing the top level dicom data, and there is no facility to swap - // between deflate and non-deflate reading. - // This also sets the current limit to LimitReadUntilEOF, since the original - // limits (if any) will be based on uncompressed bytes. - SetDeflate() -} - -type reader struct { +type Reader struct { in *bufio.Reader bo binary.ByteOrder implicit bool @@ -97,9 +37,9 @@ type reader struct { cs charset.CodingSystem } -// NewReader creates and returns a new dicomio.Reader. -func NewReader(in *bufio.Reader, bo binary.ByteOrder, limit int64) Reader { - return &reader{ +// NewReader creates and returns a new *dicomio.Reader. +func NewReader(in *bufio.Reader, bo binary.ByteOrder, limit int64) *Reader { + return &Reader{ in: in, bo: bo, limit: limit, @@ -107,14 +47,14 @@ func NewReader(in *bufio.Reader, bo binary.ByteOrder, limit int64) Reader { } } -func (r *reader) BytesLeftUntilLimit() int64 { +func (r *Reader) BytesLeftUntilLimit() int64 { if r.limit == LimitReadUntilEOF { return math.MaxInt64 } return r.limit - r.bytesRead } -func (r *reader) Read(p []byte) (int, error) { +func (r *Reader) Read(p []byte) (int, error) { // Check if we've hit the limit if r.BytesLeftUntilLimit() <= 0 { if len(p) == 0 { @@ -135,43 +75,50 @@ func (r *reader) Read(p []byte) (int, error) { return n, err } -func (r *reader) ReadUInt8() (uint8, error) { +// ReadUInt8 reads an uint8 from the underlying *Reader. +func (r *Reader) ReadUInt8() (uint8, error) { var out uint8 err := binary.Read(r, r.bo, &out) return out, err } -func (r *reader) ReadUInt16() (uint16, error) { +// ReadUInt16 reads an uint16 from the underlying *Reader. +func (r *Reader) ReadUInt16() (uint16, error) { var out uint16 err := binary.Read(r, r.bo, &out) return out, err } -func (r *reader) ReadUInt32() (uint32, error) { +// ReadUInt32 reads an uint32 from the underlying *Reader. +func (r *Reader) ReadUInt32() (uint32, error) { var out uint32 err := binary.Read(r, r.bo, &out) return out, err } -func (r *reader) ReadInt16() (int16, error) { +// ReadInt16 reads an int16 from the underlying *Reader. +func (r *Reader) ReadInt16() (int16, error) { var out int16 err := binary.Read(r, r.bo, &out) return out, err } -func (r *reader) ReadInt32() (int32, error) { +// ReadInt32 reads an int32 from the underlying *Reader. +func (r *Reader) ReadInt32() (int32, error) { var out int32 err := binary.Read(r, r.bo, &out) return out, err } -func (r *reader) ReadFloat32() (float32, error) { +// ReadFloat32 reads a float32 from the underlying *Reader. +func (r *Reader) ReadFloat32() (float32, error) { var out float32 err := binary.Read(r, r.bo, &out) return out, err } -func (r *reader) ReadFloat64() (float64, error) { +// ReadFloat64 reads a float64 from the underlying *Reader. +func (r *Reader) ReadFloat64() (float64, error) { var out float64 err := binary.Read(r, r.bo, &out) return out, err @@ -192,7 +139,8 @@ func internalReadString(data []byte, d *encoding.Decoder) (string, error) { return string(bytes), nil } -func (r *reader) ReadString(n uint32) (string, error) { +// ReadString reads a string from the underlying *Reader. +func (r *Reader) ReadString(n uint32) (string, error) { data := make([]byte, n) _, err := io.ReadFull(r, data) if err != nil { @@ -200,7 +148,9 @@ func (r *reader) ReadString(n uint32) (string, error) { } return internalReadString(data, r.cs.Ideographic) } -func (r *reader) Skip(n int64) error { + +// Skip skips the *Reader ahead by n bytes. +func (r *Reader) Skip(n int64) error { if r.BytesLeftUntilLimit() < n { // not enough left to skip return ErrorInsufficientBytesLeft @@ -211,8 +161,8 @@ func (r *reader) Skip(n int64) error { return err } -// PushLimit creates a limit n bytes from the current position -func (r *reader) PushLimit(n int64) error { +// PushLimit creates a limit n bytes from the current position. +func (r *Reader) PushLimit(n int64) error { newLimit := r.bytesRead + n if newLimit > r.limit && r.limit != LimitReadUntilEOF { return fmt.Errorf("new limit exceeds current limit of buffer, new limit: %d, limit: %d", newLimit, r.limit) @@ -223,7 +173,10 @@ func (r *reader) PushLimit(n int64) error { r.limit = newLimit return nil } -func (r *reader) PopLimit() { + +// PopLimit removes the most recent limit set, and restores the limit before +// that one. +func (r *Reader) PopLimit() { if r.bytesRead < r.limit && r.limit != LimitReadUntilEOF { // didn't read all the way to the limit, so skip over what's left. _ = r.Skip(r.limit - r.bytesRead) @@ -234,33 +187,47 @@ func (r *reader) PopLimit() { r.limitStack = r.limitStack[:last] } -func (r *reader) IsLimitExhausted() bool { +func (r *Reader) IsLimitExhausted() bool { // if limit < 0 than we should read until EOF return (r.BytesLeftUntilLimit() <= 0 && r.limit != LimitReadUntilEOF) } -func (r *reader) SetTransferSyntax(bo binary.ByteOrder, implicit bool) { +func (r *Reader) SetTransferSyntax(bo binary.ByteOrder, implicit bool) { r.bo = bo r.implicit = implicit } -func (r *reader) SetDeflate() { +// SetDeflate applies deflate decompression to the underlying *Reader for all +// subsequent reads. This should be set when working with a deflated +// transfer syntax. Right now this is expected to be called once when +// parsing the top level dicom data, and there is no facility to swap +// between deflate and non-deflate reading. +// This also sets the current limit to LimitReadUntilEOF, since the original +// limits (if any) will be based on uncompressed bytes. +func (r *Reader) SetDeflate() { r.in = bufio.NewReader(flate.NewReader(r.in)) // TODO(https://github.com/suyashkumar/dicom/issues/320): consider always // having the top level limit read until EOF. - r.limit = LimitReadUntilEOF // needed because original limits may not apply to the deflated reader + r.limit = LimitReadUntilEOF // needed because original limits may not apply to the deflated *Reader } -func (r *reader) IsImplicit() bool { return r.implicit } +// IsImplicit returns if the currently set transfer syntax on this *Reader is +// implicit or not. +func (r *Reader) IsImplicit() bool { return r.implicit } -func (r *reader) SetCodingSystem(cs charset.CodingSystem) { +// SetCodingSystem sets the charset.CodingSystem to be used when ReadString +// is called. +func (r *Reader) SetCodingSystem(cs charset.CodingSystem) { r.cs = cs } -func (r *reader) Peek(n int) ([]byte, error) { +// Peek reads and returns the next n bytes (if possible) without advancing the +// underlying reader. +func (r *Reader) Peek(n int) ([]byte, error) { return r.in.Peek(n) } -func (r *reader) ByteOrder() binary.ByteOrder { +// ByteOrder returns the current byte order. +func (r *Reader) ByteOrder() binary.ByteOrder { return r.bo } diff --git a/pkg/dicomio/writer.go b/pkg/dicomio/writer.go index a3736466..10cc748f 100644 --- a/pkg/dicomio/writer.go +++ b/pkg/dicomio/writer.go @@ -14,8 +14,8 @@ type Writer struct { } // NewWriter initializes and returns a Writer. -func NewWriter(out io.Writer, bo binary.ByteOrder, implicit bool) Writer { - return Writer{ +func NewWriter(out io.Writer, bo binary.ByteOrder, implicit bool) *Writer { + return &Writer{ out: out, bo: bo, implicit: implicit, diff --git a/read.go b/read.go index eb45cc92..e696cfc5 100644 --- a/read.go +++ b/read.go @@ -34,12 +34,12 @@ var ( ) // reader is responsible for mid-level dicom parsing capabilities, like -// reading tags, VRs, and elements from the low-level dicomio.Reader dicom data. +// reading tags, VRs, and elements from the low-level *dicomio.Reader dicom data. // TODO(suyashkumar): consider revisiting naming of this struct "reader" as it -// interplays with the rawReader dicomio.Reader. We could consider combining -// them, or embedding the dicomio.Reader struct into reader. +// interplays with the rawReader *dicomio.Reader. We could consider combining +// them, or embedding the *dicomio.Reader struct into reader. type reader struct { - rawReader dicomio.Reader + rawReader *dicomio.Reader opts parseOptSet } @@ -315,7 +315,7 @@ func getNthBit(data byte, n int) int { return 0 } -func fillBufferSingleBitAllocated(pixelData []int, d dicomio.Reader, bo binary.ByteOrder) error { +func fillBufferSingleBitAllocated(pixelData []int, d *dicomio.Reader, bo binary.ByteOrder) error { debug.Logf("len of pixeldata: %d", len(pixelData)) if len(pixelData)%8 > 0 { return errors.New("when bitsAllocated is 1, we can't read a number of samples that is not a multiple of 8") diff --git a/read_test.go b/read_test.go index b8fe8002..03af584d 100644 --- a/read_test.go +++ b/read_test.go @@ -915,7 +915,7 @@ func BenchmarkReadNativeFrames(b *testing.B) { } } -func buildReadNativeFramesInput(rows, cols, numFrames, samplesPerPixel int, b *testing.B) (*Dataset, dicomio.Reader) { +func buildReadNativeFramesInput(rows, cols, numFrames, samplesPerPixel int, b *testing.B) (*Dataset, *dicomio.Reader) { b.Helper() dataset := Dataset{ Elements: []*Element{ diff --git a/write.go b/write.go index 0da1720e..ed4a5ad9 100644 --- a/write.go +++ b/write.go @@ -32,7 +32,7 @@ var ( // Writer is a struct that allows element-by element writing to a DICOM writer. type Writer struct { - writer dicomio.Writer + writer *dicomio.Writer optSet *writeOptSet } @@ -145,7 +145,7 @@ func toWriteOptSet(opts ...WriteOption) *writeOptSet { return optSet } -func writeFileHeader(w dicomio.Writer, ds *Dataset, metaElems []*Element, opts writeOptSet) error { +func writeFileHeader(w *dicomio.Writer, ds *Dataset, metaElems []*Element, opts writeOptSet) error { // File headers are always written in littleEndian explicit w.SetTransferSyntax(binary.LittleEndian, false) @@ -211,7 +211,7 @@ func writeFileHeader(w dicomio.Writer, ds *Dataset, metaElems []*Element, opts w return nil } -func writeElement(w dicomio.Writer, elem *Element, opts writeOptSet) error { +func writeElement(w *dicomio.Writer, elem *Element, opts writeOptSet) error { vr := elem.RawValueRepresentation var err error vr, err = verifyVROrDefault(elem.Tag, elem.RawValueRepresentation, opts) @@ -257,7 +257,7 @@ func writeElement(w dicomio.Writer, elem *Element, opts writeOptSet) error { return nil } -func writeMetaElem(w dicomio.Writer, t tag.Tag, ds *Dataset, tagsUsed *map[tag.Tag]bool, optSet writeOptSet) error { +func writeMetaElem(w *dicomio.Writer, t tag.Tag, ds *Dataset, tagsUsed *map[tag.Tag]bool, optSet writeOptSet) error { elem, err := ds.FindElementByTag(t) if err != nil { return err @@ -336,7 +336,7 @@ func verifyValueType(t tag.Tag, value Value, vr string) error { return nil } -func writeTag(w dicomio.Writer, t tag.Tag, vl uint32) error { +func writeTag(w *dicomio.Writer, t tag.Tag, vl uint32) error { if vl%2 != 0 && vl != tag.VLUndefinedLength { return fmt.Errorf("ERROR dicomio.writeTag: Value Length must be even, but for Tag=%v, ValueLength=%v", tag.DebugString(t), vl) @@ -347,7 +347,7 @@ func writeTag(w dicomio.Writer, t tag.Tag, vl uint32) error { return w.WriteUInt16(t.Element) } -func writeVRVL(w dicomio.Writer, t tag.Tag, vr string, vl uint32) error { +func writeVRVL(w *dicomio.Writer, t tag.Tag, vr string, vl uint32) error { // Rectify Undefined Length VL if vl == 0xffff { vl = tag.VLUndefinedLength @@ -397,7 +397,7 @@ func writeVRVL(w dicomio.Writer, t tag.Tag, vr string, vl uint32) error { return nil } -func writeRawItem(w dicomio.Writer, data []byte) error { +func writeRawItem(w *dicomio.Writer, data []byte) error { length := uint32(len(data)) if err := writeTag(w, tag.Item, length); err != nil { return err @@ -411,7 +411,7 @@ func writeRawItem(w dicomio.Writer, data []byte) error { return nil } -func writeBasicOffsetTable(w dicomio.Writer, offsets []uint32) error { +func writeBasicOffsetTable(w *dicomio.Writer, offsets []uint32) error { byteOrder, implicit := w.GetTransferSyntax() data := &bytes.Buffer{} subWriter := dicomio.NewWriter(data, byteOrder, implicit) @@ -423,7 +423,7 @@ func writeBasicOffsetTable(w dicomio.Writer, offsets []uint32) error { return writeRawItem(w, data.Bytes()) } -func encodeElementHeader(w dicomio.Writer, t tag.Tag, vr string, vl uint32) error { +func encodeElementHeader(w *dicomio.Writer, t tag.Tag, vr string, vl uint32) error { err := writeTag(w, t, vl) if err != nil { return err @@ -435,7 +435,7 @@ func encodeElementHeader(w dicomio.Writer, t tag.Tag, vr string, vl uint32) erro return nil } -func writeValue(w dicomio.Writer, t tag.Tag, value Value, valueType ValueType, vr string, vl uint32, opts writeOptSet) error { +func writeValue(w *dicomio.Writer, t tag.Tag, value Value, valueType ValueType, vr string, vl uint32, opts writeOptSet) error { if vl == tag.VLUndefinedLength && valueType <= 2 { // strings, bytes or ints return fmt.Errorf("encoding undefined-length element not yet supported: %v", t) } @@ -461,7 +461,7 @@ func writeValue(w dicomio.Writer, t tag.Tag, value Value, valueType ValueType, v } } -func writeStrings(w dicomio.Writer, values []string, vr string) error { +func writeStrings(w *dicomio.Writer, values []string, vr string) error { s := "" for i, substr := range values { if i > 0 { @@ -490,7 +490,7 @@ func writeStrings(w dicomio.Writer, values []string, vr string) error { return nil } -func writeBytes(w dicomio.Writer, values []byte, vr string) error { +func writeBytes(w *dicomio.Writer, values []byte, vr string) error { var err error switch vr { case vrraw.OtherWord, vrraw.Unknown: @@ -506,7 +506,7 @@ func writeBytes(w dicomio.Writer, values []byte, vr string) error { return nil } -func writeInts(w dicomio.Writer, values []int, vr string) error { +func writeInts(w *dicomio.Writer, values []int, vr string) error { for _, value := range values { switch vr { // TODO(suyashkumar): consider additional validation of VR=AT elements. @@ -525,7 +525,7 @@ func writeInts(w dicomio.Writer, values []int, vr string) error { return nil } -func writeFloats(w dicomio.Writer, v Value, vr string) error { +func writeFloats(w *dicomio.Writer, v Value, vr string) error { if v.ValueType() != Floats { return ErrorUnexpectedValueType } @@ -551,7 +551,7 @@ func writeFloats(w dicomio.Writer, v Value, vr string) error { return nil } -func writePixelData(w dicomio.Writer, t tag.Tag, value Value, vr string, vl uint32) error { +func writePixelData(w *dicomio.Writer, t tag.Tag, value Value, vr string, vl uint32) error { image := MustGetPixelDataInfo(value) if vl == tag.VLUndefinedLength { @@ -628,7 +628,7 @@ var sequenceDelimitationItem = &Element{ ValueLength: 0, // This should be 00000000H in base32 } -func writeSequence(w dicomio.Writer, t tag.Tag, values []*SequenceItemValue, vr string, vl uint32, opts writeOptSet) error { +func writeSequence(w *dicomio.Writer, t tag.Tag, values []*SequenceItemValue, vr string, vl uint32, opts writeOptSet) error { // We always write out sequences using the undefined length encoding. // Note: we currently don't validate that the length of the sequence matches // the VL if it's not undefined VL. @@ -663,7 +663,7 @@ var item = &Element{ ValueLength: tag.VLUndefinedLength, } -func writeSequenceItem(w dicomio.Writer, t tag.Tag, values []*Element, vr string, vl uint32, opts writeOptSet) error { +func writeSequenceItem(w *dicomio.Writer, t tag.Tag, values []*Element, vr string, vl uint32, opts writeOptSet) error { // Write out item header. if err := writeElement(w, item, opts); err != nil { return err @@ -680,7 +680,7 @@ func writeSequenceItem(w dicomio.Writer, t tag.Tag, values []*Element, vr string return writeElement(w, sequenceItemDelimitationItem, opts) } -func writeOtherWordString(w dicomio.Writer, data []byte) error { +func writeOtherWordString(w *dicomio.Writer, data []byte) error { if len(data)%2 != 0 { return ErrorOWRequiresEvenVL } @@ -699,7 +699,7 @@ func writeOtherWordString(w dicomio.Writer, data []byte) error { return nil } -func writeOtherByteString(w dicomio.Writer, data []byte) error { +func writeOtherByteString(w *dicomio.Writer, data []byte) error { if err := w.WriteBytes(data); err != nil { return err }