PantelisGeorgiadis / dcmjs-dimse

DICOM DIMSE implementation for Node.js using the dcmjs library
MIT License
68 stars 13 forks source link

Handling of inability to C-STORE by clients #42

Open richard-viney opened 1 year ago

richard-viney commented 1 year ago

Just noticed that it would be useful if the A transfer syntax transcoding from ${t.getTransferSyntaxUid()} to ${s} is currently not supported. Skipping... error could be surfaced/handled by clients of dcmjs-dimse. Is there a good way to achieve this currently? I think I can loop through the accepted presentation contexts to see if one will match, but it would nice to have a way to directly handle this case when dcmjs-dimse does run into it.

A possible extension to this, though not one I have any need of, would be to allow the client to transcode the DICOM to a supported transfer syntax when this error does happen, which could be used to convert to Implicit VR Little Endian for maximum compatibility with other systems. Obviously there's more involved there, and any acutal transcoding itself is completely out of scope for dcmjs-dimse.

PantelisGeorgiadis commented 1 year ago

Hi @richard-viney, Thanks for bringing this up! Regarding your last sentence... "Obviously there's more involved there, and any acutal transcoding itself is completely out of scope for dcmjs-dimse", the answer is yes and no! Actually, half of the transcoding work required has already been done for the NativePixelDecoder module of dcmjs-imaging. We just need to implement the opposite (well... there's more involved there)! In any case, you are right... clients should be able to handle an "unable to transcode" situation. Maybe we can propagate this, as an event (e.g. transcodingError) to the Client class and let the caller decide whether to halt or continue. Thoughts?

richard-viney commented 1 year ago

Yep for sure, dcmjs-imaging is a very good step towards implementing transcoding should a client want to do that, but this falls outside of the scope of dcmjs-dimse.

Propagating an event to the client is reasonable, and if that event goes unhandled the current behaviour of reporting a log message and continuing one could be kept unchanged in order to preserve compatibility. However, in my opinion the current behaviour of only emitting a log message and continuing on as if there was no problem isn't an ideal default, so perhaps a breaking change to the current behaviour is preferable in order to surface this case more reliably to all clients, i.e. treat it as an error in all cases.

If the client wants to, they can detect the error, transcode the dataset, and then re-send it.

Thoughts?

PantelisGeorgiadis commented 1 year ago

Hi Richard! Just realized that I haven't responded to your last comment... sorry for that! I agree with your proposal and will try to include it in the next release!

jimOnAir commented 11 months ago

It would be a great feature. And the same thing for Unsupported Abstract Syntax is appreciated. Currently only the 'done' event is emitted by Request on Unsupported Transfer Syntax and the 'closed' event is emitted by the Client on Unsupported Abstract syntax