Merge pull request #478 from Enet4/change/core/element-dict-xvr

Implement virtual VR concept in data element dictionary entries

core/src/dictionary/data_element.rs

@@ -140,6 +140,82 @@ impl FromStr for TagRange {
     }
 }
 
+/// A "virtual" value representation (VR) descriptor
+/// which extends the standard enumeration with context-dependent VRs.
+///
+/// It is used by element dictionary entries to describe circumstances
+/// in which the real VR may depend on context.
+/// As an example, the _Pixel Data_ attribute
+/// can have a value representation of either [`OB`](VR::OB) or [`OW`](VR::OW).
+#[derive(Debug, Copy, Clone, Eq, Hash, PartialEq)]
+#[non_exhaustive]
+pub enum VirtualVr {
+    /// The value representation is exactly known
+    /// and does not depend on context.
+    Exact(VR),
+    /// Represents a pixel data sample value
+    /// with a short magnitude.
+    ///
+    /// The value representation depends on
+    /// the pixel data value sample representation.
+    /// If pixel data values are signed
+    /// (represented by a _Pixel Representation_ value of `1`),
+    /// then values with this virtual VR
+    /// should be interpreted as signed 16 bit integers
+    /// ([`SS`](VR::SS)),
+    /// otherwise they should be interpreted as unsigned 16 bit integers
+    /// ([`US`](VR::US)).
+    Xs,
+    /// Represents overlay data sample values.
+    /// 
+    /// It can be either [`OB`](VR::OB) or [`OW`](VR::OW).
+    Ox,
+    /// Represents pixel data sample value.
+    ///
+    /// It can be either [`OB`](VR::OB) or [`OW`](VR::OW).
+    Px,
+    /// Represents LUT data, which can be [`US`](VR::US) or [`OW`](VR::OW)
+    Lt,
+}
+
+impl From<VR> for VirtualVr {
+    fn from(value: VR) -> Self {
+        VirtualVr::Exact(value)
+    }
+}
+
+impl VirtualVr {
+    /// Return the underlying value representation
+    /// in the case that it can be unambiguously defined without context.
+    pub fn exact(self) -> Option<VR> {
+        match self {
+            VirtualVr::Exact(vr) => Some(vr),
+            _ => None,
+        }
+    }
+
+    /// Return the underlying value representation,
+    /// making a relaxed conversion if it cannot be
+    /// accurately resolved without context.
+    ///
+    /// - [`Xs`](VirtualVr::Xs) is relaxed to [`US`](VR::US)
+    /// - [`Ox`](VirtualVr::Ox) is relaxed to [`OW`](VR::OW)
+    /// - [`Px`](VirtualVr::Px) is relaxed to [`OW`](VR::OW)
+    /// - [`Lt`](VirtualVr::Lt) is relaxed to [`OW`](VR::OW)
+    /// 
+    /// This method is ill-advised for uses where
+    /// the corresponding attribute is important.
+    pub fn relaxed(self) -> VR {
+        match self {
+            VirtualVr::Exact(vr) => vr,
+            VirtualVr::Xs => VR::US,
+            VirtualVr::Ox => VR::OW,
+            VirtualVr::Px => VR::OW,
+            VirtualVr::Lt => VR::OW,
+        }
+    }
+}
+
 /// An error during attribute selector parsing
 #[derive(Debug, Snafu)]
 pub struct ParseSelectorError(ParseSelectorErrorInner);
@@ -324,9 +400,11 @@ pub trait DataDictionaryEntry {
     /// The alias of the attribute, with no spaces, usually in UpperCamelCase.
     fn alias(&self) -> &str;
 
-    /// The _typical_ value representation of the attribute.
-    /// In some edge cases, an element might not have this VR.
-    fn vr(&self) -> VR;
+    /// The extended value representation descriptor of the attribute.
+    /// The use of [`VirtualVr`] is to attend to edge cases
+    /// in which the representation of a value
+    /// depends on surrounding context.
+    fn vr(&self) -> VirtualVr;
 }
 
 /// A data type for a dictionary entry with full ownership.
