[MRG] Stage 1 of new pixels module #1985

scaramallion · 2023-12-27T07:58:19Z

Describe the changes

Adds pixels module
Adds pixels.decoders module
Adds four pixel decoding methods via pixels.decoders.base.Decoder:
- as_array(), iter_array(), as_buffer() and iter_buffer()
- New methods support frame indexing, iterating over frames, returning RGB as default, returning a view over the original buffer, and more
Native pixel data handled by Decoder
Adds decoder plugin for RLE Lossless
Is related to many issues, but these open ones:
Adds generic tests for pixel data in tests/pixels/pixels_reference.py because I'm really tired of writing the same tests over and over again.

Notes:

I may not keep the pixels.enums module.
Also not sure if I'll keep the DecoderRunner.test_for() method
I'll probably end up chopping down the **kwargs section of the docstrings on as_array()/iter_array()/as_buffer()/iter_buffer() and referencing the corresponding decoder guides document page instead.
I thought it'd be more convenient to split out the pixel data related tests to their own sub-folder (pytest tests/pixels ftw), but I can lump them back in with all the rest

Future work

Stage 2

Add the remaining plugins (gdcm, pillow, pylibjpeg, jpeg_ls)
This probably requires an update to pylibjpeg + plugins

Stage 3

Add support for BinaryIO as a src for as_array(), iter_array(), as_buffer() and iter_buffer()
Add a pair of functions to access the pixel data as an ndarray without having to go through dcmread()
- func(src: str | PathLike[str] | BinaryIO, ...) -> numpy.ndarray: and a corresponding frame iterator
- pxlread()? pixel_array()? read_array()? Haven't thought of any really good names for it. I'd like some symmetry with dcmread() though.
Add tutorial + docs

And at some point

Move encoders module under pixels
Move pixel utils to pixels module
Deprecate pixel_data_handlers, switch Dataset pixel array methods over

Tasks

Unit tests added that reproduce the issue or prove feature is working
Fix or feature added
Code typed and mypy shows no errors
Documentation updated (if relevant)
- Preview link
Unit tests passing and overall coverage the same or better

codecov · 2023-12-27T08:03:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (69c7abc) 97.97% compared to head (ed6103e) 98.10%.
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1985      +/-   ##
==========================================
+ Coverage   97.97%   98.10%   +0.12%     
==========================================
  Files          63       67       +4     
  Lines       10968    11710     +742     
==========================================
+ Hits        10746    11488     +742     
  Misses        222      222

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

scaramallion · 2024-01-01T01:17:17Z

src/pydicom/pixels/decoders/base.py

+            if self.photometric_interpretation == PI.YBR_FULL_422:
+                # PS 3.3, Annex C.7.6.3
+                ybr_length = expected / 2 * 3 + expected / 2 * 3 % 2
+                if actual >= ybr_length:
+                    raise ValueError(
+                        "The number of bytes of pixel data is a third larger "
+                        f"than expected ({actual} vs {expected} bytes) which "
+                        "indicates the set photometric interpretation "
+                        "'YBR_FULL_422' is incorrect"
+                    )


Related to #1133, would you like a more detailed message @darcymason ?

scaramallion · 2024-01-01T01:18:53Z

src/pydicom/pixels/decoders/base.py

+    def as_array(
+        self,
+        src: Buffer | Dataset,
+        *,
+        index: int | None = None,
+        validate: bool = True,
+        raw: bool = False,
+        decoding_plugin: str = "",
+        **kwargs: DecodeOptions,
+    ) -> "np.ndarray":


The index parameter here and the indices parameter for iter_array() are related to #1263 and #1243

scaramallion · 2024-01-01T01:22:04Z

src/pydicom/pixels/decoders/base.py

+            * `view_only`: :class:`bool` - if ``True`` then make a best effort
+              attempt to return an :class:`~numpy.ndarray` that's a `view
+              <https://numpy.org/doc/stable/user/basics.copies.html#view>`_
+              on the original buffer (default ``False``).


