# HG changeset patch
# User Sebastien Jodogne <s.jodogne@gmail.com>
# Date 1481279044 -3600
# Node ID 6dc3bdb4088bdc37fca95d6ee85311a381000fdf
# Parent  27106f7e37593d081fcd9b986497b6c337fa505e
Fix handling of encodings in C-FIND for worklists

diff -r 27106f7e3759 -r 6dc3bdb4088b NEWS
--- a/NEWS	Thu Dec 08 16:29:34 2016 +0100
+++ b/NEWS	Fri Dec 09 11:24:04 2016 +0100
@@ -30,7 +30,7 @@
 Maintenance
 -----------
 
-* Fix handling of encodings in C-FIND requests
+* Fix handling of encodings in C-FIND requests (including for worklists)
 * Use of DCMTK 3.6.1 dictionary of private tags in standalone builds
 * Avoid hard crash if not enough memory (handling of std::bad_alloc)
 * Improved robustness of Orthanc Explorer wrt. query/retrieve (maybe fix issue 24)
diff -r 27106f7e3759 -r 6dc3bdb4088b OrthancServer/DicomProtocol/DicomFindAnswers.cpp
--- a/OrthancServer/DicomProtocol/DicomFindAnswers.cpp	Thu Dec 08 16:29:34 2016 +0100
+++ b/OrthancServer/DicomProtocol/DicomFindAnswers.cpp	Fri Dec 09 11:24:04 2016 +0100
@@ -33,8 +33,8 @@
 #include "../PrecompiledHeadersServer.h"
 #include "DicomFindAnswers.h"
 
+#include "../OrthancInitialization.h"
 #include "../FromDcmtkBridge.h"
-#include "../ToDcmtkBridge.h"
 #include "../../Core/OrthancException.h"
 
 #include <memory>
@@ -44,72 +44,42 @@
 
 namespace Orthanc
 {
-  class DicomFindAnswers::Answer : public boost::noncopyable
+  void DicomFindAnswers::AddAnswerInternal(ParsedDicomFile* answer)
   {
-  private:
-    ParsedDicomFile* dicom_;
-    DicomMap*        map_;
+    std::auto_ptr<ParsedDicomFile> protection(answer);
 
-    void CleanupDicom(bool isWorklist)
+    if (isWorklist_)
     {
-      if (isWorklist &&
-          dicom_ != NULL)
-      {
-        // These lines are necessary when serving worklists, otherwise
-        // Orthanc does not behave as "wlmscpfs"
-        dicom_->Remove(DICOM_TAG_MEDIA_STORAGE_SOP_INSTANCE_UID);
-        dicom_->Remove(DICOM_TAG_SOP_INSTANCE_UID);
-      }
-    }
-
-  public:
-    Answer(bool isWorklist,
-           ParsedDicomFile& dicom) : 
-      dicom_(dicom.Clone()),
-      map_(NULL)
-    {
-      CleanupDicom(isWorklist);
+      // These lines are necessary when serving worklists, otherwise
+      // Orthanc does not behave as "wlmscpfs"
+      protection->Remove(DICOM_TAG_MEDIA_STORAGE_SOP_INSTANCE_UID);
+      protection->Remove(DICOM_TAG_SOP_INSTANCE_UID);
     }
 
-    Answer(bool isWorklist,
-           const void* dicom,
-           size_t size) : 
-      dicom_(new ParsedDicomFile(dicom, size)),
-      map_(NULL)
+    protection->ChangeEncoding(encoding_);
+
+    answers_.push_back(protection.release());
+  }
+
+
+  DicomFindAnswers::DicomFindAnswers(bool isWorklist) : 
+    encoding_(Configuration::GetDefaultEncoding()),
+    isWorklist_(isWorklist),
+    complete_(true)
+  {
+  }
+
+
+  void DicomFindAnswers::SetEncoding(Encoding encoding)
+  {
+    for (size_t i = 0; i < answers_.size(); i++)
     {
-      CleanupDicom(isWorklist);
-    }
-
-    Answer(const DicomMap& map) : 
-      dicom_(NULL),
-      map_(map.Clone())
-    {
+      assert(answers_[i] != NULL);
+      answers_[i]->ChangeEncoding(encoding);
     }
 
-    ~Answer()
-    {
-      if (dicom_ != NULL)
-      {
-        delete dicom_;
-      }
-
-      if (map_ != NULL)
-      {
-        delete map_;
-      }
-    }
-
-    ParsedDicomFile& GetDicomFile()
-    {
-      if (dicom_ == NULL)
-      {
-        assert(map_ != NULL);
-        dicom_ = new ParsedDicomFile(*map_);
-      }
-
-      return *dicom_;
-    }
-  };
+    encoding_ = encoding;
+  }
 
 
   void DicomFindAnswers::SetWorklist(bool isWorklist)
