Skip to content

Don't log ignored errors unless debug logging is enabled#115

Open
bgilbert wants to merge 2 commits into
ImagingDataCommons:mainfrom
bgilbert:errors
Open

Don't log ignored errors unless debug logging is enabled#115
bgilbert wants to merge 2 commits into
ImagingDataCommons:mainfrom
bgilbert:errors

Conversation

@bgilbert
Copy link
Copy Markdown
Collaborator

@bgilbert bgilbert commented Jun 1, 2026

Avoid the dcm_error_newf() and dcm_error_free() if we won't log the message anyway.

With the current state of openslide/openslide#648, this avoids allocating, string-formatting, and freeing 690,658 dcm_filehandle_get_frame_number() errors when opening 3DHISTECH-2:

Before:
real	0m0.303s
user	0m0.224s
sys	0m0.077s

After:
real	0m0.210s
user	0m0.133s
sys	0m0.077s

bgilbert added 2 commits June 1, 2026 03:05
Avoid the dcm_error_newf() and dcm_error_free() if we won't log the
message anyway.
@bgilbert
Copy link
Copy Markdown
Collaborator Author

bgilbert commented Jun 1, 2026

Even with this fix I think there's an API problem. If the DICOM file is corrupt, causing dcm_filehandle_get_frame_number(NULL, ...) to fail, OpenSlide will mark the tile sparse and keep going. As a result, there's no opportunity to notice the problem later when that region is read. And if we explicitly check for DCM_ERROR_CODE_MISSING_FRAME, we're back to allocating and freeing hundreds of thousands of errors.

The MISSING_FRAME error could be a static one that doesn't get string-formatted or freed, but it feels like that gives libdicom too much of a magic understanding of how its API is being used. What I'd really like is

bool dcm_filehandle_has_frame(**error, *filehandle, column, row, *has_frame)

but that's an API change and probably adds some duplicated code.

What do you think?

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.

1 participant