Add comments, NativeFrame tests

pkg/frame/native.go

@@ -11,14 +11,42 @@ import (
 
 var UnsupportedSamplesPerPixel = errors.New("unsupported samples per pixel")
 
+// INativeFrame is an interface representation of NativeFrame[I]'s capabilities,
+// and offers a way to use a NativeFrame _without_ requiring propogation of
+// type parameters. This allows for some more ergonomic signatures, though
+// NativeFrame[I] can be used directly as well for those who prefer it.
 type INativeFrame interface {
+	// Rows returns the number of rows in this frame.
 	Rows() int
+	// Cols returns the number of columns in this frame.
 	Cols() int
+	// SamplesPerPixel returns the number of samples per pixel in this frame.
 	SamplesPerPixel() int
+	// BitsPerSample returns the bits per sample in this frame.
 	BitsPerSample() int
-	GetPixel(x, y int) []int
-	GetPixelAtIdx(idx int) []int
+	// GetPixel returns the samples (as a slice) for the pixel at (x, y).
+	// The coordinate system of the image starts with (0, 0) in the upper left
+	// corner of the image, with X increasing to the right, and Y increasing
+	// down:
+	//
+	//  0 -------▶ X
+	//  |
+	//  |
+	//  ▼
+	//  Y
+	GetPixel(x, y int) ([]int, error)
+	// RawDataSlice will return the underlying data slice, which will be of type
+	// []I. Based on BitsPerSample, you can anticipate what type of slice you'll
+	// get, and type assert as needed:
+	//  BitsPerSample    Slice
+	//  8                []uint8
+	//  16               []uint16
+	//  32               []uint32
 	RawDataSlice() any
+	// Equals returns true if this INativeFrame exactly equals the provided
+	// INativeFrame. This checks every pixel value, so may be expensive.
+	// In the future we may compute a one time hash during construction to make
+	// this less expensive in the future if called multiple time.
 	Equals(frame INativeFrame) bool
 	CommonFrame
 }
@@ -51,25 +79,21 @@ func NewNativeFrame[I constraints.Integer](bitsPerSample, rows, cols, pixelsPerF
 	}
 }
 
-func (n *NativeFrame[I]) Rows() int          { return n.InternalRows }
-func (n *NativeFrame[I]) Cols() int          { return n.InternalCols }
-func (n *NativeFrame[I]) BitsPerSample() int { return n.InternalBitsPerSample }
-
+func (n *NativeFrame[I]) Rows() int            { return n.InternalRows }
+func (n *NativeFrame[I]) Cols() int            { return n.InternalCols }
+func (n *NativeFrame[I]) BitsPerSample() int   { return n.InternalBitsPerSample }
 func (n *NativeFrame[I]) SamplesPerPixel() int { return n.InternalSamplesPerPixel }
