Add support for decoding 6, 12, 30, 42 bits per pixel tiff's #1647

brianpopow · 2021-06-01T22:01:11Z

Prerequisites

I have written a descriptive pull-request title
I have verified that there are no overlapping pull-requests open
I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
I have provided test coverage for my change (where applicable)

Description

This PR adds support for decoding:

12 bit (4 bits per channel, RGB chunky) TIFF images.
6 bit (2 bits per channel, RGB chunky) TIFF images.
30 bit (10 bits per channel, RGB chunky) TIFF images.
42 bit (14 bits per channel, RGB chunky) TIFF images.

/cc @IldarKhayrutdinov

brianpopow · 2021-06-01T22:03:18Z

src/ImageSharp/Formats/Tiff/TiffEncoderEntriesCollector.cs

@@ -274,6 +278,7 @@ public void Process(TiffEncoderCore encoder)
                    Value = (ushort)encoder.PhotometricInterpretation
                };

+                this.collector.AddOrReplace(planarConfig);


note that I have explicitly added the chunky planar configuration, otherwise it could have accidentally be possible that it would be set to planar, if the decoded images was planar.

I don't really understand this change. Could you please explain to me what the issue is with pulling the value from the decoder if present?

There are two possible values of the PlanarConfiguration:

Chunky: The data is stored as RGBRGBRGB. This is the default mode.

Planar: This is more uncommon. The data is stored as:
RRR
GGG
BBB

The Tiff Encoder does always encodes the data as Chunky, but does not make this explicitly by storing a value for the PlanarConfiguration. The value is omitted and because decoders will assume Chunky this will work.

Now in the case of when we decode an actual 'Planar' Tiff and the reencode this TIFF again, the PlanarConfiguration would carry over to the encoded image, because its present in the EXIF profile. This would change the interpretation of the data and make the encoded image invalid. Therefore i thought it would be a good idea to make it explicit and always set it to Chunky.

codecov · 2021-06-01T22:14:41Z

Codecov Report

Merging #1647 (036b95b) into master (3764b1c) will increase coverage by 0.05%.
The diff coverage is 74.69%.

@@            Coverage Diff             @@
##           master    #1647      +/-   ##
==========================================
+ Coverage   84.15%   84.20%   +0.05%     
==========================================
  Files         812      813       +1     
  Lines       35634    35710      +76     
  Branches     4147     4154       +7     
==========================================
+ Hits        29987    30070      +83     
+ Misses       4831     4824       -7     
  Partials      816      816

