Refact storescp

[dwikler]

This merge request addresses part of issue #85.
It refactors storescp.py from the tdwii_plus_examples/TDWII_PPVS_subscriber folder.
The StoreSCP class is refactored as CStoreSCP and moved to tdwii_plus_examples package folder in cstorescp.py
The main function is refactored as an storescp.py CLI application and moved to the tdwii_plus_examples/cli folder.
cstore_handler.py is also refactored and moved to tdwii_plus_examples package folder.
Tests are provided: a unit test for each module and an integration test to test the CStoreSCP server function.

[sjswerdloff]

LGTM.
But I think there might be some rough edges around private elements.

[sjswerdloff]

SR usually refers to Structured Report.
Do we have to be constrained to a two character prefix?

[dwikler]

Agreed. Using a 2 letter prefix is difficult considering the number of modalities. We could use 3 when 2 is colliding or we could remove the first 2 letter prefixes items from the list and only use one short prefix using the value of the first or second item like this:

SOP_CLASS_PREFIXES = {
    "1.2.840.10008.5.1.4.1.1.2": ("CT", "CT Image Storage"),
    "1.2.840.10008.5.1.4.1.1.4": ("MR", "MR Image Storage"),
    "1.2.840.10008.5.1.4.1.1.7": ("SC", "Secondary Capture Image Storage"),
    "1.2.840.10008.5.1.4.1.1.128": (
        "PT", "Positron Emission Tomography Image Storage"),
    "1.2.840.10008.5.1.4.1.1.481.1": (
        "RTI", "RTIMAGE",
        "RT Image Storage"),
    "1.2.840.10008.5.1.4.1.1.481.2": (
        "RTDOSE",
        "RT Dose Storage"),
    "1.2.840.10008.5.1.4.1.1.481.3": (
        "RTSTRUCT", "RTSTRUCTURESET",
        "RT Structure Set Storage"),
    "1.2.840.10008.5.1.4.1.1.481.4": (
        "RTREC", "RTRECORD",
        "RT Beam Treatment Record Storage"),
    "1.2.840.10008.5.1.4.1.1.481.5": (
        "RTPLAN", "RTPLAN",
        "RT Plan Storage"),
    "1.2.840.10008.5.1.4.1.1.481.6": (
        "RTBYREC", "RTBRACHYRECORD",
        "RT Brachy Treatment Record Storage"),
    "1.2.840.10008.5.1.4.1.1.481.7": (
        "SUMREC", "RTSUMRECORD",
        "RT Treatment Summary Record Storage"),
    "1.2.840.10008.5.1.4.1.1.481.8": (
        "RTIONPLN", "RTIONPLAN",
        "RT Ion Plan Storage"),
    "1.2.840.10008.5.1.4.1.1.481.9": (
        "RTIONREC", "RTIONRECORD",
        "RT Ion Beams Treatment Record Storage"),
    "1.2.840.10008.5.1.4.34.7": (
        "RTBDI", "RTBEAMDELIVERYINSTRUCTION",
        "RT Beams Delivery Instruction Storage"),
    "1.2.840.10008.5.1.4.1.1.66.1": (
        "REG", "IMAGEREGISTRATION",
        "Spatial Registration Storage"),
    "1.2.840.10008.5.1.4.1.1.66.3": (
        "DIR", "DEFORMABLEIMAGEREGISTRATION",
        "Deformable Spatial Registration Storage")
}

What do you think is best?

[sjswerdloff]

My preference is to use two letters and three when there is a collision on two.
REG is an example of that.

[dwikler]

ok, I will fix this

[sjswerdloff]

Things get a bit tricky on saving a file. Other than RT Ion Plans that contain Ion Range Compensator (thicknesses), and RT Structure Set (Contour Data) that can have problems with the 64 KB limit for an explicit encoding, it's best to use Explicit (Little Endian) so that one doesn't lose the private element information. Ion Range compensators are becoming pretty rare. The RT SS issue is still a problem with vendors who encode at double precision (plenty of them do that) for the contour data (implying sub-angstrom precision, which is kind of silly).

[dwikler]

As it is, the cstorescp always add ImplicitVR LittleEndian in the supported presentation contexts even if only ExplicitVRLittleEndian is specified in args, as without this, it would not be compliant to DICOM rules. So I am not sure there is a way to force the SCU to use of ExplicitVRLittleEndian unless there is a way to only accept one of the supported syntax when both are proposed. In the meatime, this can be achieved by setting only ExplicitVRLittleEndian in the SCU association request. Ultimately, there is no way for the SCP to force using Explicit as the SCU has always the right to only propose the ImplicitLittleEndian default.
The private elements will be retained in all situations but with Implicit, the file will not contain the VR of the private elements. Actually, even in this case, the VR could be set to UN (Unknown) by the DICOM Service Provider. So, the receiving application always need to have knowledge of the dictionary of the private elements in order to use them and whether the transfer syntax is implicit or explicit does not change this. It may be easier to have explicit VR but as the semantic is also required, the VR can also be documented along with the semantics.

[dwikler]

I will see if there is a way with pynetdicom to make the SCU select another syntax than the default one.

[sjswerdloff]

We should test a private element as well.
There are still plenty of private elements still in use by vendors who implement TDW-II.

[dwikler]

Definitely, I created a dev branch for adding this and fixing point in previous comments (fix-comments-PR88). Adding private elements confirms that the stored file contains the private elements whatever transfer syntax is used.

[dwikler]

There is a way to specify which transfer syntax to accept if there are more than one presented in the association request's presentation context. As the pynetdicom.presentation.negotiate_as_acceptor method selects the first transfer syntax matching an item in its list of supported transfer syntaxes, sorting the transfer syntaxes in this list allows to defined the preferred transfer syntaxes.
We could either defined a default order or have both a default order and an argument to change it as in https://pydicom.github.io/pynetdicom/stable/apps/storescp.html#preferred-transfer-syntaxes

[sjswerdloff]

It's always nice to include an option to change from default but we definitely should default to preferring explicit LE.