Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
## main

* add support for encapsulated pixel data reading [weanti]
* fix one-byte overread into struct padding [bgilbert]
* support single-frame DICOM images and allow BitsStored > 8 [tokyovigilante]

Expand Down
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
7 changes: 0 additions & 7 deletions src/dicom-data.c
Original file line number Diff line number Diff line change
Expand Up @@ -1813,13 +1813,6 @@ DcmFrame *dcm_frame_create(DcmError **error,
return NULL;
}

if (bits_stored != 1 && bits_stored % 8 != 0) {
dcm_error_set(error, DCM_ERROR_CODE_INVALID,
"constructing frame item failed",
"wrong number of bits stored");
return NULL;
}

if (pixel_representation != 0 && pixel_representation != 1) {
dcm_error_set(error, DCM_ERROR_CODE_INVALID,
"constructing frame item failed",
Expand Down
25 changes: 19 additions & 6 deletions src/dicom-file.c
Original file line number Diff line number Diff line change
Expand Up @@ -1365,12 +1365,25 @@ DcmFrame *dcm_filehandle_read_frame(DcmError **error,
return NULL;
}

uint32_t length;
char *frame_data = dcm_parse_frame(error,
filehandle->io,
filehandle->implicit,
&filehandle->desc,
&length);
const char *syntax = dcm_filehandle_get_transfer_syntax_uid(filehandle);
uint32_t length = 0;
char* frame_data = NULL;
if (dcm_is_encapsulated_transfer_syntax(syntax)) {
int64_t frame_end_offset = frame_number < filehandle->num_frames ?
filehandle->offset_table[i+1] : 0xFFFFFFFF;
frame_data = dcm_parse_encapsulated_frame(error,
filehandle->io,
filehandle->implicit,
frame_end_offset,
&length );
} else {
frame_data = dcm_parse_frame(error,
filehandle->io,
filehandle->implicit,
&filehandle->desc,
&length);
}

if (frame_data == NULL) {
return NULL;
}
Expand Down
157 changes: 111 additions & 46 deletions src/dicom-parse.c
Original file line number Diff line number Diff line change
Expand Up @@ -954,61 +954,75 @@ bool dcm_parse_pixeldata_offsets(DcmError **error,
// the BOT is missing, we must scan pixeldata to find the position of
// each frame

// we could use our generic parser above ^^ but we have a special loop
// here as an optimisation (we can skip over the pixel data itself)

dcm_log_info("building Offset Table from Pixel Data");

// 0 in the BOT is the offset to the start of frame 1, ie. here
*first_frame_offset = position;

position = 0;
for (int i = 0; i < num_frames; i++) {
if (!read_tag(&state, &tag, &position) ||
!read_uint32(&state, &length, &position)) {
return false;
// each frame may consist of several fragments, so we need to scan each fragment to find the next frame
Copy link
Collaborator

@jcupitt jcupitt Nov 8, 2025

Choose a reason for hiding this comment

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

I don't think you need this complicated loop. I think (I think! I hope!) all you need to do is scan num_frames items and collect the offsets. This will collect the right number of frames whether or not frames are split into many items.

How about changing this back to:

turn false;
        }
    } else {
        // the BOT is missing, we must scan pixeldata to find the position of
        // each frame
   
        dcm_log_info("building Offset Table from Pixel Data");
   
        // 0 in the BOT is the offset to the start of frame 1, ie. here
        *first_frame_offset = position;

        position = 0;
        for (int i = 0; i < num_frames; i++) {
            if (!read_tag(&state, &tag, &position) ||
                !read_uint32(&state, &length, &position)) {
                return false;
            }

            if (tag == TAG_SQ_DELIM) {
                dcm_error_set(error, DCM_ERROR_CODE_PARSE,
                              "reading BasicOffsetTable failed",
                              "too few frames in PixelData");
                return false;
            }
         
            if (tag != TAG_ITEM) {
                dcm_error_set(error, DCM_ERROR_CODE_PARSE,
                              "building BasicOffsetTable failed",
                              "frame Item #%d has wrong tag '%08x'",
                              i + 1,
                              tag);
                return false;
            }
            
            // step back to the start of the item for this frame
            offsets[i] = position - 8;
            
            // and seek forward over the value
            if (!dcm_seekcur(&state, length, &position)) {
                return false;
            }
        }
    }

    return true;
}

ie. just removing the check for the end of sequence tag.

I suppose we could check for end-of-sequence in the num_frames > 1 case, but I don't know if it'd add much.

if (!read_tag(&state, &tag, &position)) {
dcm_error_set(error, DCM_ERROR_CODE_PARSE,
"building BasicOffsetTable failed",
"failed to read tag");
return false;
}
// all fragments belong to the only frame
if ( num_frames == 1 ) {
// according to the standard the first frame's offset is 0
offsets[0] = 0;
} else {
// 1 fragment shall contain 1 frame
int fragment_idx = 0;
offsets[fragment_idx] = 0; // by definition the first fragment is at offset 0
fragment_idx++;
while( tag != TAG_SQ_DELIM ) {
if ( tag != TAG_ITEM || !read_uint32(&state, &length, &position)) {
dcm_error_set(error, DCM_ERROR_CODE_PARSE,
"building BasicOffsetTable failed",
"failed to read fragment");
return false;
}
// skip actual content to find next offset
dcm_seekcur(&state, length, &position);
if (!read_tag(&state, &tag, &position)) {
dcm_error_set(error, DCM_ERROR_CODE_PARSE,
"building BasicOffsetTable failed",
"failed to read tag");
return false;
}
if ( fragment_idx < num_frames ) {
offsets[fragment_idx] = offsets[fragment_idx-1] + 8/*tag and length field size*/ + length;
}
fragment_idx++;
}

if (tag == TAG_SQ_DELIM) {
dcm_error_set(error, DCM_ERROR_CODE_PARSE,
"reading BasicOffsetTable failed",
"too few frames in PixelData");
// fragment_idx shall equal to num_frames+1 at the end
fragment_idx--;
if ( fragment_idx > num_frames ) {
dcm_error_set(error, DCM_ERROR_CODE_INVALID,
"building BasicOffsetTable failed",
"Too many fragments" );
return false;
}

if (tag != TAG_ITEM) {
dcm_error_set(error, DCM_ERROR_CODE_PARSE,
if ( num_frames < fragment_idx ) {
dcm_error_set(error, DCM_ERROR_CODE_INVALID,
"building BasicOffsetTable failed",
"frame Item #%d has wrong tag '%08x'",
i + 1,
tag);
"Too many frames" );
return false;
}

// step back to the start of the item for this frame
offsets[i] = position - 8;

// and seek forward over the value
if (!dcm_seekcur(&state, length, &position)) {
if ( !read_uint32(&state, &length, &position ) || length != 0 ) {
dcm_error_set(error, DCM_ERROR_CODE_PARSE,
"building BasicOffsetTable failed",
"Sequence Delimiter Tag failure" );
return false;
}
}

// the next thing should be the end of sequence tag
if (!read_tag(&state, &tag, &position)) {
return false;
}
if (tag != TAG_SQ_DELIM) {
dcm_error_set(error, DCM_ERROR_CODE_PARSE,
"reading BasicOffsetTable failed",
"too many frames in PixelData");
return false;
}
}

return true;
}


char *dcm_parse_frame(DcmError **error,
DcmIO *io,
bool implicit,
Expand All @@ -1022,33 +1036,84 @@ char *dcm_parse_frame(DcmError **error,
.big_endian = is_big_endian(),
};

*length = desc->rows * desc->columns * desc->samples_per_pixel * (desc->bits_allocated / 8);

char *value = DCM_MALLOC(error, *length);
if (value == NULL) {
return NULL;
}
int64_t position = 0;
if (!dcm_require(&state, value, *length, &position)) {
free(value);
return NULL;
}

if (dcm_is_encapsulated_transfer_syntax(desc->transfer_syntax_uid)) {
uint32_t tag;
if (!read_tag(&state, &tag, &position) ||
!read_uint32(&state, length, &position)) {
return value;
}

/* Read encapsulated frame. Return NULL in case of error.
*/
char *dcm_parse_encapsulated_frame(DcmError **error,
DcmIO *io,
bool implicit,
int64_t frame_end_offset,
uint32_t* length)
{
DcmParseState state = {
.error = error,
.io = io,
.implicit = implicit,
.big_endian = is_big_endian(),
};

int64_t position = 0;
*length = 0;
uint32_t tag;
uint32_t fragment_length = 0;

// first determine the total length of bytes to be read
while(position < frame_end_offset) {
if (!read_tag(&state, &tag, &position)) {
return NULL;
}

if (tag == TAG_SQ_DELIM) {
break;
}
if (tag != TAG_ITEM) {
dcm_error_set(error, DCM_ERROR_CODE_PARSE,
"reading frame item failed",
"no item tag found for frame item");
return NULL;
}
} else {
*length = desc->rows * desc->columns * desc->samples_per_pixel *
(desc->bits_allocated / 8);
if (!read_uint32(&state, &fragment_length, &position)) {
return NULL;
}
dcm_seekcur(&state, fragment_length, &position);
*length += fragment_length;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose there's a potential uint32 overflow here. length should ideally be a uint64, though I don't suppose it matters much.

}

char *value = DCM_MALLOC(error, *length);
if (value == NULL) {
return NULL;
}
if (!dcm_require(&state, value, *length, &position)) {
free(value);
return NULL;
// if frame end is unknown/undefined then update it
if (frame_end_offset == 0xFFFFFFFF) {
frame_end_offset = position;
}
// reposition to the beginning of encapsulated pixel data
dcm_seekcur(&state, -position, &position);

fragment_length = 0;
char* fragment = value;
position = 0;
while(position < frame_end_offset) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

read_tag(&state, &tag, &position);
read_uint32(&state, &fragment_length, &position);
if (!dcm_require(&state, fragment, fragment_length, &position)) {
free(value);
return NULL;
}
fragment += fragment_length;
}

return value;
Expand Down
6 changes: 6 additions & 0 deletions src/pdicom.h
Original file line number Diff line number Diff line change
Expand Up @@ -167,3 +167,9 @@ char *dcm_parse_frame(DcmError **error,
bool implicit,
struct PixelDescription *desc,
uint32_t *length);

char *dcm_parse_encapsulated_frame(DcmError **error,
DcmIO *io,
bool implicit,
int64_t frame_end_offset,
uint32_t* length);
Loading