dicoogle icon indicating copy to clipboard operation
dicoogle copied to clipboard

Extensions to the SOP classes and Transfer Syntaxes hardcoded lists.

Open Almeida-a opened this issue 3 years ago • 4 comments

Changes by acceptance criteria

  • The list of supported SOP classes and the list of supported transfer syntaxes should both be extensible independently, via configuration.
    • Let's try using new categories in confs/server.xml for this.

Schema example is shown further ahead.

  • Each custom SOP class or transfer syntax should specify a UID and alias. Once done, that entry is added alongside the hardcoded default list (so these records cannot be removed)

All entries are successfully added alongside the lists at:

  • TransfersStorage. However, they are not added right after the hardcoded lists’ initialization, for the reason that the settings (ServerSettingsManager.inner and thus, the additional entries present in server.xml) are initialized a posteriori. To solve this problem, the “additionals” are completed right after the settings reading process, in this case using TransfersStorage.completeList().
  • SOPList. The situation is similar. The list is updated after the settings’ initialization, in SOPList.updateList(). There is, in this function, however, one other aspect. Since the hardcoded list is initialized prior to the settings, and that in that initialization, each SOP class is associated with the list of transfer syntaxes, that list is also updated (with the new transfer syntaxes, if any).
  • SOPClassSettings. There is no need for posterior updates, since this class is only instantiated after the settings.
  • This configuration can be empty or unspecified.

Empty tags are ignored.

  • This list is not sufficient for the DICOM storage server to accept those SOP classes and transfer syntaxes. This must be configured as usual.

All SOP classes in the transfer options, be it the hardcoded or the additional ones, will have the additional transfer syntaxes unchecked in their list.

  • Server settings should be validated for unspecified UIDs, but it must let users specify well configured UIDs and have that applied in the DICOM storage server.

All additional SOP classes as well as TSs will have their UIDs validated using UIDUtils.isValid(uid) from dcm4che2.

  • The user interface should acknowledge the extended list of SOP classes and show options for the extended list of transfer syntaxes.

Done successfully.

New schema at settings example

<config>
...
    <additional-sop-classes>
      <additional-sop-class uid="1.2.840.100008.5.1.4.1.1.131 "alias="BasicStructuredDisplayStorage" />
    </additional-sop-classes>
    <additional-transfer-syntaxes>
      <additional-transfer-syntax uid="1.2.840.100008.1.2.4.95" alias="JPIPReferencedDeflated" explicitVR="true" deflated="true" encapsulated="false"/>
    </additional-transfer-syntaxes>
...
</config>

Note: Element additional-transfer-syntax can have the following attributes:

  • explicitVR: VR representation, "true" means explicit (default) and "false" means implicit
  • bigEndian: Endianness, "true" is for Big Endian and "false" means Little Endian (default);
  • encapsulated: Encapsulated pixel data ("true", default) or native pixel data ("false")
  • deflated: whether the whole data set is deflated (default is "false").

Testing

The ServerSettingsTest class tests the process of reading the new schema section from the server.xml configuration file into the ServerSettingsManager (inner) settings.

Almeida-a avatar Oct 04 '21 12:10 Almeida-a

Thank you for your feedback! The comments were addressed in the latest 2 commits and are ready for more testing and reviews.

Almeida-a avatar Oct 07 '21 12:10 Almeida-a

Ready again for another review whenever possible.

Almeida-a avatar Oct 08 '21 16:10 Almeida-a

Hello @Almeida-a, we are pushing towards a new patch release 3.0.3. If all goes well, we can admit this one for version 3.1.0 at a later time.

No rush, but when you can, please resolve the current git conflicts.

Enet4 avatar Nov 26 '21 17:11 Enet4

I'm ready to resolve the conflicts, however I don't understand what is the one at LegacyServerSettings class, there seems to be no differences in my view.

Regarding ServerSettingsTest, my intention is to keep the new line from new/issue498 if possible.

Edit: I identified the differences with another tool. I will carry out the merge now.

Almeida-a avatar Dec 16 '21 16:12 Almeida-a

@Almeida-a thanks for your contributions. it's our intention to bring your PR on-board. I see that you already take some of the suggestions. Could you also resolve the pending conflits? Thanks!

bastiao avatar Oct 18 '22 19:10 bastiao

Thank you! The conflicts are resolved.

Almeida-a avatar Oct 19 '22 10:10 Almeida-a

my perception is that we should merge it. It does not seems to bring regressions. However, while testing also detected a few issues on migrations and webapp ui that should be fixe in a different PR.

bastiao avatar Oct 23 '22 23:10 bastiao