From 87c7d7e41fca7227af3582174c5933fb91981eb7 Mon Sep 17 00:00:00 2001 From: Yoshiyuki Harada <70787551+marineotter@users.noreply.github.com> Date: Thu, 18 Nov 2021 08:58:33 +0900 Subject: [PATCH 1/7] Interpret "" DICOM Character Set as iso-8859-1 (#219) --- pkg/charset/charset.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/charset/charset.go b/pkg/charset/charset.go index a0dca827..9b4dcaa6 100644 --- a/pkg/charset/charset.go +++ b/pkg/charset/charset.go @@ -40,6 +40,7 @@ const ( // htmlEncodingNames represents a mapping of DICOM charset name to golang encoding/htmlindex name. "" means // 7bit ascii. var htmlEncodingNames = map[string]string{ + "": "iso-8859-1", "ISO_IR 6": "iso-8859-1", "ISO 2022 IR 6": "iso-8859-1", "ISO_IR 13": "shift_jis", From 965296c6d05192a59ec48407e5dc402d2d62ba92 Mon Sep 17 00:00:00 2001 From: Suyash Kumar Date: Fri, 28 Jan 2022 22:36:47 -0500 Subject: [PATCH 2/7] Treat Unknown Tags with defined VL as OW (#232) This change ensures that unknown tags with a defined VL are read as bytes (OW). This should fix #231. Previously they would have been read as strings by default. --- pkg/tag/tag.go | 2 +- read.go | 3 +-- read_test.go | 9 ++++++++- write.go | 4 ++-- write_test.go | 11 +++++++++++ 5 files changed, 23 insertions(+), 6 deletions(-) diff --git a/pkg/tag/tag.go b/pkg/tag/tag.go index 36f1db57..5c250e54 100644 --- a/pkg/tag/tag.go +++ b/pkg/tag/tag.go @@ -128,7 +128,7 @@ func GetVRKind(tag Tag, vr string) VRKind { return VRDate case "AT": return VRTagList - case "OW", "OB": + case "OW", "OB", "UN": return VRBytes case "LT", "UT": return VRString diff --git a/read.go b/read.go index 78567994..fe4b829b 100644 --- a/read.go +++ b/read.go @@ -422,7 +422,7 @@ func readSequenceItem(r dicomio.Reader, t tag.Tag, vr string, vl uint32) (Value, func readBytes(r dicomio.Reader, t tag.Tag, vr string, vl uint32) (Value, error) { // TODO: add special handling of PixelData - if vr == vrraw.OtherByte { + if vr == vrraw.OtherByte || vr == vrraw.Unknown { data := make([]byte, vl) _, err := io.ReadFull(r, data) return &bytesValue{value: data}, err @@ -466,7 +466,6 @@ func readString(r dicomio.Reader, t tag.Tag, vr string, vl uint32) (Value, error // Split multiple strings strs := strings.Split(str, "\\") - return &stringsValue{value: strs}, err } diff --git a/read_test.go b/read_test.go index 715ccf17..0d9820d4 100644 --- a/read_test.go +++ b/read_test.go @@ -168,12 +168,19 @@ func TestReadOWBytes(t *testing.T) { expectedErr error }{ { - name: "even-number bytes", + name: "OW VR with even-number bytes", bytes: []byte{0x1, 0x2, 0x3, 0x4}, VR: vrraw.OtherWord, want: &bytesValue{value: []byte{0x1, 0x2, 0x3, 0x4}}, expectedErr: nil, }, + { + name: "UN VR even-number bytes", + bytes: []byte{0x1, 0x2, 0x3, 0x4}, + VR: vrraw.Unknown, + want: &bytesValue{value: []byte{0x1, 0x2, 0x3, 0x4}}, + expectedErr: nil, + }, { name: "error on odd-number bytes", bytes: []byte{0x1, 0x2, 0x3}, diff --git a/write.go b/write.go index 4b148ff5..7d1f6409 100644 --- a/write.go +++ b/write.go @@ -313,7 +313,7 @@ func verifyValueType(t tag.Tag, value Value, vr string) error { ok = valueType == Sequences case "NA": ok = valueType == SequenceItem - case vrraw.OtherWord, vrraw.OtherByte: + case vrraw.OtherWord, vrraw.OtherByte, vrraw.Unknown: if t == tag.PixelData { ok = valueType == PixelData } else { @@ -488,7 +488,7 @@ func writeStrings(w dicomio.Writer, values []string, vr string) error { func writeBytes(w dicomio.Writer, values []byte, vr string) error { var err error switch vr { - case vrraw.OtherWord: + case vrraw.OtherWord, vrraw.Unknown: err = writeOtherWordString(w, values) case vrraw.OtherByte: err = writeOtherByteString(w, values) diff --git a/write_test.go b/write_test.go index ae87fade..3b682002 100644 --- a/write_test.go +++ b/write_test.go @@ -47,6 +47,17 @@ func TestWrite(t *testing.T) { mustNewElement(tag.FloatingPointValue, []float64{128.10}), mustNewElement(tag.DimensionIndexPointer, []int{32, 36950}), mustNewElement(tag.RedPaletteColorLookupTableData, []byte{0x1, 0x2, 0x3, 0x4}), + mustNewElement(tag.SelectorSLValue, []int{-20}), + // Some tag with an unknown VR. + { + Tag: tag.Tag{0x0019, 0x1027}, + ValueRepresentation: tag.VRBytes, + RawValueRepresentation: "UN", + ValueLength: 4, + Value: &bytesValue{ + value: []byte{0x1, 0x2, 0x3, 0x4}, + }, + }, }}, expectedError: nil, }, From 4bb843a85b925fc73129b4602964b8bdd2ebc589 Mon Sep 17 00:00:00 2001 From: Suyash Kumar Date: Thu, 28 Oct 2021 23:07:13 -0400 Subject: [PATCH 3/7] Initial commit to default reading VR=UN as SQ when reading with an implicit transfer syntax and undefined length --- pkg/tag/tag.go | 5 +++++ read.go | 8 ++++++++ 2 files changed, 13 insertions(+) diff --git a/pkg/tag/tag.go b/pkg/tag/tag.go index 5c250e54..c007c0fb 100644 --- a/pkg/tag/tag.go +++ b/pkg/tag/tag.go @@ -114,6 +114,9 @@ const ( VRDate // VRPixelData means the element stores a PixelDataInfo VRPixelData + // VRUnknown means the VR of the element is unknown (possibly a private + // element seen while reading DICOMs in implicit transfer syntax). + VRUnknown ) // GetVRKind returns the golang value encoding of an element with . @@ -146,6 +149,8 @@ func GetVRKind(tag Tag, vr string) VRKind { return VRFloat64List case "SQ": return VRSequence + case "UN": + return VRUnknown default: return VRStringList } diff --git a/read.go b/read.go index fe4b829b..975cebf6 100644 --- a/read.go +++ b/read.go @@ -108,6 +108,14 @@ func readValue(r dicomio.Reader, t tag.Tag, vr string, vl uint32, isImplicit boo return readDate(r, t, vr, vl) case tag.VRUInt16List, tag.VRUInt32List, tag.VRInt16List, tag.VRInt32List, tag.VRTagList: return readInt(r, t, vr, vl) + // We read Unknown VRs as SQ VRs by default. More details on why can be + // found at https://github.com/suyashkumar/dicom/issues/220. It remains + // to be seen if this fits most DICOMs we see in the wild. + case tag.VRUnknown: + if isImplicit && vl == tag.VLUndefinedLength { + return readSequence(r, t, vr, vl) + } + return readString(r, t, vr, vl) case tag.VRSequence: return readSequence(r, t, vr, vl) case tag.VRItem: From 4c1f1e6423a2c88712a9c348e8b210698c643c31 Mon Sep 17 00:00:00 2001 From: Suyash Kumar Date: Thu, 28 Oct 2021 23:16:12 -0400 Subject: [PATCH 4/7] add todo --- read.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/read.go b/read.go index 975cebf6..51e3e834 100644 --- a/read.go +++ b/read.go @@ -111,6 +111,9 @@ func readValue(r dicomio.Reader, t tag.Tag, vr string, vl uint32, isImplicit boo // We read Unknown VRs as SQ VRs by default. More details on why can be // found at https://github.com/suyashkumar/dicom/issues/220. It remains // to be seen if this fits most DICOMs we see in the wild. + // TODO(suyashkumar): consider replacing UN VRs with SQ earlier on if they + // meet this criteria, so users of the Dataset can interact with it + // correctly. case tag.VRUnknown: if isImplicit && vl == tag.VLUndefinedLength { return readSequence(r, t, vr, vl) From 487a1ae0270f79a52c2c66d7d1c66f8359c36007 Mon Sep 17 00:00:00 2001 From: Julian Billings Date: Fri, 11 Feb 2022 22:29:16 +0000 Subject: [PATCH 5/7] handle all forms of unknown data elements --- dataset_test.go | 28 +++++++++++++++++---------- element_test.go | 2 +- pkg/tag/tag.go | 2 +- read.go | 24 +++++++++++++----------- write.go | 4 +++- write_test.go | 50 ++++++++++++++++++++++++++++++++++++++++++------- 6 files changed, 79 insertions(+), 31 deletions(-) diff --git a/dataset_test.go b/dataset_test.go index 0607e21e..361fbab2 100644 --- a/dataset_test.go +++ b/dataset_test.go @@ -8,13 +8,13 @@ import ( "github.com/suyashkumar/dicom/pkg/tag" ) -func makeSequenceElement(tg tag.Tag, items [][]*Element) *Element { +func makeSequenceElement(tg tag.Tag, items [][]*Element, unknown bool) *Element { sequenceItems := make([]*SequenceItemValue, 0, len(items)) for _, item := range items { sequenceItems = append(sequenceItems, &SequenceItemValue{elements: item}) } - return &Element{ + e := &Element{ Tag: tg, ValueRepresentation: tag.VRSequence, RawValueRepresentation: "SQ", @@ -22,6 +22,14 @@ func makeSequenceElement(tg tag.Tag, items [][]*Element) *Element { value: sequenceItems, }, } + + if unknown { + e.ValueRepresentation = tag.VRUnknown + e.RawValueRepresentation = "UN" + e.ValueLength = tag.VLUndefinedLength + } + + return e } func TestDataset_FindElementByTag(t *testing.T) { @@ -83,9 +91,9 @@ func TestDataset_FlatStatefulIterator(t *testing.T) { { mustNewElement(tag.PatientName, []string{"Bob", "Smith"}), }, - }), + }, false), }, - }), + }, false), }}, expectedFlatElements: []*Element{ // First, expect the entire SQ element @@ -98,9 +106,9 @@ func TestDataset_FlatStatefulIterator(t *testing.T) { { mustNewElement(tag.PatientName, []string{"Bob", "Smith"}), }, - }), + }, false), }, - }), + }, false), // Then expect the inner elements mustNewElement(tag.PatientName, []string{"Bob", "Jones"}), // Inner SQ element @@ -108,7 +116,7 @@ func TestDataset_FlatStatefulIterator(t *testing.T) { { mustNewElement(tag.PatientName, []string{"Bob", "Smith"}), }, - }), + }, false), // Inner element of the inner SQ mustNewElement(tag.PatientName, []string{"Bob", "Smith"}), }, @@ -139,7 +147,7 @@ func ExampleDataset_FlatIterator() { Elements: []*Element{ mustNewElement(tag.Rows, []int{100}), mustNewElement(tag.Columns, []int{100}), - makeSequenceElement(tag.AddOtherSequence, nestedData), + makeSequenceElement(tag.AddOtherSequence, nestedData, false), }, } @@ -172,7 +180,7 @@ func ExampleDataset_FlatIteratorWithExhaustAllElements() { Elements: []*Element{ mustNewElement(tag.Rows, []int{100}), mustNewElement(tag.Columns, []int{100}), - makeSequenceElement(tag.AddOtherSequence, nestedData), + makeSequenceElement(tag.AddOtherSequence, nestedData, false), }, } @@ -220,7 +228,7 @@ func ExampleDataset_FlatStatefulIterator() { value: []int{200}, }, }, - makeSequenceElement(tag.AddOtherSequence, nestedData), + makeSequenceElement(tag.AddOtherSequence, nestedData, false), }, } diff --git a/element_test.go b/element_test.go index a5d88a41..13f45604 100644 --- a/element_test.go +++ b/element_test.go @@ -21,7 +21,7 @@ func TestElement_MarshalJSON_NestedElements(t *testing.T) { }, }, } - seqElement := makeSequenceElement(tag.AddOtherSequence, nestedData) + seqElement := makeSequenceElement(tag.AddOtherSequence, nestedData, false) want := `{"tag":{"Group":70,"Element":258},"VR":9,"rawVR":"SQ","valueLength":0,"value":[[{"tag":{"Group":16,"Element":16},"VR":2,"rawVR":"","valueLength":0,"value":["Bob"]}]]}` diff --git a/pkg/tag/tag.go b/pkg/tag/tag.go index c007c0fb..1a49ac0b 100644 --- a/pkg/tag/tag.go +++ b/pkg/tag/tag.go @@ -131,7 +131,7 @@ func GetVRKind(tag Tag, vr string) VRKind { return VRDate case "AT": return VRTagList - case "OW", "OB", "UN": + case "OW", "OB": return VRBytes case "LT", "UT": return VRString diff --git a/read.go b/read.go index 51e3e834..aa17d2ca 100644 --- a/read.go +++ b/read.go @@ -108,17 +108,6 @@ func readValue(r dicomio.Reader, t tag.Tag, vr string, vl uint32, isImplicit boo return readDate(r, t, vr, vl) case tag.VRUInt16List, tag.VRUInt32List, tag.VRInt16List, tag.VRInt32List, tag.VRTagList: return readInt(r, t, vr, vl) - // We read Unknown VRs as SQ VRs by default. More details on why can be - // found at https://github.com/suyashkumar/dicom/issues/220. It remains - // to be seen if this fits most DICOMs we see in the wild. - // TODO(suyashkumar): consider replacing UN VRs with SQ earlier on if they - // meet this criteria, so users of the Dataset can interact with it - // correctly. - case tag.VRUnknown: - if isImplicit && vl == tag.VLUndefinedLength { - return readSequence(r, t, vr, vl) - } - return readString(r, t, vr, vl) case tag.VRSequence: return readSequence(r, t, vr, vl) case tag.VRItem: @@ -127,6 +116,19 @@ func readValue(r dicomio.Reader, t tag.Tag, vr string, vl uint32, isImplicit boo return readPixelData(r, t, vr, vl, d, fc) case tag.VRFloat32List, tag.VRFloat64List: return readFloat(r, t, vr, vl) + // More details on how we treat Unknown VRs can be found at + // https://github.com/suyashkumar/dicom/issues/220 + // and + // https://github.com/suyashkumar/dicom/issues/231. + // It remains to be seen if this fits most DICOMs we see in the wild. + // TODO(suyashkumar): consider replacing UN VRs with SQ earlier on if they + // meet this criteria, so users of the Dataset can interact with it + // correctly. + case tag.VRUnknown: + if isImplicit && vl == tag.VLUndefinedLength { + return readSequence(r, t, vr, vl) + } + return readBytes(r, t, vr, vl) default: return readString(r, t, vr, vl) } diff --git a/write.go b/write.go index 7d1f6409..758d2662 100644 --- a/write.go +++ b/write.go @@ -313,7 +313,7 @@ func verifyValueType(t tag.Tag, value Value, vr string) error { ok = valueType == Sequences case "NA": ok = valueType == SequenceItem - case vrraw.OtherWord, vrraw.OtherByte, vrraw.Unknown: + case vrraw.OtherWord, vrraw.OtherByte: if t == tag.PixelData { ok = valueType == PixelData } else { @@ -321,6 +321,8 @@ func verifyValueType(t tag.Tag, value Value, vr string) error { } case vrraw.FloatingPointSingle, vrraw.FloatingPointDouble: ok = valueType == Floats + case vrraw.Unknown: + ok = (valueType == Bytes || valueType == Sequences) default: ok = valueType == Strings } diff --git a/write_test.go b/write_test.go index 3b682002..6aee7b97 100644 --- a/write_test.go +++ b/write_test.go @@ -51,7 +51,7 @@ func TestWrite(t *testing.T) { // Some tag with an unknown VR. { Tag: tag.Tag{0x0019, 0x1027}, - ValueRepresentation: tag.VRBytes, + ValueRepresentation: tag.VRUnknown, RawValueRepresentation: "UN", ValueLength: 4, Value: &bytesValue{ @@ -119,7 +119,7 @@ func TestWrite(t *testing.T) { }, }, }, - }), + }, false), }}, expectedError: nil, }, @@ -169,7 +169,7 @@ func TestWrite(t *testing.T) { }, }, }, - }), + }, false), }}, expectedError: nil, opts: []WriteOption{SkipVRVerification()}, @@ -204,9 +204,45 @@ func TestWrite(t *testing.T) { }, }, }, - }), + }, false), }, - }), + }, false), + }}, + expectedError: nil, + }, + { + name: "nested unknown sequences", + dataset: Dataset{Elements: []*Element{ + mustNewElement(tag.MediaStorageSOPClassUID, []string{"1.2.840.10008.5.1.4.1.1.1.13"}), + mustNewElement(tag.MediaStorageSOPInstanceUID, []string{"1.2.3.4.5.6.7"}), + mustNewElement(tag.TransferSyntaxUID, []string{uid.ImplicitVRLittleEndian}), + mustNewElement(tag.PatientName, []string{"Bob", "Jones"}), + makeSequenceElement(tag.Tag{0x0019, 0x1027}, [][]*Element{ + // Item 1. + { + { + Tag: tag.Tag{0x0019, 0x1028}, + ValueRepresentation: tag.VRUnknown, + RawValueRepresentation: "UN", + Value: &bytesValue{ + value: []byte{0x1, 0x2, 0x3, 0x4}, + }, + }, + // Nested Sequence. + makeSequenceElement(tag.Tag{0x0019, 0x1029}, [][]*Element{ + { + { + Tag: tag.PatientName, + ValueRepresentation: tag.VRStringList, + RawValueRepresentation: "PN", + Value: &stringsValue{ + value: []string{"Bob", "Jones"}, + }, + }, + }, + }, true), + }, + }, true), }}, expectedError: nil, }, @@ -240,9 +276,9 @@ func TestWrite(t *testing.T) { }, }, }, - }), + }, false), }, - }), + }, false), }}, expectedError: nil, opts: []WriteOption{SkipVRVerification()}, From 9183c066f9fb7acad184fc5d8bdc08140575d823 Mon Sep 17 00:00:00 2001 From: Suyash Kumar Date: Sat, 21 May 2022 19:45:39 +0000 Subject: [PATCH 6/7] Update go version in build --- .github/workflows/bench_pr.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/bench_pr.yml b/.github/workflows/bench_pr.yml index bbfc8a7e..8c1a5319 100644 --- a/.github/workflows/bench_pr.yml +++ b/.github/workflows/bench_pr.yml @@ -6,10 +6,10 @@ jobs: runs-on: ubuntu-latest steps: - - name: Set up Go 1.15 + - name: Set up Go 1.18 uses: actions/setup-go@v2 with: - go-version: 1.15 + go-version: 1.18 id: go - name: Check out code From 72c477da83571c15e375d17744b39345803eb1de Mon Sep 17 00:00:00 2001 From: Suyash Kumar Date: Sat, 21 May 2022 19:52:00 +0000 Subject: [PATCH 7/7] update other go build versions --- .github/workflows/bench_push.yml | 4 ++-- .github/workflows/go.yml | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/bench_push.yml b/.github/workflows/bench_push.yml index 22c037af..8e9de188 100644 --- a/.github/workflows/bench_push.yml +++ b/.github/workflows/bench_push.yml @@ -5,10 +5,10 @@ jobs: name: Benchmark (push) runs-on: ubuntu-latest steps: - - name: Set up Go 1.15 + - name: Set up Go 1.18 uses: actions/setup-go@v2 with: - go-version: 1.15 + go-version: 1.18 id: go - name: Check out code diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index ff8f59ad..698668e2 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -7,10 +7,10 @@ jobs: runs-on: ubuntu-latest steps: - - name: Set up Go 1.15 + - name: Set up Go 1.18 uses: actions/setup-go@v2 with: - go-version: 1.15 + go-version: 1.18 id: go - name: Check out code