@@ -149,28 +119,27 @@
 
   void DicomFindAnswers::Add(const DicomMap& map)
   {
-    answers_.push_back(new Answer(map));
+    AddAnswerInternal(new ParsedDicomFile(map, encoding_));
   }
 
 
   void DicomFindAnswers::Add(ParsedDicomFile& dicom)
   {
-    answers_.push_back(new Answer(isWorklist_, dicom));
+    AddAnswerInternal(dicom.Clone());
   }
 
-
   void DicomFindAnswers::Add(const void* dicom,
                              size_t size)
   {
-    answers_.push_back(new Answer(isWorklist_, dicom, size));
+    AddAnswerInternal(new ParsedDicomFile(dicom, size));
   }
 
 
-  DicomFindAnswers::Answer& DicomFindAnswers::GetAnswerInternal(size_t index) const
+  ParsedDicomFile& DicomFindAnswers::GetAnswer(size_t index) const
   {
     if (index < answers_.size())
     {
-      return *answers_.at(index);
+      return *answers_[index];
     }
     else
     {
@@ -179,12 +148,6 @@
   }
 
 
-  ParsedDicomFile& DicomFindAnswers::GetAnswer(size_t index) const
-  {
-    return GetAnswerInternal(index).GetDicomFile();
-  }
-
-
   DcmDataset* DicomFindAnswers::ExtractDcmDataset(size_t index) const
   {
     return new DcmDataset(*GetAnswer(index).GetDcmtkObject().getDataset());
diff -r 27106f7e3759 -r 6dc3bdb4088b OrthancServer/DicomProtocol/DicomFindAnswers.h
--- a/OrthancServer/DicomProtocol/DicomFindAnswers.h	Thu Dec 08 16:29:34 2016 +0100
+++ b/OrthancServer/DicomProtocol/DicomFindAnswers.h	Fri Dec 09 11:24:04 2016 +0100
@@ -39,26 +39,28 @@
   class DicomFindAnswers : public boost::noncopyable
   {
   private:
-    class Answer;
+    Encoding                      encoding_;
+    bool                          isWorklist_;
+    std::vector<ParsedDicomFile*> answers_;
+    bool                          complete_;
 
-    bool                 isWorklist_;
-    std::vector<Answer*> answers_;
-    bool                 complete_;
-
-    Answer& GetAnswerInternal(size_t index) const;
+    void AddAnswerInternal(ParsedDicomFile* answer);
 
   public:
-    DicomFindAnswers(bool isWorklist) : 
-      isWorklist_(isWorklist),
-      complete_(true)
-    {
-    }
+    DicomFindAnswers(bool isWorklist);
 
     ~DicomFindAnswers()
     {
       Clear();
     }
 
+    Encoding GetEncoding() const
+    {
+      return encoding_;
+    }
+
+    void SetEncoding(Encoding encoding);
+
     void SetWorklist(bool isWorklist);
 
     bool IsWorklist() const
diff -r 27106f7e3759 -r 6dc3bdb4088b OrthancServer/FromDcmtkBridge.cpp
--- a/OrthancServer/FromDcmtkBridge.cpp	Thu Dec 08 16:29:34 2016 +0100
+++ b/OrthancServer/FromDcmtkBridge.cpp	Fri Dec 09 11:24:04 2016 +0100
@@ -1897,4 +1897,50 @@
       target.SetValue(ParseTag(members[i]), value.asString(), false);
     }
   }
+
+
+  void FromDcmtkBridge::ChangeStringEncoding(DcmItem& dataset,
+                                             Encoding source,
+                                             Encoding target)
+  {
+    // Recursive exploration of a dataset to change the encoding of
+    // each string-like element
+
+    if (source == target)
+    {
+      return;
+    }
+
+    for (unsigned long i = 0; i < dataset.card(); i++)
+    {
+      DcmElement* element = dataset.getElement(i);
+      if (element)
+      {
+        if (element->isLeaf())
+        {
+          char *c = NULL;
+          if (element->isaString() &&
+              element->getString(c).good() && 
+              c != NULL)
+          {
+            std::string a = Toolbox::ConvertToUtf8(c, source);
+            std::string b = Toolbox::ConvertFromUtf8(a, target);
+            element->putString(b.c_str());
+          }
+        }
+        else
+        {
+          // "All subclasses of DcmElement except for DcmSequenceOfItems
+          // are leaf nodes, while DcmSequenceOfItems, DcmItem, DcmDataset
+          // etc. are not." The following dynamic_cast is thus OK.
+          DcmSequenceOfItems& sequence = dynamic_cast<DcmSequenceOfItems&>(*element);
+
+          for (unsigned long j = 0; j < sequence.card(); j++)
+          {
+            ChangeStringEncoding(*sequence.getItem(j), source, target);
+          }
+        }
+      }
+    }
+  }
 }
