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

ENH: Virtualize DICOM database #1154

Merged
merged 10 commits into from
Dec 13, 2023
Merged

ENH: Virtualize DICOM database #1154

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3b8182e
ENH: allow URL "filenames" in database
pieper Oct 25, 2023
fbae32f
ENH: Always use upper case hex for tags
pieper Nov 5, 2023
03e6ecc
ENH: applying DICOM display updates even if some fail
pieper Nov 5, 2023
ff515f9
ENH: use fileName in dicom db only for files
pieper Nov 13, 2023
5200de9
ENH: Updates dicom db schema to support urls
pieper Nov 13, 2023
8f9f14a
BUG: Fix dicom db insert with URL field
pieper Nov 14, 2023
0c47c6a
ENH: Allow file or url to be null in dicom-schema.sql
pieper Nov 14, 2023
d6be887
ENH: update dicom db schema and api for files and urls
pieper Nov 14, 2023
2bfc42a
Merge branch 'master' into virtualize-database
pieper Nov 16, 2023
df63274
ENH: Add index for URL to dicom database schema
pieper Dec 13, 2023
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
93 changes: 61 additions & 32 deletions Libs/DICOM/Core/ctkDICOMDatabase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ QStringList ctkDICOMDatabasePrivate::allFilesInDatabase()

while (allFilesQuery.next())
{
allFileNames << this->absolutePathFromInternal(allFilesQuery.value(0).toString());
allFileNames << this->absolutePathFromInternalIfFile(allFilesQuery.value(0).toString());
Copy link
Member

Choose a reason for hiding this comment

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

Variables that can store urls should not be named as "fileName" (e.g., here "allFileNames") because it is important to make developers aware that the string may contain not just a local file path but it may be a url. Most method names fortunately just use "file" (instead of "filepath"), so they don't need to be renamed.

Suggested change
allFileNames << this->absolutePathFromInternalIfFile(allFilesQuery.value(0).toString());
allUrls << this->absoluteUrl(allFilesQuery.value(0).toString());

Copy link
Member Author

Choose a reason for hiding this comment

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

In general I agree with this, but since the term fileName is used in the schema I felt like keeping this consistency would lead to the most readable code. For the reasons you list in your next comment I'm not sure allUrls is correct either. Perhaps something generic like instanceLocators is more correct. We could make a more sweeping change, including the schema, if we want to generalize.

Part of the reason for this is that some protocols don't use actual URLs to identify the concept of an "DICOM instance". For example, DIMSE doesn't have URLs, but we might want to refer to an instance with something like cget://<pacs ip>/<instance uid>. Currently I have to do this for AHI since frame-level access is a REST API call with dataset and image frame IDs as parameters, so I have a synthetic "URL" string.

}
return allFileNames;
}
Expand Down Expand Up @@ -235,11 +235,12 @@ QString ctkDICOMDatabasePrivate::readValueFromFile(const QString& fileName, cons
return "";
}

