Allow custom pixel data parsing and streaming pixel data directly from the reader.

[DmitrijsBarbarinsFreiheit]

No description provided.

[ghost]

Is it possible to upstream this change?

[suyashkumar]

Hi, thanks for the PR. I think the intent here is to do something like what was requested in #224?

I have some concerns with this PR, summarized below:

• Introducing the GetPixelDataReader() API to Parser is problematic (in its current form), because it's too easy to use it incorrectly. For example, if a user creates a new Parser, and then immediately calls GetPixelDataReader() it will actually return bytes that are not the PixelData. Maybe we could try to fix this by reading the dataset first, adding some guards, or just reading the elements we need to read the PixelData (as I described a little in Add option to return Reader for PixelData element #224). Either way, if we were to go with this approach, I think we would need to think about the right APIs and how to make it clear and intuitive for the user what to do.
• I think introducing GetDataset complicates the Parser API (which was designed to act as an element-by-element API via the use of Next()). This actually would prevent us from doing the kind of optimization I mentioned in Add option to return Reader for PixelData element #224, where the Parser itself only holds onto elements that are needed for processing "later" elements (this is one of the approaches I was thinking of starting with to make things a little more lightweight).
• I need to look a little more closely to verify and double check this, but I don't think your stopAtPixelDataValue implementation is correct, because I think readElement may encounter PixelData tags before the actual main PixelData of the DICOM in some cases. For example, consider PixelData that's part of an IconImageSequence which I think in your implementation would be skipped when parsing.

I think these points along with the ones in #224 may require some more careful discussion, and I'd also like to hear more about your motivating use cases so that we can arrive at the right solution. Some quick thoughts from me on possible solutions:

