Add WriteOption to override missing transfer syntax in dataset (#332)

This fixes #328. This change will also help set the stage for roundtrip tests for transfer syntax uid inference in the case of missing transfer syntax UIDs (part of #327). For the future: should we give this option as something to always override whatever transfer syntax is present (or if not present, use the override as well)?
suyashkumar · Jun 10, 2024 · 0b4bb9f · 0b4bb9f
1 parent fe2aa28
commit 0b4bb9f
Show file tree

Hide file tree

Showing 3 changed files with 132 additions and 22 deletions.
diff --git a/read_test.go b/read_test.go
@@ -606,7 +606,10 @@ type headerData struct {
 // Write a collection of elements and return them as an encoded buffer of bytes.
 func writeElements(elements []*Element) ([]byte, error) {
 	buff := bytes.Buffer{}
-	dcmWriter := NewWriter(&buff)
+	dcmWriter, err := NewWriter(&buff)
+	if err != nil {
+		return []byte{}, err
+	}
 	dcmWriter.SetTransferSyntax(binary.LittleEndian, true)
 
 	for _, e := range elements {

diff --git a/write.go b/write.go
@@ -37,14 +37,17 @@ type Writer struct {
 }
 
 // NewWriter returns a new Writer, that points to the provided io.Writer.
-func NewWriter(out io.Writer, opts ...WriteOption) *Writer {
+func NewWriter(out io.Writer, opts ...WriteOption) (*Writer, error) {
 	optSet := toWriteOptSet(opts...)
+	if err := optSet.validate(); err != nil {
+		return nil, err
+	}
 	w := dicomio.NewWriter(out, nil, false)
 
 	return &Writer{
 		writer: w,
 		optSet: optSet,
-	}
+	}, nil
 }
 
 // SetTransferSyntax sets the transfer syntax for the underlying dicomio.Writer.
@@ -66,16 +69,20 @@ func (w *Writer) writeDataset(ds Dataset) error {
 		return err
 	}
 
-	endian, implicit, err := ds.transferSyntax()
-	if (err != nil && err != ErrorElementNotFound) || (err == ErrorElementNotFound && !w.optSet.defaultMissingTransferSyntax) {
+	bo, implicit, err := ds.transferSyntax()
+	if errors.Is(err, ErrorElementNotFound) && w.optSet.defaultMissingTransferSyntax {
+		bo = binary.LittleEndian
+		implicit = true
+	} else if errors.Is(err, ErrorElementNotFound) && w.optSet.overrideMissingTransferSyntaxUID != "" {
+		bo, implicit, err = uid.ParseTransferSyntaxUID(w.optSet.overrideMissingTransferSyntaxUID)
+		if err != nil {
+			return err
+		}
+	} else if err != nil {
 		return err
 	}
 
-	if err == ErrorElementNotFound && w.optSet.defaultMissingTransferSyntax {
-		w.writer.SetTransferSyntax(binary.LittleEndian, true)
-	} else {
-		w.writer.SetTransferSyntax(endian, implicit)
-	}
+	w.writer.SetTransferSyntax(bo, implicit)
 
 	for _, elem := range ds.Elements {
 		if elem.Tag.Group != tag.MetadataGroup {
@@ -97,7 +104,10 @@ func (w *Writer) WriteElement(e *Element) error {
 // Write will write the input DICOM dataset to the provided io.Writer as a complete DICOM (including any header
 // information if available).
 func Write(out io.Writer, ds Dataset, opts ...WriteOption) error {
-	w := NewWriter(out, opts...)
+	w, err := NewWriter(out, opts...)
+	if err != nil {
+		return err
+	}
 	return w.writeDataset(ds)
 }
 
@@ -124,17 +134,43 @@ func SkipValueTypeVerification() WriteOption {
 // transferSyntax should not raise an error, and instead the default
 // LittleEndian Implicit transfer syntax should be used and written out as a
 // Metadata element in the Dataset.
+// TODO(suyashkumar): consider deprecating in favor of
+// OverrideMissingTransferSyntax.
 func DefaultMissingTransferSyntax() WriteOption {
 	return func(set *writeOptSet) {
 		set.defaultMissingTransferSyntax = true
 	}
 }
 
+// OverrideMissingTransferSyntax indicates that if the Dataset to be written
+// does _not_ have a transfer syntax UID in its metadata, the Dataset should
+// be written out with the provided transfer syntax UID if possible. A
+// transfer syntax uid element with the specified transfer syntax will be
+// written to the metadata as well.
+//
+// If the Writer is unable to recognize or write the dataset using the provided
+// transferSyntaxUID, an error will be returned at initialization time.
+func OverrideMissingTransferSyntax(transferSyntaxUID string) WriteOption {
+	return func(set *writeOptSet) {
+		set.overrideMissingTransferSyntaxUID = transferSyntaxUID
+	}
+}
+
 // writeOptSet represents the flattened option set after all WriteOptions have been applied.
 type writeOptSet struct {
-	skipVRVerification           bool
-	skipValueTypeVerification    bool
-	defaultMissingTransferSyntax bool
+	skipVRVerification               bool
+	skipValueTypeVerification        bool
+	defaultMissingTransferSyntax     bool
+	overrideMissingTransferSyntaxUID string
+}
+
+func (w *writeOptSet) validate() error {
+	if w.overrideMissingTransferSyntaxUID != "" {
+		if _, _, err := uid.ParseTransferSyntaxUID(w.overrideMissingTransferSyntaxUID); err != nil {
+			return fmt.Errorf("unable to parse OverrideMissingTransferSyntax transfer syntax uid %v due to: %s", w.overrideMissingTransferSyntaxUID, err)
+		}
+	}
+	return nil
 }
 
 func toWriteOptSet(opts ...WriteOption) *writeOptSet {
@@ -155,26 +191,33 @@ func writeFileHeader(w *dicomio.Writer, ds *Dataset, metaElems []*Element, opts
 	tagsUsed[tag.FileMetaInformationGroupLength] = true
 
 	err := writeMetaElem(subWriter, tag.FileMetaInformationVersion, ds, &tagsUsed, opts)
-	if err != nil && err != ErrorElementNotFound {
+	if err != nil && !errors.Is(err, ErrorElementNotFound) {
 		return err
 	}
 	err = writeMetaElem(subWriter, tag.MediaStorageSOPClassUID, ds, &tagsUsed, opts)
-	if err != nil && err != ErrorElementNotFound {
+	if err != nil && !errors.Is(err, ErrorElementNotFound) {
 		return err
 	}
 	err = writeMetaElem(subWriter, tag.MediaStorageSOPInstanceUID, ds, &tagsUsed, opts)
-	if err != nil && err != ErrorElementNotFound {
+	if err != nil && !errors.Is(err, ErrorElementNotFound) {
 		return err
 	}
+
 	err = writeMetaElem(subWriter, tag.TransferSyntaxUID, ds, &tagsUsed, opts)
-	if err != nil && err != ErrorElementNotFound || err == ErrorElementNotFound && !opts.defaultMissingTransferSyntax {
-		return err
-	}
-	if err == ErrorElementNotFound && opts.defaultMissingTransferSyntax {
+
+	if errors.Is(err, ErrorElementNotFound) && opts.defaultMissingTransferSyntax {
 		// Write the default transfer syntax
 		if err = writeElement(subWriter, mustNewElement(tag.TransferSyntaxUID, []string{uid.ImplicitVRLittleEndian}), opts); err != nil {
 			return err
 		}
+	} else if errors.Is(err, ErrorElementNotFound) && opts.overrideMissingTransferSyntaxUID != "" {
+		// Write the override transfer syntax
+		if err = writeElement(subWriter, mustNewElement(tag.TransferSyntaxUID, []string{opts.overrideMissingTransferSyntaxUID}), opts); err != nil {
+			return err
+		}
+	} else if err != nil {
+		// Return the error if none of the above conditions/overrides apply.
+		return err
 	}
 
 	for _, elem := range metaElems {

diff --git a/write_test.go b/write_test.go
@@ -866,7 +866,10 @@ func TestWriteElement(t *testing.T) {
 	}}
 
 	buf := bytes.Buffer{}
-	w := NewWriter(&buf)
+	w, err := NewWriter(&buf)
+	if err != nil {
+		t.Fatalf("NewWriter() returned unexpected error: %v", err)
+	}
 	w.SetTransferSyntax(binary.LittleEndian, true)
 
 	for _, e := range writeDS.Elements {
@@ -892,3 +895,64 @@ func TestWriteElement(t *testing.T) {
 		}
 	}
 }
+
+func TestWrite_OverrideMissingTransferSyntax(t *testing.T) {
+	dsWithMissingTS := 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.PatientName, []string{"Bob", "Jones"}),
+		mustNewElement(tag.Rows, []int{128}),
+		mustNewElement(tag.FloatingPointValue, []float64{128.10}),
+		mustNewElement(tag.DimensionIndexPointer, []int{32, 36950}),
+		mustNewElement(tag.RedPaletteColorLookupTableData, []byte{0x1, 0x2, 0x3, 0x4}),
+	}}
+
+	cases := []struct {
+		name                   string
+		overrideTransferSyntax string
+	}{
+		{
+			name:                   "Little Endian Implicit",
+			overrideTransferSyntax: "1.2.840.10008.1.2",
+		},
+		{
+			name:                   "Little Endian Explicit",
+			overrideTransferSyntax: "1.2.840.10008.1.2.1",
+		},
+		{
+			name:                   "Big Endian Explicit",
+			overrideTransferSyntax: "1.2.840.10008.1.2.2",
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			// Write out dicom with OverrideMissingTransferSyntax option.
+			writtenDICOM := &bytes.Buffer{}
+			if err := Write(writtenDICOM, dsWithMissingTS, OverrideMissingTransferSyntax(tc.overrideTransferSyntax)); err != nil {
+				t.Errorf("Write(OverrideMissingTransferSyntax(%v)) returned unexpected error: %v", tc.overrideTransferSyntax, err)
+			}
+
+			// Read dataset back in to ensure no roundtrip errors, and also
+			// check that the written out transfer syntax tag matches.
+			parsedDS, err := ParseUntilEOF(writtenDICOM, nil)
+			if err != nil {
+				t.Fatalf("ParseUntilEOF returned unexpected error when reading written dataset back in: %v", err)
+			}
+			tsElem, err := parsedDS.FindElementByTag(tag.TransferSyntaxUID)
+			if err != nil {
+				t.Fatalf("unable to find transfer syntax uid element in written dataset: %v", err)
+			}
+			tsVal, ok := tsElem.Value.GetValue().([]string)
+			if !ok {
+				t.Fatalf("TransferSyntaxUID tag was not of type []")
+			}
+			if len(tsVal) != 1 {
+				t.Errorf("TransferSyntaxUID []string contained more than one element unexpectedly: %v", tsVal)
+			}
+			if tsVal[0] != tc.overrideTransferSyntax {
+				t.Errorf("TransferSyntaxUID in written dicom did not contain the override transfer syntax value. got: %v, want: %v", tsVal[0], tc.overrideTransferSyntax)
+			}
+		})
+	}
+}