Baseline support for deflate transfer syntaxes with dataset adapter rewrite by naterichman · Pull Request #438 · Enet4/dicom-rs

naterichman · 2023-11-18T20:23:10Z

No description provided.

object/src/mem.rs

Enet4 · 2024-12-08T10:44:32Z

I had another try at this. Last time we talked about deflate TS support, the main problem was that the data RW adapter would not allow us to bind the lifetime of the adapted reader/writer to the lifetime of the input reader/writer. With the current signature, this is simply impossible.

So we had to change the trait DataRWAdapter. One way that we can do in modern Rust thanks to GATs is something the likes of this:

pub trait DataRWAdapter {
    type Reader<'r, R: 'r>: Read
        where Self: 'r;
    type Writer<'w, W: 'w>: Write
        where Self: 'w;

    /// Adapt a byte reader.
    fn adapt_reader<R>(&self, reader: R) -> Self::Reader<R>;

    /// Adapt a byte writer.
    fn adapt_reader<W>(&self, writer: W) -> Self::Writer<W>;
}

As delightful as this trait is, it is not dyn-compatible (the new nomenclature for what was previously known as object-safe), so we could not have a registry for any adapter of this kind.

Ultimately, we have to go for a fully dynamic approach with an added lifetime parameter at each method.

pub trait DataRWAdapter {
    /// Adapt a byte reader.
    fn adapt_reader<'r>(&self, reader: Box<dyn Read + 'r>) -> Box<dyn Read + 'r>;

    /// Adapt a byte writer.
    fn adapt_writer<'w>(&self, writer: Box<dyn Write + 'w>) -> Box<dyn Write + 'w>;
}

After propagating the changes to the rest of the code, we thus bring support for deflated DICOM data sets in dicom-dump, dicom-toimage, and many other tools.

This is not yet ready though.

dicom-transcode needs a few more tweaks in order to enable reading and writing deflated DICOM files. ~~I can work on this soon-ish.~~ Done
Two of the new tests need to be changed. A round trip from deflated DICOM data and back to deflated DICOM data does not guarantee a file with the same size, since it could be done using a different deflate encoder implementation with different compression parameters.

Enet4 · 2025-04-18T09:39:30Z

Good news and bad news. The good news is that this pull request is ready. The bad news is that I could not find any issues in the latest revision so far. Maybe you can help cracking down any eventual bugs? 😆

naterichman

Looks good to me, just a couple comments. Sorry for the long delay on this, I've been super busy!

object/src/lib.rs

encoding/src/transfer_syntax/mod.rs

naterichman · 2025-06-16T16:28:38Z

transfer-syntax-registry/src/entries.rs

I can't remember what the behavior of this was, but I feel like we should make a Dataset encoding for this that panics with a message about adding the deflate feature.

A bit torn on that one. Unless I overlooked something, the way to do that is to create and declare a stub data set implementation which raises an error whenever it is called. Semantically, the system would be claiming to support deflated explicit VR little endian at Codec level (on checking that it matches Codec::Dataset(Some(_))), but then still fail at run-time. So any application depending on this check in an automatic discovery of supported transfer syntaxes would be at risk of inadvertently including the stub.

OK, there is one other thing that we can try, which is to adapt the error message in places where support for certain transfer syntaxes were requested, and suggest enabling features there. Need to investigate.

I extended the error types ReadError and WriteError in dicom_object so that they can provide a suggestion to enable a Cargo feature in order to support that transfer syntax. For example:

❯ cargo run -p dicom-toimage --no-default-features --bin dicom-toimage -- ../dicom-test-files/data/pydicom/image_dfl.dcm 025-06-21T18:45:02.927669Z ERROR dicom_toimage: could not read DICOM file ../dicom-test-files/data/pydicom/image_dfl.dcm Caused by this error: 1: Unsupported reading for transfer syntax `1.2.840.10008.1.2.1.99` (Deflated Explicit VR Little Endian, try enabling feature `dicom-transfer-syntax-registry/deflate`)

Enet4

Thank you for the feedback!

encoding/src/transfer_syntax/mod.rs

object/src/file.rs

object/src/lib.rs

Enet4 · 2025-06-18T16:55:42Z

transfer-syntax-registry/src/entries.rs

