# HG changeset patch
# User Alain Mazy <am@osimis.io>
# Date 1700755156 -3600
# Node ID ac68a4383e51047f5406eb94a36ae93714efe424
# Parent  99fa307438e1bbae5751b49b0e91785cd9f3df64
improved C-Store negotiation and logging

diff -r 99fa307438e1 -r ac68a4383e51 NEWS
--- a/NEWS	Wed Nov 22 21:18:16 2023 +0100
+++ b/NEWS	Thu Nov 23 16:59:16 2023 +0100
@@ -63,8 +63,11 @@
   https://discourse.orthanc-server.org/t/c-find-fails-on-unknown-specific-character-set-iso-2022-ir-6-iso-2022-ir-100/3947
 * When exporting a study archive, make sure to use the PatientName from the study and not from the patient
   in case of PatientID collision.
-* Added a new 'Telemis' manufacturer for DicomModalities.  This forces a new DICOM renegociation in case the
-  SopClassUid+TransferSyntax was rejected in the current association.
+* DICOM C-Store:
+  - Avoid some unneccessary renegotiation of DICOM association.
+  - Force renegotiation in case no presentation context were accepted in previous association (we have
+    observed PACS that were not consistent in the accepted presentation contexts)
+  - Improved logging
 * Upgraded dependencies for static builds:
   - boost 1.83.0
 * Upgraded minizip library to stay away from CVE-2023-45853 although Orthanc is likely not affected since zip
