Update to 1.0 #56
Update to 1.0 #56
Conversation
Some DICOMS (eg. Leica) don't have this element.
test suite still crashes though, needs some cleaning up
dcm-dump and getframe both work now
now uses almost the same callbacks as the main parser
with a special parse thing that just records the index
Many DICOMs seem to have overlong DT fields, so make this check a warning rather than a fatal error. See openslide/openslide#439 (comment)
and move implicit into the parser state
- no more BOT object, it's built into filehandle - a new API call dcm_filehandle_read_pixeldata() gets ready for frame fetch - dcm_filehandle_read_frame() no longer needs metadata or bot
we never used it
So we don't load all of localizer images unnecessarily.
We were just printing length.
We were using char*
…into yarikoptic-enh-codespell
ooops, two silly errors
Meson >= 0.60 lets users filter the installed files with
`meson install --tags`. Tell Meson that the tools are "executables
bundled with a library meant to be used by end users". Older Meson
versions complain about the install_tags argument but don't fail:
WARNING: Passed invalid keyword argument "install_tag"
meson: apply `bin` tag to tools
doc/source/usage.rst
Outdated
| :c:func:`dcm_filehandle_get_metadata()`. This function will stop read on tags | ||
| which are likely to take a long time to process. |
There was a problem hiding this comment.
Does is stop on any tags other than Pixel Data tags? These tags were given very high numbers so that they end up being located at the end of the data set.
If it's only Pixel Data tags, then I suggest stating that to not confuse the reader and make them wonder about the implementation details and behavior.
There was a problem hiding this comment.
Yes, it'll stop on any of these tags:
const DcmDataSet *dcm_filehandle_get_metadata(DcmError **error,
DcmFilehandle *filehandle)
{
// we stop on any of the tags that start a huge group that
// would take a long time to parse
static uint32_t stop_tags[] = {
TAG_REFERENCED_IMAGE_NAVIGATION_SEQUENCE,
TAG_PER_FRAME_FUNCTIONAL_GROUP_SEQUENCE,
TAG_PIXEL_DATA,
TAG_FLOAT_PIXEL_DATA,
TAG_DOUBLE_PIXEL_DATA,
0,
};I wanted to avoid listing the tags explicitly so people didn't depend on this behaviour and we could change it in the future without breaking compatibility.
There was a problem hiding this comment.
The TAG_PER_FRAME_FUNCTIONAL_GROUP_SEQUENCE is unfortunately very bulky, but strictly shouldn't be considered a bulkdata element (the sequence is large but the individual nested elements are small).
I suggest loading this metadata element, even if it may take a bit to parse.
There was a problem hiding this comment.
It can take many, many seconds to parse, unfortunately.
I tried a test with a 1.9gb Leica DICOM I have:
john@banana ~/pics/openslide/dicom/272320e7-5abb-ccac-8f8b-9c2008f53845_114626 $ time openslide-show-properties 1.3.6.1.4.1.36533.1214640142408762111756320324051133413.dcm > log
real 0m0.083s
user 0m0.012s
sys 0m0.071s
So about 0.1s to assemble the files into a pyramid and get the metadata. If I disable the skip-per-frame-functional-group-sequence feature I see:
john@banana ~/pics/openslide/dicom/272320e7-5abb-ccac-8f8b-9c2008f53845_114626 $ time openslide-show-properties 1.3.6.1.4.1.36533.1214640142408762111756320324051133413.dcm > log
real 0m5.412s
user 0m5.423s
sys 0m0.493s
Over 5s, so around 50x slower. I think a 5s pause on file open would make interactive viewing of DICOM files frustrating, and there's no benefit, since the information is not useful.
| You can read all metadata and control read stop using a sequence of calls to | ||
| :c:func:`dcm_filehandle_read_metadata()`. |
There was a problem hiding this comment.
It appears the implementation has changed. Metadata in DICOM is defined as elements with a value below a given size threshold.
A way to parametrize the behavior could be to pass an size_threshold argument (which by default could be a relatively large number of bytes). This would allow the caller to tune the behavior to their needs, rather than having to call the function several times.
In general, there are only a few elements that tend to have large elements (Pixel Data, ICC Profile, etc.).
There was a problem hiding this comment.
It's a speed thing -- we need to stop parsing as soon as we see a tag that might potentially be dodgy, otherwise it's too late and we're committed to reading something horrible like TAG_REFERENCED_IMAGE_NAVIGATION_SEQUENCE.
There was a problem hiding this comment.
I have little experience with TAG_REFERENCED_IMAGE_NAVIGATION_SEQUENCE, but guess that it's similarly bad as TAG_PER_FRAME_FUNCTIONAL_GROUP_SEQUENCE.
While these sequence elements are annoying and take time to parse, their content is considered part of the metadata and are generally needed to understand the pixel data element. Therefore, I suggest reading them along with the rest of the metadata.
There was a problem hiding this comment.
I tried enabling read of TAG_REFERENCED_IMAGE_NAVIGATION_SEQUENCE and TAG_PER_FRAME_FUNCTIONAL_GROUP_SEQUENCE.
john@banana ~/pics/openslide/dicom/272320e7-5abb-ccac-8f8b-9c2008f53845_114626 $ time openslide-show-properties 1.3.6.1.4.1.36533.1214640142408762111756320324051133413.dcm > log
real 0m8.913s
user 0m8.151s
sys 0m0.748s
I think 9s is too slow, and for openslide at least, the information is not useful.
There was a problem hiding this comment.
You can use /usr/bin/time -f %M:%e ... to get peak ram use for a command, which can be very useful. With git master libdicom I see:
john@banana ~/pics/openslide/dicom/272320e7-5abb-ccac-8f8b-9c2008f53845_114626 $ /usr/bin/time -f %M:%e openslide-show-properties 1.3.6.1.4.1.36533.1214640142408762111756320324051133413.dcm > log
115148:0.06
So 115mb of memory and 60ms. If I enable the parsing of these two elements I see:
john@banana ~/pics/openslide/dicom/272320e7-5abb-ccac-8f8b-9c2008f53845_114626 $ /usr/bin/time -f %M:%e openslide-show-properties 1.3.6.1.4.1.36533.1214640142408762111756320324051133413.dcm > log
995708:8.91
Almost 1gb of memory use, and 9s. That's more than 100x slower, almost 10x more memory use, and no benefit for openslide.
There was a problem hiding this comment.
For the dcm-dump tool, we could introduce a --stop-tag option that enables users to limit reading of metadata.
I agree, that would be a nice addition (dcmdump has something similar).
There was a problem hiding this comment.
Perhaps renaming dcm_filehandle_get_metadata() as dcm_filehandle_get_metadata_subset() would be clearer?
There was a problem hiding this comment.
I've done the rename.
There was a problem hiding this comment.
I really think these kind of performance optimizations belong into OpenSlide.
Providing the caller a (poorly defined) subset of the metadata is in my opinion against our core principle of providing a library that is free of surprises. The library includes functions that allow an application to load metadata in a more sophisticated, performance-optimized manner if needed. We can give examples in the documentation on how this can be achieved.
There was a problem hiding this comment.
dcm_filehandle_get_metadata_subset() is an optional call which can help performance, so to my mind it seems OK.
The main metadata API call (dcm_filehandle_read_metadata()) is very surprise-free, don't you think? One call, reads in all the metadata.
| * the DcmVRTag enum. As the DICOM standard evolves, numbering must be | ||
| * maintained for ABI compatibility. | ||
| */ | ||
| typedef enum _DcmVR { |
There was a problem hiding this comment.
I just wanted to look at the online documentation at https://libdicom.readthedocs.io to see whether types and functions with names starting with an underscore get included in the API documentation.
Unfortunately, I realized that the API documentation is empty: https://libdicom.readthedocs.io/en/latest/api.html
There was a problem hiding this comment.
There's a formatted copy of the docs here:
There was a problem hiding this comment.
Thanks for the link!
It looks like these private type definitions get included into the API documentation: http://www.rollthepotato.net/~john/libdicom/api.html#c._DcmVR
It would be nice if we could configure Sphinx or Hawkmoth to ignore declarations with a leading underscore. My understanding is that they are considered private and shouldn't be relied up by users.
There was a problem hiding this comment.
Regarding the empty API doc page, is there an issue with the API doc compilation?
There was a problem hiding this comment.
I've not updated that copy of the formatted docs for a little while, I'll update it now. You can generate the docs on your machine too, of course.
There was a problem hiding this comment.
I've updated the formatted docs again for these renames.
| * Scans the PixelData sequence and loads the PerFrameFunctionalGroupSequence, | ||
| * if present. |
There was a problem hiding this comment.
The Per-Frame Functional Group Sequence is not part of the Pixel Data element.
Unless the function reads the entire pixel data, I suggest renaming it. Maybe dcm_filehandle_seek_pixeldata?
There was a problem hiding this comment.
It scans pixeldata and finds all the offsets, and also loads in per frame functional group sequence, if present.
There was a problem hiding this comment.
The name dcm_filehandle_read_pixeldata implies that the function reads the entire pixel data elements and loads the data into memory.
It scans pixeldata and finds all the offsets
I suggest naming the function dcm_filehandle_find_frame_offsets, dcm_filehandle_build_frame_offset_table, or something along those lines.
also loads in per frame functional group sequence, if present
I understand the motivation to group reading of the per-frame functional groups items with reading the frame offset items. However, conceptually they are two different things since the latter is part of the pixel data element and the former are metadata elements. Therefore, I would keep them separate.
There was a problem hiding this comment.
In my mind it was something like "get ready to read pixeldata", so it fetches all the parts that will be required for that.
Maybe dcm_filehandle_prepare_read_frame()?
There was a problem hiding this comment.
What bothers me about this function is that its behavior is opaque and that it introduces state.
Previously, the caller needed to get the (complete) metadata and the frame offset table prior to reading a frame and that information needed to get passed to the read_frame function. Now, a number of functions need to be called in the right order and it's not clear why this is the case and what's happening under the hood.
Thoughts?
There was a problem hiding this comment.
You can call the functions in any order, and they are all optional. Although there is internal state (necessary for performance), the user doesn't need to know about it.
dcm_filehandle_prepare_read_frame() is only split out as a separate function for programs which want to control the delay you get with the first read_frame.
| DcmFrame *dcm_filehandle_read_frame_position(DcmError **error, | ||
| DcmFilehandle *filehandle, | ||
| uint32_t column, | ||
| uint32_t row); |
There was a problem hiding this comment.
This assumes that the DICOM object represents a tiled image.
I suggest renaming to dcm_filehandle_read_frame_at_tile_position to make that more clear. I also suggest implementing a check to confirm that this assumption is met.
There was a problem hiding this comment.
It'll work for non-tiled images too -- just set col, row to (0, 0).
There was a problem hiding this comment.
Hmm...I find this behavior surprising, because non-tiled images don't really have any columns or rows. I would find it cleaner to implement a dcm_filehandle_is_tiled_image function to check whether the DICOM object is a tiled image (we may also want dcm_filehandle_is_image, which can be called internally) and fails if it isn't.
See corresponding Python implementation: https://github.com/ImagingDataCommons/dicomslide/blob/dc63981c5ac5a3a49e74e9ed373d06d5ebd3d46e/src/dicomslide/utils.py#L34
There was a problem hiding this comment.
I imagined a one-frame image (perhaps the label, for example) as a 1x1 array, so dcm_filehandle_read_frame_position() can make sense, I think. It makes openslide a bit simpler:
You can see the get-associated-image-pixel-data function can just call off into the main fetch-a-tile, it doesn't need a special read-untiled function, and it doesn't need an is-tiled tester.
There was a problem hiding this comment.
We should design the library not just with OpenSlide in mind. In my opinion, the library should be useful beyond whole slide imaging. Indexing image frames at a (column, row) position only makes sense for tiled images. If the image is not tiled, there is per definition no tile to be found at (0, 0) position.
OpenSlide will need to check for the image type and handle "associated images" differently.
There was a problem hiding this comment.
Openslide was just an example use case -- I think all programs would benefit from a single code path for both tiled and untiled images.
It checks that (column, row) is (0, 0) for untiled images, so I think it's safe and reasonable. Users can always call read_frame(0) if they wish.
There was a problem hiding this comment.
I agree John, let's close this discussion so that we can release this, unless anyone strenuously objects.
Co-authored-by: Markus D. Herrmann <hackermd@users.noreply.github.com>
rename dcm_filehandle_read_pixeldata() as dcm_filehandle_prepare_read_frame() rename dcm_filehandle_get_metadata() as dcm_filehandle_get_metadata_subset() revise docs and tests
We were treating AT as a string rather than a fixed length numeric array. Improve display with dcm-dump as well.
since "check" needs 0.54.1
|
Looks like the windows runner is failing for some config reason. Maybe |
|
There is an error about missing Ninja (https://github.com/ImagingDataCommons/libdicom/actions/runs/6283904801/job/17064777492?pr=56#step:6:210) - maybe try adding its setup as part of the workflow? https://github.com/marketplace/actions/install-ninja-build-tool
|
|
Looks like Ninja was recently removed from the Windows images: actions/runner-images#8348 |
this was installed transitively before
|
Woo! I'll kick the tyres a bit, then tag it as 1.0-rc1 for downstream integration and further tests. |
This PR updates libdicom to 1.0.
The headline changes are:
New parser
The DICOM file parser has been split off to
dicom-parse.c. It's a callback-based parser and knows about the internal structure of PixelData. This generic parser is used bydicom-file.cto load parts of DICOM files into memory, to scan files for features, and to print files to stdout.This parse API is only internal, for now at least.
Revised Filehandle API
The various filehandle API calls can now be called any number of times, and in any order. They are all optional, so it's now possible to simply open a file and immediately call
dcm_filehandle_read_frame().dcm_filehandle_read_file_meta()anddcm_filehandle_read_metadata()have been renamed asdcm_filehandle_get_file_meta()anddcm_filehandle_get_metadata(). These new functions return aconstpointer to libdicom's internal copy of the parsed metadata, and should NOT be destroyed. Usecloneto make a copy if you need the result to live longer.The metadata returned by
dcm_filehandle_get_metadata()is only the metadata which can be read quickly and without using much memory. To read all metadata (so including, for example, the sometimes extremely largePerFrameFunctionalGroupSequence) there's a new API calldcm_filehandle_read_metadata(). This function takes a set of stop tags. If necessary, it can be called many times to read all the file metadata.There's a new API call
dcm_filehandle_read_frame_position()which will read a frame at a certain (column, row) position. It automatically takes account of any ordering inPerFrameFunctionalGroupSequence, if present.DcmBOTis no longer exposed ion the API, since this is now all handled automatically.Automatic handling of byte ordering and implicit versus explicit encoding has been improved.
Revised data model
dcm_sequence_foreach()anddcm_dataset_foreach()have a client pointer, allow early termination, and track sequence index.A new function,
dcm_element_set_value(), can set the value of an element from a generic byte buffer.A new function,
dcm_element_value_to_string(), makes a formatted character string representing the value of an element. It is handy for displaying values to users in an understandable way.Some more of
dicom-dict.cis in the public API, notablyDcmVRClassand associated functions. This was needed by openslide.Revised logging
There's now
dcm_log_set_level()to set the log level (rather than a global variable).If the environment variable
DCM_DEBUGis set, logging defaults toDCM_LOG_DEBUGindcm_init(). For example:New
dcm-dumpThere's a new function,
dcm_filehandle_print(), which prints all metadata in a file, including pixeldata. This is the function used bydcm-dump.The output looks like eg.:
You can see it's displaying the first few bytes of the frame in hex. It knows about OW and OF and OD as well, and encapsulated and native pixeldata.
It has the very nice feature of printing as it parses, so it can display DICOM files of any size very quickly and only using a little memory.
New argument parser
The two tools now use something like getopt to parse command-line arguments and switches, making them more consistent and easier to use.