@@ -337,7 +415,7 @@ pub struct DataDictionaryEntryBuf {
     /// The alias of the attribute, with no spaces, usually InCapitalizedCamelCase
     pub alias: String,
     /// The _typical_  value representation of the attribute
-    pub vr: VR,
+    pub vr: VirtualVr,
 }
 
 impl DataDictionaryEntry for DataDictionaryEntryBuf {
@@ -347,7 +425,7 @@ impl DataDictionaryEntry for DataDictionaryEntryBuf {
     fn alias(&self) -> &str {
         self.alias.as_str()
     }
-    fn vr(&self) -> VR {
+    fn vr(&self) -> VirtualVr {
         self.vr
     }
 }
@@ -359,8 +437,8 @@ pub struct DataDictionaryEntryRef<'a> {
     pub tag: TagRange,
     /// The alias of the attribute, with no spaces, usually InCapitalizedCamelCase
     pub alias: &'a str,
-    /// The _typical_  value representation of the attribute
-    pub vr: VR,
+    /// The extended value representation descriptor of the attribute
+    pub vr: VirtualVr,
 }
 
 impl<'a> DataDictionaryEntry for DataDictionaryEntryRef<'a> {
@@ -370,7 +448,7 @@ impl<'a> DataDictionaryEntry for DataDictionaryEntryRef<'a> {
     fn alias(&self) -> &str {
         self.alias
     }
-    fn vr(&self) -> VR {
+    fn vr(&self) -> VirtualVr {
         self.vr
     }
 }

core/src/dictionary/mod.rs

@@ -8,7 +8,7 @@ mod uid;
 
 pub use data_element::{
     DataDictionary, DataDictionaryEntry, DataDictionaryEntryBuf, DataDictionaryEntryRef, TagByName,
-    TagRange,
+    TagRange, VirtualVr,
 };
 
 pub use uid::{UidDictionary, UidDictionaryEntry, UidDictionaryEntryRef, UidType};

devtools/dictionary-builder/src/tags.rs

@@ -1,6 +1,5 @@
 //! DICOM data element (tag) dictionary builder
 use std::{
-    borrow::Cow,
     fs::{create_dir_all, File},
     io::{BufRead, BufReader, BufWriter, Write},
     path::Path,
@@ -98,29 +97,11 @@ fn parse_entries<R: BufRead>(source: R) -> Result<Vec<Entry>> {
             continue;
         }
 
-        let mut vr = parts[1].to_string();
+        let vr = parts[1].to_string();
 
-        // Some fields are dependent upon context.
-        // We may want to support this at some point, but for now,
-        // let's just choose some good defaults.
-        if vr == "up" {
-            vr = "UL".to_string();
-        }
-        if vr == "xs" {
-            vr = "US".to_string();
-        }
-        if vr == "ox" {
-            vr = "OB".to_string();
-        }
-        if vr == "px" {
-            vr = "OB".to_string();
-        }
-        if vr == "lt" {
-            vr = "OW".to_string();
-        }
         if vr == "na" {
             // These are the "Item", "ItemDelimitationItem", "SequenceDelimitationItem", etc, values.
-            // Question, should we generate const values for these, and use them internally?
+            // Do not include them for now.
             continue;
         }
 
@@ -234,7 +215,7 @@ where
 
     f.write_all(
         b"\n\
-    use dicom_core::dictionary::{DataDictionaryEntryRef, TagRange, TagRange::*};\n\
+    use dicom_core::dictionary::{DataDictionaryEntryRef, TagRange, TagRange::*, VirtualVr::*};\n\
     use dicom_core::Tag;\n\
     use dicom_core::VR::*;\n\n",
     )?;
@@ -287,14 +268,16 @@ where
         if retired_options == RetiredOptions::Ignore && e.is_retired {
             continue;
         }
-
-        let (vr1, vr2) = e.vr.split_at(2);
-
-        let vr2 = vr2.trim();
-        let second_vr: Cow<str> = if !vr2.is_empty() {
-            format!(" /*{} */", vr2).into()
-        } else {
-            vr2.into()
+        // Some fields are dependent upon context
+        let (vr1, vr2, vr3) = match &*e.vr {
+            "xs" => ("Xs", "", ""),
+            "ox" => ("Ox", "", ""),
+            "px" => ("Px", "", ""),
+            "lt" => ("Lt", "", ""),
+            // for now we will always map "up" to "UL"
+            "up" => ("Exact(", "UL", ")"),
+            // assume exact in all other cases
+            _ => ("Exact(", &*e.vr, ")"),
         };
 
         let tag_set = match e.tag_type {
@@ -304,8 +287,8 @@ where
 
         writeln!(
             f,
-            "    E {{ tag: {}, alias: \"{}\", vr: {}{} }}, // {}",
-            tag_set, e.alias, vr1, second_vr, e.obs
+            "    E {{ tag: {}, alias: \"{}\", vr: {}{}{} }}, // {}",
+            tag_set, e.alias, vr1, vr2, vr3, e.obs
         )?;
     }
     f.write_all(b"];\n")?;

dictionary-std/src/data_element.rs

@@ -1,7 +1,7 @@
 //! Data element dictionary implementation
 
 use crate::tags::ENTRIES;
-use dicom_core::dictionary::{DataDictionary, DataDictionaryEntryRef, TagRange::*};
+use dicom_core::dictionary::{DataDictionary, DataDictionaryEntryRef, TagRange::*, VirtualVr};
 use dicom_core::header::Tag;
 use dicom_core::VR;
 use once_cell::sync::Lazy;
@@ -69,14 +69,14 @@ impl StandardDataDictionaryRegistry {
 static GROUP_LENGTH_ENTRY: DataDictionaryEntryRef<'static> = DataDictionaryEntryRef {
     tag: GroupLength,
     alias: "GenericGroupLength",
-    vr: VR::UL,
+    vr: VirtualVr::Exact(VR::UL),
 };
 
 /// Generic Private Creator dictionary entry.
 static PRIVATE_CREATOR_ENTRY: DataDictionaryEntryRef<'static> = DataDictionaryEntryRef {
     tag: PrivateCreator,
     alias: "PrivateCreator",
-    vr: VR::LO,
+    vr: VirtualVr::Exact(VR::LO),
 };
 
 /// A data element dictionary which consults
@@ -172,7 +172,7 @@ mod tests {
     use crate::tags;
 
     use super::StandardDataDictionary;
-    use dicom_core::dictionary::{DataDictionary, DataDictionaryEntryRef, TagRange::*};
+    use dicom_core::dictionary::{DataDictionary, DataDictionaryEntryRef, TagRange::*, VirtualVr};
     use dicom_core::header::{Tag, VR};
     use dicom_core::ops::AttributeSelector;
 
@@ -187,7 +187,7 @@ mod tests {
             Some(&DataDictionaryEntryRef {
                 tag: Single(Tag(0x0010, 0x0010)),
                 alias: "PatientName",
-                vr: VR::PN,
+                vr: VR::PN.into(),
             })
         );
 
@@ -196,7 +196,7 @@ mod tests {
             Some(&DataDictionaryEntryRef {
                 tag: Single(Tag(0x0008, 0x0060)),
                 alias: "Modality",
-                vr: VR::CS,
+                vr: VR::CS.into(),
             })
         );
 
@@ -206,22 +206,22 @@ mod tests {
         eprintln!("{:X?}", pixel_data.tag);
         assert_eq!(pixel_data.tag, Single(Tag(0x7FE0, 0x0010)));
         assert_eq!(pixel_data.alias, "PixelData");
-        assert!(pixel_data.vr == VR::OB || pixel_data.vr == VR::OW);
+        assert!(pixel_data.vr == VirtualVr::Px);
 
         let overlay_data = dict
             .by_tag(Tag(0x6000, 0x3000))
             .expect("Overlay Data attribute should exist");
         assert_eq!(overlay_data.tag, Group100(Tag(0x6000, 0x3000)));
         assert_eq!(overlay_data.alias, "OverlayData");
-        assert!(overlay_data.vr == VR::OB || overlay_data.vr == VR::OW);
+        assert!(overlay_data.vr == VirtualVr::Ox);
 
         // repeated overlay data
         let overlay_data = dict
             .by_tag(Tag(0x60EE, 0x3000))
             .expect("Repeated Overlay Data attribute should exist");
         assert_eq!(overlay_data.tag, Group100(Tag(0x6000, 0x3000)));
         assert_eq!(overlay_data.alias, "OverlayData");
-        assert!(overlay_data.vr == VR::OB || overlay_data.vr == VR::OW);
+        assert!(overlay_data.vr == VirtualVr::Ox);
     }
 
     #[test]
@@ -250,7 +250,7 @@ mod tests {
             Some(&DataDictionaryEntryRef {
                 tag: Single(crate::tags::PATIENT_NAME),
                 alias: "PatientName",
-                vr: VR::PN,
+                vr: VR::PN.into(),
             })
         );
 
@@ -259,7 +259,7 @@ mod tests {
             Some(&DataDictionaryEntryRef {
                 tag: Single(crate::tags::MODALITY),
                 alias: "Modality",
-                vr: VR::CS,
+                vr: VR::CS.into(),
             })
         );
 
@@ -268,7 +268,7 @@ mod tests {
             Some(&DataDictionaryEntryRef {
                 tag: Single(crate::tags::OPERATORS_NAME),
                 alias: "OperatorsName",
-                vr: VR::PN,
+                vr: VR::PN.into(),
             })
         );
 
@@ -291,7 +291,7 @@ mod tests {
             Some(&DataDictionaryEntryRef {
                 tag: Single(FILE_META_INFORMATION_GROUP_LENGTH),
                 alias: "FileMetaInformationGroupLength",
-                vr: VR::UL,
+                vr: VR::UL.into(),
             }),
         );
 
@@ -300,7 +300,7 @@ mod tests {
             Some(&DataDictionaryEntryRef {
                 tag: Single(COMMAND_GROUP_LENGTH),
                 alias: "CommandGroupLength",
-                vr: VR::UL,
+                vr: VR::UL.into(),
             }),
         );
 
@@ -311,7 +311,7 @@ mod tests {
             Some(&DataDictionaryEntryRef {
                 tag: GroupLength,
                 alias: "GenericGroupLength",
-                vr: VR::UL,
+                vr: VR::UL.into(),
             }),
         );
 
@@ -320,7 +320,7 @@ mod tests {
             Some(&DataDictionaryEntryRef {
                 tag: GroupLength,
                 alias: "GenericGroupLength",
-                vr: VR::UL,
+                vr: VR::UL.into(),
             }),
         );
     }
@@ -332,7 +332,7 @@ mod tests {
         let private_creator = DataDictionaryEntryRef {
             tag: PrivateCreator,
             alias: "PrivateCreator",
-            vr: VR::LO,
+            vr: VR::LO.into(),
         };
 
         assert_eq!(dict.by_tag(Tag(0x0009, 0x0010)), Some(&private_creator));