Do not add duplicate presentation contexts

[amoerie]

Checklist

• The pull request branch is in sync with latest commit on the fo-dicom/development branch
• I have updated API documentation
• I have included unit tests
• I have updated the change log
• I am listed in the CONTRIBUTORS file

Changes proposed in this pull request:

• Do not add a presentation context if it is a complete, identical copy of an existing one

[codecov]

Codecov Report

Patch coverage: 89.43% and project coverage change: -0.04% ⚠️

Comparison is base (7008542) 79.83% compared to head (e4066bd) 79.80%.
Report is 11 commits behind head on development.

Additional details and impacted files

@@               Coverage Diff               @@
##           development    #1596      +/-   ##
===============================================
- Coverage        79.83%   79.80%   -0.04%     
===============================================
  Files              273      274       +1     
  Lines            25053    25143      +90     
===============================================
+ Hits             20002    20066      +64     
- Misses            5051     5077      +26     

Files Changed	Coverage Δ
FO-DICOM.Core/IO/Buffer/MemoryByteBuffer.cs	81.25% <ø> (ø)
FO-DICOM.Core/Network/Client/DicomClient.cs	96.96% <ø> (ø)
FO-DICOM.Core/Network/Client/DicomClientOptions.cs	100.00% <ø> (ø)
FO-DICOM.Core/Imaging/DicomPixelData.cs	77.08% <61.53%> (+0.36%)	⬆️
FO-DICOM.Core/Setup.cs	72.00% <75.00%> (-21.19%)	⬇️
...Core/Network/DicomPresentationContextCollection.cs	72.89% <78.94%> (+2.89%)	⬆️
FO-DICOM.Core/DicomEncoding.cs	90.50% <100.00%> (+0.19%)	⬆️
FO-DICOM.Core/DicomTransferSyntax.cs	98.71% <100.00%> (+0.01%)	⬆️
FO-DICOM.Core/IO/FileByteSource.cs	88.63% <100.00%> (+1.62%)	⬆️
FO-DICOM.Core/IO/StreamByteSource.cs	89.04% <100.00%> (+1.94%)	⬆️
... and 5 more

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[gofal]

This is a PR without an issue. So generally asked: what is the reason or issue for this change?
take the unittest as example. You are adding 5 presentation contexts to the association request. with this PR this is optimized to 4 presentation contexts. ok, thats fine. But the downside is now, that you have a n^2 runtime deep coparision check for each presentation context you add. so there are a lot of checks included now in every assocation for all clients in all circumstances, to reduce the number of sent bytes in some certain cases?

maybe some other solution, like an extra method "RemoveDuplicatePresentationContexts", that the user can call and that then checks all the presentation contexts before sending the request?

[gofal]

in case the idea of this PR would be, to solve the issue, that the maximum number of Presentation Contexts is exceeded but could be solved by reducing duplicate, then you could add a check before sending the AssocationRequest if the number is higher than the allowed. If this is true, then remove duplicates.

[amoerie]

This is a PR without an issue. So generally asked: what is the reason or issue for this change? take the unittest as example. You are adding 5 presentation contexts to the association request. with this PR this is optimized to 4 presentation contexts. ok, thats fine.

Sorry if this was missing a bit of context! I'm going through all our custom changes in our fork and - if they are not specific to our case - share them here.
This particular change was introduced because we have a configuration system related to transfer syntaxes per SOP Class UID, and it is possible to misconfigure it so that duplicate presentation contexts are the end result. Even though this is not necessarily a big problem, we did see PACS systems in the wild misbehave due to these duplicate presentation contexts.
On top of improving our configuration UI, we also added this safety net so that you can never have duplicate presentation contexts.

But the downside is now, that you have a n^2 runtime deep coparision check for each presentation context you add. so there are a lot of checks included now in every assocation for all clients in all circumstances, to reduce the number of sent bytes in some certain cases?
maybe some other solution, like an extra method "RemoveDuplicatePresentationContexts", that the user can call and that then checks all the presentation contexts before sending the request?

It's true that the worst case scenario now introduces a O(n²) algorithm, but this worst case scenario only occurs if it is always the same SOP Class UID with different transfer syntaxes. In reality, what we tend to see, is that the number of presentation contexts in an association and the number of transfer syntaxes per presentation context is quite limited. (<100 presentation contexts and < 5 transfer syntaxes per context).

Anyway, if you believe this feature is not worth the runtime cost, we can also always reject it. I personally don't see a lot of value in a dedicated RemoveDuplicatePresentationContexts method, because people will simply never call it. In that case, I'd rather drop the duplicate detection altogether.

[amoerie]

But the downside is now, that you have a n^2 runtime deep coparision check for each presentation context you add. so there are a lot of checks included now in every assocation for all clients in all circumstances, to reduce the number of sent bytes in some certain cases?
maybe some other solution, like an extra method "RemoveDuplicatePresentationContexts", that the user can call and that then checks all the presentation contexts before sending the request?

To address your performance concerns (which were valid, even though I still believe the real world impact would have been small), I have switched the implementation to a HashSet based one.

We now keep a hash set of existing presentation contexts, and only add a new presentation context if there is no collision with the hash set. Instead of a n^2 loop when adding a new presentation context, we now calculate the hash code of this presentation context and look it up in the hashset, which should be a O(1) operation.
We do consume a little bit of extra memory for the hash set, but this collection only contains references to the presentation contexts, so the overhead should be extremely tiny.

[mrbean-bremen]

The other possibility would be to remove an existing PC and add it to the end, which would change the order - but I think this is not something the user would think about, so it is most probably ok as it is.

[amoerie]

If the user wants to add it at the end, he can always the existing one first. I think the current implementation makes the most sense now..