Flag	Coverage Δ
unittests	`84.20% <74.69%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...metricInterpretation/RgbPlanarTiffColor{TPixel}.cs	`100.00% <ø> (ø)`
src/ImageSharp/Formats/Tiff/TiffDecoderCore.cs	`87.61% <ø> (+20.95%)`	⬆️
...eSharp/Formats/Tiff/TiffBitsPerSampleExtensions.cs	`18.91% <18.18%> (+1.27%)`	⬆️
...mageSharp/Formats/Tiff/TiffDecoderOptionsParser.cs	`61.46% <83.33%> (+7.70%)`	⬆️
...ImageSharp/Formats/Tiff/Constants/TiffConstants.cs	`100.00% <100.00%> (ø)`
...otometricInterpretation/Rgb444TiffColor{TPixel}.cs	`100.00% <100.00%> (ø)`
...cInterpretation/TiffColorDecoderFactory{TPixel}.cs	`61.11% <100.00%> (+11.11%)`	⬆️
src/ImageSharp/Formats/Tiff/TiffEncoderCore.cs	`94.77% <100.00%> (+0.07%)`	⬆️
...eSharp/Formats/Tiff/TiffEncoderEntriesCollector.cs	`85.61% <100.00%> (+0.40%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3764b1c...036b95b. Read the comment docs.

IldarKhayrutdinov

SamplesPerPixel tag can have arbitrary value and can have different bits per component for each component corresponding to a pixel.

We don't need to support different different bits per component other than 565 (and 555), but we can support the same arbitrary bit values: {2,2,2}, {3,3,3}...

IldarKhayrutdinov · 2021-06-02T07:58:31Z

src/ImageSharp/Formats/Tiff/TiffDecoderOptionsParser.cs

@@ -179,7 +180,18 @@ private static void ParseColorType(this TiffDecoderCore options, ExifProfile exi

                    if (options.PlanarConfiguration == TiffPlanarConfiguration.Chunky)
                    {
-                        options.ColorType = options.BitsPerSample == TiffBitsPerSample.Bit24 ? TiffColorType.Rgb888 : TiffColorType.Rgb;


@brianpopow TiffColorType.Rgb - it seems that we have stopped supporting custom RGB (not in this PR, but even earlier).

IldarKhayrutdinov · 2021-06-02T08:12:44Z

The patch is workaround support for RGB {2,2,2} using RgbTiffColor:
custom_rgb.zip

Test images:
tiger-rgb-float-strip-contig-16.tif
tiger-rgb-strip-contig-03.tif
tiger-rgb-strip-contig-02.tif

brianpopow · 2021-06-02T08:19:15Z

The patch is workaround support for RGB {2,2,2} using RgbTiffColor:
custom_rgb.zip

Test images:
tiger-rgb-float-strip-contig-16.tif
tiger-rgb-strip-contig-03.tif
tiger-rgb-strip-contig-02.tif

yeah you are right, we can do it this way, but I think this should be another PR. Feel free to open another PR with it.

edit: @IldarKhayrutdinov good spot, that we can actually use the magick decoder for 2 and 4 bit!

…ence decoder

brianpopow · 2021-06-02T09:53:53Z

@dlemstra could you please double check, if the usage of ImageMagick for 4 bit and 2 bit is correct here: bc723d3

IldarKhayrutdinov · 2021-06-02T10:23:05Z

we can do it this way, but I think this should be another PR. Feel free to open another PR with it.

yeah, this PR nothing breaks, it should be another PR

…2 bpp

src/ImageSharp/Formats/Tiff/TiffBitsPerSampleExtensions.cs

src/ImageSharp/Formats/Tiff/TiffDecoderOptionsParser.cs

Co-authored-by: Günther Foidl <gue@korporal.at>

JimBobSquarePants · 2021-06-02T22:06:49Z

SamplesPerPixel tag can have arbitrary value and can have different bits per component for each component corresponding to a pixel.

We don't need to support different different bits per component other than 565 (and 555), but we can support the same >arbitrary bit values: {2,2,2}, {3,3,3}...

@IldarKhayrutdinov So what does this mean in regards to this PR?

brianpopow · 2021-06-02T23:26:03Z

SamplesPerPixel tag can have arbitrary value and can have different bits per component for each component corresponding to a pixel.
We don't need to support different different bits per component other than 565 (and 555), but we can support the same >arbitrary bit values: {2,2,2}, {3,3,3}...

@IldarKhayrutdinov So what does this mean in regards to this PR?

Now that I thought about it again, I think Ildar meant we can use RgbTiffColor{TPixel} for all bit values which have the same values for all channels. So we dont actually need Rgb444TiffColor{TPixel}.cs and can use RgbTiffColor{TPixel}. I think i misunderstood that when I first read it. Is that correct @IldarKhayrutdinov? I will convert this to WIP and check it again.

IldarKhayrutdinov · 2021-06-03T06:50:41Z

SamplesPerPixel tag can have arbitrary value and can have different bits per component for each component corresponding to a pixel.
We don't need to support different different bits per component other than 565 (and 555), but we can support the same >arbitrary bit values: {2,2,2}, {3,3,3}...

@IldarKhayrutdinov So what does this mean in regards to this PR?

@JimBobSquarePants
This PR doesn't change anything, but only adds support for 4-bit RGB, so can separate different SamplesPerPixel problem into new PR (this will require api change). It seems like once upon a time support was conceived and it was supposed to work (but were no tests), but at some point it stopped being supported, I missed this moment.

In principle, this is not a bug, but we must support it, since it is worth a little effort for us.
Do it in this PR or in the new PR, you decide ☺️

IldarKhayrutdinov · 2021-06-03T07:05:06Z

Now that I thought about it again, I think Ildar meant we can use RgbTiffColor{TPixel} for all bit values which have the same values for all channels. So we dont actually need Rgb444TiffColor{TPixel}.cs and can use RgbTiffColor{TPixel}. I think i misunderstood that when I first read it. Is that correct @IldarKhayrutdinov? I will convert this to WIP and check it again.

@brianpopow Rgb444TiffColor{TPixel} is needed because RgbTiffColor{TPixel} is slow, also worth adding most relevant 555/565. Non-standard cases are rare, for them can use RgbTiffColor{TPixel}.

By the way, RgbTiffColor{TPixel} is can to decode any arbitrary SamplesPerPixel for 1-16 (can expand to 32) bits per sample: {2,2,2}, {3,3,3}, {5,6,5}, {7,6,5}..

brianpopow · 2021-06-03T11:33:39Z

In principle, this is not a bug, but we must support it, since it is worth a little effort for us.
Do it in this PR or in the new PR, you decide ☺️

@IldarKhayrutdinov thanks for the clarification.
I have added support for 6, 10 and 14 bit per channel and use RgbTiffColor{TPixel} for that. Note that I decided to not add 3 bits per channel support, since I think its not really a valid option. 9 bit per pixel would be completely unaligned at the byte border and I did not see those in the tiff-testsuite either. Also 565 should be a separate PR since i think its worth to implement its own TiffColorDecoder.

JimBobSquarePants · 2021-06-03T12:31:22Z

src/ImageSharp/Formats/Tiff/PhotometricInterpretation/TiffColorDecoderFactory{TPixel}.cs

+                case TiffColorType.Rgb222:
+                    DebugGuard.IsTrue(
+                        bitsPerSample.Length == 3
+                        && bitsPerSample[0] == 2


You can use the indexer check skipping trick discussed the other day here I reckon.

JimBobSquarePants

Very cool! Love how well structured this all is. 👍

Add support for decoding 12 bits per pixel tiff's

c6f5a8a

brianpopow added the formats:tiff label Jun 1, 2021

brianpopow commented Jun 1, 2021

View reviewed changes

IldarKhayrutdinov reviewed Jun 2, 2021

View reviewed changes

brianpopow added 2 commits June 2, 2021 10:38

Add 4 bit and 2 bit depth to the valid bit depth for the magick refer…

bc723d3

…ence decoder

Dont skip tests on linux, use magick decoder

5e0f75f

brianpopow and others added 2 commits June 2, 2021 14:09

Use magick decoder for 4bit test, add test for encoding option with 1…

cc081b0

…2 bpp

Merge branch 'master' into bp/tiff12bit

1283710

gfoidl reviewed Jun 2, 2021

View reviewed changes

src/ImageSharp/Formats/Tiff/TiffBitsPerSampleExtensions.cs Outdated Show resolved Hide resolved

src/ImageSharp/Formats/Tiff/TiffDecoderOptionsParser.cs Outdated Show resolved Hide resolved

brianpopow and others added 2 commits June 2, 2021 20:25

Apply suggestions from code review

85ef0fe

Co-authored-by: Günther Foidl <gue@korporal.at>

Flip order of comparing BitsPerSample

3a6a5e9

brianpopow changed the title ~~Add support for decoding 12 bits per pixel tiff's~~ WIP: Add support for decoding 12 bits per pixel tiff's Jun 2, 2021

brianpopow added 4 commits June 3, 2021 10:34

Add test for encode and reload planar tiff

580723f

Add support for decoding 6 bit per pixel tiff's

42d5d9e

Add support for decoding 30 bit per pixel tiff's

8e6fad8

Add support for decoding 10 bit per channel rgb tiff's

deed748

brianpopow changed the title ~~WIP: Add support for decoding 12 bits per pixel tiff's~~ Add support for decoding 6, 12, 30, 42 bits per pixel tiff's Jun 3, 2021

JimBobSquarePants reviewed Jun 3, 2021

View reviewed changes

Flip order of comparing bitsPerSample

036b95b

JimBobSquarePants approved these changes Jun 3, 2021

View reviewed changes

brianpopow merged commit 4d7e829 into master Jun 3, 2021

brianpopow deleted the bp/tiff12bit branch June 3, 2021 15:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for decoding 6, 12, 30, 42 bits per pixel tiff's #1647

Add support for decoding 6, 12, 30, 42 bits per pixel tiff's #1647

brianpopow commented Jun 1, 2021 •

edited

brianpopow Jun 1, 2021

JimBobSquarePants Jun 2, 2021

brianpopow Jun 2, 2021

codecov bot commented Jun 1, 2021 •

edited

IldarKhayrutdinov left a comment

IldarKhayrutdinov Jun 2, 2021

IldarKhayrutdinov commented Jun 2, 2021 •

edited

brianpopow commented Jun 2, 2021 •

edited

brianpopow commented Jun 2, 2021

IldarKhayrutdinov commented Jun 2, 2021

JimBobSquarePants commented Jun 2, 2021

brianpopow commented Jun 2, 2021

IldarKhayrutdinov commented Jun 3, 2021 •

edited

IldarKhayrutdinov commented Jun 3, 2021 •

edited

brianpopow commented Jun 3, 2021

JimBobSquarePants Jun 3, 2021

JimBobSquarePants left a comment

Add support for decoding 6, 12, 30, 42 bits per pixel tiff's #1647

Add support for decoding 6, 12, 30, 42 bits per pixel tiff's #1647

Conversation

brianpopow commented Jun 1, 2021 • edited

Prerequisites

Description

brianpopow Jun 1, 2021

Choose a reason for hiding this comment

JimBobSquarePants Jun 2, 2021

Choose a reason for hiding this comment

brianpopow Jun 2, 2021

Choose a reason for hiding this comment

codecov bot commented Jun 1, 2021 • edited

Codecov Report

IldarKhayrutdinov left a comment

Choose a reason for hiding this comment

IldarKhayrutdinov Jun 2, 2021

Choose a reason for hiding this comment

IldarKhayrutdinov commented Jun 2, 2021 • edited

brianpopow commented Jun 2, 2021 • edited

brianpopow commented Jun 2, 2021

IldarKhayrutdinov commented Jun 2, 2021

JimBobSquarePants commented Jun 2, 2021

brianpopow commented Jun 2, 2021

IldarKhayrutdinov commented Jun 3, 2021 • edited

IldarKhayrutdinov commented Jun 3, 2021 • edited

brianpopow commented Jun 3, 2021

JimBobSquarePants Jun 3, 2021

Choose a reason for hiding this comment

JimBobSquarePants left a comment

Choose a reason for hiding this comment

brianpopow commented Jun 1, 2021 •

edited

codecov bot commented Jun 1, 2021 •

edited

IldarKhayrutdinov commented Jun 2, 2021 •

edited

brianpopow commented Jun 2, 2021 •

edited

IldarKhayrutdinov commented Jun 3, 2021 •

edited

IldarKhayrutdinov commented Jun 3, 2021 •

edited