Add support for high bit depth multichannel images #8224

yoursunny · 2024-07-10T22:36:30Z

Fixes #1888 .

Changes proposed in this pull request:

Add support for high bit depth multichannel images

This only gets us to opening the first band, it doesn't actually allow the other bands to be retrieved.

aclark4life · 2024-07-11T01:16:50Z

Thanks!

for more information, see https://pre-commit.ci

wiredfool

Wrote a review a few days back but apparently it got lost in GH somewhere. So from memory...

There are 2 potential issues that the multiband storage should be able to solve:

Allowing RGB/CMYK images with more than 8 bytes/channel
Allowing N channel images, where n>4.

For example, we should be able to read a 16 bit RGB image and do all operations on it, and we should be able to read a 5 band image and use 4 of the bands as RGBA.

This is currently complicated by modes encoding two separate issues -- The band numerical representation (8 bit int/float/etc) and logical meaning (L/rgb/cmyk).

What I'm generally missing in this PR is an indication of:

What's the metadata design for the pixel format and band layout
What's the approach to doing native operations on these images of arbitrary struct shape/size?

wiredfool · 2024-07-15T08:01:41Z

src/_imaging.c

@@ -433,6 +433,36 @@ float16tofloat32(const FLOAT16 in) {
    return out[0];
 }

