Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add some robustness in meta file group reading #29

Merged
merged 2 commits into from
Dec 27, 2019
Merged

Add some robustness in meta file group reading #29

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2bc48e5
[object] #28 Robustness in meta file group
Enet4 Dec 19, 2019
fb85487
[object] use unwrap_or_else
Enet4 Dec 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
148 changes: 62 additions & 86 deletions object/src/meta.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,34 +46,6 @@ pub struct FileMetaTable {
pub private_information: Option<Vec<u8>>,
}

/// Utility function for reading the whole DICOM element as a string, with the given tag.
fn read_str_as_tag<'s, S: 's, D, T>(
source: &'s mut S,
decoder: &D,
text: &T,
group_length_remaining: &mut u32,
tag: Tag,
) -> Result<String>
where
S: Read,
D: Decode<Source = S>,
T: TextCodec,
{
let elem_len = {
let (elem, _bytes_read) = decoder.decode_header(source)?;
if elem.tag() != tag {
return Err(Error::UnexpectedTag(elem.tag()));
}
match elem.len().get() {
None => {
return Err(Error::from(InvalidValueReadError::UnresolvedValueLength));
}
Some(len) => len,
}
};
read_str_body(source, text, group_length_remaining, elem_len)
}

/// Utility function for reading the body of the DICOM element as a UID.
fn read_str_body<'s, S: 's, T>(
source: &'s mut S,
Expand All @@ -85,6 +57,7 @@ where
S: Read,
T: TextCodec,
{
eprintln!("> read string of {} bytes", len);
let mut v = vec![0; len as usize];
source.read_exact(&mut v)?;
*group_length_remaining -= 8 + len;
Expand Down Expand Up @@ -127,49 +100,7 @@ impl FileMetaTable {

let mut group_length_remaining = group_length;

let mut builder = builder
.group_length(group_length)
.information_version({
let (elem, _bytes_read) = decoder.decode_header(&mut file)?;
if elem.tag() != (0x0002, 0x0001) {
return Err(Error::UnexpectedTag(elem.tag()));
}
if elem.len() != Length(2) {
return Err(Error::UnexpectedDataValueLength);
}
let mut hbuf = [0u8; 2];
file.read_exact(&mut hbuf[..])?;
group_length_remaining -= 14;
hbuf
})
.media_storage_sop_class_uid(read_str_as_tag(
&mut file,
&decoder,
&text,
&mut group_length_remaining,
Tag(0x0002, 0x0002),
)?)
.media_storage_sop_instance_uid(read_str_as_tag(
&mut file,
&decoder,
&text,
&mut group_length_remaining,
Tag(0x0002, 0x0003),
)?)
.transfer_syntax(read_str_as_tag(
&mut file,
&decoder,
&text,
&mut group_length_remaining,
Tag(0x0002, 0x0010),
)?)
.implementation_class_uid(read_str_as_tag(
&mut file,
&decoder,
&text,
&mut group_length_remaining,
Tag(0x0002, 0x0012),
)?);
let mut builder = builder.group_length(group_length);

// Fetch optional data elements
while group_length_remaining > 0 {
Expand All @@ -178,57 +109,99 @@ impl FileMetaTable {
None => {
return Err(Error::from(InvalidValueReadError::UnresolvedValueLength));
}
Some(len) => len as usize,
Some(len) => len,
};
dbg!((&elem, group_length_remaining));
eprintln!("element length: {}", elem_len);
builder = match elem.tag() {
Tag(0x0002, 0x0001) => {
// Implementation Version
if elem.len() != Length(2) {
return Err(Error::UnexpectedDataValueLength);
}
let mut hbuf = [0u8; 2];
file.read_exact(&mut hbuf[..])?;
group_length_remaining -= 14;

builder.information_version(hbuf)
}
// Media Storage SOP Class UID
Tag(0x0002, 0x0002) => builder.media_storage_sop_class_uid(read_str_body(
&mut file,
&text,
&mut group_length_remaining,
elem_len,
)?),
// Media Storage SOP Instance UID
Tag(0x0002, 0x0003) => builder.media_storage_sop_instance_uid(read_str_body(
&mut file,
&text,
&mut group_length_remaining,
elem_len,
)?),
// Transfer Syntax
Tag(0x0002, 0x0010) => builder.transfer_syntax(read_str_body(
&mut file,
&text,
&mut group_length_remaining,
elem_len,
)?),
// Implementation Class UID
Tag(0x0002, 0x0012) => builder.implementation_class_uid(read_str_body(
&mut file,
&text,
&mut group_length_remaining,
elem_len,
)?),
Tag(0x0002, 0x0013) => {
// Implementation Version Name
let mut v = vec![0; elem_len];
let mut v = vec![0; elem_len as usize];
file.read_exact(&mut v)?;
group_length_remaining -= 8 + elem_len as u32;
group_length_remaining -= 8 + elem_len;
builder.implementation_version_name(text.decode(&v)?)
}
Tag(0x0002, 0x0016) => {
// Source Application Entity Title
let mut v = vec![0; elem_len];
let mut v = vec![0; elem_len as usize];
file.read_exact(&mut v)?;
group_length_remaining -= 8 + elem_len as u32;
group_length_remaining -= 8 + elem_len;
builder.source_application_entity_title(text.decode(&v)?)
}
Tag(0x0002, 0x0017) => {
// Sending Application Entity Title
let mut v = vec![0; elem_len];
let mut v = vec![0; elem_len as usize];
file.read_exact(&mut v)?;
group_length_remaining -= 8 + elem_len as u32;
group_length_remaining -= 8 + elem_len;
builder.sending_application_entity_title(text.decode(&v)?)
}
Tag(0x0002, 0x0018) => {
// Receiving Application Entity Title
let mut v = vec![0; elem_len];
let mut v = vec![0; elem_len as usize];
file.read_exact(&mut v)?;
group_length_remaining -= 8 + elem_len as u32;
group_length_remaining -= 8 + elem_len;
builder.receiving_application_entity_title(text.decode(&v)?)
}
Tag(0x0002, 0x0100) => {
// Private Information Creator UID
let mut v = vec![0; elem_len];
let mut v = vec![0; elem_len as usize];
file.read_exact(&mut v)?;
group_length_remaining -= 8 + elem_len as u32;
group_length_remaining -= 8 + elem_len;
builder.private_information_creator_uid(text.decode(&v)?)
}
Tag(0x0002, 0x0102) => {
// Private Information
let mut v = vec![0; elem_len];
let mut v = vec![0; elem_len as usize];
file.read_exact(&mut v)?;
group_length_remaining -= 12 + elem_len as u32;
group_length_remaining -= 12 + elem_len;
builder.private_information(v)
}
Tag(0x0002, _) => {
// unknown tag, do nothing
// could be an unsupported or non-standard attribute
Comment on lines 199 to +200
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit, they're reserved
Values of all Tags (0002,xxxx) are reserved for use by this Standard and later versions of DICOM.
which mentioned below the table on http://dicom.nema.org/medical/dicom/current/output/chtml/part10/chapter_7.html#table_7.1-1

Copy link
Owner Author

@Enet4 Enet4 Dec 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well yes, but with this comment I am keeping two possible cases in mind:

  • the standard is extended to admit more file meta attributes, but the library does not know them yet;
  • a pseudo-DICOM producer invents its own attribute within the same group.

In either case, nothing happens, but in the future we might be interested in logging it differently from the case below this one.

builder
}
_ => {
// unexpected tag! do nothing for now, although this could represent invalid content
// unexpected tag from another group! do nothing for now
builder
}
}
Expand Down Expand Up @@ -372,7 +345,10 @@ impl FileMetaTableBuilder {
.ok_or_else(|| Error::InvalidFormat)?;
let information_version = self
.information_version
.ok_or_else(|| Error::InvalidFormat)?;
.or_else(|| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unwrap_or_else may be handy here

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, thanks! For some reason my IDE did not suggest me that method.

// Missing information version, will assume (00H, 01H). See #28
Some([0, 1])
}).unwrap();
let media_storage_sop_class_uid = self
.media_storage_sop_class_uid
.ok_or_else(|| Error::InvalidFormat)?;
Expand Down