-func (n *NativeFrame[I]) GetPixelAtIdx(idx int) []int {
-	vals := make([]int, n.InternalSamplesPerPixel)
-	for i := 0; i < n.InternalSamplesPerPixel; i++ {
-		vals[i] = int(n.RawData[idx+i])
+
+func (n *NativeFrame[I]) GetPixel(x, y int) ([]int, error) {
+	if x < 0 || y < 0 || x >= n.InternalCols || y >= n.InternalRows {
+		return nil, fmt.Errorf("provided zero-indexed coordinate (%v, %v) is out of bounds for this frame which has dimension %v x %v", x, y, n.InternalCols, n.InternalRows)
 	}
-	return vals
-}
-func (n *NativeFrame[I]) GetPixel(x, y int) []int {
 	pixelIdx := (x * n.InternalSamplesPerPixel) + (y * (n.Cols() * n.InternalSamplesPerPixel))
 	vals := make([]int, n.InternalSamplesPerPixel)
 	for i := 0; i < n.InternalSamplesPerPixel; i++ {
 		vals[i] = int(n.RawData[pixelIdx+i])
 	}
-	return vals
+	return vals, nil
 }
 
 func (n *NativeFrame[I]) GetSample(x, y, sampleIdx int) int {

pkg/frame/native_test.go

@@ -2,9 +2,11 @@ package frame_test
 
 import (
 	"errors"
+	"fmt"
 	"image"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/suyashkumar/dicom/pkg/frame"
 )
 
@@ -111,6 +113,97 @@ func TestNativeFrame_GetImage_Errors(t *testing.T) {
 	}
 }
 
+func TestNativeFrame_SimpleHelpers(t *testing.T) {
+	// Tests various helper methods on NativeFrame[I]. GetImage is tested in a
+	// separate top-level test case.
+	f := frame.NativeFrame[uint8]{
+		RawData:                 []uint8{1, 2, 3, 4, 5, 6},
+		InternalSamplesPerPixel: 2,
+		InternalRows:            1,
+		InternalCols:            3,
+		InternalBitsPerSample:   8,
+	}
+
+	if got := f.Rows(); got != 1 {
+		t.Errorf("Rows() unexpected value, got: %v, want: %v", got, 1)
+	}
+	if got := f.Cols(); got != 3 {
+		t.Errorf("Cols() unexpected value, got: %v, want: %v", got, 3)
+	}
+	if got := f.SamplesPerPixel(); got != 2 {
+		t.Errorf("SamplesPerPixel() unexpected value, got: %v, want: %v", got, 2)
+	}
+	if got := f.BitsPerSample(); got != 8 {
+		t.Errorf("BitsPerSample() unexpected value, got: %v, want: %v", got, 8)
+	}
+}
+
+func TestNativeFrame_GetPixel(t *testing.T) {
+	f := frame.NativeFrame[uint8]{
+		RawData:                 []uint8{1, 2, 3, 4, 5, 6, 7, 8},
+		InternalSamplesPerPixel: 2,
+		InternalRows:            2,
+		InternalCols:            2,
+		InternalBitsPerSample:   8,
+	}
+	cases := []struct {
+		x    int
+		y    int
+		want []int
+	}{
+		{
+			x:    0,
+			y:    0,
+			want: []int{1, 2},
+		},
+		{
+			x:    1,
+			y:    0,
+			want: []int{3, 4},
+		},
+		{
+			x:    0,
+			y:    1,
+			want: []int{5, 6},
+		},
+		{
+			x:    1,
+			y:    1,
+			want: []int{7, 8},
+		},
+	}
+	for _, tc := range cases {
+		t.Run(fmt.Sprintf("x: %d, y: %d", tc.x, tc.y), func(t *testing.T) {
+			got, err := f.GetPixel(tc.x, tc.y)
+			if err != nil {
+				t.Errorf("GetPixel(%d, %d) got unexpected error: %v", tc.x, tc.y, err)
+			}
+			if diff := cmp.Diff(got, tc.want); diff != "" {
+				t.Errorf("GetPixel(%d, %d) got unexpected slice. diff: %v", tc.x, tc.y, diff)
+			}
+		})
+	}
+}
+
+func TestNativeFrame_RawDataSlice(t *testing.T) {
+	f := frame.NativeFrame[uint8]{
+		RawData:                 []uint8{1, 2, 3, 4, 5, 6, 7, 8},
+		InternalSamplesPerPixel: 2,
+		InternalRows:            2,
+		InternalCols:            2,
+		InternalBitsPerSample:   8,
+	}
+
+	sl := f.RawDataSlice()
+	rd, ok := sl.([]uint8)
+	if !ok {
+		t.Errorf("RawDataSlice() should have returned a []uint8, but unable to type cast to []uint8")
+	}
+	if diff := cmp.Diff(rd, f.RawData); diff != "" {
+		t.Errorf("RawDataSlice() got unexpected slice. diff: %v", diff)
+	}
+}
+
 // within returns true if pt is in the []point
 func within(pt point, set []point) bool {
 	for _, item := range set {

read.go

@@ -503,8 +503,9 @@ func (r *reader) readNativeFrames(parsedData *Dataset, fc chan<- *frame.Frame, v
 	return &image, bytesToRead, nil
 }
 
+// readNativeFrame builds and reads a single NativeFrame[I] from the rawReader.
+// TODO(suyashkumar): refactor args to an options struct, or something more compact and readable.
 func readNativeFrame[I constraints.Integer](bitsAllocated, rows, cols, bytesToRead, samplesPerPixel, pixelsPerFrame int, pixelBuf []byte, rawReader *dicomio.Reader) (frame.Frame, int, error) {
-	// Init current frame
 	nativeFrame := frame.NewNativeFrame[I](bitsAllocated, rows, cols, pixelsPerFrame, samplesPerPixel)
 	currentFrame := frame.Frame{
 		Encapsulated: false,
@@ -519,29 +520,29 @@ func readNativeFrame[I constraints.Integer](bitsAllocated, rows, cols, bytesToRe
 				return frame.Frame{}, bytesToRead,
 					fmt.Errorf("could not read uint%d from input: %w", bitsAllocated, err)
 			}
-			if bitsAllocated == 8 {
+			switch bitsAllocated {
+			case 8:
 				v, ok := any(pixelBuf[0]).(I)
 				if !ok {
-
+					return frame.Frame{}, bytesToRead, fmt.Errorf("internal error - readNativeFrame unexpectedly unable to type cast pixel buffer data to the I type (%T), where bitsAllocated=%v", *new(I), bitsAllocated)
 				}
 				nativeFrame.RawData[(pixel*samplesPerPixel)+value] = v
-			} else if bitsAllocated == 16 {
+			case 16:
 				v, ok := any(bo.Uint16(pixelBuf)).(I)
 				if !ok {
-
+					return frame.Frame{}, bytesToRead, fmt.Errorf("internal error - readNativeFrame unexpectedly unable to type cast pixel buffer data to the I type (%T), where bitsAllocated=%v", *new(I), bitsAllocated)
 				}
 				nativeFrame.RawData[(pixel*samplesPerPixel)+value] = v
-			} else if bitsAllocated == 32 {
+			case 32:
 				v, ok := any(bo.Uint32(pixelBuf)).(I)
 				if !ok {
-
+					return frame.Frame{}, bytesToRead, fmt.Errorf("internal error - readNativeFrame unexpectedly unable to type cast pixel buffer data to the I type (%T), where bitsAllocated=%v", *new(I), bitsAllocated)
 				}
 				nativeFrame.RawData[(pixel*samplesPerPixel)+value] = v
-			} else {
-				return frame.Frame{}, bytesToRead, fmt.Errorf("bitsAllocated=%d : %w", bitsAllocated, ErrorUnsupportedBitsAllocated)
+			default:
+				return frame.Frame{}, bytesToRead, fmt.Errorf("readNativeFrame unsupported bitsAllocated=%d : %w", bitsAllocated, ErrorUnsupportedBitsAllocated)
 			}
 		}
-		// nativeFrame.Data[pixel] = buf[pixel*samplesPerPixel : (pixel+1)*samplesPerPixel]
 	}
 	return currentFrame, bytesToRead, nil
 }

write.go

@@ -638,7 +638,7 @@ func writePixelData(w *dicomio.Writer, t tag.Tag, value Value, vr string, vl uin
 		}
 		numFrames := len(image.Frames)
 		numPixels := image.Frames[0].NativeData.Rows() * image.Frames[0].NativeData.Cols()
-		numValues := len(image.Frames[0].NativeData.GetPixelAtIdx(0))
+		numValues := image.Frames[0].NativeData.SamplesPerPixel()
 		// Total required buffer length in bytes:
 		length := numFrames * numPixels * numValues * image.Frames[0].NativeData.BitsPerSample() / 8