view_only parameter and behaviour is related to #746

scaramallion · 2024-01-01T01:28:04Z

src/pydicom/pixels/decoders/base.py

+        if force_ybr:
+            arr = convert_color_space(arr, PI.RGB, PI.YBR_FULL)
+            self.set_option("photometric_interpretation", PI.YBR_FULL)
+        elif to_rgb:
+            arr = convert_color_space(arr, PI.YBR_FULL, PI.RGB)
+            self.set_option("photometric_interpretation", PI.RGB)


Related to #1781 (and others), converts YBR to RGB by default

scaramallion · 2024-01-01T02:05:57Z

src/pydicom/pixels/decoders/base.py

+        # Check to see if we have any more frames available
+        #   Should only apply to JPEG transfer syntaxes
+        excess = []
+        for frame in frame_generator:
+            if len(frame) == bytes_per_frame:
+                excess.append(np.frombuffer(frame, dtype))
+                runner.set_option("number_of_frames", runner.number_of_frames + 1)
+
+        if excess:
+            warn_and_log(
+                "More frames have been found in the encapsulated pixel data "
+                "than expected from the supplied number of frames"
+            )
+            arr = np.concatenate([arr, *excess])


Include any excess frames, related to #1666

mrbean-bremen

@scaramallion - again a lot of new code, and I'm slower with reviewing than you have been with writing... I will finish it another day, but will post the few comments and questions I had so far.
I shall add that I like this refactoring, awesome work as usual!

doc/guides/decoding/decoder_plugins.rst

mrbean-bremen · 2023-12-31T10:12:12Z

doc/guides/decoding/decoder_plugins.rst

+    decoding plugins.
+
+  When possible it's recommended that the decoding function return the decoded
+  pixel data as a :class:`bytearray` to minimise later memory usage.


doc/guides/decoding/decoder_plugins.rst

src/pydicom/pixels/decoders/base.py

mrbean-bremen · 2024-01-01T11:15:56Z

src/pydicom/pixels/decoders/base.py

+        actual = len(self._src)
+
+        if self.transfer_syntax.is_encapsulated:
+            if actual == expected:


Shouldn't you add the padding before you make this check?

Ah, true. Will fix

src/pydicom/pixels/decoders/base.py

mrbean-bremen

As usual, I only skimmed the tests, not checking any numbers - I trust that you to test the correct ones. Sorry for the long delay, I had kind of a stack overflow in my personal backlog...

scaramallion · 2024-01-07T11:33:27Z

No problem, thanks for looking at it

darcymason · 2024-01-10T22:44:15Z

Hey, I somehow missed this being MRG, haven't been checking in much lately because, well, Christmas etc., and I had a terrible cold. I'll check it out in the next couple of days.

scaramallion · 2024-01-10T23:31:24Z

No worries

src/pydicom/pixels/decoders/base.py

darcymason

Did about half of a review -- lots here! Just a few comments so far...

darcymason · 2024-01-12T23:03:46Z

doc/guides/decoding/decoder_plugin_options.rst

+|                          | Key                   | Value                                          |
+==========================+=======================+================================================+
+|:attr:`RLELosslessDecoder`| ``rle_segment_order`` | ``"<"`` for big endian segment order (default) |
+|                          |                       | or ``">"`` for little endian segment order     |


Are the characters correct? ">" is big endian in Python struct module.

Yeah, it should be the other way around. Fixed

doc/guides/decoding/decoder_plugins.rst

src/pydicom/pixels/decoders/base.py

darcymason

So finally got through at least quickly going over most of the code, although with limited review of the tests, similar to @mrbean-bremen said.

This is quite the epic piece of work - it puts the pixel handling in a much better structure for future additions/changes.

darcymason · 2024-01-13T23:08:23Z

src/pydicom/pixels/decoders/base.py

+
+            8-bit pixel data encoded as **OW** using Explicit VR Big Endian will
+            be yielded as-is and may need byte-swapping. To facilitate this
+            an extra byte before the expected start (for and odd `index`) or after