diff -r 99fa307438e1 -r ac68a4383e51 OrthancFramework/Sources/DicomNetworking/DicomAssociation.cpp
--- a/OrthancFramework/Sources/DicomNetworking/DicomAssociation.cpp	Wed Nov 22 21:18:16 2023 +0100
+++ b/OrthancFramework/Sources/DicomNetworking/DicomAssociation.cpp	Thu Nov 23 16:59:16 2023 +0100
@@ -108,6 +108,8 @@
     
   void DicomAssociation::CloseInternal()
   {
+    CLOG(INFO, DICOM) << "Closing DICOM association";
+
 #if ORTHANC_ENABLE_SSL == 1
     tls_.reset(NULL);  // Transport layer must be destroyed before the association itself
 #endif
@@ -393,8 +395,10 @@
       LST_Position(l, (LST_NODE*)pc);
       while (pc)
       {
-        if (pc->result == ASC_P_ACCEPTANCE)
+        if (pc->result == ASC_P_ACCEPTANCE && strlen(pc->abstractSyntax) > 0)
         {
+          CLOG(TRACE, DICOM) << "DicomAssociation::Open, adding SOPClassUID " << pc->abstractSyntax << " - TS " << pc->acceptedTransferSyntax << " - PC ID " << boost::lexical_cast<std::string>(static_cast<int>(pc->presentationContextID));
+
           DicomTransferSyntax transferSyntax;
           if (LookupTransferSyntax(transferSyntax, pc->acceptedTransferSyntax))
           {
diff -r 99fa307438e1 -r ac68a4383e51 OrthancFramework/Sources/DicomNetworking/DicomStoreUserConnection.cpp
--- a/OrthancFramework/Sources/DicomNetworking/DicomStoreUserConnection.cpp	Wed Nov 22 21:18:16 2023 +0100
+++ b/OrthancFramework/Sources/DicomNetworking/DicomStoreUserConnection.cpp	Thu Nov 23 16:59:16 2023 +0100
@@ -253,9 +253,7 @@
     const std::string& sopClassUid,
     DicomTransferSyntax transferSyntax,
     bool hasPreferred,
-    DicomTransferSyntax preferred,
-    bool alwaysRenegotiate,
-    bool enableLogs)
+    DicomTransferSyntax preferred)
   {
     /**
      * Step 1: Check whether this presentation context is already
@@ -264,34 +262,33 @@
 
     if (LookupPresentationContext(presentationContextId, sopClassUid, transferSyntax))
     {
+      CLOG(INFO, DICOM) << "Found an accepted presentation context for SOPClassUID " << sopClassUid << " and transfer syntax " << GetTransferSyntaxUid(transferSyntax);
       return true;
     }
 
     // The association must be re-negotiated
     if (association_->IsOpen())
     {
-      if (enableLogs)
-      {
-        CLOG(INFO, DICOM) << "Re-negotiating DICOM association with "
-                          << parameters_.GetRemoteModality().GetApplicationEntityTitle()
-                          << " for SOPClassUID " << sopClassUid;
-      }
+      CLOG(INFO, DICOM) << "No accepted presentation context found, re-negotiating DICOM association with "
+                        << parameters_.GetRemoteModality().GetApplicationEntityTitle()
+                        << " for SOPClassUID " << sopClassUid << " TransferSyntax =" << GetTransferSyntaxUid(transferSyntax);
 
-      // Don't renegociate if we know that the remote modality was
+      // Check if we know that the remote modality was
       // already proposed this individual transfer syntax (**)
-      if (!alwaysRenegotiate && 
-          proposedOriginalClasses_.find(std::make_pair(sopClassUid, transferSyntax)) != proposedOriginalClasses_.end())
+      if (proposedOriginalClasses_.find(std::make_pair(sopClassUid, transferSyntax)) != proposedOriginalClasses_.end())
       {
-        if (enableLogs)
-        {
-          CLOG(INFO, DICOM) << "The remote modality has already rejected SOP class UID \""
-                            << sopClassUid << "\" with transfer syntax \""
-                            << GetTransferSyntaxUid(transferSyntax) << "\", don't renegotiate";
-        }
-
-        return false;
+        CLOG(INFO, DICOM) << "The remote modality has already rejected SOP class UID \""
+                          << sopClassUid << "\" with transfer syntax \""
+                          << GetTransferSyntaxUid(transferSyntax) << "\", but we will renegotiate anyway";
+        // always renegotiating since 1.12.2 // return false;
       }
     }
+    else
+    {
+      CLOG(INFO, DICOM) << "Negotiating DICOM association with "
+                        << parameters_.GetRemoteModality().GetApplicationEntityTitle()
+                        << " for SOPClassUID " << sopClassUid << " TransferSyntax =" << GetTransferSyntaxUid(transferSyntax);
+    }
 
     association_->ClearPresentationContexts();
     proposedOriginalClasses_.clear();
@@ -379,15 +376,16 @@
                                        DcmFileFormat& dicom,
                                        bool hasMoveOriginator,
                                        const std::string& moveOriginatorAET,
-                                       uint16_t moveOriginatorID,
-                                       bool alwaysRenegotiate)
+                                       uint16_t moveOriginatorID)
   {
     DicomTransferSyntax transferSyntax;
     LookupParameters(sopClassUid, sopInstanceUid, transferSyntax, dicom);
 
+    LOG(INFO) << "Performing C-Store on instance of SOPClassUID '" << sopClassUid << "'";
+
     uint8_t presID;
     if (!NegotiatePresentationContext(presID, sopClassUid, transferSyntax, proposeUncompressedSyntaxes_,
-                                      DicomTransferSyntax_LittleEndianExplicit, alwaysRenegotiate, true))
+                                      DicomTransferSyntax_LittleEndianExplicit))
     {
       throw OrthancException(ErrorCode_NetworkProtocol,
                              "No valid presentation context was negotiated for "
@@ -469,8 +467,7 @@
                                        size_t size,
                                        bool hasMoveOriginator,
                                        const std::string& moveOriginatorAET,
-                                       uint16_t moveOriginatorID,
-                                       bool alwaysRenegotiate)
+                                       uint16_t moveOriginatorID)
   {
     std::unique_ptr<DcmFileFormat> dicom(
       FromDcmtkBridge::LoadFromMemoryBuffer(buffer, size));
@@ -480,7 +477,7 @@
       throw OrthancException(ErrorCode_InternalError);
     }
     
-    Store(sopClassUid, sopInstanceUid, *dicom, hasMoveOriginator, moveOriginatorAET, moveOriginatorID, alwaysRenegotiate);
+    Store(sopClassUid, sopInstanceUid, *dicom, hasMoveOriginator, moveOriginatorAET, moveOriginatorID);
   }
 
 
@@ -489,18 +486,20 @@
                                                    const std::string& sopClassUid,
                                                    DicomTransferSyntax sourceSyntax,
                                                    bool hasPreferred,
-                                                   DicomTransferSyntax preferred,
-                                                   bool alwaysRenegotiate)
+                                                   DicomTransferSyntax preferred)
   {
     acceptedSyntaxes.clear();
+    std::map<DicomTransferSyntax, uint8_t> contexts;
 
     // Make sure a negotiation has already occurred for this transfer
-    // syntax. We don't use the return code: Transcoding is possible
-    // even if the "sourceSyntax" is not supported.
-    uint8_t presID;
-    NegotiatePresentationContext(presID, sopClassUid, sourceSyntax, hasPreferred, preferred, alwaysRenegotiate, false);
+    // syntax if we have not negotiated yet. 
+    // We don't use the return code: Transcoding is possible even if the "sourceSyntax" is not supported.
+    if (!association_->IsOpen() || !association_->LookupAcceptedPresentationContext(contexts, sopClassUid))
+    {
+      uint8_t presID;
+      NegotiatePresentationContext(presID, sopClassUid, sourceSyntax, hasPreferred, preferred);
+    }
 
-    std::map<DicomTransferSyntax, uint8_t> contexts;
     if (association_->LookupAcceptedPresentationContext(contexts, sopClassUid))
     {
       for (std::map<DicomTransferSyntax, uint8_t>::const_iterator
@@ -522,8 +521,7 @@
                                            DicomTransferSyntax preferredTransferSyntax,
                                            bool hasMoveOriginator,
                                            const std::string& moveOriginatorAET,
-                                           uint16_t moveOriginatorID,
-                                           bool alwaysRenegotiate)
+                                           uint16_t moveOriginatorID)
   {
     std::unique_ptr<DcmFileFormat> dicom(FromDcmtkBridge::LoadFromMemoryBuffer(buffer, size));
     if (dicom.get() == NULL ||
@@ -536,19 +534,19 @@
     LookupParameters(sopClassUid, sopInstanceUid, sourceSyntax, *dicom);
 
     std::set<DicomTransferSyntax> accepted;
-    LookupTranscoding(accepted, sopClassUid, sourceSyntax, true, preferredTransferSyntax, alwaysRenegotiate);
+    LookupTranscoding(accepted, sopClassUid, sourceSyntax, true, preferredTransferSyntax);
 
     if (accepted.size() == 0)
     {
-      throw OrthancException(ErrorCode_NoPresentationContext, "Cannot store instance of SOPClassUID " + 
-                              sopClassUid + ", the destination has not accepted any TransferSyntax for this SOPClassUID.");
+      throw OrthancException(ErrorCode_NoPresentationContext, "Cannot C-Store an instance of SOPClassUID " + 
+                             sopClassUid + ", the destination has not accepted any TransferSyntax for this SOPClassUID.");
     }
 
     if (accepted.find(sourceSyntax) != accepted.end())
     {
       // No need for transcoding
       Store(sopClassUid, sopInstanceUid, *dicom,
-            hasMoveOriginator, moveOriginatorAET, moveOriginatorID, alwaysRenegotiate);
+            hasMoveOriginator, moveOriginatorAET, moveOriginatorID);
     }
     else
     {
@@ -564,6 +562,8 @@
       bool isDestructiveCompressionAllowed = false;
       std::set<DicomTransferSyntax> attemptedSyntaxes;
 
+      LOG(INFO) << "Transcoding is required to C-Store an instance of SOPClassUID '" << sopClassUid << "', preferredTransferSyntax is " << GetTransferSyntaxUid(preferredTransferSyntax);
+
       if (accepted.find(preferredTransferSyntax) != accepted.end())
       {
         // New in Orthanc 1.9.0: The preferred transfer syntax is
@@ -637,7 +637,7 @@
         else
         {
           Store(sopClassUid, sopInstanceUid, transcoded.GetParsed(),
-                hasMoveOriginator, moveOriginatorAET, moveOriginatorID, alwaysRenegotiate);
+                hasMoveOriginator, moveOriginatorAET, moveOriginatorID);
         }
       }
       else
@@ -667,11 +667,10 @@
                                            size_t size,
                                            bool hasMoveOriginator,
                                            const std::string& moveOriginatorAET,
-                                           uint16_t moveOriginatorID,
-                                           bool alwaysRenegotiate)
+                                           uint16_t moveOriginatorID)
   {
     Transcode(sopClassUid, sopInstanceUid, transcoder, buffer, size, DicomTransferSyntax_LittleEndianExplicit,
-              hasMoveOriginator, moveOriginatorAET, moveOriginatorID, alwaysRenegotiate);
+              hasMoveOriginator, moveOriginatorAET, moveOriginatorID);
   }
 #endif
 }
diff -r 99fa307438e1 -r ac68a4383e51 OrthancFramework/Sources/DicomNetworking/DicomStoreUserConnection.h
--- a/OrthancFramework/Sources/DicomNetworking/DicomStoreUserConnection.h	Wed Nov 22 21:18:16 2023 +0100
+++ b/OrthancFramework/Sources/DicomNetworking/DicomStoreUserConnection.h	Thu Nov 23 16:59:16 2023 +0100
@@ -94,17 +94,14 @@
                                       const std::string& sopClassUid,
                                       DicomTransferSyntax transferSyntax,
                                       bool hasPreferred,
-                                      DicomTransferSyntax preferred,
-                                      bool alwaysRenegotiate,
-                                      bool enableLogs);
+                                      DicomTransferSyntax preferred);
 
 #if ORTHANC_ENABLE_DCMTK_TRANSCODING == 1
     void LookupTranscoding(std::set<DicomTransferSyntax>& acceptedSyntaxes,
                            const std::string& sopClassUid,
                            DicomTransferSyntax sourceSyntax,
                            bool hasPreferred,
-                           DicomTransferSyntax preferred,
-                           bool alwaysRenegotiate);
+                           DicomTransferSyntax preferred);
 #endif
 
   public:
@@ -132,8 +129,7 @@
                DcmFileFormat& dicom,
                bool hasMoveOriginator,
                const std::string& moveOriginatorAET,
-               uint16_t moveOriginatorID,
-               bool alwaysRenegotiate);
+               uint16_t moveOriginatorID);
 
     void Store(std::string& sopClassUid,
                std::string& sopInstanceUid,
@@ -141,8 +137,7 @@
                size_t size,
                bool hasMoveOriginator,
                const std::string& moveOriginatorAET,
-               uint16_t moveOriginatorID,
-               bool alwaysRenegotiate);
+               uint16_t moveOriginatorID);
 
     void LookupParameters(std::string& sopClassUid,
                           std::string& sopInstanceUid,
@@ -158,8 +153,7 @@
                    DicomTransferSyntax preferredTransferSyntax,
                    bool hasMoveOriginator,
                    const std::string& moveOriginatorAET,
-                   uint16_t moveOriginatorID,
-                   bool alwaysRenegotiate);
+                   uint16_t moveOriginatorID);
 #endif
     
 #if ORTHANC_ENABLE_DCMTK_TRANSCODING == 1
@@ -170,8 +164,7 @@
                    size_t size,
                    bool hasMoveOriginator,
                    const std::string& moveOriginatorAET,
-                   uint16_t moveOriginatorID,
-                   bool alwaysRenegotiate);
+                   uint16_t moveOriginatorID);
 #endif
   };
 }
diff -r 99fa307438e1 -r ac68a4383e51 OrthancFramework/Sources/DicomNetworking/RemoteModalityParameters.cpp
--- a/OrthancFramework/Sources/DicomNetworking/RemoteModalityParameters.cpp	Wed Nov 22 21:18:16 2023 +0100
+++ b/OrthancFramework/Sources/DicomNetworking/RemoteModalityParameters.cpp	Thu Nov 23 16:59:16 2023 +0100
@@ -186,12 +186,6 @@
     manufacturer_ = StringToModalityManufacturer(manufacturer);
   }
 
-  bool RemoteModalityParameters::IsAlwaysRenegotiate() const
-  {
-    return manufacturer_ == ModalityManufacturer_Telemis;
-  }
-
-
   void RemoteModalityParameters::UnserializeArray(const Json::Value& serialized)
   {
     assert(serialized.type() == Json::arrayValue);
diff -r 99fa307438e1 -r ac68a4383e51 OrthancFramework/Sources/DicomNetworking/RemoteModalityParameters.h
--- a/OrthancFramework/Sources/DicomNetworking/RemoteModalityParameters.h	Wed Nov 22 21:18:16 2023 +0100
+++ b/OrthancFramework/Sources/DicomNetworking/RemoteModalityParameters.h	Thu Nov 23 16:59:16 2023 +0100
@@ -85,8 +85,6 @@
 
     void SetManufacturer(const std::string& manufacturer);
 
-    bool IsAlwaysRenegotiate() const;
-
     bool IsRequestAllowed(DicomRequestType type) const;
 
     void SetRequestAllowed(DicomRequestType type,
diff -r 99fa307438e1 -r ac68a4383e51 OrthancFramework/Sources/Enumerations.cpp
--- a/OrthancFramework/Sources/Enumerations.cpp	Wed Nov 22 21:18:16 2023 +0100
+++ b/OrthancFramework/Sources/Enumerations.cpp	Thu Nov 23 16:59:16 2023 +0100
@@ -807,9 +807,6 @@
       case ModalityManufacturer_GE:
         return "GE";
       
-      case ModalityManufacturer_Telemis:
-        return "Telemis";
-
       default:
         throw OrthancException(ErrorCode_ParameterOutOfRange);
     }
@@ -1589,10 +1586,6 @@
     {
       return ModalityManufacturer_GE;
     }
-    else if (manufacturer == "Telemis")
-    {
-      return ModalityManufacturer_Telemis;
-    }
     else if (manufacturer == "AgfaImpax" ||
              manufacturer == "SyngoVia")
     {
diff -r 99fa307438e1 -r ac68a4383e51 OrthancFramework/Sources/Enumerations.h
--- a/OrthancFramework/Sources/Enumerations.h	Wed Nov 22 21:18:16 2023 +0100
+++ b/OrthancFramework/Sources/Enumerations.h	Thu Nov 23 16:59:16 2023 +0100
@@ -660,8 +660,7 @@
     ModalityManufacturer_GenericNoWildcardInDates,
     ModalityManufacturer_GenericNoUniversalWildcard,
     ModalityManufacturer_Vitrea,
-    ModalityManufacturer_GE,
-    ModalityManufacturer_Telemis
+    ModalityManufacturer_GE
   };
 
   enum DicomRequestType
diff -r 99fa307438e1 -r ac68a4383e51 OrthancFramework/UnitTestsSources/FromDcmtkTests.cpp
--- a/OrthancFramework/UnitTestsSources/FromDcmtkTests.cpp	Wed Nov 22 21:18:16 2023 +0100
+++ b/OrthancFramework/UnitTestsSources/FromDcmtkTests.cpp	Thu Nov 23 16:59:16 2023 +0100
@@ -3567,7 +3567,7 @@
         try
         {
           scu.Transcode(c, k, transcoder, source.c_str(), source.size(),
-                        DicomTransferSyntax_LittleEndianExplicit, false, "", 0, false);
+                        DicomTransferSyntax_LittleEndianExplicit, false, "", 0);
         }
         catch (OrthancException& e)
         {
diff -r 99fa307438e1 -r ac68a4383e51 OrthancServer/Sources/OrthancRestApi/OrthancRestModalities.cpp
--- a/OrthancServer/Sources/OrthancRestApi/OrthancRestModalities.cpp	Wed Nov 22 21:18:16 2023 +0100
+++ b/OrthancServer/Sources/OrthancRestApi/OrthancRestModalities.cpp	Thu Nov 23 16:59:16 2023 +0100
@@ -1522,8 +1522,7 @@
     connection.Store(sopClassUid, sopInstanceUid, call.GetBodyData(),
                      call.GetBodySize(), 
                      false /* Not a C-MOVE */, 
-                     "", 0, 
-                     false /* AlwaysRenegotiate: no need to renegotiate since there is only a single file*/);
+                     "", 0);
 
     Json::Value answer = Json::objectValue;
     answer[SOP_CLASS_UID] = sopClassUid;
diff -r 99fa307438e1 -r ac68a4383e51 OrthancServer/Sources/ServerContext.cpp
--- a/OrthancServer/Sources/ServerContext.cpp	Wed Nov 22 21:18:16 2023 +0100
+++ b/OrthancServer/Sources/ServerContext.cpp	Thu Nov 23 16:59:16 2023 +0100
@@ -1958,12 +1958,12 @@
         !modality.IsTranscodingAllowed())
     {
       connection.Store(sopClassUid, sopInstanceUid, data, dicom.size(),
-                       hasMoveOriginator, moveOriginatorAet, moveOriginatorId, modality.IsAlwaysRenegotiate());
+                       hasMoveOriginator, moveOriginatorAet, moveOriginatorId);
     }
     else
     {
       connection.Transcode(sopClassUid, sopInstanceUid, *this, data, dicom.size(), preferredTransferSyntax_,
-                           hasMoveOriginator, moveOriginatorAet, moveOriginatorId, modality.IsAlwaysRenegotiate());
+                           hasMoveOriginator, moveOriginatorAet, moveOriginatorId);
     }
   }