A bit torn on that one. Unless I overlooked something, the way to do that is to create and declare a stub data set implementation which raises an error whenever it is called. Semantically, the system would be claiming to support deflated explicit VR little endian at Codec level (on checking that it matches Codec::Dataset(Some(_))), but then still fail at run-time. So any application depending on this check in an automatic discovery of supported transfer syntaxes would be at risk of inadvertently including the stub.

OK, there is one other thing that we can try, which is to adapt the error message in places where support for certain transfer syntaxes were requested, and suggest enabling features there. Need to investigate.

object/src/lib.rs

object/src/mem.rs

rforsyth · 2025-07-15T21:53:15Z

Thanks for adding this feature. We would like to be able to use it. Are there any blockers to merging the PR Eduardo?

@Enet4

Enet4 · 2025-07-16T14:09:28Z

Hello @rforsyth! The short-term plan is to gather the contributions without breaking changes into v0.8.2, then we'll immediately start joining all pending breaking changes for v0.9.0. I hope to get to this soon, as I understand it's been a while since the last release.

* Note, not all paths lead to writing, for example using `InMemDicomObject.read_dataset_with_dict` would bypass deflate. * No support for writing right now

- make it fully dynamic and pass a lifetime from input to output - change the rest of the code to fit DataRWAdapter - revert 'static lifetime constraints - remove unused file

- remove use of dicom_pixeldata - inspect dicom object contents

- JPIP Referenced Deflate - JPIP HTJ2K Referenced Deflate

- check for differences in encapsulated data from source TS to target TS instead of whether they are codec-free

- only run it if JPIP Referenced Deflate is not fully supported yet

- split unsupported transfer syntax into unrecognized (not found in registry) and unsupported (requires dataset adapter impl) - split unsupported TS error further to show suggested feature name (in case of deflated TSes)

- move common logic into read_parts_with_all_options_impl - add SOP attribute inference to from_reader - move detect_preamble function

- do not try to read it if the transfer syntax requires a dataset adapter that the TS specified does not have

- the features are not needed for baseline dicom-object capabilities

- remove deflate as a default feature - propagate deflate feature to dicom-object and dicom-pixeldata - [pixeldata] add deflate as a capability in "native" - [ts-registry][pixeldata] extend and reorganize root documentation

- add support for other deflated TSes

Enet4 · 2025-07-27T12:25:41Z

We're ready to merge this. Thanks again @naterichman. 💪

naterichman commented Nov 18, 2023

View reviewed changes

object/src/mem.rs Outdated Show resolved Hide resolved

Enet4 added A-lib Area: library C-object Crate: dicom-object C-pixeldata Crate: dicom-pixeldata C-transfer-syntax Crate: dicom-transfer-syntax-registry labels Mar 22, 2024

Enet4 added the breaking change Hint that this may require a major version bump on release label Apr 14, 2024

Enet4 force-pushed the deflated-ts branch from 2908dd3 to 2a37efc Compare April 14, 2024 10:16

Enet4 mentioned this pull request Dec 7, 2024

Cargo update (2024-12) #609

Merged

Enet4 force-pushed the deflated-ts branch from 2a37efc to 4c1cd85 Compare December 7, 2024 14:35

Enet4 added this to the DICOM-rs 0.9 milestone Mar 11, 2025

Enet4 force-pushed the deflated-ts branch from 842d624 to d9756e5 Compare April 18, 2025 08:59

Enet4 marked this pull request as ready for review April 18, 2025 09:39

naterichman commented Jun 16, 2025

View reviewed changes

Enet4 changed the title ~~First crack at adding dataset read adapter in DS read for deflate~~ Baseline support for deflate transfer syntaxes with dataset adapter rewrite Jun 19, 2025

Enet4 reviewed Jun 19, 2025

View reviewed changes

Enet4 reviewed Jun 20, 2025

View reviewed changes

object/src/lib.rs Outdated Show resolved Hide resolved

object/src/lib.rs Outdated Show resolved Hide resolved

object/src/mem.rs Outdated Show resolved Hide resolved

Enet4 force-pushed the deflated-ts branch from 9f37894 to 89652aa Compare June 21, 2025 15:53

naterichman and others added 9 commits July 27, 2025 13:05

ENH: First crack at adding dataset read adapter in DS read

