Merge pull request #12 from slicebox/refactor-filters

Refactored filter flows, removed applyToFmi flag, added more tests

README.md

@@ -17,8 +17,8 @@ DICOM data chunk size and network utilization using back-pressure as specified i
 
 ### Usage
 
-The following example reads a DICOM file from disk, validates that it is a DICOM file, discards all attributes but
-PatientName and PatientID and writes it to a new file.
+The following example reads a DICOM file from disk, validates that it is a DICOM file, discards all private attributes
+and writes it to a new file.
 
 ```scala
 import akka.stream.scaladsl.FileIO
@@ -30,13 +30,33 @@ import se.nimsa.dcm4che.streams.DicomPartFlow._
 FileIO.fromPath(Paths.get("source-file.dcm"))
   .via(validateFlow)
   .via(partFlow)
-  .via(partFilter(Seq(Tag.PatientName, Tag.PatientID)))
+  .via(blacklistFilter(DicomParsing.isPrivateAttribute(_)))
   .map(_.bytes)
   .runWith(FileIO.toPath(Paths.get("target-file.dcm")))
 ```
 
+Same result can be achieved with a whitelist filter instead, but we need to tell the filter
+to keep the preamble:
+
+```scala
+import akka.stream.scaladsl.FileIO
+import java.nio.file.Paths
+import org.dcm4che3.data.Tag
+import se.nimsa.dcm4che.streams.DicomFlows._
+import se.nimsa.dcm4che.streams.DicomPartFlow._
+
+FileIO.fromPath(Paths.get("source-file.dcm"))
+  .via(validateFlow)
+  .via(partFlow)
+  .via(whitelistFilter(!DicomParsing.isPrivateAttribute(_), keepPreamble = true))
+  .map(_.bytes)
+  .runWith(FileIO.toPath(Paths.get("target-file.dcm")))
+```
+
+
 The next example materializes the above stream as dcm4che `Attributes` objects instead of writing data to disk.
 
