Add option to skip Element Value processing of PixelData, while prese…

…rving roundtrip read/write (#256)

This addresses _some_ of the discussion in #224. The SkipProcessingPixelDataValue option causes the PixelData bytes to be read into the element, but not processed any further. This option provides an option to save the CPU cycles processing NativePixel data but still gives a roundtrip-able Dataset.

If you want to save both CPU and Memory, you can instead use the SkipPixelData option, which doesn't load the PixelData bytes into memory at all.

In the future, we should consider an option to lazy load/process all Element values if possible.

dataset.go

@@ -71,13 +71,14 @@ func (d *Dataset) FindElementByTagNested(tag tag.Tag) (*Element, error) {
 // If for some reason your code will not exhaust the iterator (read all
 // elements), be sure to call ExhaustElementChannel to prevent leaving the
 // underlying Goroutine alive (you can safely do this in a defer).
-//  c := dataset.FlatIterator()
-//  defer ExhaustElementChannel(c)
-//  for elem := range c {
-//      // Even if you exit before reading everything in c (e.g. due to an
-//      // error)
-//      // things will be ok.
-//  }
+//
+//	c := dataset.FlatIterator()
+//	defer ExhaustElementChannel(c)
+//	for elem := range c {
+//	    // Even if you exit before reading everything in c (e.g. due to an
+//	    // error)
+//	    // things will be ok.
+//	}
 //
 // Note that the sequence element itself is sent on the channel in addition to
 // the child elements in the sequence.

element.go

@@ -296,17 +296,32 @@ func (s *sequencesValue) MarshalJSON() ([]byte, error) {
 
 // PixelDataInfo is a representation of DICOM PixelData.
 type PixelDataInfo struct {
-	// IntentionallySkipped indicates if parsing/processing this PixelData tag
-	// was intentionally skipped. This is likely true if the dicom.SkipPixelData
-	// option was set. If true, the rest of this PixelDataInfo will be empty.
-	IntentionallySkipped bool
-	Frames               []frame.Frame
+	// IntentionallySkipped indicates that reading the PixelData value was
+	// intentionally skipped and no Value data for this tag was read.
+	// This is likely true if the dicom.SkipPixelData option was set. If true,
+	// the rest of this PixelDataInfo will be empty.
+	IntentionallySkipped bool `json:"intentionallySkipped"`
+
+	// Frames hold the processed PixelData frames (either Native or Encapsulated
+	// PixelData).
+	Frames []frame.Frame
+
 	// ParseErr indicates if there was an error when reading this Frame from the DICOM.
 	// If this is set, this means fallback behavior was triggered to blindly write the PixelData bytes to an encapsulated frame.
 	// The ParseErr will contain details about the specific error encountered.
 	ParseErr       error `json:"parseErr"`
 	IsEncapsulated bool  `json:"isEncapsulated"`
 	Offsets        []uint32
+
+	// IntentionallyUnprocessed indicates that the PixelData Value was actually
+	// read (as opposed to skipped over, as in IntentionallySkipped above) and
+	// blindly placed into RawData (if possible). Writing this element back out
+	// should work. This will be true if the
+	// dicom.SkipProcessingPixelDataValue flag is set with a PixelData tag.
+	IntentionallyUnprocessed bool `json:"intentionallyUnprocessed"`
+	// UnprocessedValueData holds the unprocessed Element value data if
+	// IntentionallyUnprocessed=true.
+	UnprocessedValueData []byte
 }
 
 // pixelDataValue represents DICOM PixelData

parse.go

@@ -113,7 +113,6 @@ type Parser struct {
 // provided).
 func NewParser(in io.Reader, bytesToRead int64, frameChannel chan *frame.Frame, opts ...ParseOption) (*Parser, error) {
 	optSet := toParseOptSet(opts...)
-
 	p := Parser{
 		reader: &reader{
 			rawReader: dicomio.NewReader(bufio.NewReader(in), binary.LittleEndian, bytesToRead),
@@ -211,6 +210,7 @@ type parseOptSet struct {
 	skipMetadataReadOnNewParserInit bool
 	allowMismatchPixelDataLength    bool
 	skipPixelData                   bool
+	skipProcessingPixelDataValue    bool
 }
 
 func toParseOptSet(opts ...ParseOption) parseOptSet {
@@ -236,11 +236,29 @@ func SkipMetadataReadOnNewParserInit() ParseOption {
 	}
 }
 
-// SkipPixelData skips parsing/processing the PixelData tag, wherever it appears
+// SkipPixelData skips reading data from the PixelData tag, wherever it appears
 // (e.g. even if within an IconSequence). A PixelDataInfo will be added to the
-// Dataset with the IntentionallySkipped property set to true.
+// Dataset with the IntentionallySkipped property set to true, and no other
+// data. Use this option if you don't need the PixelData value to be in the
+// Dataset at all, and want to save both CPU and Memory. If you need the
+// PixelData value in the Dataset (e.g. so it can be written out identically
+// later) but _don't_ want to process/parse the value, see the
+// SkipProcessingPixelDataValue option below.
 func SkipPixelData() ParseOption {
 	return func(set *parseOptSet) {
 		set.skipPixelData = true
 	}
 }
+
+// SkipProcessingPixelDataValue will attempt to skip processing the _value_
+// of any PixelData elements. Unlike SkipPixelData(), this means the PixelData
+// bytes will still be read into the Dataset, and can be written back out via
+// this library's write functionality. But, if possible, the value will be read
+// in as raw bytes with no further processing instead of being parsed. In the
+// future, we may be able to extend this functionality to support on-demand
+// processing of elements elsewhere in the library.
+func SkipProcessingPixelDataValue() ParseOption {
+	return func(set *parseOptSet) {
+		set.skipProcessingPixelDataValue = true
+	}
+}

parse_test.go

@@ -17,8 +17,8 @@ import (
 	"github.com/suyashkumar/dicom"
 )
 
-// TestParse is an end-to-end sanity check over DICOMs in testdata/. Currently it only checks that no error is returned
-// when parsing the files.
+// TestParse is an end-to-end sanity check over DICOMs in testdata/. Currently,
+// it only checks that no error is returned when parsing the files.
 func TestParse(t *testing.T) {
 	files, err := ioutil.ReadDir("./testdata")
 	if err != nil {
@@ -69,40 +69,85 @@ func TestNewParserSkipMetadataReadOnNewParserInit(t *testing.T) {
 	}
 }
 
-func TestNewParserSkipPixelData(t *testing.T) {
+func TestParseFile_SkipPixelData(t *testing.T) {
 	t.Run("WithSkipPixelData", func(t *testing.T) {
-		dataset, err := dicom.ParseFile("./testdata/1.dcm", nil, dicom.SkipPixelData())
-		if err != nil {
-			t.Errorf("Unexpected error parsing dataset: %v", dataset)
-		}
-		el, err := dataset.FindElementByTag(tag.PixelData)
-		if err != nil {
-			t.Errorf("Unexpected error when finding PixelData in Dataset: %v", err)
-		}
-		pixelData := dicom.MustGetPixelDataInfo(el.Value)
-		if !pixelData.IntentionallySkipped {
-			t.Errorf("Expected pixelData.IntentionallySkipped=true, got false")
-		}
-		if got := len(pixelData.Frames); got != 0 {
-			t.Errorf("unexpected frames length. got: %v, want: %v", got, 0)
-		}
+		runForEveryTestFile(t, func(t *testing.T, filename string) {
+			dataset, err := dicom.ParseFile(filename, nil, dicom.SkipPixelData())
+			if err != nil {
+				t.Errorf("Unexpected error parsing dataset: %v", dataset)
+			}
+			el, err := dataset.FindElementByTag(tag.PixelData)
+			if err != nil {
+				t.Errorf("Unexpected error when finding PixelData in Dataset: %v", err)
+			}
+			pixelData := dicom.MustGetPixelDataInfo(el.Value)
+			if !pixelData.IntentionallySkipped {
+				t.Errorf("Expected pixelData.IntentionallySkipped=true, got false")
+			}
+			if got := len(pixelData.Frames); got != 0 {
+				t.Errorf("unexpected frames length. got: %v, want: %v", got, 0)
+			}
+		})
 	})
 	t.Run("WithNOSkipPixelData", func(t *testing.T) {
-		dataset, err := dicom.ParseFile("./testdata/1.dcm", nil)
-		if err != nil {
-			t.Errorf("Unexpected error parsing dataset: %v", dataset)
-		}
-		el, err := dataset.FindElementByTag(tag.PixelData)
-		if err != nil {
-			t.Errorf("Unexpected error when finding PixelData in Dataset: %v", err)
-		}
-		pixelData := dicom.MustGetPixelDataInfo(el.Value)
-		if pixelData.IntentionallySkipped {
-			t.Errorf("Expected pixelData.IntentionallySkipped=false when SkipPixelData option not present, got true")
-		}
-		if len(pixelData.Frames) == 0 {
-			t.Errorf("unexpected frames length when SkipPixelData=false. got: %v, want: >0", len(pixelData.Frames))
-		}
+		runForEveryTestFile(t, func(t *testing.T, filename string) {
+			dataset, err := dicom.ParseFile(filename, nil)
+			if err != nil {
+				t.Errorf("Unexpected error parsing dataset: %v", dataset)
+			}
+			el, err := dataset.FindElementByTag(tag.PixelData)
+			if err != nil {
+				t.Errorf("Unexpected error when finding PixelData in Dataset: %v", err)
+			}
+			pixelData := dicom.MustGetPixelDataInfo(el.Value)
+			if pixelData.IntentionallySkipped {
+				t.Errorf("Expected pixelData.IntentionallySkipped=false when SkipPixelData option not present, got true")
+			}
+			if len(pixelData.Frames) == 0 {
+				t.Errorf("unexpected frames length when SkipPixelData=false. got: %v, want: >0", len(pixelData.Frames))
+			}
+		})
+	})
+}
+
+func TestParseFile_SkipProcessingPixelDataValue(t *testing.T) {
+	t.Run("WithSkipProcessingPixelDataValue", func(t *testing.T) {
+		runForEveryTestFile(t, func(t *testing.T, filename string) {
+			dataset, err := dicom.ParseFile(filename, nil, dicom.SkipProcessingPixelDataValue())
+			if err != nil {
+				t.Errorf("Unexpected error parsing dataset: %v", dataset)
+			}
+			el, err := dataset.FindElementByTag(tag.PixelData)
+			if err != nil {
+				t.Errorf("Unexpected error when finding PixelData in Dataset: %v", err)
+			}
+			pixelData := dicom.MustGetPixelDataInfo(el.Value)
+			if !pixelData.IntentionallyUnprocessed {
+				t.Errorf("Expected pixelData.IntentionallyUnprocessed=true, got false")
+			}
+			if got := len(pixelData.Frames); got != 0 {
+				t.Errorf("unexpected frames length. got: %v, want: %v", got, 0)
+			}
+		})
+	})
+	t.Run("WithNOSkipProcessingPixelDataValue", func(t *testing.T) {
+		runForEveryTestFile(t, func(t *testing.T, filename string) {
+			dataset, err := dicom.ParseFile(filename, nil)
+			if err != nil {
+				t.Errorf("Unexpected error parsing dataset: %v", dataset)
+			}
+			el, err := dataset.FindElementByTag(tag.PixelData)
+			if err != nil {
+				t.Errorf("Unexpected error when finding PixelData in Dataset: %v", err)
+			}
+			pixelData := dicom.MustGetPixelDataInfo(el.Value)
+			if pixelData.IntentionallyUnprocessed {
+				t.Errorf("Expected pixelData.IntentionallyUnprocessed=false when TestParseFile_SkipProcessingPixelDataValue option not present, got true")
+			}
+			if len(pixelData.Frames) == 0 {
+				t.Errorf("unexpected frames length when TestParseFile_SkipProcessingPixelDataValue=false. got: %v, want: >0", len(pixelData.Frames))
+			}
+		})
 	})
 }
 
@@ -175,3 +220,18 @@ func Example_getImageFrames() {
 		_ = f.Close()
 	}
 }
+
+func runForEveryTestFile(t *testing.T, testFunc func(t *testing.T, filename string)) {
+	files, err := ioutil.ReadDir("./testdata")
+	if err != nil {
+		t.Fatalf("unable to read testdata/: %v", err)
+	}
+	for _, f := range files {
+		if !f.IsDir() && strings.HasSuffix(f.Name(), ".dcm") {
+			fullName := "./testdata/" + f.Name()
+			t.Run(fullName, func(t *testing.T) {
+				testFunc(t, fullName)
+			})
+		}
+	}
+}

pkg/tag/tag.go

@@ -66,6 +66,19 @@ func (t Tag) String() string {
 	return fmt.Sprintf("(%04x,%04x)", t.Group, t.Element)
 }
 
+// Tags represents a set of Tag structs.
+type Tags []*Tag
+
+// Contains returns true if the passed in tag exists within the Tags.
+func (t Tags) Contains(item *Tag) bool {
+	for _, elem := range t {
+		if elem.Equals(*item) {
+			return true
+		}
+	}
+	return false
+}
+
 // Info stores detailed information about a Tag defined in the DICOM
 // standard.
 type Info struct {
@@ -180,7 +193,7 @@ func MustFind(tag Tag) Info {
 // not part of the DICOM standard, or is retired from the standard, it returns
 // an error.
 //
-//   Example: FindTagByName("TransferSyntaxUID")
+//	Example: FindTagByName("TransferSyntaxUID")
 func FindByName(name string) (Info, error) {
 	maybeInitTagDict()
 	for _, ent := range tagDict {

read.go

@@ -233,6 +233,13 @@ func (r *reader) readPixelData(vl uint32, d *Dataset, fc chan<- *frame.Frame) (V
 		return &pixelDataValue{PixelDataInfo{IntentionallySkipped: true}}, nil
 	}
 
+	if r.opts.skipProcessingPixelDataValue {
+		val := &pixelDataValue{PixelDataInfo{IntentionallyUnprocessed: true}}
+		val.PixelDataInfo.UnprocessedValueData = make([]byte, vl)
+		_, err := io.ReadFull(r.rawReader, val.PixelDataInfo.UnprocessedValueData)
+		return val, err
+	}
+
 	// Assume we're reading NativeData data since we have a defined value length as per Part 5 Sec A.4 of DICOM spec.
 	// We need Elements that have been already parsed (rows, cols, etc) to parse frames out of NativeData Pixel data
 	if d == nil {

read_test.go

@@ -595,6 +595,32 @@ func TestReadPixelData_SkipPixelData(t *testing.T) {
 	}
 }
 
+func TestReadPixelData_TrySkipProcessingPixelDataValue(t *testing.T) {
+	opts := parseOptSet{skipProcessingPixelDataValue: true}
+	valueBytes := []byte{1, 2, 3, 4, 5, 6}
+	dcmdata := bytes.NewBuffer(valueBytes)
+
+	r := &reader{
+		rawReader: dicomio.NewReader(bufio.NewReader(dcmdata), binary.LittleEndian, int64(dcmdata.Len())),
+		opts:      opts,
+	}
+	val, err := r.readPixelData(6, &Dataset{}, nil)
+	if err != nil {
+		t.Errorf("unexpected error in readPixelData: %v", err)
+	}
+	pixelVal, ok := val.GetValue().(PixelDataInfo)
+	if !ok {
+		t.Errorf("Expected value to be of type PixelDataInfo")
+	}
+	if !pixelVal.IntentionallyUnprocessed {
+		t.Errorf("Expected PixelDataInfo to have IntentionallyUnprocessed=true")
+	}
+	if !cmp.Equal(pixelVal.UnprocessedValueData, valueBytes) {
+		t.Errorf("expected UnprocessedValueData to match valueBytes. got: %v, want: %v", pixelVal.UnprocessedValueData, valueBytes)
+	}
+
+}
+
 func makeEncapsulatedSequence(t *testing.T) []byte {
 	t.Helper()
 	buf := &bytes.Buffer{}

write.go

@@ -562,6 +562,12 @@ func writePixelData(w dicomio.Writer, t tag.Tag, value Value, vr string, vl uint
 			return err
 		}
 	} else {
+		// For now, IntentionallyUnprocessed will only happen for Native
+		// PixelData.
+		if image.IntentionallyUnprocessed {
+			w.WriteBytes(image.UnprocessedValueData)
+			return nil
+		}
 		numFrames := len(image.Frames)
 		numPixels := len(image.Frames[0].NativeData.Data)
 		numValues := len(image.Frames[0].NativeData.Data[0])

write_test.go

@@ -34,6 +34,7 @@ func TestWrite(t *testing.T) {
 		extraElems    []*Element
 		expectedError error
 		opts          []WriteOption
+		parseOpts     []ParseOption
 		cmpOpts       []cmp.Option
 	}{
 		{
@@ -441,6 +442,24 @@ func TestWrite(t *testing.T) {
 			}},
 			expectedError: nil,
 		},
+		{
+			name: "PixelData with IntentionallyUnprocessed=true",
+			dataset: Dataset{Elements: []*Element{
+				mustNewElement(tag.MediaStorageSOPClassUID, []string{"1.2.840.10008.5.1.4.1.1.1.2"}),
+				mustNewElement(tag.MediaStorageSOPInstanceUID, []string{"1.2.3.4.5.6.7"}),
+				mustNewElement(tag.TransferSyntaxUID, []string{uid.ImplicitVRLittleEndian}),
+				mustNewElement(tag.BitsAllocated, []int{8}),
+				mustNewElement(tag.FloatingPointValue, []float64{128.10}),
+				mustNewElement(tag.DimensionIndexPointer, []int{32, 36950}),
+				mustNewElement(tag.PixelData, PixelDataInfo{
+					IntentionallyUnprocessed: true,
+					UnprocessedValueData:     []byte{1, 2, 3, 4},
+					IsEncapsulated:           false,
+				}),
+			}},
+			parseOpts:     []ParseOption{SkipProcessingPixelDataValue()},
+			expectedError: nil,
+		},
 	}
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
@@ -464,7 +483,7 @@ func TestWrite(t *testing.T) {
 					t.Fatalf("Unexpected error state file: %s: %v", file.Name(), err)
 				}
 
-				readDS, err := Parse(f, info.Size(), nil)
+				readDS, err := Parse(f, info.Size(), nil, tc.parseOpts...)
 				if err != nil {
 					t.Errorf("Parse of written file, unexpected error: %v", err)
 				}