Reading PixelData of undefined length #30
Conversation
There was a problem hiding this comment.
Thank you for working on this. I have left a few comments on the code, but it's also important at this stage to make a design decision on how to handle encapsulated pixel data. As proposed, this implementation of the parser will eagerly decode all of the encapsulated pixel data as soon as it is found. There are plans to make data set reading more lazy, which concerns pixel encapsulation too. I can picture one of the following decisions on how the data set reader should behave:
- The parser will seamlessly provide a DICOM value when it finds encapsulated pixel data. The information about pixel data encapsulation is hidden from the API consumer, with the benefit of being ready for decoding, but less transparent. (this is similar to what we have in this PR)
- The parser will decompose the encapsulated pixel data and provide tokens for the offset tables and other individual pieces. This would potentially be more memory efficient, especially when tokens are eagerly read from the file, and the pixel data could still be fetched in flat form with an additional function call that would turn the token sequence into a readable source.
- The parser is oblivious to pixel data encapsulation. It would just assume the rest of the stream to be part of the last DICOM value in the data set. This could bring more problems down the line, so I'd rather discard it now and only have it written for future reference.
In the meantime, there is value in having a working implementation.
parser/src/parser.rs
Outdated
| self.bytes_read += 8; | ||
|
|
||
| println!("PixelData of undefined length"); | ||
| println!("Tag({:#06X}, {:#06X})", u16::from_le_bytes([buf[0], buf[1]]), u16::from_le_bytes([buf[2], buf[3]])); |
There was a problem hiding this comment.
I expect these print calls to be removed before the merge. But just letting you know that since the Tag type implements Display, doing something like this would have been more intuitive:
println!("{}", Tag(u16::from_le_bytes([buf[0], buf[1]]), u16::from_le_bytes([buf[2], buf[3]])));
parser/src/parser.rs
Outdated
| self.bytes_read += len as u64; | ||
|
|
||
| let mut offsets = vec![]; | ||
| for i in 0..len / 4 { |
There was a problem hiding this comment.
This logic looks suspicious, considering that i still iterates one by one from 0 to len/4 without being adjusted accordingly. The table offset reads are overlapping.
You could probably create the list of offsets functionally, by chunking offset_table into pieces of 4. Maybe use Itertools#tuples?
Enet4
force-pushed
the
pixel-data-undefined-length
branch
from
62a7010
to
9fbf6c0
Compare
Enet4
force-pushed
the
pixel-data-undefined-length
branch
from
9fbf6c0
to
c06f377
Compare
|
I'm afraid that this PR has become a bit outdated with #35. I rebased the branch, but tests and changes are still pending. @ibaryshnikov Not sure if you are able to follow up on this, but if not, this makes an opportunity to take a step back and implement encapsulated pixel data at the data-set level instead. |
|
Thank you for the attempt, but this feature has been succeeded by #42. |
It's my first naive implementation, consider it an early WIP. I do really need your advice in structuring the code for this pr, @Enet4. But it seems to work and the file linked in #23 can be opened successfully.