+
 ```scala
 import akka.stream.scaladsl.FileIO
 import java.nio.file.Paths
@@ -50,7 +70,7 @@ val futureAttributes: Future[(Option[Attributes], Option[Attributes])] =
   FileIO.fromPath(Paths.get("source-file.dcm"))
     .via(validateFlow)
     .via(partFlow)
-    .via(partFilter(Seq(Tag.PatientName, Tag.PatientID)))
+    .via(whitelistFilter(Seq(Tag.PatientName, Tag.PatientID)))
     .via(attributeFlow) // must turn headers + chunks into complete attributes before materializing
     .runWith(attributesSink)
 

src/main/scala/se/nimsa/dcm4che/streams/DicomFlows.scala

@@ -96,64 +96,65 @@ object DicomFlows {
 
   /**
     * Filter a stream of dicom parts such that all attributes except those with tags in the white list are discarded.
-    * Applies to headers and subsequent value chunks, DicomAttribute:s (as produced by the associated flow) and
-    * fragments. Sequences, items, preamble, deflated chunks and unknown parts are not affected.
     *
-    * @param tagsWhitelist list of tags to keep
-    * @param applyToFmi    if false, this filter does not affect the FMI
+    * @param tagsWhitelist list of tags to keep.
     * @return the associated filter Flow
     */
-  def partFilter(tagsWhitelist: Seq[Int], applyToFmi: Boolean = false) = whitelistFilter(tagsWhitelist.contains(_), applyToFmi)
+  def whitelistFilter(tagsWhitelist: Seq[Int]): Flow[DicomPartFlow.DicomPart, DicomPartFlow.DicomPart, NotUsed] = whitelistFilter(tagsWhitelist.contains(_))
 
   /**
     * Filter a stream of dicom parts such that all attributes that are group length elements except
-    * file meta information group length, will be discarded.
+    * file meta information group length, will be discarded. Group Length (gggg,0000) Standard Data Elements
+    * have been retired in the standard.
     * @return the associated filter Flow
     */
-  def groupLengthDiscardFilter = blacklistFilter(DicomParsing.isGroupLength(_), applyToFmi = false)
+  def groupLengthDiscardFilter = blacklistFilter((tag: Int) => DicomParsing.isGroupLength(tag) && !DicomParsing.isFileMetaInformation(tag))
+
+  /**
+    * Discards the file meta information.
+    * @return the associated filter Flow
+    */
+  def fmiDiscardFilter = blacklistFilter((tag: Int) => DicomParsing.isFileMetaInformation(tag), keepPreamble = false)
 
   /**
     * Blacklist filter for DICOM parts.
-    * @param tagCondition blacklist condition
-    * @param applyToFmi if false, this filter does not affect the FMI
+    * @param tagCondition blacklist tag condition
+    * @param keepPreamble true if preamble should be kept, else false
     * @return Flow of filtered parts
     */
-  def blacklistFilter(tagCondition: (Int) => Boolean, applyToFmi: Boolean = false) = tagFilter(tagCondition, applyToFmi, isWhitelist = false)
+  def blacklistFilter(tagCondition: (Int) => Boolean, keepPreamble: Boolean = true) = tagFilter(tagCondition, isWhitelist = false, keepPreamble)
 
   /**
-    * Whitelist filter for DICOM parts.
+    * Tag based whitelist filter for DICOM parts.
     * @param tagCondition whitelist condition
-    * @param applyToFmi if false, this filter does not affect the FMI
+    * @param keepPreamble true if preamble should be kept, else false
     * @return Flow  of filtered parts
     */
-  def whitelistFilter(tagCondition: (Int) => Boolean, applyToFmi: Boolean = false) = tagFilter(tagCondition, applyToFmi, isWhitelist = true)
+  def whitelistFilter(tagCondition: (Int) => Boolean, keepPreamble: Boolean = false):  Flow[DicomPartFlow.DicomPart, DicomPartFlow.DicomPart, NotUsed] = tagFilter(tagCondition, isWhitelist = true, keepPreamble)
 
 
-  private def tagFilter(tagCondition: (Int) => Boolean, applyToFmi: Boolean, isWhitelist: Boolean) = Flow[DicomPart].statefulMapConcat {
+  private def tagFilter(tagCondition: (Int) => Boolean, isWhitelist: Boolean, keepPreamble: Boolean) = Flow[DicomPart].statefulMapConcat {
     () =>
       var discarding = false
 
-      def shouldDiscard(tag: Int, isFmi: Boolean, applyToFmi: Boolean, isWhitelist: Boolean) = {
+      def shouldDiscard(tag: Int, isWhitelist: Boolean) = {
         if (isWhitelist) {
-          // Whitelist: condition true or not appply to fmi => discard = false
-          !((tagCondition(tag) || isFmi && !applyToFmi))
+          !tagCondition(tag) // Whitelist: condition true => keep
         } else {
-          // Blacklist: condition true or condition true and apply to fmi => discard
-          if (tagCondition(tag)) {
-            if (isFmi) {
-              applyToFmi
-            } else {
-              true
-            }
-          } else {
-            false
-          }
+          tagCondition(tag) // Blacklist: condition true => discard
         }
       }
 
     {
+      case dicomPreamble: DicomPreamble =>
+        if (keepPreamble) {
+          dicomPreamble :: Nil
+        } else {
+          Nil
+        }
+
       case dicomHeader: DicomHeader =>
-        discarding = shouldDiscard(dicomHeader.tag, dicomHeader.isFmi, applyToFmi, isWhitelist)
+        discarding = shouldDiscard(dicomHeader.tag, isWhitelist)
         if (discarding) {
           Nil
         } else {
@@ -163,7 +164,7 @@ object DicomFlows {
       case fragment: DicomFragment => if (discarding) Nil else fragment :: Nil
 
       case dicomFragments: DicomFragments =>
-        discarding = shouldDiscard(dicomFragments.tag, false, applyToFmi, isWhitelist)
+        discarding = shouldDiscard(dicomFragments.tag, isWhitelist)
         if (discarding) {
           Nil
         } else {
@@ -172,24 +173,30 @@ object DicomFlows {
 
       case _: DicomItem if discarding => Nil
       case _: DicomItemDelimitation if discarding => Nil
-      case _: DicomFragmentsDelimitation if discarding =>
-        discarding = false
-        Nil
+      case _: DicomFragmentsDelimitation if discarding => Nil
+
+      case _: DicomSequence if discarding => Nil
+      case _: DicomSequenceDelimitation  if discarding => Nil
 
       case dicomAttribute: DicomAttribute  =>
-        discarding = shouldDiscard(dicomAttribute.header.tag, dicomAttribute.header.isFmi, applyToFmi, isWhitelist)
+        discarding = shouldDiscard(dicomAttribute.header.tag, isWhitelist)
         if (discarding) {
           Nil
         } else {
           dicomAttribute :: Nil
         }
 
       case dicomPart =>
-        discarding = false
-        dicomPart :: Nil
+        if (isWhitelist) {
+          Nil
+        } else {
+          discarding = false
+          dicomPart :: Nil
+        }
     }
   }
 
+
   /**
     * A flow which passes on the input bytes unchanged, but fails for non-DICOM files, determined by the first
     * attribute found

src/main/scala/se/nimsa/dcm4che/streams/DicomParsing.scala

@@ -188,6 +188,7 @@ trait DicomParsing {
   }
   def isSequenceDelimiter(tag: Int) = groupNumber(tag) == 0xFFFE
   def isFileMetaInformation(tag: Int) = (tag & 0xFFFF0000) == 0x00020000
+  def isPrivateAttribute(tag: Int) = groupNumber(tag) % 2 == 1
 
   def isGroupLength(tag: Int) = elementNumber(tag) == 0
 

src/main/scala/se/nimsa/dcm4che/streams/DicomSinks.scala

@@ -24,7 +24,7 @@ import scala.concurrent.Future
 
 object DicomSinks {
 
-  import DicomFlows.{attributeFlow, partFilter, validateFlow}
+  import DicomFlows.{attributeFlow, whitelistFilter, validateFlow}
   import DicomPartFlow._
 
   def bytesAndAttributesSink[A, B](bytesSink: Sink[ByteString, Future[A]],
@@ -41,7 +41,7 @@ object DicomSinks {
 
           validate.out ~> bcast.in
 
-          bcast.out(0) ~> partFlow ~> partFilter(tagsWhiteList, applyToFmi = true) ~> attributeFlow ~> attributesConsumer
+          bcast.out(0) ~> partFlow ~> whitelistFilter(tagsWhiteList) ~> attributeFlow ~> attributesConsumer
           bcast.out(1) ~> bytesConsumer
 
           SinkShape(validate.in)