-
Notifications
You must be signed in to change notification settings - Fork 78
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.
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
Transfer Syntax index abstraction & inventory-less registry #17
Conversation
- trait for the TS registry - implement it, adapt existing code
- `inventory-registry` feature for whether to include inventory-based TS - make transfery-syntax-registry work even without inventory - built-in TSes no longer go through inventory - make entries.rs an actual module - propagate feature to upper crates - make it default in parent crate and end tools such as dcmdump - improve TS registry test to also check TS name
- run tests without inventory-based registry on a few crates
That's awesome, thanks for cc'ing, I'll be happy to test! I'm updating my prototype to fully use the code from this PR. I have one blocker though, /// Retrieve the data value.
pub fn value(&self) -> &Value<I> {
&self.value
} Is it possible to add |
Sure, I just pushed PR #18, which can be merged before this one. |
@Enet4 would you mind rebasing it on master to grab the |
I'm running into another issue trying to update my code. I want to iterate through entries using let root_object = from_reader(&input[128..])
.map_err(|e| Error::new(&format!("Error parsing input buffer: {:?}", e)))?;
for data_element in root_object.into_iter() {
} The error is Since field impl<T> ::std::ops::Deref for RootDicomObject<T> {
type Target = T;
fn deref(&self) -> &Self::Target {
&self.obj
}
} I guess, having something similar with |
I tested this PR with [dependencies]
dicom-core = "0.1.0"
dicom-object = "0.1.0" and having a local checkout of this PR with minor changes [patch.crates-io.dicom-core]
path = "../../../dicom-rs/core"
[patch.crates-io.dicom-object]
path = "../../../dicom-rs/object" Both Changes I made in a local copy: impl<T> RootDicomObject<T> {
// not sure about a method name
pub fn into_object(self) -> T {
self.obj
}
} |
Just rebased. And yes, Thank you for the testing and feedback so far! |
Since #19 is merged, it'll be good to rebase this PR again. [patch.crates-io.dicom-core]
path = "../../../dicom-rs/core"
[patch.crates-io.dicom-object]
path = "../../../dicom-rs/object" All three options work fine for built-in transfer syntaxes
I ran into one compiler error with this code dicom-object = { version = "0.1.0", features = ["inventory-registry"] } submit_transfer_syntax! {
TransferSyntax::new(
"1.2.840.10008.1.2.4.70",
"JPEG Lossless, Non-Hierarchical, First-Order Prediction",
Endianness::Little,
true,
Codec::EncapsulatedPixelData,
)
} The error is
|
- trait for the TS registry - implement it, adapt existing code
- `inventory-registry` feature for whether to include inventory-based TS - make transfery-syntax-registry work even without inventory - built-in TSes no longer go through inventory - make entries.rs an actual module - propagate feature to upper crates - make it default in parent crate and end tools such as dcmdump - improve TS registry test to also check TS name
- run tests without inventory-based registry on a few crates
Tested again with [patch.crates-io.dicom-core]
git = "https://github.com/Enet4/dicom-rs"
rev = "2647055"
[patch.crates-io.dicom-object]
git = "https://github.com/Enet4/dicom-rs"
rev = "2647055" Everything works fine for WebAssembly target:
Surprisingly, I even saw some speedups in decoding) |
The main In practice, it's easier to create a static variable of the TS in a lazy_static! {
static ref MY_TS: TransferSyntax = TransferSyntax::new(
"1.2.840.10008.1.2.4.70",
"JPEG Lossless, Non-Hierarchical, First-Order Prediction",
Endianness::Little,
true,
Codec::EncapsulatedPixelData,
);
}
submit_transfer_syntax!(MY_TS); (One shouldn't need to register "JPEG Lossless, Non-Hierarchical, First-Order Prediction" like this, it's already registered built-in. You would later on do this to implement pixel data encoding and decoding, which is yet to be designed at the moment.) |
Thanks, I'll try it
I know, I just commented it away in |
- add `open_file_with` and `from_reader_with` - for extra control on which TS index to use
I have just pushed another commit which includes a way to open DICOM objects using another TS registry, which means that in theory, even in an environment without |
Sorry for a delay. As default transfer syntaxes work in webassembly, I'm just testing that extension of TS registry works as expected for native target. |
@Enet4 I tested all the cases I wanted: ts extension works fine for native target and doesn't work for webassembly (as expected). I haven't tested external TS registries though, but this change looks pretty straightforward. I think, it's good to go now. Thank you for updates! PS, I figured out how to submit transfer syntax properly: submit_transfer_syntax! {
TransferSyntax::<NeverAdapter>::new(
"1.2.840.10008.1.2.4.70",
"JPEG Lossless, Non-Hierarchical, First-Order Prediction",
Endianness::Little,
true,
Codec::EncapsulatedPixelData,
)
} The key part here is |
Glad to know you have managed to work around the new features. Yes, extending a WebAssembly environment to support more TSes would require building your own registry for the time being. The encoding crate could be extended to include a basic implementation in the future. Thank you very much for testing this! |
This PR introduces a trait for transfer syntax dictionaries and makes it so that the main TS registry can work even without support for the
inventory
based registry. The inventory will be behind the Cargo featureinventory-registry
, enabled by default only at the parent cratedicom
and in end user tools such asdcmdump
. If this is disabled, built-in TSes will still be available, but the registry will be unable to be extended.We can think of ways to overcome this issue in future contributions.This can be overcome by creating a new registry implementation.This should address the main concern presented in #6, although I would be grateful to have this tested in that environment (CC @ibaryshnikov).