Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore missing metadata group length field #269

Merged
merged 11 commits into from
Jun 17, 2023
17 changes: 13 additions & 4 deletions parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
faustoespinal-philips marked this conversation as resolved.
Show resolved Hide resolved
}

func toParseOptSet(opts ...ParseOption) parseOptSet {
Expand All @@ -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
faustoespinal-philips marked this conversation as resolved.
Show resolved Hide resolved
func AllowErrorMetaElementGroupLength() ParseOption {
faustoespinal-philips marked this conversation as resolved.
Show resolved Hide resolved
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 {
Expand Down
60 changes: 45 additions & 15 deletions read.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
faustoespinal-philips marked this conversation as resolved.
Show resolved Hide resolved
return nil, ErrorMetaElementGroupLength
}
metaElementGroupLengthDefined = false
metaElems = append(metaElems, maybeMetaLen)
faustoespinal-philips marked this conversation as resolved.
Show resolved Hide resolved
}

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 {
faustoespinal-philips marked this conversation as resolved.
Show resolved Hide resolved
faustoespinal-philips marked this conversation as resolved.
Show resolved Hide resolved
// 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)
faustoespinal-philips marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, ErrorMetaElementGroupLength
}
tg := tag.Tag{}
buff := bytes.NewBuffer(tag_bytes)
binary.Read(buff, binary.LittleEndian, &tg)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what would happen here if in the future an extra field were added to the tag struct in the future? It also sounds like unexported fields may lead to Read panicing (if we were to add on in the future)

It's a bit more verbose, but we may want to consider something like

if err := binary.Read(buff, binary.LittleEndian, &tg.Group); err != nil {
// handle err
}
if err := binary.Read(buff, binary.LittleEndian, &tg.Element); err != nil {
//handle err
}

Also don't forget to handle the errors on binary.Read as well :).

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly I can change it. I guess your point is that tag could change in a way that makes it incompatible with direct mapping from the DICOM header...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, structs can/do change and if it did, it could lead to an unintentional breakage here. If the tests are good enough it's possible that regression would be protected, but it seems safer to do the slightly more verbose way to me for now!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make this update? Thanks!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, not sure if this update was applied (it's not showing up for me). basically instead of reading into the tag.Tag directly, reading each field of the tag

if err := binary.Read(buff, binary.LittleEndian, &tg.Group); err != nil {
// handle err
}
if err := binary.Read(buff, binary.LittleEndian, &tg.Element); err != nil {
//handle err
}

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
}
Expand Down