Skip to content

add dcm_filehandle_find_frame_number()#117

Merged
jcupitt merged 5 commits into
mainfrom
add-find-frame-number
Jun 5, 2026
Merged

add dcm_filehandle_find_frame_number()#117
jcupitt merged 5 commits into
mainfrom
add-find-frame-number

Conversation

@jcupitt
Copy link
Copy Markdown
Collaborator

@jcupitt jcupitt commented Jun 4, 2026

  • deprecate dcm_filehandle_get_frame_number()
  • bump version to 1.3

See #117 for context

- deprecate dcm_filehandle_get_frame_number()
- bump version to 1.3
@jcupitt
Copy link
Copy Markdown
Collaborator Author

jcupitt commented Jun 4, 2026

@bgilbert I tinkered a little and thought this was maybe the best way to fix it.

This PR adds:

bool dcm_filehandle_find_frame_number(DcmError **error,
                                      DcmFilehandle *filehandle,
                                      uint32_t column,
                                      uint32_t row,
                                      uint32_t *frame_number)

This new function only returns false on a parse error; if you have a missing tile, it sets frame_number to 0. DICOM frames number from 1, so this should be an unused value. It deprecates dcm_filehandle_get_frame_number().

With this obvious change to openslide:

        for (file_num = 0; file_num < l->file_count; file_num++) {
          DcmError *dcm_error = NULL;
          uint32_t n;
          if (!dcm_filehandle_find_frame_number(&dcm_error,
                                                l->files[file_num]->filehandle,
                                                tile_col, tile_row, &n)) {
            _openslide_dicom_propagate_error(err, dcm_error);
            return false;
          }
              
          if (n != 0) {
            // found one, record the file that has this tile and break
            l->tile_files[tile_index] = file_num;
            break; 
          } 
        }

Before this change, with the 6gb tiled full, no offset DICOM I have:

$ time slidetool slide open 4_1

real	0m1.861s
user	0m1.845s
sys	0m0.016s

With this PR and openslide change:

$ time slidetool slide open 4_1

real	0m0.328s
user	0m0.311s
sys	0m0.017s

Blimey!

@jcupitt
Copy link
Copy Markdown
Collaborator Author

jcupitt commented Jun 4, 2026

The speed improvement is so useful I would be tempted to make the next openslide release require libdicom 1.3 with this change. It'd save an ugly #ifdef in openslide too.

What do you think?

Copy link
Copy Markdown
Collaborator

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

The approach LGTM! Thanks for handling this. I agree with bumping the OpenSlide requirement directly to 1.3.0, since we haven't yet had a release that requires 1.2.0.

Comment thread include/dicom/dicom.h
Comment thread src/dicom-file.c Outdated
Comment thread src/dicom-file.c
and add DCM_DEPRECATED() macro
@jcupitt
Copy link
Copy Markdown
Collaborator Author

jcupitt commented Jun 5, 2026

I think I fixed the things you saw, thanks for the careful review.

Comment thread CHANGELOG.md
Comment thread src/dicom-file.c Outdated
Copy link
Copy Markdown
Collaborator

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

LGTM except for two small things.

Comment thread src/dicom-file.c Outdated
Comment thread CHANGELOG.md
Copy link
Copy Markdown
Collaborator

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for putting this together so quickly!

@jcupitt jcupitt merged commit a8e99a4 into main Jun 5, 2026
7 checks passed
@jcupitt jcupitt deleted the add-find-frame-number branch June 5, 2026 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants