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

DICOM: use FrameTimePointer to detect timelapse data #4196

Closed
wants to merge 1 commit into from
Closed

DICOM: use FrameTimePointer to detect timelapse data #4196

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
68 changes: 61 additions & 7 deletions components/formats-bsd/src/loci/formats/in/DicomReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,8 @@ public class DicomReader extends SubResolutionFormatReader {

private List<DicomTag> tags;

private DicomTag frameTime = null;

private Set<Integer> privateContentHighWords = new HashSet<Integer>();

// -- Constructor --
Expand Down Expand Up @@ -282,8 +284,14 @@ public byte[] openBytes(int no, byte[] buf, int x, int y, int w, int h)
int bpp = FormatTools.getBytesPerPixel(getPixelType());
int pixel = bpp * getRGBChannelCount();
Region currentRegion = new Region(x, y, w, h);
int z = getZCTCoords(no)[0];
int c = getZCTCoords(no)[1];
int[] coords = getZCTCoords(no);
int z = coords[0];
int c = coords[1];
int timepoint = coords[2];
if (isTimelapse()) {
z = coords[2];
timepoint = coords[1];
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, this inversion is related to the fact that the frame offsets are currently stored as DicomTile.zOffset.

Do you have a feeling about the cost vs value of updating DicomTile to also store tOffset instead?

}

if (!tilePositions.containsKey(getCoreIndex())) {
LOGGER.warn("No tiles for core index = {}", getCoreIndex());
Expand All @@ -293,9 +301,10 @@ public byte[] openBytes(int no, byte[] buf, int x, int y, int w, int h)
// look for any tiles that match the requested tile and plane
List<Double> zs = zOffsets.get(getCoreIndex());
List<DicomTile> tiles = tilePositions.get(getCoreIndex());
int compareDimension = isTimelapse() ? getSizeT() : getSizeZ();
for (int t=0; t<tiles.size(); t++) {
DicomTile tile = tiles.get(t);
if ((getSizeZ() == 1 || (getSizeZ() <= zs.size() && tile.zOffset.equals(zs.get(z))) || (getSizeZ() == tiles.size() && t == z)) &&
if ((compareDimension == 1 || (compareDimension <= zs.size() && tile.zOffset.equals(zs.get(z))) || (compareDimension == tiles.size() && t == z)) &&
(tile.channel == c || getEffectiveSizeC() == 1) &&
tile.region.intersects(currentRegion))
{
Expand Down Expand Up @@ -372,6 +381,7 @@ public void close(boolean fileOnly) throws IOException {
concatenationNumber = null;
edf = false;
tags = null;
frameTime = null;
privateContentHighWords.clear();
}
}
Expand Down Expand Up @@ -437,6 +447,7 @@ protected void initFile(String id) throws FormatException, IOException {
int opticalChannels = 0;

List<Integer> opticalPathIDs = new ArrayList<Integer>();
DicomAttribute frameIncrementPointer = null;

while (decodingTags) {
if (in.getFilePointer() + 4 >= in.length()) {
Expand Down Expand Up @@ -677,6 +688,23 @@ else if (child.attribute == OPTICAL_PATH_DESCRIPTION) {
case EXTENDED_DEPTH_OF_FIELD:
edf = tag.getStringValue().equalsIgnoreCase("yes");
break;
case FRAME_INCREMENT_POINTER:
short[] tagParts = null;
if (tag.value instanceof short[][]) {
tagParts = ((short[][]) tag.value)[0];
}
else if (tag.value instanceof short[]) {
tagParts = (short[]) tag.value;
}

if (tagParts != null && tagParts.length >= 2) {
int groupWord = tagParts[0];
int elementWord = tagParts[1];
int pointerValue = ((groupWord << 16) & 0xffff0000) | (elementWord & 0xffff);
frameIncrementPointer = DicomAttribute.get(pointerValue);
}
in.seek(tag.getEndPointer());
break;
case TRAILING_PADDING:
decodingTags = false;
break;
Expand All @@ -689,6 +717,23 @@ else if (child.attribute == OPTICAL_PATH_DESCRIPTION) {
}
if (imagesPerFile == 0) imagesPerFile = 1;

// pointer could be FrameTime or FrameTimeVector, or possibly something else?
Copy link
Member

Choose a reason for hiding this comment

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

Since these two tags are defined in

bioformats/components/formats-bsd/src/loci/formats/dicom/DicomAttribute.java

Line 314 in a1d05e4

FRAME_TIME(0x00181063),
and

bioformats/components/formats-bsd/src/loci/formats/dicom/DicomAttribute.java

Line 316 in a1d05e4

FRAME_TIME_VECTOR(0x00181065),

should we make this logic defensive and check the tag attribute before setting frameTime?

Copy link
Member Author

Choose a reason for hiding this comment

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

My thinking here was to not assume that the pointer would necessarily be FRAME_TIME or FRAME_TIME_VECTOR. For NM data in particular, I think that assumption would not hold (see https://dicom.nema.org/medical/dicom/current/output/chtml/part03/sect_C.8.4.8.html#sect_C.8.4.8.1.1), but could also imagine a file that has FrameIncrementPointer pointing to a private tag.

Copy link
Member

Choose a reason for hiding this comment

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

Completely agreed that the reader should not assume the tag will reference one of these two tags or even official tags.

Maybe my question was more specifically about how we would handle both scenarios:

  • if Frame Increment Pointer points to Frame Time or Frame Time Vector then the logic should be updated as proposed here to detect the image as multi-T and optionally set the time metadata appropriately
  • if Frame Increment Pointer points to another tag (or multiple tags), should the current reader logic be preserved i.e. use the Z dimension to aggregate the frames? Or would we still switch to using the T dimension similarly to what MinimalTiffReader does for multi-page TIFF without additional metadata?

if (frameIncrementPointer != null) {
// referenced tag might occur before or after the pointer tag (usually before)

for (DicomTag t : tags) {
if (frameIncrementPointer.equals(t.attribute)) {
frameTime = t;
Copy link
Member

Choose a reason for hiding this comment

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

Two possible metadata follow-ups

  • FrameTime could likely be mapped into Pixels.TimeIncrement
  • FrameTimeVector could be mapped into Plane.DeltaT

break;
}
}
}

if (isTimelapse()) {
m.sizeT = m.sizeZ;
m.sizeZ = 1;
}

if (new Location(currentId).getName().equals("DICOMDIR")) {
String parent = new Location(currentId).getAbsoluteFile().getParent();
Integer[] fileKeys = fileList.keySet().toArray(new Integer[0]);
Expand Down Expand Up @@ -733,6 +778,7 @@ else if (child.attribute == OPTICAL_PATH_DESCRIPTION) {
int cols = (int) Math.ceil((double) getSizeX() / originalX);
int rows = (int) Math.ceil((double) getSizeY() / originalY);
int tilesPerPlane = rows * cols;

int c = frameOffsetNumber / (tilesPerPlane * getSizeZ());
int newOffset = frameOffsetNumber - (c * tilesPerPlane * getSizeZ());
int z = newOffset / tilesPerPlane;
Expand Down Expand Up @@ -813,7 +859,7 @@ else if (y + originalY < getSizeY()) {
DicomFileInfo fileInfo = createFileInfo(currentFileList.get(0));
zOffsets.put(i, fileInfo.zOffsets);
fileInfo.coreMetadata.sizeZ *= currentFileList.size();
fileInfo.coreMetadata.imageCount = fileInfo.coreMetadata.sizeZ;
fileInfo.coreMetadata.imageCount = fileInfo.coreMetadata.sizeZ * fileInfo.coreMetadata.sizeT;
core.add(fileInfo.coreMetadata);

List<DicomTile> positions = new ArrayList<DicomTile>();
Expand Down Expand Up @@ -1834,18 +1880,26 @@ private DicomFileInfo createFileInfo(String file) throws FormatException, IOExce
}

private void updateCoreMetadata(CoreMetadata ms) {
if (ms.sizeC == 0) ms.sizeC = 1;
ms.sizeT = 1;
if (ms.sizeC == 0) {
ms.sizeC = 1;
}
if (ms.sizeT == 0) {
ms.sizeT = 1;
}
ms.dimensionOrder = "XYCZT";
ms.metadataComplete = true;
ms.falseColor = false;
if (isRLE) ms.interleaved = false;
ms.imageCount = ms.sizeZ;
ms.imageCount = ms.sizeZ * ms.sizeT;
if (!ms.rgb) {
ms.imageCount *= ms.sizeC;
}
}

public boolean isTimelapse() {
return frameTime != null;
}

public String getImageType() {
return imageType;
}
Expand Down