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 8 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
8 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
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) )
Copy link
Collaborator

@jcupitt jcupitt Oct 1, 2025

Choose a reason for hiding this comment

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

There's a getter for this, I would write:

    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
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
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;
Copy link
Collaborator

@jcupitt jcupitt Oct 1, 2025

Choose a reason for hiding this comment

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

I'm not sure I understand. I think you can have many fragments in each frame, so don't you need two loops here?

for (i = 0; i < num_frames; i++) {
    record position as start of frame i 
    while (frame has fragments)
        skip fragment
}

Copy link
Author

@weanti weanti Oct 1, 2025

Choose a reason for hiding this comment

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

This part is a kind of an assumption. It may be according to the standard, but the standard is not an easy reading.
This part covers the case where there is no Basic Offset Table (so the frame offsets are absent). In this case we need to find the frame offsets ourselves.
Here is the assumption: if there is no BOT then I assume that each frame consists of only 1 fragment.
Why do I assume this: If each frame could have arbitrary number of fragments then it would be impossible to find out which fagment belongs to which frame. This means that each frame shall have the same amount of fragments.
We could use a different assumption here: number of fragments = C * number of frames, where C >= 1 and C is an integer. This would complicate things a bit. I think encapsulation is used mainly for compressed Transfer Syntaxes and there is no guarantee that each frame can be compressed in a way that they span across exactly C fragment.
Feel free to ask or propose a different approach to assign fragments to frames.

}
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;
Copy link
Collaborator

@jcupitt jcupitt Oct 1, 2025

Choose a reason for hiding this comment

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

bits_allocated is defined as a uint16, so I think uint8 could (potentially!) be too small here. Let's make this uint16 as well.

libdicom style is for spaces around operators, so:

const uint16_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 )
Copy link
Collaborator

@jcupitt jcupitt Oct 1, 2025

Choose a reason for hiding this comment

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

Two tag reads and checking the first tag at the head of the loop is a little ugly. How about (untested!):

    int64_t position = 0;

    // first determine the total length of bytes to be read
    *length = 0;
    while (position < frame_end_offset) {
        uint32_t tag;
        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;
        }

        uint32_t fragment_length;
        if (!read_uint32(&state, &fragment_length, &position)) {
            return NULL;
        }
        *length += fragment_length;
        dcm_seekcur(&state, fragment_length, &position);
    }

Now you only have one tag read and the tag check always tests the tag read just before.

You can reorder the loop below in the same way.

Copy link
Author

@weanti weanti Oct 2, 2025

Choose a reason for hiding this comment

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

Thanks. I managed to simplify the other loop.

{
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;
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 );
Copy link
Collaborator

@jcupitt jcupitt Oct 1, 2025

Choose a reason for hiding this comment

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

You need dcm_require() here, you must loop on dcm_read() if you need a certain number of bytes.

I would also check the return status of require in case of IO errors.

Copy link
Collaborator

@jcupitt jcupitt Oct 1, 2025

Choose a reason for hiding this comment

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

So this second loop could perhaps be (untested!):

    char *fragment = value;
    while (position < frame_end_offset) {
        uint32_t tag;
        read_tag(&state, &tag, &position);
        if (tag == TAG_SQ_DELIM) {
            break;
        }

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

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);