Conversation
Enet4
left a comment
There was a problem hiding this comment.
Great follow-up on the new frame decoding feature, much appreciated!
I assume that this is still a work in progress, but I will leave some comments for now.
| .for_each(|fragment| buf.append(&mut fragment.clone())); | ||
| buf | ||
| } else { | ||
| fragments[frame].to_vec() |
There was a problem hiding this comment.
What if the frame comprises more than one fragment in a multi-frame pixel data fragment sequence? I do not think that we can make this assumption.
There was a problem hiding this comment.
Only single frame dicoms can have more than 1 fragment. Multi frame files, must be 1 fragment per frame. At least according to the standard.
There was a problem hiding this comment.
I guess we can move forward with this assumption and revisit if we find a conflicting sample case. 👍
There was a problem hiding this comment.
Just a heads-up, I received clarifications from DICOM WG6, who asserted that it is allowed for frames in a multi-frame instance to span through multiple fragments each, though more recent transfer syntaxes tend to disallow this.
Still, I won't make this a blocker so that these improvements can be delivered. The affected area is only for the integration with GDCM, which hopefully the project will depend less on over time.
|
So far, I have tested this with Compressed (jpeg, jpeg-ls and j2k) and Uncompressed (little endian) data. With RGB, YBR_FULL_422, MONOCHROME1, MONOCRHOME2 and PALETTE_COLOR (at least decompressing) photometric interpretations. |
|
Hello @dougyau! Are there any other pending concerns with this PR, so that it can be marked as ready? |
|
This code is working, what I have to do is cover it with unit testing. I have been short of time, but once I do have some time, I will add the tests. |
|
I have validated the existing test, and it uses the new code path. Of the test cases I believe we could convert |
I have been slowly working on a new release for the |
Enet4
left a comment
There was a problem hiding this comment.
I tested this with a few test files and could validate that the behavior has improved in accuracy and performance after these changes. I believe we are better off merging this now and reiterating on the test suite after dicom-test-files is updated. Thanks!
Extending from the PR #421 when using GDCM to decode images it will load the whole image to memory. This PR implements the logic required to load only one image.