+static inline PyObject *
+getpixel_mb(Imaging im, ImagingAccess access, int x, int y) {
+    UINT8 pixel[sizeof(INT32) * 6];


What's the 6 from here?

This used to be UINT8 pixel[im->pixelsize] but a strict compiler is unhappy.
Currently the largest entry in OPEN_INFO has 6 bands, so that it's defined as 6.
I may have to put a pre-allocated uint8[pixelsize] buffer in Imaging struct.

The C layer has to be internally safe, we can't be relying on inderict python restrictions for memory safety like that. If we can't do this statically, it's going to have to be dynamic, or there needs to be an arbitrary band limit constant somewhere in the code. (Which ultimately, may be a good idea, if only to prevent someone from creating an image with 2^32-1 bands)

wiredfool · 2024-07-15T08:06:20Z

src/PIL/TiffImagePlugin.py

@@ -182,6 +182,43 @@
    (II, 1, (1,), 1, (12,), ()): ("I;16", "I;12"),
    (II, 0, (1,), 1, (16,), ()): ("I;16", "I;16"),
    (II, 1, (1,), 1, (16,), ()): ("I;16", "I;16"),
+    (II, 1, (1, 1), 1, (16, 16), (0,)): ("MB", "MB"),
+    (


This formatting is actively bad.

It's forced by make lint.

Well then, make lint is wrong. ;>

This may just be my long term fight against minless conformity, but sometimes black's formatting obscures the code. And this is one of the cases.

I would suggest just # noqa around whatever you don't want formatted for now … also the kids are using ruff these days 😄

We can use # fmt: off+# fmt: on or # fmt: skip here:

https://black.readthedocs.io/en/stable/usage_and_configuration/the_basics.html#ignoring-sections

wiredfool · 2024-07-15T08:15:40Z

src/libImaging/Crop.c

@@ -37,7 +37,8 @@ ImagingCrop(Imaging imIn, int sx0, int sy0, int sx1, int sy1) {
        ysize = 0;
    }

-    imOut = ImagingNewDirty(imIn->mode, xsize, ysize);
+    imOut = ImagingNewDirty(
+        imIn->mode, (ImagingNewParams){xsize, ysize, imIn->depth, imIn->bands});


Does it make sense to have im->params as {xsize, ysize, depth, bands} for passing into ImagingNewDirty?

wiredfool · 2024-07-15T08:21:12Z

src/libImaging/Imaging.h

+    int xsize;
+    int ysize;
+    int depth; /** MB mode only. */
+    int bands; /** MB mode only. */


Why aren't we seting bands/depth for the non MB modes?

The depth variable appears to be aspirational and may never have been used, possibly the same is true for bands, but haven't confirmed either yet.

If so, and depth was "ignored in this version" in Fredrik's original implementation and subsequent development in the 90s and 2000s, that makes this feature even more compelling to add now, as it appears to have been envisioned in some form from the beginning. 🤔

It's my hope that all other modes are eventually converted to MB mode only.

#WIPFor25Years Can be a thing

wiredfool · 2024-07-15T08:23:14Z

src/libImaging/Storage.c

    Imaging im;

    /* linesize overflow check, roughly the current largest space req'd */
-    if (xsize > (INT_MAX / 4) - 1) {
+    if (p.xsize > (INT_MAX / 4) - 1) {


This will need to be updated for bounds checking for MB images.

Specifically, xsize > (INT_MAX / (bands*depth) -1) needs to error.

wiredfool · 2024-07-15T08:25:34Z

src/libImaging/Storage.c

+                "multi-band missing bands and depth");
+        }
+        im->bands = p.bands;
+        im->depth = p.depth;


Where are we checking for valid Bands/Depth?

wiredfool · 2024-07-15T08:26:46Z

src/map.c

            stride = xsize * 2;
+        } else if (strcmp(mode, IMAGING_MODE_MB) == 0) {
+            stride = xsize * depth * bands;


Bounds checking here?

wiredfool · 2024-07-15T08:28:37Z

src/libImaging/Imaging.h

@@ -68,10 +78,21 @@ typedef struct ImagingPaletteInstance *ImagingPalette;
 #define IMAGING_TYPE_INT32 1
 #define IMAGING_TYPE_FLOAT32 2
 #define IMAGING_TYPE_SPECIAL 3 /* check mode for details */
+#define IMAGING_TYPE_MB 4      /* multi-band format */

 #define IMAGING_MODE_LENGTH \
    6 + 1 /* Band names ("1", "L", "P", "RGB", "RGBA", "CMYK", "YCbCr", "BGR;xy") */


wiredfool · 2024-07-15T08:37:57Z

src/_imaging.c

+    UINT8 *pos = pixel;
+    for (int i = 0; i < im->bands; ++i) {
+        switch (im->depth) {
+            case CHAR_BIT:


Style note - CHAR_BIT isn't used elsewhere here, We're just using 8

Yay295 · 2024-07-19T14:49:42Z

What's the metadata design for the pixel format and band layout

I proposed one back in #6547 if anyone wants to look at that again.

aclark4life · 2024-07-19T14:56:35Z

Thanks for the review @wiredfool ! I'm hopeful that between now and October or EOY at the latest we (@yoursunny @radarhere @Yay295 et al.) can develop this into something you'll approve for merging. Adding all four of us as reviewers to confirm acceptance when we get there, most importantly @hugovk and @radarhere will need to sign off on this as they are currently on the "front lines" of Pillow development and we don't want to release something that will make any of our lives harder. My current assumption is that an implementation that satisfies the requirements and does not introduce any or minimal technical debt is possible.

yoursunny

What's the metadata design for the pixel format and band layout

There's a brief description in Imaging.h.

yoursunny · 2024-07-19T14:45:57Z

src/PIL/TiffImagePlugin.py

@@ -182,6 +182,43 @@
    (II, 1, (1,), 1, (12,), ()): ("I;16", "I;12"),
    (II, 0, (1,), 1, (16,), ()): ("I;16", "I;16"),
    (II, 1, (1,), 1, (16,), ()): ("I;16", "I;16"),
+    (II, 1, (1, 1), 1, (16, 16), (0,)): ("MB", "MB"),
+    (


It's forced by make lint.

yoursunny · 2024-07-19T14:48:11Z

src/decode.c

+                dst[i + 1] = src[i];
+            }
+            return;
+            case CHAR_BIT:


This should be moved below line330.
(I ducked up)

yoursunny · 2024-07-19T14:53:54Z

src/_imaging.c

@@ -433,6 +433,36 @@ float16tofloat32(const FLOAT16 in) {
    return out[0];
 }

+static inline PyObject *
+getpixel_mb(Imaging im, ImagingAccess access, int x, int y) {
+    UINT8 pixel[sizeof(INT32) * 6];


This used to be UINT8 pixel[im->pixelsize] but a strict compiler is unhappy.
Currently the largest entry in OPEN_INFO has 6 bands, so that it's defined as 6.
I may have to put a pre-allocated uint8[pixelsize] buffer in Imaging struct.

yoursunny · 2024-07-19T14:56:41Z

src/libImaging/Imaging.h

+    int xsize;
+    int ysize;
+    int depth; /** MB mode only. */
+    int bands; /** MB mode only. */


It's my hope that all other modes are eventually converted to MB mode only.

wiredfool · 2024-07-19T15:17:08Z

What's the metadata design for the pixel format and band layout

There's a brief description in Imaging.h.

Can you put together a markdown summary of what are the crux issues you see for this feature, and your plan/tradeoffs?

You've got a good start, but I'd like to see your understanding of the complexity of the problem and your plan of attack. I'm a little worried that there's a required complexity out there that's not part of the early code that will be a showstopper.

aclark4life · 2024-07-19T15:37:52Z

You've got a good start, but I'd like to see your understanding of the complexity of the problem and your plan of attack. I'm a little worried that there's a required complexity out there that's not part of the early code that will be a showstopper.

Well said and I'd also encourage other folks tracking this issue to provide such ideas too. The way this gets to production is:

Implementation is solid and agreed upon and covers all known use cases
All known use cases are tested prior to release with test data from GIS, VFX and other industries.
A plan to support the implementation is made prior to release e.g.
- "convert everything to MB over X number of releases … "

🚀 🚀 🚀

aclark4life · 2024-07-31T15:07:38Z

Can you put together a markdown summary of what are the crux issues you see for this feature, and your plan/tradeoffs?

@ericvsmith Can you help with this one? We have a description of the requirements and are in need of a better definition of the format requirements and detailed description of how we plan to support those formats. We know @yoursunny is using the existing data structures so maybe that makes the job of describing our implementation in slightly easier 🤔 Thank you for any advice or guidance.

fdintino · 2024-08-02T13:51:06Z

My hope is that whatever solution tackles #1888 would also support YCbCr(A), and perhaps chroma subsampling. I'm not sure that a single multi-band image mode is compatible with being able to store raw YUV images.

ericvsmith · 2024-08-02T18:25:23Z

@ericvsmith Can you help with this one? We have a description of the requirements and are in need of a better definition of the format requirements and detailed description of how we plan to support those formats. We know @yoursunny is using the existing data structures so maybe that makes the job of describing our implementation in slightly easier 🤔 Thank you for any advice or guidance.

Sorry, I just noticed this. I'm not going to be able to look at this for a few weeks, but will check when I return.

mairsbw and others added 21 commits April 20, 2016 07:54

Add tests for opening 2-5 layer uint16 greyscale TIFFs.

1c2d465

Add open() support for 2-5 band uint16 TIFFs.

e0bb623

This only gets us to opening the first band, it doesn't actually allow the other bands to be retrieved.

Merge PR1839 into hack202406

9cbb840

XXX disable PyImaging_MapBuffer

90840ae

TIFF sample format table

03df357

introduce multi-band format (TIFF only)

936439b

re-enable PyImaging_MapBuffer

a4fab13

TIFF more entries in OPEN_INFO

76d336d

cgetpixel_mb constant size buffer

ed15ed9

mb_shuffle big endian

fb7702f

copy() with multi-band format

86e7fc6

crop() with multi-band format

587bb98

ImagingNew2Dirty update mismatch condition

0df0935

FLIP_LEFT_RIGHT with multi-band format

c4434df

explain how mode=MB is stored

c5ebc81

hack202406 - requested changes

e0a5d81

Added type hints

44da8b6

Declare variables at start of function

5bdda4c

transpose() with multi-band format

df98223

Merge remote-tracking branch 'upstream/main' into multiband

2426e57

fix mypy warning in test_open_tiff_uint16_multiband()

3cf311b

yoursunny mentioned this pull request Jul 10, 2024

Add support for high bit depth multichannel images #8223

Closed

radarhere and others added 2 commits July 19, 2024 22:28

Merge branch 'main' into multiband

b6ce6ab

[pre-commit.ci] auto fixes from pre-commit.com hooks

d5053fb

for more information, see https://pre-commit.ci

wiredfool requested changes Jul 19, 2024

View reviewed changes

aclark4life requested review from hugovk and radarhere July 19, 2024 14:57

yoursunny commented Jul 19, 2024

View reviewed changes

aclark4life mentioned this pull request Jul 31, 2024

Add support for high bit depth multichannel images #1888

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for high bit depth multichannel images #8224

Add support for high bit depth multichannel images #8224

yoursunny commented Jul 10, 2024

aclark4life commented Jul 11, 2024

wiredfool left a comment

wiredfool Jul 15, 2024

yoursunny Jul 19, 2024

wiredfool Jul 19, 2024

wiredfool Jul 15, 2024

yoursunny Jul 19, 2024

wiredfool Jul 19, 2024

aclark4life Jul 19, 2024

hugovk Jul 19, 2024

wiredfool Jul 15, 2024

wiredfool Jul 15, 2024

aclark4life Jul 19, 2024 •

edited

Loading

yoursunny Jul 19, 2024

wiredfool Jul 19, 2024

wiredfool Jul 15, 2024

wiredfool Jul 15, 2024

wiredfool Jul 15, 2024

wiredfool Jul 15, 2024

wiredfool Jul 15, 2024

Yay295 commented Jul 19, 2024

aclark4life commented Jul 19, 2024 •

edited

Loading

yoursunny left a comment

yoursunny Jul 19, 2024

yoursunny Jul 19, 2024

yoursunny Jul 19, 2024

yoursunny Jul 19, 2024

wiredfool commented Jul 19, 2024

aclark4life commented Jul 19, 2024 •

edited

Loading

aclark4life commented Jul 31, 2024

fdintino commented Aug 2, 2024

ericvsmith commented Aug 2, 2024

Add support for high bit depth multichannel images #8224

Are you sure you want to change the base?

Add support for high bit depth multichannel images #8224

Conversation

yoursunny commented Jul 10, 2024

aclark4life commented Jul 11, 2024

wiredfool 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

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

aclark4life Jul 19, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yay295 commented Jul 19, 2024

aclark4life commented Jul 19, 2024 • edited Loading

yoursunny 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

wiredfool commented Jul 19, 2024

aclark4life commented Jul 19, 2024 • edited Loading

aclark4life commented Jul 31, 2024

fdintino commented Aug 2, 2024

ericvsmith commented Aug 2, 2024

aclark4life Jul 19, 2024 •

edited

Loading

aclark4life commented Jul 19, 2024 •

edited

Loading

aclark4life commented Jul 19, 2024 •

edited

Loading