Comments (8)
Thanks for raising this @thavlik, I think it makes sense for us to make a fix here to account for DICOMs like this!
I'll look deeper into this either this weekend or later next week when I'm back from vacation. The solution you indicated that PyDicom does seem workable, but it would be great to avoid rewinding if possible.
I think that we might be able to get by without rewinding (which would be awesome) so long as the correct MetaElementGroupLength
exists somewhere before the end of the metadata tags (and if we keep track of metadata bytes read so far)--will look into this a bit more closely when I'm back from vacation!
We'll want to implement a fix for the current version and also the s/1.0-rewrite rewrite/refactor that I'm working on to update/fix many things from upstream branches.
Are you able to share an example DICOM?
Thank you so much for raising this!
from dicom.
I think that we might be able to get by without rewinding (which would be awesome) so long as the correct MetaElementGroupLength exists somewhere before the end of the metadata tags (and if we keep track of metadata bytes read so far)
That's a great idea! I'm unable to confirm the eventual existence of MetaElementGroupLength for each item, so it might be missing entirely. Maybe they are present - I haven't checked.
Here is an example DICOM:
ID_0015ba0f2.zip
EDIT: can confirm ID_0015ba0f2.dcm only has:
(0x0002, 0x0002)
(0x0002, 0x0003)
(0x0002, 0x0010)
(0x0002, 0x0012)
(0x0002, 0x0013)
from dicom.
@thavlik Thank you for the investigation and the sample dicom! To support unstandardized dicoms like this, it appears that we may indeed need to investigate a rewinding mechanism, or some sort of peek
ahead capability (maybe a small buffer to allow something like this).
I think it will be a couple weeks before I have bandwidth to really dig into this. PRs are welcome so let me know if you might be interested in proposing a fix (no worries if not).
Will keep this issue updated as I revisit and in mind for the 1.0 rewrite too!
from dicom.
One possibility is to read until the first non-metadata tag (this much is unavoidable) and then store a pointer to said tag within internal state. Parsing the body then entails considering this initialTag if it is non-nil, and subsequently setting it to nil - resuming current behavior. Kinda hacky but it's all I've got.
from dicom.
@suyashkumar I'm looking to implement this fix now.
from dicom.
I added an example DCM and made some changes at thavlik@15f3120.
The tests do not pass. Currently digging into:
$ go test -v -run TestAllFiles
=== RUN TestAllFiles
TestAllFiles: parse_test.go:35: Reading CT-MONO2-16-ort.dcm
2020/05/23 18:23:50 Encountered odd length (vl=71042085) when reading implicit VR 'UN' for tag (042f,044f)[private] (file offset 328068)
--- FAIL: TestAllFiles (0.06s)
panic: Encountered odd length (vl=71042085) when reading implicit VR 'UN' for tag (042f,044f)[private] (file offset 328068) [recovered]
panic: Encountered odd length (vl=71042085) when reading implicit VR 'UN' for tag (042f,044f)[private] (file offset 328068)
goroutine 19 [running]:
testing.tRunner.func1.1(0x7e3040, 0xc000811c90)
/usr/local/go/src/testing/testing.go:941 +0x3d0
testing.tRunner.func1(0xc0002d8480)
/usr/local/go/src/testing/testing.go:944 +0x3f9
panic(0x7e3040, 0xc000811c90)
/usr/local/go/src/runtime/panic.go:967 +0x15d
log.Panic(0xc000307ec8, 0x1, 0x1)
/usr/local/go/src/log/log.go:351 +0xac
github.com/suyashkumar/dicom_test.mustReadFile(0xc0001fe200, 0x1c, 0x0, 0x0, 0x0, 0x0, 0x0, 0x456de9)
/mnt/c/Users/tlhavlik/go/src/github.com/suyashkumar/dicom/parse_test.go:24 +0x12f
github.com/suyashkumar/dicom_test.TestAllFiles(0xc0002d8480)
/mnt/c/Users/tlhavlik/go/src/github.com/suyashkumar/dicom/parse_test.go:36 +0x22d
testing.tRunner(0xc0002d8480, 0x8923e0)
/usr/local/go/src/testing/testing.go:992 +0xdc
created by testing.(*T).Run
/usr/local/go/src/testing/testing.go:1043 +0x357
exit status 2
FAIL github.com/suyashkumar/dicom 0.086s
from dicom.
Hey @thavlik thanks for taking a look at this! I'll need to look a little closer at the test failure on your branch to help debug.
In terms of the general approach, off the top of my head I think it might be sufficient to read until the first non group 0x0002
group tag and then proceeding with typical parsing after that. I'll have to double check though if we need to enforce that certain metadata tags be present. To make the interface a little less hacky, we can have parseFileHeader
return two []*element.Element
slices--one containing the metadata tags, and one containing the extra element we read, to start off the elements to be read downstream. It'll still be a little hacky, but maybe an okay place to start.
Will need to double check that there isn't anything else special we need to consider when reading that extra non-metadata tag (I think it should be okay, but we need to look up and see if metadata tags have any interaction with reading some of the downstream non-metadata tags and how we might want to deal with it if so)
from dicom.
My branch is, functionally, doing exactly this. It's unclear why the rest of the file fails to parse as normal.
from dicom.
Related Issues (20)
- File with dicom seg throws an error - writing dicom file: unsupported BitsPerSample value
- Converting DICOM to Image results into "blacked" image HOT 4
- using dicom.Parse function ,unexpected EOF HOT 1
- Write element with sequence item or nested sequence HOT 2
- The reason why the converted DICOM displays as blank after converting from RAW format might be attributed to the following factors: HOT 1
- Rewrite & Update README
- Remove Mocks
- Refactor internal-only packages into internal/
- Refactor PixelDataInfo API
- Remove dicomio.Reader interface in favor of dicomio.Reader direct struct
- Consider deprecating top level bytesLimit in Parse() API in favor of always parsing until EOF.
- Write API: When using WriteElement API, ensure proper transfer syntax is used to encode the data (Write() works fine)
- Support deflate transfer syntax writes
- Add write benchmarks
- Consider Parse API refactor HOT 1
- Improve support for non-compliant DICOM and edge cases HOT 8
- write: option to override default missing transfer syntax in dicom (mostly useful for testing, since users could just update the uid in the dataset)
- For missing Transfer Syntax UID fallback, should we also try reading with a deflated syntax?
- Error when parsing with Little Endian Explicit transfer syntax HOT 2
- Support for RGB for pixeldata HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from dicom.