diff -r 27106f7e3759 -r 6dc3bdb4088b OrthancServer/FromDcmtkBridge.h
--- a/OrthancServer/FromDcmtkBridge.h	Thu Dec 08 16:29:34 2016 +0100
+++ b/OrthancServer/FromDcmtkBridge.h	Fri Dec 09 11:24:04 2016 +0100
@@ -88,6 +88,10 @@
                                    unsigned int maxStringLength,
                                    Encoding defaultEncoding);
 
+    static void ChangeStringEncoding(DcmItem& dataset,
+                                     Encoding source,
+                                     Encoding target);
+
   public:
     static void InitializeDictionary(bool loadPrivateDictionary);
 
diff -r 27106f7e3759 -r 6dc3bdb4088b OrthancServer/ParsedDicomFile.cpp
--- a/OrthancServer/ParsedDicomFile.cpp	Thu Dec 08 16:29:34 2016 +0100
+++ b/OrthancServer/ParsedDicomFile.cpp	Fri Dec 09 11:24:04 2016 +0100
@@ -1384,4 +1384,16 @@
   {
     return DicomFrameIndex::GetFramesCount(*pimpl_->file_);
   }
+
+
+  void ParsedDicomFile::ChangeEncoding(Encoding target)
+  {
+    Encoding source = GetEncoding();
+
+    if (source != target)  // Avoid unnecessary conversion
+    {
+      ReplacePlainString(DICOM_TAG_SPECIFIC_CHARACTER_SET, GetDicomSpecificCharacterSet(target));
+      FromDcmtkBridge::ChangeStringEncoding(*pimpl_->file_->getDataset(), source, target);
+    }
+  }
 }
diff -r 27106f7e3759 -r 6dc3bdb4088b OrthancServer/ParsedDicomFile.h
--- a/OrthancServer/ParsedDicomFile.h	Thu Dec 08 16:29:34 2016 +0100
+++ b/OrthancServer/ParsedDicomFile.h	Fri Dec 09 11:24:04 2016 +0100
@@ -125,6 +125,7 @@
       RemovePrivateTagsInternal(&toKeep);
     }
 
+    // WARNING: This function handles the decoding of strings to UTF8
     bool GetTagValue(std::string& value,
                      const DicomTag& tag);
 
@@ -143,6 +144,8 @@
 
     Encoding GetEncoding() const;
 
+    // WARNING: This function only sets the encoding, it will not
+    // convert the encoding of the tags. Use "ChangeEncoding()" if need be.
     void SetEncoding(Encoding encoding);
 
     void DatasetToJson(Json::Value& target, 
@@ -173,6 +176,7 @@
 
     static ParsedDicomFile* CreateFromJson(const Json::Value& value,
                                            DicomFromJsonFlags flags);
+
+    void ChangeEncoding(Encoding target);
   };
-
 }
diff -r 27106f7e3759 -r 6dc3bdb4088b Resources/Configuration.json
--- a/Resources/Configuration.json	Thu Dec 08 16:29:34 2016 +0100
+++ b/Resources/Configuration.json	Fri Dec 09 11:24:04 2016 +0100
@@ -88,10 +88,10 @@
 
   // The default encoding that is assumed for DICOM files without
   // "SpecificCharacterSet" DICOM tag, and that is used when answering
