Skip to content

Fix encapsulated pixeldata handling #103

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Fix encapsulated pixeldata handling #103

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
87c0436
remove restriction for bits stored, because its value can be modality…
Jun 26, 2025
835ed96
fix offset reading and frame reading for encapsulated pixel data
Jun 26, 2025
d190231
fix offset calculation for encapsualted fragments: length was essenti…
Jun 30, 2025
0ceb9cc
split regular and encapsulated frame parsing
Jul 1, 2025
c02f192
fix offset calculation for non-encapsulated frames: bits allocated wa…
Jul 3, 2025
7a08c78
Merge branch 'main' into main
weanti Sep 30, 2025
ab8b9f0
coding style, code documentation, review fixes: simplification, error…
Oct 2, 2025
ac71ff4
functional tests for encapsulated pixel data handling
weanti Oct 8, 2025
2c33aa4
simplify BOT construction
weanti Nov 11, 2025
9cb222d
simplify first BOT offset calculation
weanti Nov 12, 2025
3fd0390
review fixesL coding style, comments, error messages
weanti Nov 12, 2025
9950851
indentation
weanti Nov 13, 2025
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
7 changes: 0 additions & 7 deletions src/dicom-data.c
View file
Open in desktop
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
26 changes: 20 additions & 6 deletions src/dicom-file.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1365,12 +1365,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;
}
Expand Down
174 changes: 123 additions & 51 deletions src/dicom-parse.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -954,56 +954,80 @@ 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 •
edited
Loading

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.

Copy link
Author

@weanti weanti Nov 11, 2025

Choose a reason for hiding this comment

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

with a small addition this (original) code seems to work fine

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;
Expand All @@ -1022,34 +1046,82 @@ 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;

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 NULL;
}
return value;
}

if (tag != TAG_ITEM) {
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;
}
} 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

@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 suppose there's a potential uint32 overflow here. length should ideally be a uint64, though I don't suppose it matters much.

Copy link
Author

@weanti weanti Nov 11, 2025

Choose a reason for hiding this comment

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

Theoretically it could happen. I'll use uint64_inside and add some range checking.

if (!read_tag(&state, &tag, &position))
{
return NULL;
}
}

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;
}
6 changes: 6 additions & 0 deletions src/pdicom.h
View file
Open in desktop
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);