[WIP] Avoid truncating valid PixelData by computing the default expected frames instead of assuming 1 #2041
Conversation
Updated pixel_data_handlers.util.get_expected_length() - Accounts for predicted number of frames if the tag NumberOfFrames is not defined and the rows/columns reflect the size of a single frame in a multiframe DICOM slice. - Does not remove previous behavior of defaulting to 1 frame if NumberOfFrames is defined as None. - Refactored variable naming to reflect when computing pixels vs. bytes for the expected length. - closes pydicom#2035
Adding unit tests to check get_expected_length() for pixel and byte counts for multiframe DICOM missing NumberOfFrames in the header.
Fixed leftover if statement in test.
for more information, see https://pre-commit.ci
| @@ -1016,27 +1018,29 @@ def get_expected_length(ds: "Dataset", unit: str = "bytes") -> int: | |||
| columns = cast(int, ds.Columns) | |||
| samples_per_pixel = cast(int, ds.SamplesPerPixel) | |||
| bits_allocated = cast(int, ds.BitsAllocated) | |||
| pixel_data_buffer_length = cast(int, len(ds.PixelData)) | |||
There was a problem hiding this comment.
Reliance on the length of Pixel Data will return incorrect results for compressed transfer syntaxes, particularly JPEG when there's no Basic Offset Table. You may want to limit this to just uncompressed transfer syntaxes.
There was a problem hiding this comment.
Hmm, I assumed that PixelData would always be in an uncompressed state after the library parsed the input slice. You bring up a good point. Without an accurate count of the pixel bytes we have any attempts at predicting the likely frame count present will be unreliable as well. I will need to think about this.
|
The two functions you're modifying are used by the (to be) deprecated |
Do you mean to review pixel.decoders to ensure it addresses this issue as well? Also, yes, I was targeting the pixel_data_handlers to help pre v3 updates correct the issue until the new version release is ready and deprecated this module. I will try to do some proper adjustments and testing locally towards the end of the week. I did not have a chance to set up everything in my regular development environment (I was on a trip when I created the initial PR with a tablet). Thank you! |
Describe the changes
Addresses #2035 .
Basically, I compute the nearest integer number of frames based on actual length of pixel data divided by number of bytes computed. I then pass this value as the default return for get_nr_frames. If NumberOfFrames is undefined, we default to default number of frames, which is our predicted frame count.
The issue is that the current get_nr_frames assumes that a multiframe DICOM slice has only 1 frame when NumberOfFrames is not defined. This is not true in all cases and it is less accurate if a service omits adding the NumberOfFrames to header but still concatenates n frames into the PixelData buffer. The service also leaves Rows and Columns set such that it only reflects the dimensions of a single frame within this larger buffer. Calling pixel_array then truncates the data and returns only the first frame.
Added accompanying tests.
Updated comments to reflect the new requirement of PixelData.
Tasks
doc/_build/html/index.html)