• Create a utility function (e.g. not part of the Parser API) that uses the element-by-element Parser API to return (Dataset, pixelData io.Reader)
• Here's an optimization I wanted to do anyway as mentioned in Add option to return Reader for PixelData element #224--namely, in the element-by-element Parser, don't hold onto the full Dataset of all parsed elements--only hold the elements we need to successfully Parse later elements, leave it up to the caller what they wish to do with Elements returned from Next(). I can send a PR for this soon.
• Include an option and new Element type to hold unparsed pixeldata (e.g. we just read it into a buffer without any parsing--this really only matters for NativePixelData dicoms. saves some CPU but not that much memory). One option here is to reuse the existing frame streaming channel to stream unprocessed frames to the caller (but don't save them into an element, if a certain option is set)--this could help save on memory if done correctly, though this would apply to all PixelData by deafult (incl Icons. Maybe we could exclude those if it made sense).

[dvaruas]

Hi, thank you for the comments on the PR and your thoughts on it.

The use-case which we are trying to handle is that for some DICOM tags (could be PixelData or RawData held in a private tag), we do not wish the library to parse the contents of the value but rather stream it directly from the reader into some writer of our choice. This would save us in terms of processing of the value for the tag and also memory.

As you suggested in point 3 earlier for possible solutions, maybe some option could be set in the parser for certain tags which the user does not want to be parsed by the library, but rather stream the bytes somewhere else (this would also mean that these elements are not held in the dataset). It does makes sense to ensure that the tags used in this option are not something whose values are needed to successfully parse later elements. This would also mean that the solution is not restricted to PixelData but any other tag which can potentially hold data. I do agree that the existing channel (which is only used in the case of PixelData currently) could be used for this purpose.

Looking forward to hear your thoughts on this.

[suyashkumar]

Hi there! I have a draft PR I'm playing around with at #256 that addresses this to some degree--it skips the processing of NativePixelData in favor of just blindly loading the data into memory (so that it can be roundtripped out later). It's not quite an arbitrary writer solution though (the API and usage for which may not be ideal) and still loads the data into memory (but may save you some CPU for NativePixelData)

Ultimately I think the right long term solution might be to instead have an option that indicates only certain elements should be processed, or a lazy load type of situation. However, for now, it seems like this is most important for PixelData so I figure it is good to start there.

[suyashkumar]

See also #257

[suyashkumar]

#267 has a working draft of a solution that should allow you not to hold the whole dataset in memory at once--if you are interested, please give it a try and see if it helps at all!

You would have to use the Parser.Next() API something like

var elem *dicom.Element
for err != dicom.EndOfDICOM {
    elem, err = parser.Next()
    newElem := doSomeProcessing(elem)
    writer.WriteElement(elem)
}

[dvaruas]

Hi @suyashkumar thank you for the exposed options which helps us inch a bit closer to our original use case.

Currently, there's a way to skipPixelData (skips completely) and skipProcessingPixelDataValue (skips processing but still reads the value), maybe we can have a third option which sets a custom writer for the pixeldata part and instead of reading it and holding it locally it just writes to wherever the writer is pointing.

We want to essentially skip the double writing of pixeldata, once to memory and once again to file. This option could be a general option applied for any tag (other than datasetContextElementPassList tags whose values are required for the parsing of other tags). For these options, the values are written to the writers provided.

Looking forward to hearing your thoughts on this. Thanks.

[suyashkumar]

@dvaruas in principle it should not be difficult to have a RedirectPixelDataToWriter option, but it will require careful use, and could be error prone. Can you tell me more about your use case? Do you really want just the value of the tag to be written out or do you want the entire element to be written to the writer (e.g. including the tag, VR if any, VL, value)?

As mentioned above there can be multiple PixelData tags (some inside sequences) in a single DICOM. If this option works by blindly writing out data to the provided writer, multiple PixelData elements could be sent at different times and it'd be up to the writer to decide what to do.

If you're trying to interweave this into a Read - Modify some elements -Write pipeline, and you're trying to pass in the same underlying writer that the dicom.Writer is writing to that could be error prone because the Writer isn't aware of these external writes to the file, and those writes may not be in accordance with the dicom.Writers configuration (implicit vs. explicity transfer syntax), and any other house keeping the dicom.Writer may be doing to the file.

If it is the Read-Modify-Write pipeline, I understand your use case. It may make more sense for us to provide a separate Pipeline or Modification API where you can pass in some list of tags you wish to mutate and a mutation function (or a struct that meets some Mutator interface that could also include a list of tags to call the function for), and then we can have the Modification pipeline oversee and safely manage processing the file and calling your mutation function when appropriate for the right tags only. This seems a little safer to me than the arbitrary writer option where the writer is being written to from two separate owners without any real coordination.

WDYT?

[suyashkumar]

Instead of a passlist of tags to mutate, we could also consider and option for tags not to mutate, if that is more commonly definable (or maybe an option to do both).

Either way, I'd like to understand the use case better and try to understand how common of a use case this is.

I opened #272 to discuss this further and think about what it could look like.

[DmitrijsBarbarinsFreiheit]

Hi, sorry for the late response.
I will also follow up on this here, because i think the use case we have is not really a read- modify one, but rather read -> stream somewhere else, while still parsing one.

Or in more detail: We receive DICOM's from our "own" service, after some upload processor and coversion took place, so we know that the file follows certain rules.
The service receives the DICOM file over HTTP and the files tend to be big, they contain multiple frames all stored in the PixelData section of the file.
The processing currently looks like this:

• service performs HTTP get and retrieves the response
• while still streaming the body, we already start processing of the data using reader provided by the library
• as soon as we locate the PixelData tag, we create a special file on disk, in binary format, write some of the data collected from the DICOM tags as a header and then stream the whole pixel data, as is, into the file
• different service is notified and reads this new binary file to do the rendering etc

This was done mostly as an optimization, to speed up the loading process (by a factor of about 6):

• we skip all the bit/byte
• we skip copying the data around, as it might be rather big (up to a few gigabytes)
• we stream the pixel data directly to a file, while still downloading it

I hope this clarifies the use case a bit.

Let me know, if you still think we should move to the #272

[suyashkumar]

Thanks for the context @DmitrijsBarbarinsFreiheit! That is definitely helpful.

Some questions:

• How do you handle nested PixelData elements, for example Icons in an IconSequence?

Some off-the-cuff thoughts:

• If we can handle this kind of use case with ParseOptions instead of introducing new top level functional APIs, I would have a slight preference for that.
• What if we introduced a ParseOption that could take a tag(s) and io.Writer, and just blindly write out the raw bytes values to that if a tag in the option is encountered? (like the RedirectPixelDataToWriter I mentioned above in Allow custom pixel data parsing and streaming pixel data directly from the reader. #223 (comment))
• What if we took it a step forward and considered a ParseOption that could take a tag(s) and a callback for custom handling? This could be dangerous based on how we design the callback API (e.g. if you read too far ahead, advance some of the internal state of readers etc that could mess up subsequent parsing). We could advertise this danger by naming the option UnsafeCustomCallback or something, or restricting it only to the final PixelData tag or something to start with. For PixelData we might be able to have a narrow version of this that is safer, since if we're in a Sequence we can read until the end of the limit (even if undefined length encapsulated PixelData) or we should be at the end of the DICOM and read until the end of the buffer. With defined length PixelData ofc we can have errors if the callback tries to read beyond the allotted vl.

[suyashkumar]

@DmitrijsBarbarinsFreiheit it's been a while but wanted to follow up on this--do you think the approach outlined above might still make sense for y'all?