Pixel encapsulation #312

dougyau · 2022-11-25T08:27:55Z

Add the function to encapsulate a vector of frames. The function will return a PixelSequence which can be used with DataElement::new() to add the tag to the dataset.

let mut frames: Vec<Vec<u8>> = ...

let encapsulated_pixels = dicom::pixeldata::encaspulation::encapsulate(&mut frames, 1)?;
dcm.put(DataElement::new(dicom::dictionary_std::tags::PIXEL_DATA, dicom::core::VR::OB, encapsulated_pixels);

…se the minimum posible size. Consume the input vector intead of using a reference.

…l on how to add the frames, this gives options on how much data is stored at the time

dougyau · 2022-11-27T13:11:24Z

I have done a refactor, to allow different kind of scenarios.

In the first scenario, you could use the simplest call. The bad part of this is that at some point, the input frames and the output frames are allocated.

let frames: Vec<Vec<u8>> = ...

let encapsulated_pixels = dicom::pixeldata::encapsulation::encapsulate(&mut frames, 1);
dcm.put(DataElement::new(dicom::dictionary_std::tags::PIXEL_DATA, dicom::core::VR::OB, encapsulated_pixels);

Now if memory is an issue (mostly when dealing with multiframe images) the frames can be processed individually.

let frames: Vec<Vec<u8>> = ...
let encapsulated_pixels = frames.
    .into_iter()
    .map(|frame| ...do some encoding)
    .for_each(|encoded_frame| encapsulated_pixels.add_frame(encoded_frame));

dcm.put(DataElement::new(dicom::dictionary_std::tags::PIXEL_DATA, dicom::core::VR::OB, encapsulated_pixels);

And the last way is useful when using rayon

let frames: Vec<Vec<u8>> = ...
let fragments = frames.
    .into_par_iter()
    .map(|frame| ...do some encoding)
    .flat_map(|encoded_frame| dicom::pixeldata::encapsulation::fragment_frame(encoded_frame, 1))
    .collect::<Vec<Vec<u8>>>;
let encapsulated_pixels = dicom::pixeldata::encapsulation::EncapsulatedPixels::from(fragments);

dcm.put(DataElement::new(dicom::dictionary_std::tags::PIXEL_DATA, dicom::core::VR::OB, encapsulated_pixels);

Enet4

Thank you for continuing this contribution. I like the idea of introducing a more substantial API! But I also feel that there is a lot to improve and fix before it is ready. Please attend to the comments inline.

pixeldata/src/encapsulation.rs

…gment size

dougyau · 2023-01-17T02:36:51Z

@Enet4 I have improved the code based on the suggestions. Please let me know if you see more things that can be improved.

Enet4

Hey @dougyau! I have been doing some changes to most of the crates in the DICOM-rs project. In particular, I felt that we would be better served with the pixel data sequence type being declared in the core crate itself, which may subvert some of the decisions made here.
Nevertheless, it is still important and useful to have some mechanisms to turn frames into fragments and vice versa (which we do not have yet), so I do not want to put this pull request to waste! :) Please see the inline comment and let me know if you have any other plans here.

Enet4 · 2023-06-20T13:16:07Z

pixeldata/src/encapsulation.rs

+/// dcm.put(DataElement::new(PIXEL_DATA, OB, encapsulated_pixels));
+/// ```
+#[derive(Debug, Default)]
+pub struct EncapsulatedPixels {


The upstream version (master branch) of dicom_core now features a pixel data fragment sequence type dicom_core::value::PixelFragmentSequence, so we want to build up our frames-to-fragments abstraction from this. EncapsulatedPixels would be removed and PixelFragmentSequence would be given an impl From<FrameFragments>.

If there are any good methods that would be important to have in PixelFragmentSequence, it should be possible to accommodate them there as well.

I think this is a great idea as the API would be simpler.

Currently this code should be compatible with the changes made on upstream. But I do like your proposal.

core/src/value/fragments.rs

dougyau · 2023-06-23T21:46:50Z

@Enet4 I have updated the tests and the doc blocks on the PR.

ingwinlu · 2023-06-24T05:28:31Z

pixeldata/src/encapsulation.rs

+use dicom_core::value::Value;
+
+/// Encapsulate the pixel data of the frames. If fragment_size > 0 it will use 1 fragment per frame.
+/// This parameter is ignored for multi frame data, as 1 fragment per frame is required.


isn't that depending on the transfer syntax used?
https://dicom.nema.org/dicom/2013/output/chtml/part05/sect_A.4.html
Whether more than one fragment per frame is permitted or not is defined per Transfer Syntax.

Thank you for having a look @ingwinlu. The limitations of this abstraction are known, as it aims to facilitate the constructions in the following scenarios (which I believe are the most common ones):

Each frame is given a single fragment, as in RLE Lossless and all JPEG-based transfer syntaxes;

A simple image frame is split into multiple fragments.

With that said, there is probably a better way to describe this function in a way which does not confuse API consumers, maybe by having a signature encapsulate_single_frame(frame: Vec<u8>, max_fragment_size: u32) -> Value instead. Unless it happens that the current function is convenient for both situations, because otherwise it may become a source of confusion that fragment_size is ignored when more than one frame is passed.

I think it is better to have 1 function for this, otherwise when you intend to encapsulate you need to verify if it is single or multi frame, then call the corresponding function.

Enet4

I made a few more suggestions. Primarily, I think it makes sense to split encapsulate for the two key scenarios (unless there is a reason I haven't grasped yet). encapsulate_single_frame would receive a single frame and a maximum fragment size, whereas the other encapsulate_frames would give each frame its own fragment and provide a basic offset table accordingly.

Enet4 · 2023-06-24T14:50:17Z

core/src/value/fragments.rs

+/// Represents the fragments of a single frame. [PixelFragmentSequence] can be generated from a list
+/// of [Fragments]. In case of multi frame a list of frames composed by 1 fragment is expected.


I usually prefer adding backticks when referring to other types.

The first paragraph is dedicated as a the type's short description.

Suggested change

/// Represents the fragments of a single frame. [PixelFragmentSequence] can be generated from a list

/// of [Fragments]. In case of multi frame a list of frames composed by 1 fragment is expected.

/// Represents the fragments of a single frame.

///

/// A [`PixelFragmentSequence`] can be generated from a list of [`Fragments`].

/// In case of multi-frame, a list of frames composed by 1 fragment is expected.

Enet4 · 2023-06-24T14:55:17Z

pixeldata/src/encapsulation.rs

+use dicom_core::value::Value;
+
+/// Encapsulate the pixel data of the frames. If fragment_size > 0 it will use 1 fragment per frame.
+/// This parameter is ignored for multi frame data, as 1 fragment per frame is required.


Thank you for having a look @ingwinlu. The limitations of this abstraction are known, as it aims to facilitate the constructions in the following scenarios (which I believe are the most common ones):

Each frame is given a single fragment, as in RLE Lossless and all JPEG-based transfer syntaxes;

A simple image frame is split into multiple fragments.

With that said, there is probably a better way to describe this function in a way which does not confuse API consumers, maybe by having a signature encapsulate_single_frame(frame: Vec<u8>, max_fragment_size: u32) -> Value instead. Unless it happens that the current function is convenient for both situations, because otherwise it may become a source of confusion that fragment_size is ignored when more than one frame is passed.

core/src/value/fragments.rs

…ames are handled by separately

dougyau · 2023-06-24T19:35:42Z

The changes have been implemented.

pixeldata/src/encapsulation.rs

Enet4

OK, we are in condition to bring this upstream. Thank you @dougyau for your commitment.

dougyau added 4 commits November 25, 2022 02:02

Add function to encapsulate the frame data

940522e

The fragment size should be local to the frame

7c20e80

Merge branch 'Enet4:master' into pixel-encapsulation

66f6b65

Add function description and set fragments to 1 if frames > 1

29a2f89

Enet4 added the A-lib Area: library label Nov 25, 2022

Enet4 self-requested a review November 25, 2022 10:26

dougyau added 4 commits November 25, 2022 18:16

Fix typo in the file name

967dc8b

Use the corrected filename

650830f

Instead of throwing an error when too many fragments are requested, u…

95cd2a9

…se the minimum posible size. Consume the input vector intead of using a reference.

Split the frame fragmentation logic. Allow different levels of contro…

b615764

…l on how to add the frames, this gives options on how much data is stored at the time

dougyau added 2 commits November 27, 2022 20:22

Add unit tests

6540609

Use rustfmt

cf7a37a

Enet4 requested changes Dec 10, 2022

View reviewed changes

Enet4 added enhancement C-pixeldata Crate: dicom-pixeldata labels Dec 10, 2022

dougyau added 9 commits January 16, 2023 13:39

Merge branch 'Enet4:master' into pixel-encapsulation

eb91273

Replace Into with From

b60f2f0

Rename EncapsulatiedPixels.bot to EncapsulatedPixels.offset_table

b704266

Use fragment_size instead of number_of_fragments to calculate the fra…

ac60fdb

…gment size

Instead of implementing From to import fragments, use from_fragments

4b80d07

Remove unnecessary derreference

ce0783b

Remove unnecessary fields. Improve the calculation of the offset table

3245fc0

Add the documentation of the EncapsulatedPixels struct

dc0fa38

Add documentation and improve error handling

2a726df

Enet4 self-requested a review January 17, 2023 08:41

dougyau added 2 commits January 17, 2023 12:28

Fix the doctests so CI doesn't fail

8bb07de

Use the new api from dicom_object

6d7ada0

Enet4 reviewed Jun 20, 2023

View reviewed changes

dougyau added 2 commits June 20, 2023 09:46

Merge branch 'Enet4:master' into pixel-encapsulation

f61e6e6

Implement fragments in core

3cda881

dougyau commented Jun 20, 2023

View reviewed changes

core/src/value/fragments.rs Show resolved Hide resolved

dougyau marked this pull request as draft June 20, 2023 18:19

dougyau marked this pull request as ready for review June 20, 2023 18:56

dougyau added 4 commits June 23, 2023 15:02

Document dicom_core::fragments

e8cdfee

Put the comment on the correct line

509d830

Use InMemFragment instead of Vec<u8>

029b5fe

Remove the code that was moved to dicom_core.

1266154

ingwinlu reviewed Jun 24, 2023

View reviewed changes

Enet4 reviewed Jun 24, 2023

View reviewed changes

Improve documentation. Split encapsulate so single frame and multi fr…

5388c07

…ames are handled by separately

Enet4 reviewed Jun 25, 2023

View reviewed changes

pixeldata/src/encapsulation.rs Outdated Show resolved Hide resolved

pixeldata/src/encapsulation.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

dcea826

Enet4 approved these changes Jun 25, 2023

View reviewed changes

Enet4 merged commit d4ccc60 into Enet4:master Jun 25, 2023
4 checks passed

dougyau deleted the pixel-encapsulation branch July 23, 2023 22:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pixel encapsulation #312

Pixel encapsulation #312

dougyau commented Nov 25, 2022

dougyau commented Nov 27, 2022

Enet4 left a comment

dougyau commented Jan 17, 2023

Enet4 left a comment

Enet4 Jun 20, 2023

dougyau Jun 20, 2023

dougyau commented Jun 23, 2023

ingwinlu Jun 24, 2023 •

edited

Enet4 Jun 24, 2023 •

edited

dougyau Jun 24, 2023

Enet4 left a comment

Enet4 Jun 24, 2023

Enet4 Jun 24, 2023 •

edited

dougyau commented Jun 24, 2023

Enet4 left a comment

		/// Represents the fragments of a single frame. [PixelFragmentSequence] can be generated from a list
		/// of [Fragments]. In case of multi frame a list of frames composed by 1 fragment is expected.

Pixel encapsulation #312

Pixel encapsulation #312

Conversation

dougyau commented Nov 25, 2022

dougyau commented Nov 27, 2022

Enet4 left a comment

Choose a reason for hiding this comment

dougyau commented Jan 17, 2023

Enet4 left a comment

Choose a reason for hiding this comment

Enet4 Jun 20, 2023

Choose a reason for hiding this comment

dougyau Jun 20, 2023

Choose a reason for hiding this comment

dougyau commented Jun 23, 2023

ingwinlu Jun 24, 2023 • edited

Choose a reason for hiding this comment

Enet4 Jun 24, 2023 • edited

Choose a reason for hiding this comment

dougyau Jun 24, 2023

Choose a reason for hiding this comment

Enet4 left a comment

Choose a reason for hiding this comment

Enet4 Jun 24, 2023

Choose a reason for hiding this comment

Enet4 Jun 24, 2023 • edited

Choose a reason for hiding this comment

dougyau commented Jun 24, 2023

Enet4 left a comment

Choose a reason for hiding this comment

ingwinlu Jun 24, 2023 •

edited

Enet4 Jun 24, 2023 •

edited

Enet4 Jun 24, 2023 •

edited