for an_ index

Fixed, here and in as_buffer

darcymason · 2024-01-13T23:13:10Z

src/pydicom/pixels/decoders/base.py

+        .. warning::
+
+            This method is not thread-safe.
+


I'm thinking it could be thread-safe if the dict.pop() was used again? And I guess a missing label would have to be ignored.

Not that pydicom overall has actually tried to be thread-safe in any way, so I don't think it is worth worrying about.

Mostly I'm concerned about there being a single instance of the specific UID decoder, so any changes to it in one thread will propagate to the other threads. I've been trying to make everything threading/multiprocessing friendly (hopefully) since going off our citations in published papers a significant use case for our pixel data handling is machine learning/image processing pipelines.

I think this affects the encoder implementation too, I'm planning on refactoring it when I add in support for encoding J2K.

Add pixels module and new pixel decoding method

d2f14fb

scaramallion added 3 commits December 28, 2023 10:49

Add tests for missing coverage

e366fd6

Doc updates

77a5ae7

Merge upstream

a678e30

scaramallion changed the title ~~Stage 1 of new pixels module~~ [MRG] Stage 1 of new pixels module Dec 30, 2023

scaramallion marked this pull request as ready for review December 30, 2023 21:48

Change from warning to exception for YBR_FULL_422 length check

0fd7520

scaramallion commented Jan 1, 2024

View reviewed changes

Add support for excess frames to as_array() and as_buffer()

d5ba9bb

scaramallion commented Jan 1, 2024

View reviewed changes

mrbean-bremen reviewed Jan 1, 2024

View reviewed changes

Changes from review

74155c1

mrbean-bremen approved these changes Jan 7, 2024

View reviewed changes

scaramallion commented Jan 11, 2024

View reviewed changes

src/pydicom/pixels/decoders/base.py Outdated Show resolved Hide resolved

darcymason reviewed Jan 12, 2024

View reviewed changes

Changes from review 2

903c90d

darcymason approved these changes Jan 15, 2024

View reviewed changes

Fix typos

ed6103e

scaramallion merged commit 800635f into pydicom:main Jan 15, 2024
9 checks passed

scaramallion deleted the dev-pixel-decoding branch January 15, 2024 23:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Stage 1 of new pixels module #1985

[MRG] Stage 1 of new pixels module #1985

scaramallion commented Dec 27, 2023 •

edited

codecov bot commented Dec 27, 2023 •

edited

scaramallion Jan 1, 2024

scaramallion Jan 1, 2024 •

edited

scaramallion Jan 1, 2024

scaramallion Jan 1, 2024

scaramallion Jan 1, 2024

mrbean-bremen left a comment

mrbean-bremen Dec 31, 2023

scaramallion Jan 2, 2024

mrbean-bremen Jan 1, 2024

scaramallion Jan 1, 2024

scaramallion Jan 2, 2024

mrbean-bremen left a comment

scaramallion commented Jan 7, 2024

darcymason commented Jan 10, 2024

scaramallion commented Jan 10, 2024

darcymason left a comment

darcymason Jan 12, 2024

scaramallion Jan 13, 2024

darcymason left a comment

darcymason Jan 13, 2024

scaramallion Jan 15, 2024

darcymason Jan 13, 2024

scaramallion Jan 15, 2024

[MRG] Stage 1 of new pixels module #1985

[MRG] Stage 1 of new pixels module #1985

Conversation

scaramallion commented Dec 27, 2023 • edited

Describe the changes

Future work

Tasks

codecov bot commented Dec 27, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

scaramallion Jan 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrbean-bremen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrbean-bremen left a comment

Choose a reason for hiding this comment

scaramallion commented Jan 7, 2024

darcymason commented Jan 10, 2024

scaramallion commented Jan 10, 2024

darcymason left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darcymason left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scaramallion commented Dec 27, 2023 •

edited

codecov bot commented Dec 27, 2023 •

edited

scaramallion Jan 1, 2024 •

edited