QString upperTag = tag.toUpper();
QString value;
unsigned short group, element;
q->tagToGroupElement(tag, group, element);
q->tagToGroupElement(upperTag, group, element);
DcmTagKey tagKey(group, element);
if (this->TagsToExcludeFromStorage.contains(tag))
if (this->TagsToExcludeFromStorage.contains(upperTag))
{
if (dataset.TagExists(tagKey))
{
Expand All @@ -256,7 +257,7 @@ QString ctkDICOMDatabasePrivate::readValueFromFile(const QString& fileName, cons
}

// Store result in cache
q->cacheTag(sopInstanceUID, tag, value);
q->cacheTag(sopInstanceUID, upperTag, value);

return value;
}
Expand Down Expand Up @@ -311,7 +312,7 @@ QStringList ctkDICOMDatabasePrivate::filenames(QString table)

while (allFilesQuery.next())
{
allFileNames << this->absolutePathFromInternal(allFilesQuery.value(0).toString());
allFileNames << this->absolutePathFromInternalIfFile(allFilesQuery.value(0).toString());
}
return allFileNames;
}
Expand Down Expand Up @@ -562,11 +563,12 @@ void ctkDICOMDatabasePrivate::precacheTags(const ctkDICOMItem& dataset, const QS
QStringList sopInstanceUIDs, tags, values;
foreach (const QString &tag, this->TagsToPrecache)
{
QString upperTag = tag.toUpper();
unsigned short group, element;
q->tagToGroupElement(tag, group, element);
q->tagToGroupElement(upperTag, group, element);
DcmTagKey tagKey(group, element);
QString value;
if (this->TagsToExcludeFromStorage.contains(tag))
if (this->TagsToExcludeFromStorage.contains(upperTag))
{
if (dataset.TagExists(tagKey))
{
Expand All @@ -582,7 +584,7 @@ void ctkDICOMDatabasePrivate::precacheTags(const ctkDICOMItem& dataset, const QS
value = dataset.GetAllElementValuesAsString(tagKey);
}
sopInstanceUIDs << sopInstanceUID;
tags << tag;
tags << upperTag;
values << value;
}

Expand Down Expand Up @@ -1076,11 +1078,12 @@ void ctkDICOMDatabase::insert(const QList<ctkDICOMDatabase::IndexingResult>& ind
insertTags.bindValue(0, sopInstanceUID);
foreach(const QString & tag, d->TagsToPrecache)
{
QString upperTag = tag.toUpper();
unsigned short group, element;
this->tagToGroupElement(tag, group, element);
this->tagToGroupElement(upperTag, group, element);
DcmTagKey tagKey(group, element);
QString value;
if (d->TagsToExcludeFromStorage.contains(tag))
if (d->TagsToExcludeFromStorage.contains(upperTag))
{
if (dataset.TagExists(tagKey))
{
Expand All @@ -1095,7 +1098,7 @@ void ctkDICOMDatabase::insert(const QList<ctkDICOMDatabase::IndexingResult>& ind
{
value = dataset.GetAllElementValuesAsString(tagKey);
}
insertTags.bindValue(1, tag);
insertTags.bindValue(1, upperTag);
if (value.isEmpty())
{
insertTags.bindValue(2, TagNotInInstance);
Expand Down Expand Up @@ -1259,7 +1262,7 @@ QString ctkDICOMDatabasePrivate::getDisplaySeriesFieldsKey(QString seriesInstanc
return seriesInstanceUID;
}

logger.error("Failed to find series with SeriesInstanceUID=" + seriesInstanceUID);
logger.error("in getDisplaySeriesFieldsKey: Failed to find series with SeriesInstanceUID=" + seriesInstanceUID);
return QString();
}

Expand Down Expand Up @@ -1375,8 +1378,8 @@ bool ctkDICOMDatabasePrivate::applyDisplayedFieldsChanges( QMap<QString, QMap<QS
}
else
{
logger.error("Failed to find study with StudyInstanceUID=" + currentStudyInstanceUid);
return false;
logger.error("in applyDisplayedFieldsChanges: Failed to find study with StudyInstanceUID=" + currentStudyInstanceUid);
continue;
}
} // For each study in displayedFieldsMapStudy

Expand Down Expand Up @@ -1421,8 +1424,8 @@ bool ctkDICOMDatabasePrivate::applyDisplayedFieldsChanges( QMap<QString, QMap<QS
}
else
{
logger.error("Failed to find series with SeriesInstanceUID=" + currentSeriesInstanceUid);
return false;
logger.error("in applyDisplayedFieldsChanges: Failed to find series with SeriesInstanceUID=" + currentSeriesInstanceUid);
continue;
}
} // For each series in displayedFieldsMapSeries

Expand All @@ -1444,6 +1447,19 @@ QString ctkDICOMDatabasePrivate::internalPathFromAbsolute(const QString& filenam
return ctk::internalPathFromAbsolute(filename, q->databaseDirectory());
}

//------------------------------------------------------------------------------
QString ctkDICOMDatabasePrivate::absolutePathFromInternalIfFile(const QString& filename)
{
Q_Q(ctkDICOMDatabase);
QString returnFileName = filename;
if (QUrl(filename).scheme().isEmpty()) // local path, not url
pieper marked this conversation as resolved.
Show resolved Hide resolved
{
returnFileName = this->absolutePathFromInternal(filename);
}
return returnFileName;
}


//------------------------------------------------------------------------------
CTK_GET_CPP(ctkDICOMDatabase, bool, isDisplayedFieldsTableAvailable, DisplayedFieldsTableAvailable);
CTK_GET_CPP(ctkDICOMDatabase, bool, useShortStoragePath, UseShortStoragePath);
Expand All @@ -1459,7 +1475,7 @@ ctkDICOMDatabase::ctkDICOMDatabase(QString databaseFile)
: d_ptr(new ctkDICOMDatabasePrivate(*this))
{
Q_D(ctkDICOMDatabase);
d->TagsToExcludeFromStorage << groupElementToTag(0x7fe0,0x0010); // pixel data
d->TagsToExcludeFromStorage << groupElementToTag(0x7FE0,0x0010); // pixel data
d->registerCompressionLibraries();
d->init(databaseFile);
}
Expand All @@ -1469,7 +1485,7 @@ ctkDICOMDatabase::ctkDICOMDatabase(QObject* parent)
{
Q_UNUSED(parent);
Q_D(ctkDICOMDatabase);
d->TagsToExcludeFromStorage << groupElementToTag(0x7fe0, 0x0010); // pixel data
d->TagsToExcludeFromStorage << groupElementToTag(0x7FE0, 0x0010); // pixel data
d->registerCompressionLibraries();
}

Expand Down Expand Up @@ -2094,7 +2110,9 @@ QStringList ctkDICOMDatabase::filesForSeries(QString seriesUID, int hits/*=-1*/)
QStringList allFileNames;
while (query.next())
{
allFileNames << d->absolutePathFromInternal(query.value(0).toString());
QString fileName = query.value(0).toString();
fileName = d->absolutePathFromInternalIfFile(fileName);
allFileNames << fileName;
if (hits > 0 && allFileNames.size() >= hits)
{
// reached the number of requested files
Expand All @@ -2115,7 +2133,7 @@ QString ctkDICOMDatabase::fileForInstance(QString sopInstanceUID)
QString result;
if (query.next())
{
result = d->absolutePathFromInternal(query.value(0).toString());
result = d->absolutePathFromInternalIfFile(query.value(0).toString());
}
return result;
}
Expand Down Expand Up @@ -2239,6 +2257,7 @@ void ctkDICOMDatabase::loadFileHeader(QString fileName)
if (dO)
{
QString tag = QString("%1,%2").arg(dO->getGTag(),4,16,QLatin1Char('0')).arg(dO->getETag(),4,16,QLatin1Char('0'));
tag = tag.toUpper();
std::ostringstream s;
dO->print(s);
d->LoadedHeader[tag] = QString(s.str().c_str());
Expand Down Expand Up @@ -2270,6 +2289,7 @@ QString ctkDICOMDatabase::headerValue (QString key)
QString ctkDICOMDatabase::instanceValue(QString sopInstanceUID, QString tag)
{
Q_D(ctkDICOMDatabase);
tag = tag.toUpper();
// Read from cache, if available
QString value = this->cachedTag(sopInstanceUID, tag);
if (value == TagNotInInstance || value == ValueIsEmptyString || value == ValueIsNotStored)
Expand Down Expand Up @@ -2304,6 +2324,7 @@ QString ctkDICOMDatabase::fileValue(const QString fileName, QString tag)
Q_D(ctkDICOMDatabase);

// Read from cache, if available
tag = tag.toUpper();
QString sopInstanceUID = this->instanceForFile(fileName);
QString value = this->cachedTag(sopInstanceUID, tag);
if (value == TagNotInInstance || value == ValueIsEmptyString || value == ValueIsNotStored)
Expand Down Expand Up @@ -2332,7 +2353,8 @@ QString ctkDICOMDatabase::fileValue(const QString fileName, const unsigned short
bool ctkDICOMDatabase::instanceValueExists(const QString sopInstanceUID, const QString tag)
{
Q_D(ctkDICOMDatabase);
QString value = this->cachedTag(sopInstanceUID, tag);
QString upperTag = tag.toUpper();
QString value = this->cachedTag(sopInstanceUID, upperTag);
if (value == TagNotInInstance || value == ValueIsEmptyString)
{
return false;
Expand All @@ -2348,7 +2370,7 @@ bool ctkDICOMDatabase::instanceValueExists(const QString sopInstanceUID, const Q
{
return false;
}
value = d->readValueFromFile(filePath, sopInstanceUID, tag);
value = d->readValueFromFile(filePath, sopInstanceUID, upperTag);
return (value != TagNotInInstance && value != ValueIsEmptyString);
}

Expand All @@ -2363,6 +2385,7 @@ bool ctkDICOMDatabase::instanceValueExists(const QString sopInstanceUID, const u
bool ctkDICOMDatabase::fileValueExists(const QString fileName, QString tag)
{
Q_D(ctkDICOMDatabase);
tag = tag.toUpper();
QString sopInstanceUID = this->instanceForFile(fileName);
QString value = this->cachedTag(sopInstanceUID, tag);
if (value == TagNotInInstance || value == ValueIsEmptyString)
Expand Down Expand Up @@ -2405,7 +2428,8 @@ bool ctkDICOMDatabase::tagToGroupElement(const QString tag, unsigned short& grou
//------------------------------------------------------------------------------
QString ctkDICOMDatabase::groupElementToTag(const unsigned short& group, const unsigned short& element)
{
return QString("%1,%2").arg(group,4,16,QLatin1Char('0')).arg(element,4,16,QLatin1Char('0'));
QString groupElement = QString("%1,%2").arg(group,4,16,QLatin1Char('0')).arg(element,4,16,QLatin1Char('0'));
return groupElement.toUpper();
}

//
Expand Down Expand Up @@ -2507,11 +2531,16 @@ const QStringList ctkDICOMDatabase::tagsToPrecache()
void ctkDICOMDatabase::setTagsToExcludeFromStorage(const QStringList tags)
{
Q_D(ctkDICOMDatabase);
if (d->TagsToExcludeFromStorage == tags)
QStringList upperTags;
foreach (const QString &tag, tags)
{
upperTags << tag.toUpper();
}
if (d->TagsToExcludeFromStorage == upperTags)
{
return;
}
d->TagsToExcludeFromStorage = tags;
d->TagsToExcludeFromStorage = upperTags;
emit tagsToExcludeFromStorageChanged();
}

Expand Down Expand Up @@ -2550,7 +2579,7 @@ bool ctkDICOMDatabase::allFilesModifiedTimes(QMap<QString, QDateTime>& modifiedT
bool success = d->loggedExec(allFilesModifiedQuery);
while (allFilesModifiedQuery.next())
{
QString filename = d->absolutePathFromInternal(allFilesModifiedQuery.value(0).toString());
QString filename = d->absolutePathFromInternalIfFile(allFilesModifiedQuery.value(0).toString());
QDateTime modifiedTime = QDateTime::fromString(allFilesModifiedQuery.value(1).toString(), Qt::ISODate);
if (modifiedTimeForFilepath.contains(filename) && modifiedTimeForFilepath[filename] <= modifiedTime)
{
Expand Down Expand Up @@ -2639,7 +2668,7 @@ bool ctkDICOMDatabase::removeSeries(const QString& seriesInstanceUID, bool clear
// check that the file is below our internal storage
if (QFileInfo(dbFilePath).isRelative())
{
QString absPath = d->absolutePathFromInternal(dbFilePath);
QString absPath = d->absolutePathFromInternalIfFile(dbFilePath);
if (QFile(absPath).remove())
{
if (d->LoggedExecVerbose)
Expand All @@ -2658,7 +2687,7 @@ bool ctkDICOMDatabase::removeSeries(const QString& seriesInstanceUID, bool clear
}
}
// Remove thumbnail (if exists)
QFile thumbnailFile(d->absolutePathFromInternal(thumbnailPath));
QFile thumbnailFile(d->absolutePathFromInternalIfFile(thumbnailPath));
if (thumbnailFile.exists())
{
if (!thumbnailFile.remove())
Expand Down Expand Up @@ -2839,7 +2868,7 @@ QString ctkDICOMDatabase::cachedTag(const QString sopInstanceUID, const QString
QSqlQuery selectValue( d->TagCacheDatabase );
selectValue.prepare( "SELECT Value FROM TagCache WHERE SOPInstanceUID = :sopInstanceUID AND Tag = :tag" );
selectValue.bindValue(":sopInstanceUID",sopInstanceUID);
selectValue.bindValue(":tag",tag);
selectValue.bindValue(":tag",tag.toUpper());
d->loggedExec(selectValue);
QString result("");
if (selectValue.next())
Expand Down Expand Up @@ -2874,7 +2903,7 @@ void ctkDICOMDatabase::getCachedTags(const QString sopInstanceUID, QMap<QString,
QString value;
while (selectValue.next())
{
tag = selectValue.value(0).toString();
tag = selectValue.value(0).toString().toUpper();
value = selectValue.value(1).toString();
if (value == TagNotInInstance || value == ValueIsEmptyString || value == ValueIsNotStored)
{
Expand All @@ -2889,7 +2918,7 @@ bool ctkDICOMDatabase::cacheTag(const QString sopInstanceUID, const QString tag,
{
QStringList sopInstanceUIDs, tags, values;
sopInstanceUIDs << sopInstanceUID;
tags << tag;
tags << tag.toUpper();
values << value;
return this->cacheTags(sopInstanceUIDs, tags, values);
}
Expand Down Expand Up @@ -2925,7 +2954,7 @@ bool ctkDICOMDatabase::cacheTags(const QStringList sopInstanceUIDs, const QStrin
for (int i = 0; i<itemCount; ++i)
{
insertTags.bindValue(0, *sopInstanceUIDsIt);
insertTags.bindValue(1, *tagsIt);
insertTags.bindValue(1, (*tagsIt).toUpper());
if (valuesIt->isEmpty())
{
// replace empty strings with special flag string
Expand Down
3 changes: 3 additions & 0 deletions Libs/DICOM/Core/ctkDICOMDatabase_p.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,9 @@ class CTK_DICOM_CORE_EXPORT ctkDICOMDatabasePrivate
/// Convert an absolute path to an internal path (absolute if outside database folder, relative if inside database folder).
QString internalPathFromAbsolute(const QString& filename);

/// If not a URL, convert to abolute path
QString absolutePathFromInternalIfFile(const QString& filename);

/// Name of the database file (i.e. for SQLITE the sqlite file)
QString DatabaseFileName;

Expand Down
6 changes: 4 additions & 2 deletions Libs/DICOM/Core/ctkDICOMItem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -958,12 +958,14 @@ QString ctkDICOMItem::TranslateDefinedTermModality( const QString& dt )

QString ctkDICOMItem::TagKey( const DcmTag& tag )
{
return QString("(%1,%2)").arg( tag.getGroup(), 4, 16, QLatin1Char('0')).arg( tag.getElement(), 4, 16, QLatin1Char('0') );
QString groupElement = QString("(%1,%2)").arg( tag.getGroup(), 4, 16, QLatin1Char('0')).arg( tag.getElement(), 4, 16, QLatin1Char('0') );
return groupElement.toUpper();
}

QString ctkDICOMItem::TagKeyStripped( const DcmTag& tag )
{
return QString("%1,%2").arg( tag.getGroup(), 4, 16, QLatin1Char('0')).arg( tag.getElement(), 4, 16, QLatin1Char('0') );
QString groupElement = QString("%1,%2").arg( tag.getGroup(), 4, 16, QLatin1Char('0')).arg( tag.getElement(), 4, 16, QLatin1Char('0') );
return groupElement.toUpper();
}

QString ctkDICOMItem::TagDescription( const DcmTag& tag )
Expand Down
Loading