-  // C-Find requests. The allowed values are "Ascii", "Utf8",
-  // "Latin1", "Latin2", "Latin3", "Latin4", "Latin5", "Cyrillic",
-  // "Windows1251", "Arabic", "Greek", "Hebrew", "Thai", "Japanese",
-  // and "Chinese".
+  // C-Find requests (including worklists). The allowed values are
+  // "Ascii", "Utf8", "Latin1", "Latin2", "Latin3", "Latin4",
+  // "Latin5", "Cyrillic", "Windows1251", "Arabic", "Greek", "Hebrew",
+  // "Thai", "Japanese", and "Chinese".
   "DefaultEncoding" : "Latin1",
 
   // The transfer syntaxes that are accepted by Orthanc C-Store SCP
diff -r 27106f7e3759 -r 6dc3bdb4088b TODO
--- a/TODO	Thu Dec 08 16:29:34 2016 +0100
+++ b/TODO	Fri Dec 09 11:24:04 2016 +0100
@@ -114,8 +114,8 @@
 
 * Use Semaphore::Locker everywhere (instead of explicit
   Release() and Acquire())
-* Avoid direct calls to FromDcmtkBridge, go through ParsedDicomFile
-  wherever possible
+* Avoid direct calls to FromDcmtkBridge (make most of its 
+  methods private), go through ParsedDicomFile wherever possible
 
 
 =================
diff -r 27106f7e3759 -r 6dc3bdb4088b UnitTestsSources/FromDcmtkTests.cpp
--- a/UnitTestsSources/FromDcmtkTests.cpp	Thu Dec 08 16:29:34 2016 +0100
+++ b/UnitTestsSources/FromDcmtkTests.cpp	Fri Dec 09 11:24:04 2016 +0100
@@ -1169,3 +1169,49 @@
     }
   }
 }
+
+
+TEST(ParsedDicomFile, ChangeEncoding)
+{
+  for (unsigned int i = 0; i < testEncodingsCount; i++)
+  {
+    // 1251 codepage is not supported by the core DICOM standard, ignore it
+    if (testEncodings[i] != Encoding_Windows1251) 
+    {
+      DicomMap m;
+      m.SetValue(DICOM_TAG_PATIENT_NAME, testEncodingsExpected[i], false);
+
+      std::string tag;
+
+      ParsedDicomFile dicom(m, Encoding_Utf8);
+      ASSERT_EQ(Encoding_Utf8, dicom.GetEncoding());
+      ASSERT_TRUE(dicom.GetTagValue(tag, DICOM_TAG_PATIENT_NAME));
+      ASSERT_EQ(tag, testEncodingsExpected[i]);
+
+      {
+        Json::Value v;
+        dicom.DatasetToJson(v, DicomToJsonFormat_Human, DicomToJsonFlags_Default, 0);
+        ASSERT_STREQ(v["SpecificCharacterSet"].asCString(), "ISO_IR 192");
+        ASSERT_STREQ(v["PatientName"].asCString(), testEncodingsExpected[i]);
+      }
+
+      dicom.ChangeEncoding(testEncodings[i]);
+
+      ASSERT_EQ(testEncodings[i], dicom.GetEncoding());
+      
+      const char* c = NULL;
+      ASSERT_TRUE(dicom.GetDcmtkObject().getDataset()->findAndGetString(DCM_PatientName, c).good());
+      EXPECT_STREQ(c, testEncodingsEncoded[i]);
+      
+      ASSERT_TRUE(dicom.GetTagValue(tag, DICOM_TAG_PATIENT_NAME));  // Decodes to UTF-8
+      EXPECT_EQ(tag, testEncodingsExpected[i]);
+
+      {
+        Json::Value v;
+        dicom.DatasetToJson(v, DicomToJsonFormat_Human, DicomToJsonFlags_Default, 0);
+        ASSERT_STREQ(v["SpecificCharacterSet"].asCString(), GetDicomSpecificCharacterSet(testEncodings[i]));
+        ASSERT_STREQ(v["PatientName"].asCString(), testEncodingsExpected[i]);
+      }
+    }
+  }
+}