b4c7605

ENH: Add support for deflate writing

f57ab8e

* Note, not all paths lead to writing, for example using `InMemDicomObject.read_dataset_with_dict` would bypass deflate. * No support for writing right now

ENH: Add reading and writing adapters for deflated TS

ebd42cd

[encoding] Change signature of trait DataRWAdapter

499c3ea

- make it fully dynamic and pass a lifetime from input to output - change the rest of the code to fit DataRWAdapter - revert 'static lifetime constraints - remove unused file

[ts-registry] Skip unsound test

1fd583c

Tweak test deflate::test_read_data_deflated

8dd2b18

- remove use of dicom_pixeldata - inspect dicom object contents

[encoding] add TransferSyntax::is_encapsulated_pixel_data

f60ba60

[ts-registry] rename flate2 feature to deflate

a36a181

[ts-registry] Extend deflate support to other transfer syntaxes

0cb16bb

- JPIP Referenced Deflate - JPIP HTJ2K Referenced Deflate

Enet4 added 20 commits July 27, 2025 13:05

[pixeldata] Add deflated support to dicom-transcode

defa001

- check for differences in encapsulated data from source TS to target TS instead of whether they are codec-free

[storescu] feature-gate Error::Transcode on "transcode"

25274e4

[ts-registry] Feature-gate deflate module on "deflate"

ee04bf0

Update test submit_replace

005ce1a

- only run it if JPIP Referenced Deflate is not fully supported yet

Update test submit_replace_precondition

6423636

[ts-registry] Remove unnecessary use

63e1cbe

[object] Remove more unnecessary 'static lifetime constraints

1a87022

[ts-registry] Rewrite test write_deflated

124be89

[object] Remove unnecessary 'static constraints

dbe6821

[object] Refactor FileDicomObject write methods

4dbe930

[object] expand object read/write error variants

1fbc110

- split unsupported transfer syntax into unrecognized (not found in registry) and unsupported (requires dataset adapter impl) - split unsupported TS error further to show suggested feature name (in case of deflated TSes)

Apply suggestions from code review

1d862e1

[object] Fix and adjust display messages in ReadError and WriteError

fa9e50c

[object] Reduce repetition in metada+dataset reading

0bf08ca

- move common logic into read_parts_with_all_options_impl - add SOP attribute inference to from_reader - move detect_preamble function

[object] Check dataset adapter on read_dataset_with_dict_ts_cs

5b4249e

- do not try to read it if the transfer syntax requires a dataset adapter that the TS specified does not have

[object] Remove default features in transfer-syntax-registry

0340734

- the features are not needed for baseline dicom-object capabilities

[ts-registry][object][pixeldata] reorganize Cargo features

4583aeb

- remove deflate as a default feature - propagate deflate feature to dicom-object and dicom-pixeldata - [pixeldata] add deflate as a capability in "native" - [ts-registry][pixeldata] extend and reorganize root documentation

[ts-registry] Update ts table

c9d7645

- add support for other deflated TSes

[ts-registry] Run deflate tests conditionally on feature

57ea37f

[ts-registry] Add doc comments on disabled "deflate" feature

3f5c7a5

Enet4 force-pushed the deflated-ts branch from e790c2a to 3f5c7a5 Compare July 27, 2025 12:09

Enet4 merged commit 1855561 into Enet4:master Jul 27, 2025
5 checks passed

Enet4 mentioned this pull request Dec 17, 2025

Deflated Explicit VR Little Endian Case - "Could not read data set token" #350

Closed

Enet4 linked an issue Dec 17, 2025 that may be closed by this pull request

Deflated Explicit VR Little Endian Case - "Could not read data set token" #350

Closed

Conversation

naterichman commented Nov 18, 2023

Uh oh!

Uh oh!

Enet4 commented Dec 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Enet4 commented Apr 18, 2025

Uh oh!

naterichman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

naterichman Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

Enet4 Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

Enet4 Jun 21, 2025

Choose a reason for hiding this comment

Uh oh!

Enet4 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Enet4 Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rforsyth commented Jul 15, 2025

Uh oh!

Enet4 commented Jul 16, 2025

Uh oh!

Enet4 commented Jul 27, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Enet4 commented Dec 8, 2024 •

edited

Loading