From 87c04367c937d49b5bbfe9ec4bc44e7b789c91e7 Mon Sep 17 00:00:00 2001 From: Antal Ispanovity Date: Thu, 26 Jun 2025 15:03:33 +0200 Subject: [PATCH 01/11] remove restriction for bits stored, because its value can be modality specific e.g. in case of CT it can be 12,13,14,15,16 --- src/dicom-data.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/dicom-data.c b/src/dicom-data.c index 38f536b..be360ba 100644 --- a/src/dicom-data.c +++ b/src/dicom-data.c @@ -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", From 835ed96fa4865d6980785280a547f4565024fd52 Mon Sep 17 00:00:00 2001 From: Antal Ispanovity Date: Thu, 26 Jun 2025 15:06:18 +0200 Subject: [PATCH 02/11] fix offset reading and frame reading for encapsulated pixel data --- src/dicom-parse.c | 139 +++++++++++++++++++++++++++++++--------------- 1 file changed, 93 insertions(+), 46 deletions(-) diff --git a/src/dicom-parse.c b/src/dicom-parse.c index 2314189..c56d7df 100644 --- a/src/dicom-parse.c +++ b/src/dicom-parse.c @@ -954,55 +954,71 @@ 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; - + // each frame may consist of several fragments, so we need to scan each fragment to find the next frame + if (!read_tag(&state, &tag, &position)) + { + dcm_error_set(error, DCM_ERROR_CODE_PARSE, + "building BasicOffsetTable failed", + "failed to read tag"); + return false; + } position = 0; - for (int i = 0; i < num_frames; i++) { - if (!read_tag(&state, &tag, &position) || - !read_uint32(&state, &length, &position)) { - 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] = 0x0; + } + else + { + // 1 fragment shall contain 1 frame + int num_fragments = 0; + 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; + } + dcm_seekcur(&state, length, &position); + offsets[num_fragments] = position - 8; + num_fragments++; + if (!read_tag(&state, &tag, &position)) + { + dcm_error_set(error, DCM_ERROR_CODE_PARSE, + "building BasicOffsetTable failed", + "failed to read tag"); + return false; + } + if ( num_fragments > num_frames ) + { + dcm_error_set(error, DCM_ERROR_CODE_INVALID, + "building BasicOffsetTable failed", + "Too many fragments" ); + return false; + } } - - if (tag == TAG_SQ_DELIM) { - dcm_error_set(error, DCM_ERROR_CODE_PARSE, - "reading BasicOffsetTable failed", - "too few frames in PixelData"); + if ( num_frames != num_fragments ) + { + dcm_error_set(error, DCM_ERROR_CODE_INVALID, + "building BasicOffsetTable failed", + "Too many frames" ); return false; } - if (tag != TAG_ITEM) { + if ( !read_uint32(&state, &length, &position ) || length != 0 ) + { dcm_error_set(error, DCM_ERROR_CODE_PARSE, "building BasicOffsetTable failed", - "frame Item #%d has wrong tag '%08x'", - i + 1, - tag); + "Sequence Delimiter Tag failure" ); 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; - } - } - - // 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; } } @@ -1023,24 +1039,55 @@ char *dcm_parse_frame(DcmError **error, }; int64_t position = 0; - if (dcm_is_encapsulated_transfer_syntax(desc->transfer_syntax_uid)) { + *length = 0; uint32_t tag; - if (!read_tag(&state, &tag, &position) || - !read_uint32(&state, length, &position)) { + if ( !read_tag(&state, &tag, &position) ) + { return NULL; } - - if (tag != TAG_ITEM) { - dcm_error_set(error, DCM_ERROR_CODE_PARSE, - "reading frame item failed", - "no item tag found for frame item"); + uint32_t fragment_length = 0; + while( tag != TAG_SQ_DELIM ) + { + 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; + } + if (!read_uint32(&state, &fragment_length, &position)) + { + return NULL; + } + dcm_seekcur(&state, fragment_length, &position); + *length += fragment_length; + if (!read_tag(&state, &tag, &position)) + { + return NULL; + } + } + char *value = DCM_MALLOC(error, *length); + if (value == NULL) { return NULL; } - } else { - *length = desc->rows * desc->columns * desc->samples_per_pixel; + // reposition to the beginning of encapsulated pixel data + dcm_seekcur(&state, -position, &position); + + fragment_length = 0; + char* fragment = value; + read_tag(&state, &tag, &position); + while( tag != TAG_SQ_DELIM ) + { + read_uint32(&state, &fragment_length, &position); + dcm_read(&state, fragment, fragment_length, &position ); + fragment += fragment_length; + read_tag(&state, &tag, &position); + } + return value; } + *length = desc->rows * desc->columns * desc->samples_per_pixel; + char *value = DCM_MALLOC(error, *length); if (value == NULL) { return NULL; From d190231ab29a3dab2d04ddd24b9946191870be35 Mon Sep 17 00:00:00 2001 From: Antal Ispanovity Date: Mon, 30 Jun 2025 07:16:55 +0200 Subject: [PATCH 03/11] fix offset calculation for encapsualted fragments: length was essentially not read, undefined length was assumed use more intuitive offset calculation: previous offset + tag and length size + current length --- src/dicom-parse.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/dicom-parse.c b/src/dicom-parse.c index c56d7df..3d2b12e 100644 --- a/src/dicom-parse.c +++ b/src/dicom-parse.c @@ -966,7 +966,6 @@ bool dcm_parse_pixeldata_offsets(DcmError **error, "failed to read tag"); return false; } - position = 0; // all fragments belong to the only frame if ( num_frames == 1 ) { @@ -977,9 +976,11 @@ bool dcm_parse_pixeldata_offsets(DcmError **error, { // 1 fragment shall contain 1 frame int num_fragments = 0; + offsets[num_fragments] = 0; // by definition the first fragment is at offset 0 while( tag != TAG_SQ_DELIM ) { - if ( tag != TAG_ITEM && !read_uint32(&state, &length, &position)) + num_fragments++; + if ( tag != TAG_ITEM || !read_uint32(&state, &length, &position)) { dcm_error_set(error, DCM_ERROR_CODE_PARSE, "building BasicOffsetTable failed", @@ -987,8 +988,7 @@ bool dcm_parse_pixeldata_offsets(DcmError **error, return false; } dcm_seekcur(&state, length, &position); - offsets[num_fragments] = position - 8; - num_fragments++; + offsets[num_fragments] = offsets[num_fragments-1] +8 + length; if (!read_tag(&state, &tag, &position)) { dcm_error_set(error, DCM_ERROR_CODE_PARSE, From 0ceb9cc180d114f4dbd9d4a6b571b10fa92c2734 Mon Sep 17 00:00:00 2001 From: Antal Ispanovity Date: Tue, 1 Jul 2025 20:47:39 +0200 Subject: [PATCH 04/11] split regular and encapsulated frame parsing fix indexing problem when creating offsets table fix frame size calculation: bits allocated was not considered --- src/dicom-file.c | 26 +++++++-- src/dicom-parse.c | 138 +++++++++++++++++++++++++++------------------- src/pdicom.h | 6 ++ 3 files changed, 108 insertions(+), 62 deletions(-) diff --git a/src/dicom-file.c b/src/dicom-file.c index 9faa187..f3127ea 100644 --- a/src/dicom-file.c +++ b/src/dicom-file.c @@ -1351,12 +1351,26 @@ 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); + uint32_t length = 0; + char* frame_data = NULL; + if ( dcm_is_encapsulated_transfer_syntax(filehandle->desc.transfer_syntax_uid) ) + { + 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; } diff --git a/src/dicom-parse.c b/src/dicom-parse.c index 3d2b12e..80f825b 100644 --- a/src/dicom-parse.c +++ b/src/dicom-parse.c @@ -970,16 +970,16 @@ bool dcm_parse_pixeldata_offsets(DcmError **error, if ( num_frames == 1 ) { // according to the standard the first frame's offset is 0 - offsets[0] = 0x0; + offsets[0] = 0; } else { // 1 fragment shall contain 1 frame - int num_fragments = 0; - offsets[num_fragments] = 0; // by definition the first fragment is at offset 0 + int fragment_idx = 0; + offsets[fragment_idx] = 0; // by definition the first fragment is at offset 0 + fragment_idx++; while( tag != TAG_SQ_DELIM ) { - num_fragments++; if ( tag != TAG_ITEM || !read_uint32(&state, &length, &position)) { dcm_error_set(error, DCM_ERROR_CODE_PARSE, @@ -987,8 +987,8 @@ bool dcm_parse_pixeldata_offsets(DcmError **error, "failed to read fragment"); return false; } + // skip actual content to find next offset dcm_seekcur(&state, length, &position); - offsets[num_fragments] = offsets[num_fragments-1] +8 + length; if (!read_tag(&state, &tag, &position)) { dcm_error_set(error, DCM_ERROR_CODE_PARSE, @@ -996,15 +996,23 @@ bool dcm_parse_pixeldata_offsets(DcmError **error, "failed to read tag"); return false; } - if ( num_fragments > num_frames ) + if ( fragment_idx < num_frames ) { - dcm_error_set(error, DCM_ERROR_CODE_INVALID, - "building BasicOffsetTable failed", - "Too many fragments" ); - return false; + offsets[fragment_idx] = offsets[fragment_idx-1] + 8/*tag and length field size*/ + length; } + fragment_idx++; + } + // 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 ( num_frames != num_fragments ) + + if ( num_frames < fragment_idx ) { dcm_error_set(error, DCM_ERROR_CODE_INVALID, "building BasicOffsetTable failed", @@ -1038,64 +1046,82 @@ char *dcm_parse_frame(DcmError **error, .big_endian = is_big_endian(), }; + const uint8_t bytes_per_pixel = (desc->bits_allocated+7)/8; + *length = desc->rows * desc->columns * desc->samples_per_pixel * bytes_per_pixel; + + char *value = DCM_MALLOC(error, *length); + if (value == NULL) { + return NULL; + } int64_t position = 0; - if (dcm_is_encapsulated_transfer_syntax(desc->transfer_syntax_uid)) { - *length = 0; - uint32_t tag; - if ( !read_tag(&state, &tag, &position) ) + if (!dcm_require(&state, value, *length, &position)) { + free(value); + return NULL; + } + + return value; +} + +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; + if ( !read_tag(&state, &tag, &position) ) + { + return NULL; + } + uint32_t fragment_length = 0; + // first determine the total length of bytes to be read + while( position < frame_end_offset && tag != TAG_SQ_DELIM ) + { + 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; } - uint32_t fragment_length = 0; - while( tag != TAG_SQ_DELIM ) + if (!read_uint32(&state, &fragment_length, &position)) { - 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; - } - if (!read_uint32(&state, &fragment_length, &position)) - { - return NULL; - } - dcm_seekcur(&state, fragment_length, &position); - *length += fragment_length; - if (!read_tag(&state, &tag, &position)) - { - return NULL; - } - } - char *value = DCM_MALLOC(error, *length); - if (value == NULL) { return NULL; } - // reposition to the beginning of encapsulated pixel data - dcm_seekcur(&state, -position, &position); - - fragment_length = 0; - char* fragment = value; - read_tag(&state, &tag, &position); - while( tag != TAG_SQ_DELIM ) + dcm_seekcur(&state, fragment_length, &position); + *length += fragment_length; + if (!read_tag(&state, &tag, &position)) { - read_uint32(&state, &fragment_length, &position); - dcm_read(&state, fragment, fragment_length, &position ); - fragment += fragment_length; - read_tag(&state, &tag, &position); + return NULL; } - return value; } - - *length = desc->rows * desc->columns * desc->samples_per_pixel; - char *value = DCM_MALLOC(error, *length); - if (value == NULL) { + if (value == NULL) + { return NULL; } - if (!dcm_require(&state, value, *length, &position)) { - free(value); - return NULL; + // reposition to the beginning of encapsulated pixel data + dcm_seekcur(&state, -position, &position); + + fragment_length = 0; + char* fragment = value; + position = 0; + read_tag(&state, &tag, &position); + while( position < frame_end_offset && tag != TAG_SQ_DELIM ) + { + read_uint32(&state, &fragment_length, &position); + dcm_read(&state, fragment, fragment_length, &position ); + fragment += fragment_length; + read_tag(&state, &tag, &position); } - return value; } diff --git a/src/pdicom.h b/src/pdicom.h index 4a8d3a4..2ef6233 100644 --- a/src/pdicom.h +++ b/src/pdicom.h @@ -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); From c02f1925b518bc37b00b1269910f4380018212ec Mon Sep 17 00:00:00 2001 From: Antal Ispanovity Date: Thu, 3 Jul 2025 09:40:31 +0200 Subject: [PATCH 05/11] fix offset calculation for non-encapsulated frames: bits allocated was not considered --- src/dicom-file.c | 3 ++- src/dicom-parse.c | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/dicom-file.c b/src/dicom-file.c index f3127ea..92a1062 100644 --- a/src/dicom-file.c +++ b/src/dicom-file.c @@ -1300,7 +1300,8 @@ bool dcm_filehandle_prepare_read_frame(DcmError **error, filehandle->offset_table[i] = i * filehandle->desc.rows * filehandle->desc.columns * - filehandle->desc.samples_per_pixel; + filehandle->desc.samples_per_pixel * + (filehandle->desc.bits_allocated/8); } filehandle->first_frame_offset = 12; diff --git a/src/dicom-parse.c b/src/dicom-parse.c index 80f825b..f335b6e 100644 --- a/src/dicom-parse.c +++ b/src/dicom-parse.c @@ -1046,7 +1046,7 @@ char *dcm_parse_frame(DcmError **error, .big_endian = is_big_endian(), }; - const uint8_t bytes_per_pixel = (desc->bits_allocated+7)/8; + const uint8_t bytes_per_pixel = desc->bits_allocated/8; *length = desc->rows * desc->columns * desc->samples_per_pixel * bytes_per_pixel; char *value = DCM_MALLOC(error, *length); From ab8b9f080b0b916691d35598b7a7eb8e2c3929e8 Mon Sep 17 00:00:00 2001 From: Antal Ispanovity Date: Thu, 2 Oct 2025 13:05:13 +0200 Subject: [PATCH 06/11] coding style, code documentation, review fixes: simplification, error handling, etc --- CHANGELOG.md | 1 + src/dicom-file.c | 11 +++---- src/dicom-parse.c | 81 ++++++++++++++++++++++------------------------- 3 files changed, 43 insertions(+), 50 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c92f8f..1dd84e3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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] diff --git a/src/dicom-file.c b/src/dicom-file.c index 568231b..ac099ba 100644 --- a/src/dicom-file.c +++ b/src/dicom-file.c @@ -1365,19 +1365,18 @@ DcmFrame *dcm_filehandle_read_frame(DcmError **error, return NULL; } + const char *syntax = dcm_filehandle_get_transfer_syntax_uid(filehandle); uint32_t length = 0; char* frame_data = NULL; - if ( dcm_is_encapsulated_transfer_syntax(filehandle->desc.transfer_syntax_uid) ) - { - int64_t frame_end_offset = frame_number < filehandle->num_frames ? filehandle->offset_table[i+1] : 0xFFFFFFFF; + 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 - { + } else { frame_data = dcm_parse_frame(error, filehandle->io, filehandle->implicit, diff --git a/src/dicom-parse.c b/src/dicom-parse.c index f335b6e..ad6648e 100644 --- a/src/dicom-parse.c +++ b/src/dicom-parse.c @@ -959,29 +959,23 @@ bool dcm_parse_pixeldata_offsets(DcmError **error, // 0 in the BOT is the offset to the start of frame 1, ie. here *first_frame_offset = position; // each frame may consist of several fragments, so we need to scan each fragment to find the next frame - if (!read_tag(&state, &tag, &position)) - { + 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 ) - { + if ( num_frames == 1 ) { // according to the standard the first frame's offset is 0 offsets[0] = 0; - } - else - { + } 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)) - { + 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"); @@ -989,39 +983,34 @@ bool dcm_parse_pixeldata_offsets(DcmError **error, } // skip actual content to find next offset dcm_seekcur(&state, length, &position); - if (!read_tag(&state, &tag, &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 ) - { + if ( fragment_idx < num_frames ) { offsets[fragment_idx] = offsets[fragment_idx-1] + 8/*tag and length field size*/ + length; } fragment_idx++; } // fragment_idx shall equal to num_frames+1 at the end fragment_idx--; - if ( fragment_idx > num_frames ) - { + if ( fragment_idx > num_frames ) { dcm_error_set(error, DCM_ERROR_CODE_INVALID, "building BasicOffsetTable failed", "Too many fragments" ); return false; } - if ( num_frames < fragment_idx ) - { + if ( num_frames < fragment_idx ) { dcm_error_set(error, DCM_ERROR_CODE_INVALID, "building BasicOffsetTable failed", "Too many frames" ); return false; } - if ( !read_uint32(&state, &length, &position ) || length != 0 ) - { + if ( !read_uint32(&state, &length, &position ) || length != 0 ) { dcm_error_set(error, DCM_ERROR_CODE_PARSE, "building BasicOffsetTable failed", "Sequence Delimiter Tag failure" ); @@ -1033,6 +1022,7 @@ bool dcm_parse_pixeldata_offsets(DcmError **error, return true; } + char *dcm_parse_frame(DcmError **error, DcmIO *io, bool implicit, @@ -1046,8 +1036,7 @@ char *dcm_parse_frame(DcmError **error, .big_endian = is_big_endian(), }; - const uint8_t bytes_per_pixel = desc->bits_allocated/8; - *length = desc->rows * desc->columns * desc->samples_per_pixel * bytes_per_pixel; + *length = desc->rows * desc->columns * desc->samples_per_pixel * (desc->bits_allocated / 8); char *value = DCM_MALLOC(error, *length); if (value == NULL) { @@ -1062,6 +1051,8 @@ char *dcm_parse_frame(DcmError **error, return value; } +/* Read encapsulated frame. Return NULL in case of error. +*/ char *dcm_parse_encapsulated_frame(DcmError **error, DcmIO *io, bool implicit, @@ -1078,50 +1069,52 @@ char *dcm_parse_encapsulated_frame(DcmError **error, int64_t position = 0; *length = 0; uint32_t tag; - if ( !read_tag(&state, &tag, &position) ) - { - return NULL; - } uint32_t fragment_length = 0; + // first determine the total length of bytes to be read - while( position < frame_end_offset && tag != TAG_SQ_DELIM ) - { - if (tag != TAG_ITEM) - { + 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; } - if (!read_uint32(&state, &fragment_length, &position)) - { + if (!read_uint32(&state, &fragment_length, &position)) { return NULL; } dcm_seekcur(&state, fragment_length, &position); *length += fragment_length; - if (!read_tag(&state, &tag, &position)) - { - return NULL; - } } + char *value = DCM_MALLOC(error, *length); - if (value == NULL) - { + if (value == NULL) { 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; - read_tag(&state, &tag, &position); - while( position < frame_end_offset && tag != TAG_SQ_DELIM ) - { + while(position < frame_end_offset) { + read_tag(&state, &tag, &position); read_uint32(&state, &fragment_length, &position); - dcm_read(&state, fragment, fragment_length, &position ); + if (!dcm_require(&state, fragment, fragment_length, &position)) { + free(value); + return NULL; + } fragment += fragment_length; - read_tag(&state, &tag, &position); } + return value; } From ac71ff4f5e751930969ce3fddf40b92d8b6cd471 Mon Sep 17 00:00:00 2001 From: Antal Ispanovity Date: Wed, 8 Oct 2025 06:58:11 +0200 Subject: [PATCH 07/11] functional tests for encapsulated pixel data handling --- ...erated_encapsulated_defined_bot_1_to_1.dcm | Bin 0 -> 402 bytes ...erated_encapsulated_defined_bot_2_to_1.dcm | Bin 0 -> 418 bytes ...erated_encapsulated_defined_bot_2_to_2.dcm | Bin 0 -> 422 bytes ...enerated_encapsulated_empty_bot_1_to_1.dcm | Bin 0 -> 398 bytes ...enerated_encapsulated_empty_bot_2_to_1.dcm | Bin 0 -> 414 bytes tests/check_dicom.c | 142 ++++++++++++++++++ 6 files changed, 142 insertions(+) create mode 100644 data/test_files/generated_encapsulated_defined_bot_1_to_1.dcm create mode 100644 data/test_files/generated_encapsulated_defined_bot_2_to_1.dcm create mode 100644 data/test_files/generated_encapsulated_defined_bot_2_to_2.dcm create mode 100644 data/test_files/generated_encapsulated_empty_bot_1_to_1.dcm create mode 100644 data/test_files/generated_encapsulated_empty_bot_2_to_1.dcm diff --git a/data/test_files/generated_encapsulated_defined_bot_1_to_1.dcm b/data/test_files/generated_encapsulated_defined_bot_1_to_1.dcm new file mode 100644 index 0000000000000000000000000000000000000000..d29dae554ff1cc225af1e5903a8d18899246b639 GIT binary patch literal 402 zcmc(bO$x#=5QQgcv{|^QE?jtmu7;#Ws9URG!5Xmi8eYMZ3Oz~dOVEN|zzJc#nRzq$ zA^NW!>#C8sLU&PghnFPCAA%>4#Omq|SuVJAhOIHCU86 z`W^3$q}7)?5A)k_W=@K#c0rB5`s#f1&8a|6Q;n(xVJb+2mQx3bPfkoEBc;$Kk%;<7 b7GE^NhLL*p{_wy$u#kzG&C_9g_91)$K#ME! literal 0 HcmV?d00001 diff --git a/data/test_files/generated_encapsulated_defined_bot_2_to_1.dcm b/data/test_files/generated_encapsulated_defined_bot_2_to_1.dcm new file mode 100644 index 0000000000000000000000000000000000000000..5f437b679390a6391499c45f0ac36e35b937add9 GIT binary patch literal 418 zcmc(bOA5j;5QZmdv{9>87cM+OS0SwtbZZqXSOb<`!z*}Fp(knm6SSZga6*`G{(0m< z^j|v`dC75w`XcBK&xw&eI8PvsRTpbYRi<`Et5j>fQ(KiPgAEn=02R_#Y?MJ9BVPS< zxPGJdn$z;DI&t$WcV6;C#E_LOMw#JxB@3hs9ms!>mDpXW#wPTz!UjKB2 zey8_FV)dmi!u&40Ss+E-EztU_ug^E%o=WBv8dfcdQ%Q=n{B%IPbK*lXQbt`82^b!E Z+-RbW69z7Z;8g%2lXN!E#_2hP@CBx;J;ldNJY6@b4ThT-kkwn5Zyn-h+;UwTIi6nXfr)lP!H$UwF z|Fu(|6_Q`5uPz?+5(4GXdXOYfoo^B8Sf`26kufIKdmZV7mn!oPD^y>x(+T5%@#d$) z_9yyaB-UTmp`BmZH$%9{Dj!98E3?bJEY2}Cg`C-l#Hpl!v>G}ftfr`mfn-k}HK!+_ kc?SJP9j)!qa4}e~EC}hiqp|mi)nq#J=Zj_Ay&9W-19%=UHUIzs literal 0 HcmV?d00001 diff --git a/tests/check_dicom.c b/tests/check_dicom.c index e99ca1d..acb94ad 100644 --- a/tests/check_dicom.c +++ b/tests/check_dicom.c @@ -807,6 +807,121 @@ START_TEST(test_file_ct_brain_single) } END_TEST + +START_TEST(test_encapsulated_empty_BOT_1_to_1) +{ + char *file_path = fixture_path("data/test_files/generated_encapsulated_empty_bot_1_to_1.dcm"); + DcmFilehandle *filehandle = + dcm_filehandle_create_from_file(NULL, file_path); + free( file_path ); + ck_assert_ptr_nonnull(filehandle); + DcmFrame* frame = dcm_filehandle_read_frame(NULL, + filehandle, + 1); + ck_assert_ptr_nonnull( frame ); + uint32_t frame_length = dcm_frame_get_length( frame ); + ck_assert_uint_eq( frame_length, 8 ); + const char* data = dcm_frame_get_value( frame ); + const char expected_data[] = { 0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7 }; + ck_assert_mem_eq( expected_data, data, sizeof(expected_data) ); +} +END_TEST + + +START_TEST(test_encapsulated_empty_BOT_2_to_1) +{ + char *file_path = fixture_path("data/test_files/generated_encapsulated_empty_bot_2_to_1.dcm"); + DcmFilehandle *filehandle = + dcm_filehandle_create_from_file(NULL, file_path); + free( file_path ); + ck_assert_ptr_nonnull(filehandle); + DcmFrame* frame = dcm_filehandle_read_frame(NULL, + filehandle, + 1); + ck_assert_ptr_nonnull( frame ); + uint32_t frame_length = dcm_frame_get_length( frame ); + ck_assert_uint_eq( frame_length, 16 ); + const char* data = dcm_frame_get_value( frame ); + const char expected_data[] = { 0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8, + 0x9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf }; + ck_assert_mem_eq( expected_data, data, sizeof(expected_data) ); +} +END_TEST + + +START_TEST(test_encapsulated_defined_BOT_1_to_1) +{ + char *file_path = fixture_path("data/test_files/generated_encapsulated_defined_bot_1_to_1.dcm"); + DcmFilehandle *filehandle = + dcm_filehandle_create_from_file(NULL, file_path); + free( file_path ); + ck_assert_ptr_nonnull(filehandle); + DcmFrame* frame = dcm_filehandle_read_frame(NULL, + filehandle, + 1); + ck_assert_ptr_nonnull( frame ); + uint32_t frame_length = dcm_frame_get_length( frame ); + ck_assert_uint_eq( frame_length, 8 ); + const char* data = dcm_frame_get_value( frame ); + const char expected_data[] = + { 0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7 }; + ck_assert_mem_eq( expected_data, data, sizeof(expected_data) ); +} +END_TEST + + +START_TEST(test_encapsulated_defined_BOT_2_to_1) +{ + char *file_path = fixture_path("data/test_files/generated_encapsulated_defined_bot_2_to_1.dcm"); + DcmFilehandle *filehandle = + dcm_filehandle_create_from_file(NULL, file_path); + free( file_path ); + ck_assert_ptr_nonnull(filehandle); + DcmFrame* frame = dcm_filehandle_read_frame(NULL, + filehandle, + 1); + ck_assert_ptr_nonnull( frame ); + uint32_t frame_length = dcm_frame_get_length( frame ); + ck_assert_uint_eq( frame_length, 16 ); + const char* data = dcm_frame_get_value( frame ); + const char expected_data[] = + { 0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8, 0x9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf }; + ck_assert_mem_eq( expected_data, data, sizeof(expected_data) ); +} +END_TEST + + +START_TEST(test_encapsulated_defined_BOT_2_to_2) +{ + char *file_path = fixture_path("data/test_files/generated_encapsulated_defined_bot_2_to_2.dcm"); + DcmFilehandle *filehandle = + dcm_filehandle_create_from_file(NULL, file_path); + free( file_path ); + ck_assert_ptr_nonnull(filehandle); + DcmFrame* frame = dcm_filehandle_read_frame(NULL, + filehandle, + 1); + ck_assert_ptr_nonnull( frame ); + uint32_t frame_length = dcm_frame_get_length( frame ); + ck_assert_uint_eq( frame_length, 8 ); + const char* data1 = dcm_frame_get_value( frame ); + const char expected_data1[] = + { 0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7 }; + ck_assert_mem_eq( expected_data1, data1, sizeof(expected_data1) ); + /*frame = dcm_filehandle_read_frame(NULL, + filehandle, + 2); + ck_assert_ptr_nonnull( frame ); + frame_length = dcm_frame_get_length( frame ); + ck_assert_uint_eq( frame_length, 8 ); + const char* data2 = dcm_frame_get_value( frame ); + const char expected_data2[] = + { 0x8, 0x9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf }; + ck_assert_mem_eq( expected_data2, data2, sizeof(expected_data2) );*/ +} +END_TEST + + static Suite *create_main_suite(void) { Suite *suite = suite_create("main"); @@ -896,6 +1011,32 @@ static Suite *create_single_frame_suite(void) return suite; } +static Suite *create_parse_suite(void) +{ + Suite *suite = suite_create("parse"); + + TCase *encapsulated_case1 = tcase_create("empty_BOT_1_to_1"); + tcase_add_test(encapsulated_case1, test_encapsulated_empty_BOT_1_to_1); + suite_add_tcase(suite, encapsulated_case1); + + TCase *encapsulated_case2 = tcase_create("empty_BOT_2_to_1"); + tcase_add_test(encapsulated_case2, test_encapsulated_empty_BOT_2_to_1); + suite_add_tcase(suite, encapsulated_case2); + + TCase *encapsulated_case3 = tcase_create("defined_BOT_1_to_1"); + tcase_add_test(encapsulated_case3, test_encapsulated_defined_BOT_1_to_1); + suite_add_tcase(suite, encapsulated_case3); + + TCase *encapsulated_case4 = tcase_create("defined_BOT_2_to_1"); + tcase_add_test(encapsulated_case4, test_encapsulated_defined_BOT_2_to_1); + suite_add_tcase(suite, encapsulated_case4); + + TCase *encapsulated_case5 = tcase_create("defined_BOT_2_to_2"); + tcase_add_test(encapsulated_case5, test_encapsulated_defined_BOT_2_to_2); + suite_add_tcase(suite, encapsulated_case5); + + return suite; +} int main(void) { @@ -903,6 +1044,7 @@ int main(void) srunner_add_suite(runner, create_data_suite()); srunner_add_suite(runner, create_file_suite()); srunner_add_suite(runner, create_single_frame_suite()); + srunner_add_suite(runner, create_parse_suite()); srunner_run_all(runner, CK_VERBOSE); int number_failed = srunner_ntests_failed(runner); srunner_free(runner); From 2c33aa4846e4abfc1bd831188a59577e7262aac8 Mon Sep 17 00:00:00 2001 From: Antal Ispanovity Date: Tue, 11 Nov 2025 23:41:24 +0100 Subject: [PATCH 08/11] simplify BOT construction avoid overflow for length value in dcm_parse_encapsulated_frame --- src/dicom-parse.c | 81 +++++++++++++++++------------------------------ 1 file changed, 29 insertions(+), 52 deletions(-) diff --git a/src/dicom-parse.c b/src/dicom-parse.c index ad6648e..6034058 100644 --- a/src/dicom-parse.c +++ b/src/dicom-parse.c @@ -955,65 +955,38 @@ bool dcm_parse_pixeldata_offsets(DcmError **error, // each frame dcm_log_info("building Offset Table from Pixel Data"); + // by definition the first offset is 0 + offsets[0] = 0; // 0 in the BOT is the offset to the start of frame 1, ie. here *first_frame_offset = position; - // each frame may consist of several fragments, so we need to scan each fragment to find the next frame - 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++; - } - // 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" ); + for (int i = 1; i < num_frames; i++) { + if (!read_tag(&state, &tag, &position) || + !read_uint32(&state, &length, &position)) { return false; } - if ( num_frames < fragment_idx ) { - dcm_error_set(error, DCM_ERROR_CODE_INVALID, - "building BasicOffsetTable failed", - "Too many frames" ); + if (tag == TAG_SQ_DELIM) { + dcm_error_set(error, DCM_ERROR_CODE_PARSE, + "reading BasicOffsetTable failed", + "too few frames in PixelData"); return false; } - - if ( !read_uint32(&state, &length, &position ) || length != 0 ) { + + if (tag != TAG_ITEM) { dcm_error_set(error, DCM_ERROR_CODE_PARSE, "building BasicOffsetTable failed", - "Sequence Delimiter Tag failure" ); + "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; } } @@ -1070,6 +1043,7 @@ char *dcm_parse_encapsulated_frame(DcmError **error, *length = 0; uint32_t tag; uint32_t fragment_length = 0; + uint64_t frame_length = 0; // first determine the total length of bytes to be read while(position < frame_end_offset) { @@ -1089,10 +1063,13 @@ char *dcm_parse_encapsulated_frame(DcmError **error, return NULL; } dcm_seekcur(&state, fragment_length, &position); - *length += fragment_length; + frame_length += fragment_length; + } + if ( frame_length > 0xFFFFFFFF ) { + return NULL; } - char *value = DCM_MALLOC(error, *length); + char *value = DCM_MALLOC(error, frame_length); if (value == NULL) { return NULL; } @@ -1115,6 +1092,6 @@ char *dcm_parse_encapsulated_frame(DcmError **error, } fragment += fragment_length; } - + *length = (uint32_t) frame_length; return value; } From 9cb222da57062914fc7a193878d06db2a52aee69 Mon Sep 17 00:00:00 2001 From: Antal Ispanovity Date: Wed, 12 Nov 2025 11:07:14 +0100 Subject: [PATCH 09/11] simplify first BOT offset calculation restore some error handling --- src/dicom-parse.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/dicom-parse.c b/src/dicom-parse.c index 6034058..48bfee7 100644 --- a/src/dicom-parse.c +++ b/src/dicom-parse.c @@ -955,12 +955,10 @@ bool dcm_parse_pixeldata_offsets(DcmError **error, // each frame dcm_log_info("building Offset Table from Pixel Data"); - // by definition the first offset is 0 - offsets[0] = 0; // 0 in the BOT is the offset to the start of frame 1, ie. here *first_frame_offset = position; - for (int i = 1; i < num_frames; i++) { + for (int i = 0; i < num_frames; i++) { if (!read_tag(&state, &tag, &position) || !read_uint32(&state, &length, &position)) { return false; @@ -983,13 +981,25 @@ bool dcm_parse_pixeldata_offsets(DcmError **error, } // step back to the start of the item for this frame - offsets[i] = position - 8; + offsets[i] = position - *first_frame_offset - 8; // and seek forward over the value if (!dcm_seekcur(&state, length, &position)) { return false; } } + + // in case multiple frames 1:1 frame to fragment mapping is assumed, + // therefore the next thing should be the end of sequence tag + if (!read_tag(&state, &tag, &position)) { + return false; + } + if (num_frames != 1 && tag != TAG_SQ_DELIM) { + dcm_error_set(error, DCM_ERROR_CODE_PARSE, + "reading BasicOffsetTable failed", + "too many frames in PixelData"); + return false; + } } return true; From 3fd03908c578660ede9c7b3824aafa9033dce5da Mon Sep 17 00:00:00 2001 From: Antal Ispanovity Date: Wed, 12 Nov 2025 14:47:43 +0100 Subject: [PATCH 10/11] review fixesL coding style, comments, error messages --- src/dicom-file.c | 2 +- src/dicom-parse.c | 27 ++++++++++++++++++--------- src/pdicom.h | 8 ++++---- tests/check_dicom.c | 4 ++-- 4 files changed, 25 insertions(+), 16 deletions(-) diff --git a/src/dicom-file.c b/src/dicom-file.c index ac099ba..1698035 100644 --- a/src/dicom-file.c +++ b/src/dicom-file.c @@ -1370,7 +1370,7 @@ DcmFrame *dcm_filehandle_read_frame(DcmError **error, 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; + filehandle->offset_table[i + 1] : 0xFFFFFFFF; frame_data = dcm_parse_encapsulated_frame(error, filehandle->io, filehandle->implicit, diff --git a/src/dicom-parse.c b/src/dicom-parse.c index 48bfee7..7c7ef11 100644 --- a/src/dicom-parse.c +++ b/src/dicom-parse.c @@ -954,11 +954,14 @@ 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; - for (int i = 0; i < num_frames; i++) { + for (int i = 0; i < num_frames; i++) { if (!read_tag(&state, &tag, &position) || !read_uint32(&state, &length, &position)) { return false; @@ -1019,7 +1022,10 @@ char *dcm_parse_frame(DcmError **error, .big_endian = is_big_endian(), }; - *length = desc->rows * desc->columns * desc->samples_per_pixel * (desc->bits_allocated / 8); + *length = desc->rows * + desc->columns * + desc->samples_per_pixel * + (desc->bits_allocated / 8); char *value = DCM_MALLOC(error, *length); if (value == NULL) { @@ -1037,10 +1043,10 @@ char *dcm_parse_frame(DcmError **error, /* 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) + DcmIO *io, + bool implicit, + int64_t frame_end_offset, + uint32_t* length) { DcmParseState state = { .error = error, @@ -1056,7 +1062,7 @@ char *dcm_parse_encapsulated_frame(DcmError **error, uint64_t frame_length = 0; // first determine the total length of bytes to be read - while(position < frame_end_offset) { + while (position < frame_end_offset) { if (!read_tag(&state, &tag, &position)) { return NULL; } @@ -1075,7 +1081,10 @@ char *dcm_parse_encapsulated_frame(DcmError **error, dcm_seekcur(&state, fragment_length, &position); frame_length += fragment_length; } - if ( frame_length > 0xFFFFFFFF ) { + if (frame_length > 0xFFFFFFFF) { + dcm_error_set(error, DCM_ERROR_CODE_PARSE, + "invalid frame size", + "frame size exceeds 4GB" ); return NULL; } @@ -1093,7 +1102,7 @@ char *dcm_parse_encapsulated_frame(DcmError **error, fragment_length = 0; char* fragment = value; position = 0; - while(position < frame_end_offset) { + while (position < frame_end_offset) { read_tag(&state, &tag, &position); read_uint32(&state, &fragment_length, &position); if (!dcm_require(&state, fragment, fragment_length, &position)) { diff --git a/src/pdicom.h b/src/pdicom.h index 2ef6233..0a0bbef 100644 --- a/src/pdicom.h +++ b/src/pdicom.h @@ -169,7 +169,7 @@ char *dcm_parse_frame(DcmError **error, uint32_t *length); char *dcm_parse_encapsulated_frame(DcmError **error, - DcmIO *io, - bool implicit, - int64_t frame_end_offset, - uint32_t* length); + DcmIO *io, + bool implicit, + int64_t frame_end_offset, + uint32_t* length); diff --git a/tests/check_dicom.c b/tests/check_dicom.c index acb94ad..6563a4d 100644 --- a/tests/check_dicom.c +++ b/tests/check_dicom.c @@ -908,7 +908,7 @@ START_TEST(test_encapsulated_defined_BOT_2_to_2) const char expected_data1[] = { 0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7 }; ck_assert_mem_eq( expected_data1, data1, sizeof(expected_data1) ); - /*frame = dcm_filehandle_read_frame(NULL, + frame = dcm_filehandle_read_frame(NULL, filehandle, 2); ck_assert_ptr_nonnull( frame ); @@ -917,7 +917,7 @@ START_TEST(test_encapsulated_defined_BOT_2_to_2) const char* data2 = dcm_frame_get_value( frame ); const char expected_data2[] = { 0x8, 0x9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf }; - ck_assert_mem_eq( expected_data2, data2, sizeof(expected_data2) );*/ + ck_assert_mem_eq( expected_data2, data2, sizeof(expected_data2) ); } END_TEST From 995085125fc5fe9f9e24f214527fa5d3ecd96eda Mon Sep 17 00:00:00 2001 From: Antal Ispanovity Date: Thu, 13 Nov 2025 06:14:04 +0100 Subject: [PATCH 11/11] indentation --- src/dicom-parse.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/dicom-parse.c b/src/dicom-parse.c index 7c7ef11..77d1450 100644 --- a/src/dicom-parse.c +++ b/src/dicom-parse.c @@ -1043,10 +1043,10 @@ char *dcm_parse_frame(DcmError **error, /* 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) + DcmIO *io, + bool implicit, + int64_t frame_end_offset, + uint32_t* length) { DcmParseState state = { .error = error,