Read elements with VR=UN and undefined VL as SQ

---------

Co-authored-by: jabillings <[email protected]>

dataset_test.go

@@ -24,6 +24,23 @@ func makeSequenceElement(tg tag.Tag, items [][]*Element) *Element {
 	}
 }
 
+func makeUNSequenceElement(tg tag.Tag, items [][]*Element) *Element {
+	sequenceItems := make([]*SequenceItemValue, 0, len(items))
+	for _, item := range items {
+		sequenceItems = append(sequenceItems, &SequenceItemValue{elements: item})
+	}
+
+	return &Element{
+		Tag:                    tg,
+		ValueRepresentation:    tag.VRUnknown,
+		RawValueRepresentation: "UN",
+		Value: &sequencesValue{
+			value: sequenceItems,
+		},
+		ValueLength: tag.VLUndefinedLength,
+	}
+}
+
 func TestDataset_FindElementByTag(t *testing.T) {
 	data := Dataset{
 		Elements: []*Element{

parse_test.go

@@ -100,18 +100,17 @@ func TestParseFile_SkipPixelData(t *testing.T) {
 		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)
+				t.Errorf("Unexpected error parsing dataset: %v, %v", err, 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)
+			if err == nil {
+				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)
+				}
 			}
 		})
 	})

pkg/tag/tag.go

@@ -130,6 +130,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 <tag, vr>.
@@ -144,7 +147,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
@@ -162,6 +165,8 @@ func GetVRKind(tag Tag, vr string) VRKind {
 		return VRFloat64List
 	case "SQ":
 		return VRSequence
+	case "UN":
+		return VRUnknown
 	default:
 		return VRStringList
 	}

read.go

@@ -143,6 +143,19 @@ func (r *reader) readValue(t tag.Tag, vr string, vl uint32, isImplicit bool, d *
 		return r.readPixelData(vl, d, fc)
 	case tag.VRFloat32List, tag.VRFloat64List:
 		return r.readFloat(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 vl == tag.VLUndefinedLength {
+			return r.readSequence(t, vr, vl, d)
+		}
+		return r.readBytes(t, vr, vl)
 	default:
 		return r.readString(t, vr, vl)
 	}

write.go

@@ -318,12 +318,14 @@ 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 {
 			ok = valueType == Bytes
 		}
+	case vrraw.Unknown:
+		ok = valueType == Bytes || valueType == Sequences
 	case vrraw.FloatingPointSingle, vrraw.FloatingPointDouble:
 		ok = valueType == Floats
 	default:

write_test.go

@@ -577,6 +577,41 @@ func TestWrite(t *testing.T) {
 			}},
 			parseOpts: []ParseOption{SkipPixelData()}, wantError: errorDeflatedTransferSyntaxUnsupported,
 		},
+		{
+			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"}),
+				makeUNSequenceElement(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.
+						makeUNSequenceElement(tag.Tag{0x0019, 0x1029}, [][]*Element{
+							{
+								{
+									Tag:                    tag.PatientName,
+									ValueRepresentation:    tag.VRStringList,
+									RawValueRepresentation: "PN",
+									Value: &stringsValue{
+										value: []string{"Bob", "Jones"},
+									},
+								},
+							},
+						}),
+					},
+				}),
+			}},
+		},
 	}
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {