First crack at adding dataset read adapter in DS read for deflate

[naterichman]

No description provided.

[naterichman]

To be honest, not sure where you had in mind to adapt the reader.

[Enet4]

Seems like a better place than most at the moment. 😅 Maybe there will be a better place after some of the efforts for lazy DICOM data loading are in place.

[naterichman]

I pushed another commit where I got this working at least with one test. After playing around with this more, I don't think this is the right place for it to go, I think it needs to go into DataSetReader and DataSetWriter constructor new_with_ts_cs_options since there are some code paths for reading a dataset that would not go through this change.

I tried doing that a bit, but I started running into issues with type mismatches:

    pub fn new_with_ts_cs_options(
        source: R,
        ts: &TransferSyntax,
        cs: SpecificCharacterSet,
        options: DataSetReaderOptions,
    ) -> Result<Self>
    where
        R: Read,
    {
        if let Codec::Dataset(Some(adapter)) = ts.codec() {
            let adapter = adapter.adapt_reader(Box::new(source));
            let parser = DynStatefulDecoder::new_with(adapter, ts, cs, 0).context(CreateDecoderSnafu)?;

            is_stateful_decode(&parser);

            Ok(DataSetReader {
                parser,
                options,
                seq_delimiters: Vec::new(),
                delimiter_check_pending: false,
                offset_table_next: false,
                in_sequence: false,
                hard_break: false,
                last_header: None,
                peek: None,
            })
        } else {
            let parser = DynStatefulDecoder::new_with(adapter, ts, cs, 0).context(CreateDecoderSnafu)?;

            is_stateful_decode(&parser);

            Ok(DataSetReader {
                parser,
                options,
                seq_delimiters: Vec::new(),
                delimiter_check_pending: false,
                offset_table_next: false,
                in_sequence: false,
                hard_break: false,
                last_header: None,
                peek: None,
            })
        }
    }

But I keep getting variants of this and I have not found a good way around it...

error[E0308]: mismatched types
   --> parser/src/dataset/read.rs:214:17
    |
175 | impl<R> DataSetReader<DynStatefulDecoder<R>> {
    |      - this type parameter
...
214 |                 parser,
    |                 ^^^^^^ expected type parameter `R`, found `Box<dyn std::io::Read>`
    |
    = note: expected struct `StatefulDecoder<Box<dyn DecodeFrom<R>>, R>`
               found struct `StatefulDecoder<Box<dyn DecodeFrom<Box<dyn std::io::Read>>>, Box<dyn std::io::Read>>`

[naterichman]

Okay, I think I understand the issue, the type parameter R is changing, so I'm thinking one of these structs needs to be dynamic instead of static, i.e.

struct DataSetReader {
    parser: Box<dyn Read>,
    ...

If we go with this method.

I'll let you decide on how deep you want to have the adapter applied, seems to me it can either be

• In InMemDicomObject and FileDicomObject in the object module, which would need to be in multiple methods, i.e. write_to_file, write_all, write_dataset, open_file, read_dataset and the other variants
• In the DatasetReader and DatasetWriter in a few variants
• In the StatefulEncoder and StatefulDecoder

To me it seems the first would require the most code change, but its all straightforward (mostly copy and pasted), while the second two would require a bit of refactor on those structs, but wouldn't duplicate as much code.

Let me know if I'm interpreting this wrong, and if you have any opinions. Happy to go forward with whatever you feel is best!

[Enet4]

Thank you for describing the situation @naterichman. That is pretty much the kind of rabbit hole I might have found last time I tried to incorporate support for deflated explicit VR little endian.

The components in the dicom-parser crate were designed with the one-way handshake pattern (and occasionally three-way handshake pattern) to allow some forms of dynamic dispatching while preventing most underlying calls from having an unwanted indirection. The current implementation relies on reading and writing to be buffered and fast, as it can end up making many calls per element. If parser in DataSetReader becomes a Box<dyn Read>, we are likely to lose performance.

In the long term, I understand now that low-level primitives expecting a fast impl Read or impl Write has its issues, and might not be the best way to go towards a flexible implementation with better performance. In particular, separating the concerns of fetching/producing the byte stream from DICOM data decoding and encoding would help the constructs in dicom-encoding and dicom-parser by making them less dependent on so many type parameters.

For the time being however, support for deflate may have to be written in the upper layers, where we can more easily create a separate path for arbitrary data set encoders and decoders.

[naterichman]

@Enet4 thanks for the link to the handshake pattern that was a cool read. I understand the issue. In the meantime I will go forward with adding the support for deflate to the other places in the upper layers. Probably won't be for a week or so due to american thanksgiving! Thanks again for the info and feedback!

[naterichman]

Okay, I've added the adapters to read and write, but I had to add the 'static lifetime annotation to satisfy the Box::from() when wrapping the source/destination in the adapter. From my basic test writing a deflated TS file which doesn't currently compile, it seems like this would be a breaking change, which I'm guessing you don't want.

Any guidance on how to approach this to relax that lifetime annotation would be appreciated

[Enet4]

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 in dicom-dump, dicom-toimage, and perhaps 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.