From 53dbd80732de68fe0c425be37a0d613bdce0a96c Mon Sep 17 00:00:00 2001 From: Fausto J Espinal Date: Tue, 11 Apr 2023 11:38:01 -0500 Subject: [PATCH 1/9] Ignore missing metadata group length field --- parse.go | 17 ++++++++++++---- read.go | 60 ++++++++++++++++++++++++++++++++++++++++++-------------- 2 files changed, 58 insertions(+), 19 deletions(-) diff --git a/parse.go b/parse.go index 89e3776b..62bbd1a3 100644 --- a/parse.go +++ b/parse.go @@ -207,10 +207,11 @@ type ParseOption func(*parseOptSet) // parseOptSet represents the flattened option set after all ParseOptions have been applied. type parseOptSet struct { - skipMetadataReadOnNewParserInit bool - allowMismatchPixelDataLength bool - skipPixelData bool - skipProcessingPixelDataValue bool + skipMetadataReadOnNewParserInit bool + allowMismatchPixelDataLength bool + skipPixelData bool + skipProcessingPixelDataValue bool + allowErrorMetaElementGroupLength bool } func toParseOptSet(opts ...ParseOption) parseOptSet { @@ -228,6 +229,14 @@ func AllowMismatchPixelDataLength() ParseOption { } } +// AllowErrorMetaElementGroupLength allows parser to work around missing metaelement group length tag (0x0002,0x0000) by reading elements only +// in group 2 +func AllowErrorMetaElementGroupLength() ParseOption { + return func(set *parseOptSet) { + set.allowErrorMetaElementGroupLength = true + } +} + // SkipMetadataReadOnNewParserInit makes NewParser skip trying to parse metadata. This will make the Parser default to implicit little endian byte order. // Any metatata tags found in the dataset will still be available when parsing. func SkipMetadataReadOnNewParserInit() ParseOption { diff --git a/read.go b/read.go index 6e653be5..e3a2b2e6 100644 --- a/read.go +++ b/read.go @@ -159,28 +159,58 @@ func (r *reader) readHeader() ([]*Element, error) { return nil, err } + metaElems := []*Element{maybeMetaLen} // TODO: maybe set capacity to a reasonable initial size + metaElementGroupLengthDefined := true if maybeMetaLen.Tag != tag.FileMetaInformationGroupLength || maybeMetaLen.Value.ValueType() != Ints { - return nil, ErrorMetaElementGroupLength + if !r.opts.allowErrorMetaElementGroupLength { + return nil, ErrorMetaElementGroupLength + } + metaElementGroupLengthDefined = false + metaElems = append(metaElems, maybeMetaLen) } - metaLen := maybeMetaLen.Value.GetValue().([]int)[0] - - metaElems := []*Element{maybeMetaLen} // TODO: maybe set capacity to a reasonable initial size + if metaElementGroupLengthDefined { + metaLen := maybeMetaLen.Value.GetValue().([]int)[0] - // Read the metadata elements - err = r.rawReader.PushLimit(int64(metaLen)) - if err != nil { - return nil, err - } - defer r.rawReader.PopLimit() - for !r.rawReader.IsLimitExhausted() { - elem, err := r.readElement(nil, nil) + // Read the metadata elements + err = r.rawReader.PushLimit(int64(metaLen)) if err != nil { - // TODO: see if we can skip over malformed elements somehow return nil, err } - // log.Printf("Metadata Element: %s\n", elem) - metaElems = append(metaElems, elem) + defer r.rawReader.PopLimit() + for !r.rawReader.IsLimitExhausted() { + elem, err := r.readElement(nil, nil) + if err != nil { + // TODO: see if we can skip over malformed elements somehow + return nil, err + } + // log.Printf("Metadata Element: %s\n", elem) + metaElems = append(metaElems, elem) + } + } else if r.opts.allowErrorMetaElementGroupLength { + // We cannot use the limit functionality + debug.Log("Proceeding without metadata group length") + for { + // Lets peek into the tag field until we get to end-of-header + tag_bytes, err := r.rawReader.Peek(4) + if err != nil { + return nil, ErrorMetaElementGroupLength + } + tg := tag.Tag{} + buff := bytes.NewBuffer(tag_bytes) + binary.Read(buff, binary.LittleEndian, &tg) + debug.Logf("header-tag: %v", tg) + // Only read group 2 data + if tg.Group != 0x0002 { + break + } + elem, err := r.readElement(nil, nil) + if err != nil { + // TODO: see if we can skip over malformed elements somehow + return nil, err + } + metaElems = append(metaElems, elem) + } } return metaElems, nil } From f56f569efdf3e306513623f57f0a3fc73ba965a1 Mon Sep 17 00:00:00 2001 From: Fausto J Espinal Date: Mon, 1 May 2023 14:01:25 -0500 Subject: [PATCH 2/9] Updated unit tests --- parse_test.go | 19 ++++++++++++++ read_test.go | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+) diff --git a/parse_test.go b/parse_test.go index 7b64533f..c350dc5a 100644 --- a/parse_test.go +++ b/parse_test.go @@ -149,6 +149,25 @@ func TestParseFile_SkipProcessingPixelDataValue(t *testing.T) { } }) }) + t.Run("WithAllowErrorMetaElementGroupLength", func(t *testing.T) { + runForEveryTestFile(t, func(t *testing.T, filename string) { + dataset, err := dicom.ParseFile(filename, nil, dicom.AllowErrorMetaElementGroupLength()) + 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_WithAllowErrorMetaElementGroupLength=true. got: %v, want: >0", len(pixelData.Frames)) + } + }) + }) } // BenchmarkParse runs sanity benchmarks over the sample files in testdata. diff --git a/read_test.go b/read_test.go index 474c3ea6..dad0e37a 100644 --- a/read_test.go +++ b/read_test.go @@ -595,6 +595,78 @@ func TestReadPixelData_SkipPixelData(t *testing.T) { } } +// Returns a fake DICOM group 2 header with the FileMetaInformationGroupLength tag missing (0x0002,0x0000) +func fakeHeader_NoFileMetaInformationGroupLength() *bytes.Buffer { + preamble := make([]byte, 128) + // First element after DICM is (0x0002,0x0002) + data := []byte{ + 0x44, 0x49, 0x43, 0x4d, 0x02, 0x00, 0x02, 0x00, 0x55, 0x49, 0x20, 0x00, 0x53, 0x65, 0x63, 0x6f, + 0x6e, 0x64, 0x61, 0x72, 0x79, 0x20, 0x43, 0x61, 0x70, 0x74, 0x75, 0x72, 0x65, 0x20, 0x49, 0x6d, + 0x61, 0x67, 0x65, 0x20, 0x53, 0x74, 0x6f, 0x72, 0x61, 0x67, 0x65, 0x00, 0x02, 0x00, 0x03, 0x00, + 0x55, 0x49, 0x2c, 0x00, 0x31, 0x2e, 0x33, 0x2e, 0x36, 0x2e, 0x31, 0x2e, 0x34, 0x2e, 0x31, 0x2e, + 0x33, 0x35, 0x31, 0x39, 0x30, 0x2e, 0x34, 0x2e, 0x31, 0x2e, 0x32, 0x30, 0x32, 0x31, 0x30, 0x36, + 0x30, 0x38, 0x2e, 0x36, 0x30, 0x37, 0x37, 0x33, 0x33, 0x35, 0x34, 0x39, 0x35, 0x39, 0x33, 0x00, + 0x02, 0x00, 0x10, 0x00, 0x55, 0x49, 0x14, 0x00, 0x31, 0x2e, 0x32, 0x2e, 0x38, 0x34, 0x30, 0x2e, + 0x31, 0x30, 0x30, 0x30, 0x38, 0x2e, 0x31, 0x2e, 0x32, 0x2e, 0x35, 0x00, 0x02, 0x00, 0x12, 0x00, + 0x55, 0x49, 0x20, 0x00, 0x31, 0x2e, 0x36, 0x2e, 0x36, 0x2e, 0x31, 0x2e, 0x34, 0x2e, 0x31, 0x2e, + 0x39, 0x35, 0x39, 0x30, 0x2e, 0x31, 0x30, 0x30, 0x2e, 0x31, 0x2e, 0x30, 0x2e, 0x31, 0x30, 0x30, + 0x2e, 0x34, 0x2e, 0x30, 0x08, 0x00, 0x18, 0x00, 0x55, 0x49, 0x2c, 0x00, 0x31, 0x2e, 0x33, 0x2e, + 0x36, 0x2e, 0x31, 0x2e, 0x34, 0x2e, 0x31, 0x2e, 0x33, 0x35, 0x31, 0x39, 0x30, 0x2e, 0x34, 0x2e, + } + data = append(preamble, data...) + dcm_header := bytes.NewBuffer(data) + return dcm_header +} + +// Returns a fake DICOM group 2 header with a FileMetaInformationGroupLength tag (0x0002,0x0000) +func fakeHeader_WithFileMetaInformationGroupLength() *bytes.Buffer { + preamble := make([]byte, 128) + // First element after DICM is (0x0002,0x0000) + data := []byte{ + 0x44, 0x49, 0x43, 0x4d, 0x02, 0x00, 0x00, 0x00, 0x55, 0x4c, 0x04, 0x00, 0xbc, 0x00, 0x00, 0x00, + 0x02, 0x00, 0x01, 0x00, 0x4f, 0x42, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x01, 0x02, 0x00, + 0x02, 0x00, 0x55, 0x49, 0x1a, 0x00, 0x31, 0x2e, 0x32, 0x2e, 0x32, 0x37, 0x36, 0x2e, 0x30, 0x2e, + 0x37, 0x32, 0x33, 0x30, 0x30, 0x31, 0x30, 0x2e, 0x33, 0x2e, 0x31, 0x2e, 0x30, 0x2e, 0x31, 0x00, + 0x02, 0x00, 0x03, 0x00, 0x55, 0x49, 0x2c, 0x00, 0x31, 0x2e, 0x33, 0x2e, 0x36, 0x2e, 0x31, 0x2e, + 0x34, 0x2e, 0x31, 0x2e, 0x33, 0x35, 0x31, 0x39, 0x30, 0x2e, 0x34, 0x2e, 0x31, 0x2e, 0x32, 0x30, + 0x32, 0x31, 0x30, 0x36, 0x30, 0x38, 0x2e, 0x36, 0x30, 0x37, 0x37, 0x33, 0x33, 0x35, 0x34, 0x39, + 0x35, 0x39, 0x33, 0x00, 0x02, 0x00, 0x10, 0x00, 0x55, 0x49, 0x14, 0x00, 0x31, 0x2e, 0x32, 0x2e, + 0x38, 0x34, 0x30, 0x2e, 0x31, 0x30, 0x30, 0x30, 0x38, 0x2e, 0x31, 0x2e, 0x32, 0x2e, 0x35, 0x00, + 0x02, 0x00, 0x12, 0x00, 0x55, 0x49, 0x1c, 0x00, 0x31, 0x2e, 0x32, 0x2e, 0x32, 0x37, 0x36, 0x2e, + 0x30, 0x2e, 0x37, 0x32, 0x33, 0x30, 0x30, 0x31, 0x30, 0x2e, 0x33, 0x2e, 0x30, 0x2e, 0x33, 0x2e, + 0x36, 0x2e, 0x37, 0x00, 0x02, 0x00, 0x13, 0x00, 0x53, 0x48, 0x10, 0x00, 0x4f, 0x46, 0x46, 0x49, + 0x53, 0x5f, 0x44, 0x43, 0x4d, 0x54, 0x4b, 0x5f, 0x33, 0x36, 0x37, 0x20, 0x08, 0x00, 0x18, 0x00, + } + data = append(preamble, data...) + dcm_header := bytes.NewBuffer(data) + return dcm_header +} + +func TestReadHeader_TryAllowErrorMetaElementGroupLength(t *testing.T) { + opts := parseOptSet{allowErrorMetaElementGroupLength: true} + dcmheader_noinfogrplen := fakeHeader_NoFileMetaInformationGroupLength() + + r := &reader{ + rawReader: dicomio.NewReader(bufio.NewReader(dcmheader_noinfogrplen), binary.LittleEndian, int64(dcmheader_noinfogrplen.Len())), + opts: opts, + } + _, err := r.readHeader() + if err != nil { + t.Errorf("unsuccesful readHeader when parse option %v is turned on and header has no MetaElementGroupLength tag", opts.allowErrorMetaElementGroupLength) + } + + // ------------- + dcmheader_infogrplen := fakeHeader_WithFileMetaInformationGroupLength() + r = &reader{ + rawReader: dicomio.NewReader(bufio.NewReader(dcmheader_infogrplen), binary.LittleEndian, int64(dcmheader_infogrplen.Len())), + opts: opts, + } + _, err = r.readHeader() + if err != nil { + t.Errorf("unsuccesful readHeader when parse option %v is turned on and header has no MetaElementGroupLength tag", opts.allowErrorMetaElementGroupLength) + } +} + func TestReadPixelData_TrySkipProcessingPixelDataValue(t *testing.T) { opts := parseOptSet{skipProcessingPixelDataValue: true} valueBytes := []byte{1, 2, 3, 4, 5, 6} From 7738452230130e506929628bd32c95a7d23fb8b5 Mon Sep 17 00:00:00 2001 From: Fausto J Espinal Date: Fri, 19 May 2023 11:32:30 -0500 Subject: [PATCH 3/9] programmatic generation of test fake-headers --- read_test.go | 133 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 79 insertions(+), 54 deletions(-) diff --git a/read_test.go b/read_test.go index dad0e37a..e3235f80 100644 --- a/read_test.go +++ b/read_test.go @@ -596,75 +596,100 @@ func TestReadPixelData_SkipPixelData(t *testing.T) { } // Returns a fake DICOM group 2 header with the FileMetaInformationGroupLength tag missing (0x0002,0x0000) -func fakeHeader_NoFileMetaInformationGroupLength() *bytes.Buffer { - preamble := make([]byte, 128) - // First element after DICM is (0x0002,0x0002) - data := []byte{ - 0x44, 0x49, 0x43, 0x4d, 0x02, 0x00, 0x02, 0x00, 0x55, 0x49, 0x20, 0x00, 0x53, 0x65, 0x63, 0x6f, - 0x6e, 0x64, 0x61, 0x72, 0x79, 0x20, 0x43, 0x61, 0x70, 0x74, 0x75, 0x72, 0x65, 0x20, 0x49, 0x6d, - 0x61, 0x67, 0x65, 0x20, 0x53, 0x74, 0x6f, 0x72, 0x61, 0x67, 0x65, 0x00, 0x02, 0x00, 0x03, 0x00, - 0x55, 0x49, 0x2c, 0x00, 0x31, 0x2e, 0x33, 0x2e, 0x36, 0x2e, 0x31, 0x2e, 0x34, 0x2e, 0x31, 0x2e, - 0x33, 0x35, 0x31, 0x39, 0x30, 0x2e, 0x34, 0x2e, 0x31, 0x2e, 0x32, 0x30, 0x32, 0x31, 0x30, 0x36, - 0x30, 0x38, 0x2e, 0x36, 0x30, 0x37, 0x37, 0x33, 0x33, 0x35, 0x34, 0x39, 0x35, 0x39, 0x33, 0x00, - 0x02, 0x00, 0x10, 0x00, 0x55, 0x49, 0x14, 0x00, 0x31, 0x2e, 0x32, 0x2e, 0x38, 0x34, 0x30, 0x2e, - 0x31, 0x30, 0x30, 0x30, 0x38, 0x2e, 0x31, 0x2e, 0x32, 0x2e, 0x35, 0x00, 0x02, 0x00, 0x12, 0x00, - 0x55, 0x49, 0x20, 0x00, 0x31, 0x2e, 0x36, 0x2e, 0x36, 0x2e, 0x31, 0x2e, 0x34, 0x2e, 0x31, 0x2e, - 0x39, 0x35, 0x39, 0x30, 0x2e, 0x31, 0x30, 0x30, 0x2e, 0x31, 0x2e, 0x30, 0x2e, 0x31, 0x30, 0x30, - 0x2e, 0x34, 0x2e, 0x30, 0x08, 0x00, 0x18, 0x00, 0x55, 0x49, 0x2c, 0x00, 0x31, 0x2e, 0x33, 0x2e, - 0x36, 0x2e, 0x31, 0x2e, 0x34, 0x2e, 0x31, 0x2e, 0x33, 0x35, 0x31, 0x39, 0x30, 0x2e, 0x34, 0x2e, +func fakeHeader_NoFileMetaInformationGroupLength() (*bytes.Buffer, error) { + var buf bytes.Buffer + + dcm_writer := NewWriter(&buf) + dcm_writer.SetTransferSyntax(binary.LittleEndian, true) + + elements := []*Element{ + mustNewElement(tag.MediaStorageSOPClassUID, []string{"SecondaryCapture"}), + mustNewElement(tag.MediaStorageSOPInstanceUID, []string{"1.3.6.1.4.1.35190.4.1.20210608.607733549593"}), + mustNewElement(tag.TransferSyntaxUID, []string{"=RLELossless"}), + mustNewElement(tag.ImplementationClassUID, []string{"1.6.6.1.4.1.9590.100.1.0.100.4.0"}), + mustNewElement(tag.SOPInstanceUID, []string{"1.3.6.1.4.1.35190.4.1.20210608.607733549593"}), + } + for _, e := range elements { + err := dcm_writer.WriteElement(e) + if err != nil { + return nil, err + } } + data := buf.Bytes() + // Construct valid DICOM header preamble + data = append([]byte("DICM"), data...) + preamble := make([]byte, 128) data = append(preamble, data...) dcm_header := bytes.NewBuffer(data) - return dcm_header + return dcm_header, nil } // Returns a fake DICOM group 2 header with a FileMetaInformationGroupLength tag (0x0002,0x0000) -func fakeHeader_WithFileMetaInformationGroupLength() *bytes.Buffer { - preamble := make([]byte, 128) - // First element after DICM is (0x0002,0x0000) - data := []byte{ - 0x44, 0x49, 0x43, 0x4d, 0x02, 0x00, 0x00, 0x00, 0x55, 0x4c, 0x04, 0x00, 0xbc, 0x00, 0x00, 0x00, - 0x02, 0x00, 0x01, 0x00, 0x4f, 0x42, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x01, 0x02, 0x00, - 0x02, 0x00, 0x55, 0x49, 0x1a, 0x00, 0x31, 0x2e, 0x32, 0x2e, 0x32, 0x37, 0x36, 0x2e, 0x30, 0x2e, - 0x37, 0x32, 0x33, 0x30, 0x30, 0x31, 0x30, 0x2e, 0x33, 0x2e, 0x31, 0x2e, 0x30, 0x2e, 0x31, 0x00, - 0x02, 0x00, 0x03, 0x00, 0x55, 0x49, 0x2c, 0x00, 0x31, 0x2e, 0x33, 0x2e, 0x36, 0x2e, 0x31, 0x2e, - 0x34, 0x2e, 0x31, 0x2e, 0x33, 0x35, 0x31, 0x39, 0x30, 0x2e, 0x34, 0x2e, 0x31, 0x2e, 0x32, 0x30, - 0x32, 0x31, 0x30, 0x36, 0x30, 0x38, 0x2e, 0x36, 0x30, 0x37, 0x37, 0x33, 0x33, 0x35, 0x34, 0x39, - 0x35, 0x39, 0x33, 0x00, 0x02, 0x00, 0x10, 0x00, 0x55, 0x49, 0x14, 0x00, 0x31, 0x2e, 0x32, 0x2e, - 0x38, 0x34, 0x30, 0x2e, 0x31, 0x30, 0x30, 0x30, 0x38, 0x2e, 0x31, 0x2e, 0x32, 0x2e, 0x35, 0x00, - 0x02, 0x00, 0x12, 0x00, 0x55, 0x49, 0x1c, 0x00, 0x31, 0x2e, 0x32, 0x2e, 0x32, 0x37, 0x36, 0x2e, - 0x30, 0x2e, 0x37, 0x32, 0x33, 0x30, 0x30, 0x31, 0x30, 0x2e, 0x33, 0x2e, 0x30, 0x2e, 0x33, 0x2e, - 0x36, 0x2e, 0x37, 0x00, 0x02, 0x00, 0x13, 0x00, 0x53, 0x48, 0x10, 0x00, 0x4f, 0x46, 0x46, 0x49, - 0x53, 0x5f, 0x44, 0x43, 0x4d, 0x54, 0x4b, 0x5f, 0x33, 0x36, 0x37, 0x20, 0x08, 0x00, 0x18, 0x00, +func fakeHeader_WithFileMetaInformationGroupLength() (*bytes.Buffer, error) { + var buf bytes.Buffer + + dcm_writer := NewWriter(&buf) + dcm_writer.SetTransferSyntax(binary.LittleEndian, true) + + elements := []*Element{ + mustNewElement(tag.FileMetaInformationGroupLength, []int{188}), + mustNewElement(tag.FileMetaInformationVersion, []byte{0x00, 0x01}), + mustNewElement(tag.MediaStorageSOPClassUID, []string{"1.2.276.0.7230010.3.1.0.1"}), + mustNewElement(tag.MediaStorageSOPInstanceUID, []string{"1.3.6.1.4.1.35190.4.1.20210608.607733549593"}), + mustNewElement(tag.TransferSyntaxUID, []string{"=RLELossless"}), + mustNewElement(tag.ImplementationClassUID, []string{"1.2.276.0.7230010.3.0.3.6.7"}), + mustNewElement(tag.ImplementationVersionName, []string{"OFFIS_DCMTK_367"}), + mustNewElement(tag.SOPInstanceUID, []string{"1.3.6.1.4.1.35190.4.1.20210608.607733549593"}), + } + for _, e := range elements { + err := dcm_writer.WriteElement(e) + if err != nil { + return nil, err + } } + // Construct valid DICOM header preamble + data := buf.Bytes() + data = append([]byte("DICM"), data...) + preamble := make([]byte, 128) data = append(preamble, data...) dcm_header := bytes.NewBuffer(data) - return dcm_header + return dcm_header, nil } func TestReadHeader_TryAllowErrorMetaElementGroupLength(t *testing.T) { opts := parseOptSet{allowErrorMetaElementGroupLength: true} - dcmheader_noinfogrplen := fakeHeader_NoFileMetaInformationGroupLength() - r := &reader{ - rawReader: dicomio.NewReader(bufio.NewReader(dcmheader_noinfogrplen), binary.LittleEndian, int64(dcmheader_noinfogrplen.Len())), - opts: opts, - } - _, err := r.readHeader() - if err != nil { - t.Errorf("unsuccesful readHeader when parse option %v is turned on and header has no MetaElementGroupLength tag", opts.allowErrorMetaElementGroupLength) - } + t.Run("NoFileMetaInformationGroupLength", func(t *testing.T) { + dcmheader_noinfogrplen, err := fakeHeader_NoFileMetaInformationGroupLength() + if err != nil { + t.Errorf("unsuccesful generation of fake header data") + } else { + r := &reader{ + rawReader: dicomio.NewReader(bufio.NewReader(dcmheader_noinfogrplen), binary.LittleEndian, int64(dcmheader_noinfogrplen.Len())), + opts: opts, + } + _, err = r.readHeader() + if err != nil { + t.Errorf("unsuccesful readHeader when parse option %v is turned on and header has no MetaElementGroupLength tag", opts.allowErrorMetaElementGroupLength) + } + } + }) - // ------------- - dcmheader_infogrplen := fakeHeader_WithFileMetaInformationGroupLength() - r = &reader{ - rawReader: dicomio.NewReader(bufio.NewReader(dcmheader_infogrplen), binary.LittleEndian, int64(dcmheader_infogrplen.Len())), - opts: opts, - } - _, err = r.readHeader() - if err != nil { - t.Errorf("unsuccesful readHeader when parse option %v is turned on and header has no MetaElementGroupLength tag", opts.allowErrorMetaElementGroupLength) - } + t.Run("WithFileMetaInformationGroupLength", func(t *testing.T) { + dcmheader_infogrplen, err := fakeHeader_WithFileMetaInformationGroupLength() + if err != nil { + t.Errorf("unsuccesful generation of fake header data with FileMetaInformationGroupLength") + } else { + r := &reader{ + rawReader: dicomio.NewReader(bufio.NewReader(dcmheader_infogrplen), binary.LittleEndian, int64(dcmheader_infogrplen.Len())), + opts: opts, + } + _, err = r.readHeader() + if err != nil { + t.Errorf("unsuccesful readHeader when parse option %v is turned on and header has no MetaElementGroupLength tag", opts.allowErrorMetaElementGroupLength) + } + } + }) } func TestReadPixelData_TrySkipProcessingPixelDataValue(t *testing.T) { From 765cf42b5b4c2266d5a60bba6e7e782e3e1f754e Mon Sep 17 00:00:00 2001 From: Fausto J Espinal Date: Fri, 19 May 2023 11:39:48 -0500 Subject: [PATCH 4/9] Changed fakeHeader generation method names --- read_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/read_test.go b/read_test.go index e3235f80..81e0f755 100644 --- a/read_test.go +++ b/read_test.go @@ -596,7 +596,7 @@ func TestReadPixelData_SkipPixelData(t *testing.T) { } // Returns a fake DICOM group 2 header with the FileMetaInformationGroupLength tag missing (0x0002,0x0000) -func fakeHeader_NoFileMetaInformationGroupLength() (*bytes.Buffer, error) { +func headerWithNoFileMetaInformationGroupLength() (*bytes.Buffer, error) { var buf bytes.Buffer dcm_writer := NewWriter(&buf) @@ -625,7 +625,7 @@ func fakeHeader_NoFileMetaInformationGroupLength() (*bytes.Buffer, error) { } // Returns a fake DICOM group 2 header with a FileMetaInformationGroupLength tag (0x0002,0x0000) -func fakeHeader_WithFileMetaInformationGroupLength() (*bytes.Buffer, error) { +func headerWithFileMetaInformationGroupLength() (*bytes.Buffer, error) { var buf bytes.Buffer dcm_writer := NewWriter(&buf) @@ -660,7 +660,7 @@ func TestReadHeader_TryAllowErrorMetaElementGroupLength(t *testing.T) { opts := parseOptSet{allowErrorMetaElementGroupLength: true} t.Run("NoFileMetaInformationGroupLength", func(t *testing.T) { - dcmheader_noinfogrplen, err := fakeHeader_NoFileMetaInformationGroupLength() + dcmheader_noinfogrplen, err := headerWithNoFileMetaInformationGroupLength() if err != nil { t.Errorf("unsuccesful generation of fake header data") } else { @@ -676,7 +676,7 @@ func TestReadHeader_TryAllowErrorMetaElementGroupLength(t *testing.T) { }) t.Run("WithFileMetaInformationGroupLength", func(t *testing.T) { - dcmheader_infogrplen, err := fakeHeader_WithFileMetaInformationGroupLength() + dcmheader_infogrplen, err := headerWithFileMetaInformationGroupLength() if err != nil { t.Errorf("unsuccesful generation of fake header data with FileMetaInformationGroupLength") } else { From 4cedef80900ba873162f83024603da2941fb5b8c Mon Sep 17 00:00:00 2001 From: Fausto J Espinal Date: Tue, 6 Jun 2023 12:51:49 -0500 Subject: [PATCH 5/9] Changes for PR acceptance --- parse.go | 6 +++--- parse_test.go | 24 ++++++++++++------------ read.go | 4 ++-- read_test.go | 8 ++++---- 4 files changed, 21 insertions(+), 21 deletions(-) diff --git a/parse.go b/parse.go index 62bbd1a3..4f30eeee 100644 --- a/parse.go +++ b/parse.go @@ -229,9 +229,9 @@ func AllowMismatchPixelDataLength() ParseOption { } } -// AllowErrorMetaElementGroupLength allows parser to work around missing metaelement group length tag (0x0002,0x0000) by reading elements only -// in group 2 -func AllowErrorMetaElementGroupLength() ParseOption { +// AllowMissingMetaElementGroupLength allows parser to work around missing metaelement group length tag (0x0002,0x0000) by reading elements only +// in group 2. +func AllowMissingMetaElementGroupLength() ParseOption { return func(set *parseOptSet) { set.allowErrorMetaElementGroupLength = true } diff --git a/parse_test.go b/parse_test.go index c350dc5a..1a98f709 100644 --- a/parse_test.go +++ b/parse_test.go @@ -151,21 +151,21 @@ func TestParseFile_SkipProcessingPixelDataValue(t *testing.T) { }) t.Run("WithAllowErrorMetaElementGroupLength", func(t *testing.T) { runForEveryTestFile(t, func(t *testing.T, filename string) { - dataset, err := dicom.ParseFile(filename, nil, dicom.AllowErrorMetaElementGroupLength()) + dataset, err := dicom.ParseFile(filename, nil, dicom.AllowMissingMetaElementGroupLength()) 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_WithAllowErrorMetaElementGroupLength=true. got: %v, want: >0", len(pixelData.Frames)) - } + // 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_WithAllowErrorMetaElementGroupLength=true. got: %v, want: >0", len(pixelData.Frames)) + // } }) }) } diff --git a/read.go b/read.go index e3a2b2e6..f876cc2d 100644 --- a/read.go +++ b/read.go @@ -162,11 +162,11 @@ func (r *reader) readHeader() ([]*Element, error) { metaElems := []*Element{maybeMetaLen} // TODO: maybe set capacity to a reasonable initial size metaElementGroupLengthDefined := true if maybeMetaLen.Tag != tag.FileMetaInformationGroupLength || maybeMetaLen.Value.ValueType() != Ints { + // MetaInformationGroupLength is not present or of the wrong value type. if !r.opts.allowErrorMetaElementGroupLength { return nil, ErrorMetaElementGroupLength } metaElementGroupLengthDefined = false - metaElems = append(metaElems, maybeMetaLen) } if metaElementGroupLengthDefined { @@ -187,7 +187,7 @@ func (r *reader) readHeader() ([]*Element, error) { // log.Printf("Metadata Element: %s\n", elem) metaElems = append(metaElems, elem) } - } else if r.opts.allowErrorMetaElementGroupLength { + } else { // We cannot use the limit functionality debug.Log("Proceeding without metadata group length") for { diff --git a/read_test.go b/read_test.go index 81e0f755..c9382361 100644 --- a/read_test.go +++ b/read_test.go @@ -660,12 +660,12 @@ func TestReadHeader_TryAllowErrorMetaElementGroupLength(t *testing.T) { opts := parseOptSet{allowErrorMetaElementGroupLength: true} t.Run("NoFileMetaInformationGroupLength", func(t *testing.T) { - dcmheader_noinfogrplen, err := headerWithNoFileMetaInformationGroupLength() + dcmheaderNoInfoGrpLen, err := headerWithNoFileMetaInformationGroupLength() if err != nil { - t.Errorf("unsuccesful generation of fake header data") + t.Fatalf("unsuccesful generation of fake header data") } else { r := &reader{ - rawReader: dicomio.NewReader(bufio.NewReader(dcmheader_noinfogrplen), binary.LittleEndian, int64(dcmheader_noinfogrplen.Len())), + rawReader: dicomio.NewReader(bufio.NewReader(dcmheaderNoInfoGrpLen), binary.LittleEndian, int64(dcmheaderNoInfoGrpLen.Len())), opts: opts, } _, err = r.readHeader() @@ -678,7 +678,7 @@ func TestReadHeader_TryAllowErrorMetaElementGroupLength(t *testing.T) { t.Run("WithFileMetaInformationGroupLength", func(t *testing.T) { dcmheader_infogrplen, err := headerWithFileMetaInformationGroupLength() if err != nil { - t.Errorf("unsuccesful generation of fake header data with FileMetaInformationGroupLength") + t.Fatalf("unsuccesful generation of fake header data with FileMetaInformationGroupLength") } else { r := &reader{ rawReader: dicomio.NewReader(bufio.NewReader(dcmheader_infogrplen), binary.LittleEndian, int64(dcmheader_infogrplen.Len())), From 7b7093e81f9766cfd5b56ec97342e6a47308158b Mon Sep 17 00:00:00 2001 From: Fausto J Espinal Date: Tue, 6 Jun 2023 20:38:56 -0500 Subject: [PATCH 6/9] Fixes to unit tests per PR observations --- read.go | 9 ---- read_test.go | 120 +++++++++++++++++++++++++++++++++------------------ 2 files changed, 79 insertions(+), 50 deletions(-) diff --git a/read.go b/read.go index 50aaac98..3c497481 100644 --- a/read.go +++ b/read.go @@ -50,14 +50,6 @@ func (r *reader) readTag() (*tag.Tag, error) { if gerr == nil && eerr == nil { return &tag.Tag{Group: group, Element: element}, nil } - // If the error is an io.EOF, return the error directly instead of the compound error later. - // Once we target go1.20 we can use errors.Join: https://pkg.go.dev/errors#Join - if errors.Is(gerr, io.EOF) { - return nil, gerr - } - if errors.Is(eerr, io.EOF) { - return nil, eerr - } return nil, fmt.Errorf("error reading tag: %v %v", gerr, eerr) } @@ -170,7 +162,6 @@ func (r *reader) readHeader() ([]*Element, error) { metaElems := []*Element{maybeMetaLen} // TODO: maybe set capacity to a reasonable initial size metaElementGroupLengthDefined := true if maybeMetaLen.Tag != tag.FileMetaInformationGroupLength || maybeMetaLen.Value.ValueType() != Ints { - // MetaInformationGroupLength is not present or of the wrong value type. if !r.opts.allowErrorMetaElementGroupLength { return nil, ErrorMetaElementGroupLength } diff --git a/read_test.go b/read_test.go index c9382361..7dc6302e 100644 --- a/read_test.go +++ b/read_test.go @@ -595,12 +595,33 @@ func TestReadPixelData_SkipPixelData(t *testing.T) { } } -// Returns a fake DICOM group 2 header with the FileMetaInformationGroupLength tag missing (0x0002,0x0000) -func headerWithNoFileMetaInformationGroupLength() (*bytes.Buffer, error) { - var buf bytes.Buffer +// Used to encode the data from the generated headers. +type headerData struct { + // The byte encoded header data. + HeaderBytes *bytes.Buffer + // The decoded elements conforming the header. + Elements []*Element +} + +// 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.SetTransferSyntax(binary.LittleEndian, true) - dcm_writer := NewWriter(&buf) - dcm_writer.SetTransferSyntax(binary.LittleEndian, true) + for _, e := range elements { + err := dcmWriter.WriteElement(e) + if err != nil { + return nil, err + } + } + data := buff.Bytes() + return data, nil +} + +// Returns a fake DICOM group 2 header with the FileMetaInformationGroupLength tag missing (0x0002,0x0000). +func headerWithNoFileMetaInformationGroupLength() (*headerData, error) { + headerData := new(headerData) elements := []*Element{ mustNewElement(tag.MediaStorageSOPClassUID, []string{"SecondaryCapture"}), @@ -609,51 +630,58 @@ func headerWithNoFileMetaInformationGroupLength() (*bytes.Buffer, error) { mustNewElement(tag.ImplementationClassUID, []string{"1.6.6.1.4.1.9590.100.1.0.100.4.0"}), mustNewElement(tag.SOPInstanceUID, []string{"1.3.6.1.4.1.35190.4.1.20210608.607733549593"}), } - for _, e := range elements { - err := dcm_writer.WriteElement(e) - if err != nil { - return nil, err - } + data, err := writeElements(elements) + if err != nil { + return nil, err } - data := buf.Bytes() - // Construct valid DICOM header preamble - data = append([]byte("DICM"), data...) + + // Construct valid DICOM header preamble. + magicWord := []byte("DICM") preamble := make([]byte, 128) - data = append(preamble, data...) - dcm_header := bytes.NewBuffer(data) - return dcm_header, nil + preamble = append(preamble, magicWord...) + headerBytes := append(preamble, data...) + headerData.HeaderBytes = bytes.NewBuffer(headerBytes) + headerData.Elements = elements[0 : len(elements)-1] + return headerData, nil } -// Returns a fake DICOM group 2 header with a FileMetaInformationGroupLength tag (0x0002,0x0000) -func headerWithFileMetaInformationGroupLength() (*bytes.Buffer, error) { - var buf bytes.Buffer - - dcm_writer := NewWriter(&buf) - dcm_writer.SetTransferSyntax(binary.LittleEndian, true) +// Returns a fake DICOM group 2 header with a FileMetaInformationGroupLength tag (0x0002,0x0000). +func headerWithFileMetaInformationGroupLength() (*headerData, error) { + headerData := new(headerData) + sopInstanceUidElement := mustNewElement(tag.SOPInstanceUID, []string{"1.3.6.1.4.1.35190.4.1.20210608.607733549593"}) elements := []*Element{ - mustNewElement(tag.FileMetaInformationGroupLength, []int{188}), mustNewElement(tag.FileMetaInformationVersion, []byte{0x00, 0x01}), mustNewElement(tag.MediaStorageSOPClassUID, []string{"1.2.276.0.7230010.3.1.0.1"}), mustNewElement(tag.MediaStorageSOPInstanceUID, []string{"1.3.6.1.4.1.35190.4.1.20210608.607733549593"}), mustNewElement(tag.TransferSyntaxUID, []string{"=RLELossless"}), mustNewElement(tag.ImplementationClassUID, []string{"1.2.276.0.7230010.3.0.3.6.7"}), mustNewElement(tag.ImplementationVersionName, []string{"OFFIS_DCMTK_367"}), - mustNewElement(tag.SOPInstanceUID, []string{"1.3.6.1.4.1.35190.4.1.20210608.607733549593"}), } - for _, e := range elements { - err := dcm_writer.WriteElement(e) - if err != nil { - return nil, err - } + dataHeader, err := writeElements(elements) + if err != nil { + return nil, err } - // Construct valid DICOM header preamble - data := buf.Bytes() - data = append([]byte("DICM"), data...) + fileMetaInfoElement := mustNewElement(tag.FileMetaInformationGroupLength, []int{len(dataHeader)}) + dataFileMetaInfo, err := writeElements([]*Element{fileMetaInfoElement}) + if err != nil { + return nil, err + } + dataSopInstanceUid, err := writeElements([]*Element{sopInstanceUidElement}) + if err != nil { + return nil, err + } + data := append(dataFileMetaInfo, dataHeader...) + data = append(data, dataSopInstanceUid...) + + // Construct valid DICOM header preamble. + magicWord := []byte("DICM") preamble := make([]byte, 128) - data = append(preamble, data...) - dcm_header := bytes.NewBuffer(data) - return dcm_header, nil + preamble = append(preamble, magicWord...) + headerBytes := append(preamble, data...) + headerData.HeaderBytes = bytes.NewBuffer(headerBytes) + headerData.Elements = append([]*Element{fileMetaInfoElement}, elements...) + return headerData, nil } func TestReadHeader_TryAllowErrorMetaElementGroupLength(t *testing.T) { @@ -665,29 +693,39 @@ func TestReadHeader_TryAllowErrorMetaElementGroupLength(t *testing.T) { t.Fatalf("unsuccesful generation of fake header data") } else { r := &reader{ - rawReader: dicomio.NewReader(bufio.NewReader(dcmheaderNoInfoGrpLen), binary.LittleEndian, int64(dcmheaderNoInfoGrpLen.Len())), + rawReader: dicomio.NewReader(bufio.NewReader(dcmheaderNoInfoGrpLen.HeaderBytes), binary.LittleEndian, int64(dcmheaderNoInfoGrpLen.HeaderBytes.Len())), opts: opts, } - _, err = r.readHeader() + r.rawReader.SetTransferSyntax(binary.LittleEndian, true) + parsedElements, err := r.readHeader() if err != nil { - t.Errorf("unsuccesful readHeader when parse option %v is turned on and header has no MetaElementGroupLength tag", opts.allowErrorMetaElementGroupLength) + t.Errorf("unsuccessful readHeader when parse option %v is turned on and header has no MetaElementGroupLength tag", opts.allowErrorMetaElementGroupLength) + } + // Ensure dataset read from readHeader and the test header are the same except for the ValueLength field. + if diff := cmp.Diff(parsedElements, dcmheaderNoInfoGrpLen.Elements, cmp.AllowUnexported(allValues...), cmpopts.IgnoreFields(Element{}, "ValueLength")); diff != "" { + t.Errorf("Elements parsed from test header do not match: %v", diff) } } }) t.Run("WithFileMetaInformationGroupLength", func(t *testing.T) { - dcmheader_infogrplen, err := headerWithFileMetaInformationGroupLength() + dcmHeaderInfoGrpLen, err := headerWithFileMetaInformationGroupLength() if err != nil { t.Fatalf("unsuccesful generation of fake header data with FileMetaInformationGroupLength") } else { r := &reader{ - rawReader: dicomio.NewReader(bufio.NewReader(dcmheader_infogrplen), binary.LittleEndian, int64(dcmheader_infogrplen.Len())), + rawReader: dicomio.NewReader(bufio.NewReader(dcmHeaderInfoGrpLen.HeaderBytes), binary.LittleEndian, int64(dcmHeaderInfoGrpLen.HeaderBytes.Len())), opts: opts, } - _, err = r.readHeader() + r.rawReader.SetTransferSyntax(binary.LittleEndian, true) + parsedElements, err := r.readHeader() if err != nil { t.Errorf("unsuccesful readHeader when parse option %v is turned on and header has no MetaElementGroupLength tag", opts.allowErrorMetaElementGroupLength) } + // Ensure dataset read from readHeader and the test header are the same except for the ValueLength field. + if diff := cmp.Diff(parsedElements, dcmHeaderInfoGrpLen.Elements, cmp.AllowUnexported(allValues...), cmpopts.IgnoreFields(Element{}, "ValueLength")); diff != "" { + t.Errorf("Elements parsed from test header do not match: %v", diff) + } } }) } From d62a2d5de2baa2f276ebeb33a4dc00ebf72964a7 Mon Sep 17 00:00:00 2001 From: Fausto J Espinal Date: Tue, 6 Jun 2023 21:01:09 -0500 Subject: [PATCH 7/9] removed unnecesary test checks and added comments --- parse_test.go | 11 ----------- read.go | 1 + 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/parse_test.go b/parse_test.go index 75eefd58..584e40ca 100644 --- a/parse_test.go +++ b/parse_test.go @@ -181,17 +181,6 @@ func TestParseFile_SkipProcessingPixelDataValue(t *testing.T) { 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_WithAllowErrorMetaElementGroupLength=true. got: %v, want: >0", len(pixelData.Frames)) - // } }) }) } diff --git a/read.go b/read.go index 3c497481..f876cc2d 100644 --- a/read.go +++ b/read.go @@ -162,6 +162,7 @@ func (r *reader) readHeader() ([]*Element, error) { metaElems := []*Element{maybeMetaLen} // TODO: maybe set capacity to a reasonable initial size metaElementGroupLengthDefined := true if maybeMetaLen.Tag != tag.FileMetaInformationGroupLength || maybeMetaLen.Value.ValueType() != Ints { + // MetaInformationGroupLength is not present or of the wrong value type. if !r.opts.allowErrorMetaElementGroupLength { return nil, ErrorMetaElementGroupLength } From 19af17c206e58591d5aff156bb79cc3e0f13dba9 Mon Sep 17 00:00:00 2001 From: Fausto J Espinal Date: Wed, 7 Jun 2023 08:17:06 -0500 Subject: [PATCH 8/9] last round of fixes addressing PR review comments --- parse.go | 12 ++++++------ read.go | 9 +++++++-- read_test.go | 14 +++++++------- 3 files changed, 20 insertions(+), 15 deletions(-) diff --git a/parse.go b/parse.go index a208cd39..161cc8ce 100644 --- a/parse.go +++ b/parse.go @@ -222,11 +222,11 @@ type ParseOption func(*parseOptSet) // parseOptSet represents the flattened option set after all ParseOptions have been applied. type parseOptSet struct { - skipMetadataReadOnNewParserInit bool - allowMismatchPixelDataLength bool - skipPixelData bool - skipProcessingPixelDataValue bool - allowErrorMetaElementGroupLength bool + skipMetadataReadOnNewParserInit bool + allowMismatchPixelDataLength bool + skipPixelData bool + skipProcessingPixelDataValue bool + allowMissingMetaElementGroupLength bool } func toParseOptSet(opts ...ParseOption) parseOptSet { @@ -248,7 +248,7 @@ func AllowMismatchPixelDataLength() ParseOption { // in group 2. func AllowMissingMetaElementGroupLength() ParseOption { return func(set *parseOptSet) { - set.allowErrorMetaElementGroupLength = true + set.allowMissingMetaElementGroupLength = true } } diff --git a/read.go b/read.go index f876cc2d..defdd8a2 100644 --- a/read.go +++ b/read.go @@ -163,7 +163,7 @@ func (r *reader) readHeader() ([]*Element, error) { metaElementGroupLengthDefined := true if maybeMetaLen.Tag != tag.FileMetaInformationGroupLength || maybeMetaLen.Value.ValueType() != Ints { // MetaInformationGroupLength is not present or of the wrong value type. - if !r.opts.allowErrorMetaElementGroupLength { + if !r.opts.allowMissingMetaElementGroupLength { return nil, ErrorMetaElementGroupLength } metaElementGroupLengthDefined = false @@ -198,7 +198,12 @@ func (r *reader) readHeader() ([]*Element, error) { } tg := tag.Tag{} buff := bytes.NewBuffer(tag_bytes) - binary.Read(buff, binary.LittleEndian, &tg) + if err := binary.Read(buff, binary.LittleEndian, &tg.Group); err != nil { + return nil, err + } + if err := binary.Read(buff, binary.LittleEndian, &tg.Element); err != nil { + return nil, err + } debug.Logf("header-tag: %v", tg) // Only read group 2 data if tg.Group != 0x0002 { diff --git a/read_test.go b/read_test.go index 7dc6302e..859abb86 100644 --- a/read_test.go +++ b/read_test.go @@ -685,7 +685,7 @@ func headerWithFileMetaInformationGroupLength() (*headerData, error) { } func TestReadHeader_TryAllowErrorMetaElementGroupLength(t *testing.T) { - opts := parseOptSet{allowErrorMetaElementGroupLength: true} + opts := parseOptSet{allowMissingMetaElementGroupLength: true} t.Run("NoFileMetaInformationGroupLength", func(t *testing.T) { dcmheaderNoInfoGrpLen, err := headerWithNoFileMetaInformationGroupLength() @@ -697,12 +697,12 @@ func TestReadHeader_TryAllowErrorMetaElementGroupLength(t *testing.T) { opts: opts, } r.rawReader.SetTransferSyntax(binary.LittleEndian, true) - parsedElements, err := r.readHeader() + wantElements, err := r.readHeader() if err != nil { - t.Errorf("unsuccessful readHeader when parse option %v is turned on and header has no MetaElementGroupLength tag", opts.allowErrorMetaElementGroupLength) + t.Errorf("unsuccessful readHeader when parse option %v is turned on and header has no MetaElementGroupLength tag", opts.allowMissingMetaElementGroupLength) } // Ensure dataset read from readHeader and the test header are the same except for the ValueLength field. - if diff := cmp.Diff(parsedElements, dcmheaderNoInfoGrpLen.Elements, cmp.AllowUnexported(allValues...), cmpopts.IgnoreFields(Element{}, "ValueLength")); diff != "" { + if diff := cmp.Diff(wantElements, dcmheaderNoInfoGrpLen.Elements, cmp.AllowUnexported(allValues...), cmpopts.IgnoreFields(Element{}, "ValueLength")); diff != "" { t.Errorf("Elements parsed from test header do not match: %v", diff) } } @@ -718,12 +718,12 @@ func TestReadHeader_TryAllowErrorMetaElementGroupLength(t *testing.T) { opts: opts, } r.rawReader.SetTransferSyntax(binary.LittleEndian, true) - parsedElements, err := r.readHeader() + wantElements, err := r.readHeader() if err != nil { - t.Errorf("unsuccesful readHeader when parse option %v is turned on and header has no MetaElementGroupLength tag", opts.allowErrorMetaElementGroupLength) + t.Errorf("unsuccesful readHeader when parse option %v is turned on and header has no MetaElementGroupLength tag", opts.allowMissingMetaElementGroupLength) } // Ensure dataset read from readHeader and the test header are the same except for the ValueLength field. - if diff := cmp.Diff(parsedElements, dcmHeaderInfoGrpLen.Elements, cmp.AllowUnexported(allValues...), cmpopts.IgnoreFields(Element{}, "ValueLength")); diff != "" { + if diff := cmp.Diff(wantElements, dcmHeaderInfoGrpLen.Elements, cmp.AllowUnexported(allValues...), cmpopts.IgnoreFields(Element{}, "ValueLength")); diff != "" { t.Errorf("Elements parsed from test header do not match: %v", diff) } } From 97f59cabe9558f5fe81224be391240171a0e06fe Mon Sep 17 00:00:00 2001 From: Fausto J Espinal Date: Wed, 14 Jun 2023 08:57:07 -0500 Subject: [PATCH 9/9] fixed accidentally removed EOF check --- read.go | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/read.go b/read.go index defdd8a2..3dbdd341 100644 --- a/read.go +++ b/read.go @@ -50,6 +50,14 @@ func (r *reader) readTag() (*tag.Tag, error) { if gerr == nil && eerr == nil { return &tag.Tag{Group: group, Element: element}, nil } + // If the error is an io.EOF, return the error directly instead of the compound error later. + // Once we target go1.20 we can use errors.Join: https://pkg.go.dev/errors#Join + if errors.Is(gerr, io.EOF) { + return nil, gerr + } + if errors.Is(eerr, io.EOF) { + return nil, eerr + } return nil, fmt.Errorf("error reading tag: %v %v", gerr, eerr) } @@ -192,21 +200,18 @@ func (r *reader) readHeader() ([]*Element, error) { debug.Log("Proceeding without metadata group length") for { // Lets peek into the tag field until we get to end-of-header - tag_bytes, err := r.rawReader.Peek(4) + group_bytes, err := r.rawReader.Peek(2) if err != nil { return nil, ErrorMetaElementGroupLength } - tg := tag.Tag{} - buff := bytes.NewBuffer(tag_bytes) - if err := binary.Read(buff, binary.LittleEndian, &tg.Group); err != nil { - return nil, err - } - if err := binary.Read(buff, binary.LittleEndian, &tg.Element); err != nil { + var group uint16 + buff := bytes.NewBuffer(group_bytes) + if err := binary.Read(buff, binary.LittleEndian, &group); err != nil { return nil, err } - debug.Logf("header-tag: %v", tg) + debug.Logf("header-group: %v", group) // Only read group 2 data - if tg.Group != 0x0002 { + if group != 0x0002 { break } elem, err := r.readElement(nil, nil)