From 704440d62f3a032a3f6fee713581195c65e4ea68 Mon Sep 17 00:00:00 2001 From: Suyash Kumar Date: Sun, 9 Jun 2024 20:20:40 -0400 Subject: [PATCH 1/3] Add write option to override missing transfer syntax in dataset --- read_test.go | 5 +++- write.go | 75 ++++++++++++++++++++++++++++++++++++++++++--------- write_test.go | 67 ++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 132 insertions(+), 15 deletions(-) diff --git a/read_test.go b/read_test.go index 03af584..5b30afc 100644 --- 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 index 3e75a1d..00a1871 100644 --- a/write.go +++ b/write.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io" + "log" "slices" "github.com/suyashkumar/dicom/pkg/vrraw" @@ -37,14 +38,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,17 +70,23 @@ 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 (err != nil && err != ErrorElementNotFound) || (err == ErrorElementNotFound && (!w.optSet.defaultMissingTransferSyntax && w.optSet.overrideMissingTransferSyntaxUID == "")) { return err } - if err == ErrorElementNotFound && w.optSet.defaultMissingTransferSyntax { - w.writer.SetTransferSyntax(binary.LittleEndian, true) - } else { - w.writer.SetTransferSyntax(endian, implicit) + 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 + } } + w.writer.SetTransferSyntax(bo, implicit) + for _, elem := range ds.Elements { if elem.Tag.Group != tag.MetadataGroup { err = writeElement(w.writer, elem, *w.optSet) @@ -97,7 +107,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) } @@ -130,11 +143,33 @@ func DefaultMissingTransferSyntax() WriteOption { } } +// OverrideMissingTransferSyntaxWith returns a WriteOption indicating that if +// the dicom to be written does _not_ have a transfer syntax UID in its metadata +// that it should be written using the provided transferSyntaxUID. +// +// If the Writer is unable to recognize or write out using the provided +// transferSyntaxUID, an error will be returned at initialization time. +func OverrideMissingTransferSyntaxWith(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 OverrideMissingTransferSyntaxWith transfer syntax uid %v due to: %s", w.overrideMissingTransferSyntaxUID, err) + } + } + return nil } func toWriteOptSet(opts ...WriteOption) *writeOptSet { @@ -166,10 +201,16 @@ func writeFileHeader(w *dicomio.Writer, ds *Dataset, metaElems []*Element, opts if err != nil && err != ErrorElementNotFound { return err } + log.Println("TEST") + log.Println(opts.overrideMissingTransferSyntaxUID) + err = writeMetaElem(subWriter, tag.TransferSyntaxUID, ds, &tagsUsed, opts) - if err != nil && err != ErrorElementNotFound || err == ErrorElementNotFound && !opts.defaultMissingTransferSyntax { + + // TODO(suyashkumar): clean up this logic + if err != nil && err != ErrorElementNotFound || (err == ErrorElementNotFound && (!opts.defaultMissingTransferSyntax && opts.overrideMissingTransferSyntaxUID == "")) { return err } + if err == ErrorElementNotFound && opts.defaultMissingTransferSyntax { // Write the default transfer syntax if err = writeElement(subWriter, mustNewElement(tag.TransferSyntaxUID, []string{uid.ImplicitVRLittleEndian}), opts); err != nil { @@ -177,6 +218,14 @@ func writeFileHeader(w *dicomio.Writer, ds *Dataset, metaElems []*Element, opts } } + if err == ErrorElementNotFound && opts.overrideMissingTransferSyntaxUID != "" { + // Write the override transfer syntax + if err = writeElement(subWriter, mustNewElement(tag.TransferSyntaxUID, []string{opts.overrideMissingTransferSyntaxUID}), opts); err != nil { + return err + } + log.Println("OVERRIDDEN") + } + for _, elem := range metaElems { if elem.Tag.Group == tag.MetadataGroup { if _, ok := tagsUsed[elem.Tag]; !ok { diff --git a/write_test.go b/write_test.go index 355d5c5..94e4b1e 100644 --- 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,65 @@ func TestWriteElement(t *testing.T) { } } } + +func TestWrite_OverrideMissingTransferSyntaxWith(t *testing.T) { + ds := 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) { + writtenDICOM := &bytes.Buffer{} + if err := Write(writtenDICOM, ds, OverrideMissingTransferSyntaxWith(tc.overrideTransferSyntax)); err != nil { + t.Errorf("Write(OverrideMissingTransferSyntaxWith(%v)) returned unexpected error: %v", tc.overrideTransferSyntax, err) + } + + // Read dataset back in to see if 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) + } + + }) + } +} From 733cc81f2345a87711f261a540ae2926adec3440 Mon Sep 17 00:00:00 2001 From: Suyash Kumar Date: Sun, 9 Jun 2024 22:03:17 -0400 Subject: [PATCH 2/3] cleanup --- write.go | 44 +++++++++++++++++++------------------------- write_test.go | 13 ++++++------- 2 files changed, 25 insertions(+), 32 deletions(-) diff --git a/write.go b/write.go index 00a1871..9d789fb 100644 --- a/write.go +++ b/write.go @@ -7,7 +7,6 @@ import ( "errors" "fmt" "io" - "log" "slices" "github.com/suyashkumar/dicom/pkg/vrraw" @@ -71,10 +70,6 @@ func (w *Writer) writeDataset(ds Dataset) error { } bo, implicit, err := ds.transferSyntax() - if (err != nil && err != ErrorElementNotFound) || (err == ErrorElementNotFound && (!w.optSet.defaultMissingTransferSyntax && w.optSet.overrideMissingTransferSyntaxUID == "")) { - return err - } - if errors.Is(err, ErrorElementNotFound) && w.optSet.defaultMissingTransferSyntax { bo = binary.LittleEndian implicit = true @@ -83,6 +78,8 @@ func (w *Writer) writeDataset(ds Dataset) error { if err != nil { return err } + } else if err != nil { + return err } w.writer.SetTransferSyntax(bo, implicit) @@ -137,19 +134,23 @@ 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 } } -// OverrideMissingTransferSyntaxWith returns a WriteOption indicating that if +// OverrideMissingTransferSyntax returns a WriteOption indicating that if // the dicom to be written does _not_ have a transfer syntax UID in its metadata -// that it should be written using the provided transferSyntaxUID. +// that it should be written using the provided transferSyntaxUID. 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 out using the provided +// If the Writer is unable to recognize or write the dataset using the provided // transferSyntaxUID, an error will be returned at initialization time. -func OverrideMissingTransferSyntaxWith(transferSyntaxUID string) WriteOption { +func OverrideMissingTransferSyntax(transferSyntaxUID string) WriteOption { return func(set *writeOptSet) { set.overrideMissingTransferSyntaxUID = transferSyntaxUID } @@ -166,7 +167,7 @@ type writeOptSet struct { func (w *writeOptSet) validate() error { if w.overrideMissingTransferSyntaxUID != "" { if _, _, err := uid.ParseTransferSyntaxUID(w.overrideMissingTransferSyntaxUID); err != nil { - return fmt.Errorf("unable to parse OverrideMissingTransferSyntaxWith transfer syntax uid %v due to: %s", w.overrideMissingTransferSyntaxUID, err) + return fmt.Errorf("unable to parse OverrideMissingTransferSyntax transfer syntax uid %v due to: %s", w.overrideMissingTransferSyntaxUID, err) } } return nil @@ -190,40 +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 } - log.Println("TEST") - log.Println(opts.overrideMissingTransferSyntaxUID) err = writeMetaElem(subWriter, tag.TransferSyntaxUID, ds, &tagsUsed, opts) - // TODO(suyashkumar): clean up this logic - if err != nil && err != ErrorElementNotFound || (err == ErrorElementNotFound && (!opts.defaultMissingTransferSyntax && opts.overrideMissingTransferSyntaxUID == "")) { - 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 } - } - - if err == ErrorElementNotFound && opts.overrideMissingTransferSyntaxUID != "" { + } 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 } - log.Println("OVERRIDDEN") + } 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 index 94e4b1e..b689cc5 100644 --- a/write_test.go +++ b/write_test.go @@ -896,8 +896,8 @@ func TestWriteElement(t *testing.T) { } } -func TestWrite_OverrideMissingTransferSyntaxWith(t *testing.T) { - ds := Dataset{Elements: []*Element{ +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"}), @@ -927,14 +927,14 @@ func TestWrite_OverrideMissingTransferSyntaxWith(t *testing.T) { 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, ds, OverrideMissingTransferSyntaxWith(tc.overrideTransferSyntax)); err != nil { - t.Errorf("Write(OverrideMissingTransferSyntaxWith(%v)) returned unexpected error: %v", tc.overrideTransferSyntax, err) + 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 see if no roundtrip errors, and also + // 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) @@ -953,7 +953,6 @@ func TestWrite_OverrideMissingTransferSyntaxWith(t *testing.T) { 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) } - }) } } From 133180d2d3cc22e1f81326a9d3ab36d5f8dc1e53 Mon Sep 17 00:00:00 2001 From: Suyash Kumar Date: Sun, 9 Jun 2024 22:06:02 -0400 Subject: [PATCH 3/3] cleanup --- write.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/write.go b/write.go index 9d789fb..3b9be99 100644 --- a/write.go +++ b/write.go @@ -142,9 +142,9 @@ func DefaultMissingTransferSyntax() WriteOption { } } -// OverrideMissingTransferSyntax returns a WriteOption indicating that if -// the dicom to be written does _not_ have a transfer syntax UID in its metadata -// that it should be written using the provided transferSyntaxUID. A +// 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. //