Add 16 bit png support #613

JimBobSquarePants · 2018-06-12T14:01:25Z

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 matches 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 WIP PR will add proper 16 bit png support to the library. Fixes #610

Currently we support decoding from 16 Rgb and Rgba pngs but we cheat while doing it by removing the least significant bits and preserving only 8 bits of data. To do this properly we need to be able to use Rgba64 to losslessly read and write the pixel data.

My plan is as follows:

Refactor Rgba64 to use StructLayout.Sequential and add R,G,B,A fields.
Add To/From Rgba64 methods to IPixel
Use the new methods to read/write the pixel data. Conversion will be automatic to other formats via scaling.
Add tests

Additionally I would like to:

Create a new Rgb48 pixel format

Potentially we could create new pixel formats for greyscale and greyscale + alpha data but that could happen post launch if the workload becomes too much.

TODO: Tests

codecov · 2018-06-12T15:31:37Z

Codecov Report

Merging #613 into master will increase coverage by 0.17%.
The diff coverage is 88.54%.

@@            Coverage Diff            @@
##           master    #613      +/-   ##
=========================================
+ Coverage   88.63%   88.8%   +0.17%     
=========================================
  Files         884     886       +2     
  Lines       37032   37960     +928     
  Branches     2670    2723      +53     
=========================================
+ Hits        32822   33711     +889     
- Misses       3415    3448      +33     
- Partials      795     801       +6

Impacted Files	Coverage Δ
...geSharp.Tests/Drawing/Text/DrawTextOnImageTests.cs	`100% <ø> (ø)`	⬆️
tests/ImageSharp.Tests/TestFile.cs	`88.23% <ø> (ø)`	⬆️
....Tests/TestUtilities/Tests/TestEnvironmentTests.cs	`63.63% <ø> (ø)`	⬆️
tests/ImageSharp.Tests/TestImages.cs	`100% <ø> (ø)`	⬆️
...s/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs	`48.64% <100%> (ø)`	⬆️
src/ImageSharp/Common/Helpers/ImageMaths.cs	`83.87% <100%> (ø)`	⬆️
src/ImageSharp/PixelFormats/Byte4.cs	`100% <100%> (ø)`	⬆️
src/ImageSharp/PixelFormats/RgbaVector.cs	`95.67% <100%> (+1.58%)`	⬆️
tests/ImageSharp.Tests/PixelFormats/Alpha8Tests.cs	`100% <100%> (ø)`	⬆️
...s/ReferenceCodecs/SystemDrawingReferenceEncoder.cs	`100% <100%> (ø)`	⬆️
... and 82 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 d715eb1...46df59f. Read the comment docs.

JimBobSquarePants · 2018-06-13T14:58:57Z

@antonfirsov @tocsoft @dlemstra and anyone else reading this.

I've got the fundamentals in place for reading 16 bit png's but need some help on the writing side.

~~I need to figure out a way to set both the bit depth and the color type when encoding without complex sanitation to ensure that the values are correct.~~

~~e.g A 2 color palette image is never going to be a 16 bit image.~~

~~I'm stuck figuring it out.~~

Scratch that, enums to the rescue!

JimBobSquarePants · 2018-06-17T16:09:51Z

Failing tests are due to me changing png encoder grayscale conversion algorithm to ITU-R recommendation 709 matching the libpng implementation.

Will reference images tomorrow and add specific bitdepth reference images plus tests.

antonfirsov · 2018-06-17T20:26:20Z

@JimBobSquarePants will this resolve #285?

JimBobSquarePants · 2018-06-17T22:52:21Z

@antonfirsov Yup, as long as the image pixel format is of high enough resolution then the accuracy is maintained. We’ll have Rgb48 and Rgba64 now to make that easier.

JimBobSquarePants · 2018-06-18T13:05:59Z

This is ready to review now.

Sorry about the size of this chaps but it turned out adding two new pixel formats with associated operations was a big job touching everywhere.

I had a hell of a time with image comparison as I had to switch to comparing using Rgba64 and I had to get rid of the System.Drawing comparer for pngs as it was losing accuracy on decode.

Anyway, it works, we now have full 16 bit support.

antonfirsov

The output seems visually OK (except Decode_64Bpp_Rgba64_rgb-16-alpha.png maybe?), but I'm afraid we should rethink the testing method. Either I'm missing something, or all png tests are now testing the decoder against itself!

Suggestion:

Keep the System.Drawing decoder as reference everywhere
Use tolerant comparison for 64bit and 48bit (where System.Drawing output is inaccurate)
Do a CRC verification on the result against a hard-coded value to ensure the match is exact (might be worth to produce the expected CRC with an external tool if possible)

antonfirsov · 2018-06-18T22:44:27Z

src/ImageSharp/PixelFormats/Argb32.cs

@@ -310,6 +310,22 @@ public void ToBgra32(ref Bgra32 dest)
        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public Argb32 ToArgb32() => this;

+        /// <inheritdoc/>
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public void PackFromRgb48(Rgb48 source) => this.PackFromScaledVector4(source.ToScaledVector4());


We should probably consider generating these default implementations later.

antonfirsov · 2018-06-18T22:51:34Z

src/ImageSharp/PixelFormats/Rgb48.cs

+        public void ToRgba32(ref Rgba32 dest)
+        {
+            Vector4 vector = this.ToVector4() * 255F;
+            dest.R = (byte)MathF.Round(vector.X);


Hope we can do this later more efficiently without float-packing.
I guess doing just dest.R = (byte)(this.R / 255) will lead to rounding errors.

Maybe.... We'd probably have to scale up and down via shifting to preserve accuracy though...

Check out the new method using offset + shift I nicked from libpng. Much better!

antonfirsov · 2018-06-18T23:02:53Z

tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs

@@ -20,74 +20,94 @@ public class PngDecoderTests
    {
        private const PixelTypes PixelTypes = Tests.PixelTypes.Rgba32 | Tests.PixelTypes.RgbaVector | Tests.PixelTypes.Argb32;

+        // This should be exact but for some reason it fails in some build environments.
+        private static readonly ImageComparer ValidatorComparer = ImageComparer.TolerantPercentage(0.0001F, 26);


Is it travis? Can't we make it environment specific?

Seems to be both CI environments, well variants anyway.

antonfirsov · 2018-06-18T23:10:38Z

tests/ImageSharp.Tests/TestUtilities/TestEnvironment.Formats.cs

+                // System.Drawing on Windows can decode 48bit and 64bit pngs but
+                // it doesn't preserve the accuracy we require for comparison.
+                // This makes CompareToOriginal method non-useful.
+                configuration.Configure(new PngConfigurationModule());


We should find a different testing method for those bit depths then, otherwise we are testing PngDecoder against itself in all our test cases, loosing the point of the regression testing.

Yup, very much agree.

antonfirsov · 2018-06-18T23:14:30Z

tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs

                {
-                    image.CompareToOriginal(provider, ImageComparer.Exact);
+                    image.VerifyEncoder(provider, "png", null, encoder, customComparer: ImageComparer.Exact);


For me it looks like you are testing the decoders result against itself, running it one another time inside VerifyEncoder(). Or am I missing something?

Yeah, this is useless, but it was the only way I could do any testing just now by round tripping. We need a better reference encoder for png

antonfirsov · 2018-06-18T23:23:02Z

tests/ImageSharp.Tests/TestUtilities/ImageComparison/TolerantImageComparer.cs

@@ -105,12 +106,12 @@ public TolerantImageComparer(float imageThreshold, int perPixelManhattanThreshol
        }

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        private static int GetManhattanDistanceInRgbaSpace(ref Rgba32 a, ref Rgba32 b)
+        private static int GetManhattanDistanceInRgbaSpace(ref Rgba64 a, ref Rgba64 b)


👍 seems good, but it might be worth to have another look tomorrow. All our tests depend on this stuff 😄

Defo would like a second opinion on this. I had to change some tolerances, I'm assuming code round actually hid differences before.

antonfirsov · 2018-06-18T23:37:54Z

We can even skip the CRC point in this PR, adding an issue for it.

The point is that if our 64bpp/48bpp output is most likely fine, and we are sure we do not break existing code/test logic, that is an increment compared to what we currently have in the main branch.

antonfirsov · 2018-06-19T00:24:49Z

@dlemstra is there an easy way to dump raw pixel data from libpng output?

dlemstra · 2018-06-19T05:28:25Z

@antonfirsov You can probably do this with Magick.NET (Q16) by loading an image and getting the pixels of that image.

JimBobSquarePants · 2018-06-19T07:14:24Z

@antonfirsov The CRC won't tell us enough to determine things are correct, only that the compressed data integrity is ok so we can ignore that. I'll revert the reference decoder changes though and do some cleanup there and I'll use tolerance for the 16 bit ones. Using Rgba32 as our pixel format might actually preserve the same data.

I also just double checked the rgb-16-alpha.png image using BeyondCompare and it's an exact match so that's ok.

@dlemstra If you could rig something in the tests to bridge between an Magick.NET image and ours that would be amazing!

antonfirsov · 2018-06-19T07:47:18Z

@dlemstra do I get the full Rgba64 value from 16bit png images? ~~Is it possible without writing C++ code (eg. using Magick.NET)?~~ (Ohh .. yes)
Any hint/link for start?

@JimBobSquarePants 👍 for the revert, we should not risk our existing test coverage. If we could load a raw pixel data dump, and do a comparison against it, then we are good with the 16bit stuff! I'll see what I can do tonight.

Decode_64Bpp_Rgba64_rgb-16-alpha.png is fine, it just looked like memory garbage to me 😄

# Conflicts: # tests/ImageSharp.Tests/Drawing/Text/DrawTextOnImageTests.cs

JimBobSquarePants · 2018-06-20T00:04:36Z

Hey @antonfirsov Can you do me a biggie and have a look at the image comparison code? The new font tests are failing with some pretty big differences after the merge. It could be rounding errors creeping in during conversion to/from Rgba64 but it could be something different.

Perhaps we should use different comparers for 16 bit and 8 bit pixels formats?

e.g.

FontShapesAreRenderedCorrectly_LargeText<Rgba32>(provider: Solid2480x3508_(255,255,255,25

SixLabors.ImageSharp.Tests.TestUtilities.ImageComparison.ImageDifferenceIsOverThresholdException : Image difference is over threshold!
Report ImageFrame 0: 
Total difference: 0.1764%
[Δ(1542,1542,1542,0) @ (819,6)];
[Δ(-1542,-1542,-1542,0) @ (822,6)];
[Δ(1285,1285,1285,0) @ (832,6)];
[Δ(2570,2570,2570,0) @ (833,6)];
[Δ(1542,1542,1542,0) @ (834,6)]...

antonfirsov · 2018-06-20T00:14:36Z

@JimBobSquarePants I'll have a look tomorrow!

JimBobSquarePants · 2018-06-21T04:14:21Z

@antonfirsov No need to look over the ImageComprarer it's correct. There was something, not quite right about the text drawing reference images that made them different. I recreated them and everything works.

antonfirsov · 2018-06-21T22:31:16Z

Good news!

I won't be able do make progress with Magick.NET testing now, so I think it's fine to merge this as-is + add an issue for the 16bit testing topic.
I want to do a second review however in the next few days, hope I can do it by Saturday night.

JimBobSquarePants · 2018-06-23T15:44:38Z

Merging this, it's solid and blocking other work.

antonfirsov · 2018-06-24T19:40:06Z

src/ImageSharp/PixelFormats/Rgb48.cs

+        }
+
+        /// <inheritdoc />
+        public override string ToString() => this.ToVector4().ToString();


"({R},{G},{B})" is much better for debugging.

antonfirsov

Sorry for being late with my review, doing it post-merge now.
Have a few findings + a few worrying anomalies around comparison logic.

antonfirsov · 2018-06-24T19:43:30Z

tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs

                {
-                    image.CompareToOriginal(provider, ImageComparer.Exact);
+                    image.VerifyEncoder(provider, "png", null, encoder, customComparer: ValidatorComparer);


Two calls to DebugSave() and CompareToOriginal() would more straightforward and readable in a decoder test. ( (VerifyEncoder() does the same though.)

antonfirsov · 2018-06-24T19:45:13Z

tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs

@@ -20,79 +20,105 @@ public class PngDecoderTests
    {
        private const PixelTypes PixelTypes = Tests.PixelTypes.Rgba32 | Tests.PixelTypes.RgbaVector | Tests.PixelTypes.Argb32;

+        // TODO: Cannot use exact comparer since System.Drawing doesn't preserve more than 32bits.
+        private static readonly ImageComparer ValidatorComparer = ImageComparer.TolerantPercentage(0.1302F, 2134);


Even if S.D decoder is inaccurate, and the output seems visually perfect, this high level of tolerance seems worrying for me. It might be a sign that something is wrong somewhere (maybe the comparison logic). What is the percentage difference if you lower the threshold?

antonfirsov · 2018-06-24T19:54:57Z

tests/ImageSharp.Tests/Processing/Processors/Transforms/ResizeTests.cs

@@ -17,7 +17,7 @@ public class ResizeTests : FileTestBase
    {
        public static readonly string[] CommonTestImages = { TestImages.Png.CalliphoraPartial };

-        private static readonly ImageComparer ValidatorComparer = ImageComparer.TolerantPercentage(0.005f);
+        private static readonly ImageComparer ValidatorComparer = ImageComparer.TolerantPercentage(0.069F);


Why do we need this dramatic (~10x) increase in tolerance?

Add 16 bit png support

JimBobSquarePants added 2 commits June 12, 2018 22:28

Refactor Rgba64

e9ddf0f

Add Rgba64 methods to IPixel. Touch #610

4534537

TODO: Tests

JimBobSquarePants added the formats:png label Jun 12, 2018

JimBobSquarePants added 2 commits June 14, 2018 00:36

Read Rgba64 png + some tests

e349d4b

Add some TODOs

88cb242

JimBobSquarePants added 9 commits June 14, 2018 11:22

Fix ImageMaths change.

7c72ea5

Fix conversion add conversion tests

fa4226a

Minor cleanup

2ad603a

Add Rgb48

c4646b6

Rgba48 => Rgb48

6ea2962

Add Rgb48 tests

332dd70

Fix tests

2a7bde5

Add IPixel Rgb48 methods

3049324

Use Rgb48 for 16 bit png decoding.

aee4778

JimBobSquarePants added 9 commits June 16, 2018 01:43

Can now decode all bit depths.

97012b9

Fix alpha8 conversion.

0e9efdf

Really fix Alpha conversion

7ee6f7a

Add Rgb48 tests

4baaddf

Add pixel operation tests

f8deb06

Merge branch 'master' into js/16-bit-pngs

933ceb9

Can now encode 16bit pngs.

bb29ff9

Update submodule from master.

8492656

Merge branch 'master' into js/16-bit-pngs

a23bf45

tocsoft added this to the 1.0.0-beta5 milestone Jun 17, 2018

tocsoft approved these changes Jun 18, 2018

View reviewed changes

Merge branch 'master' into js/16-bit-pngs

692c92c

antonfirsov requested changes Jun 18, 2018

View reviewed changes

JimBobSquarePants and others added 3 commits June 19, 2018 23:40

Use SD, Better conversion and cleanup SD Bridge.

34d1441

Lol Whut?

6d415b5

Merge branch 'master' into js/16-bit-pngs

d52a7b2

# Conflicts: # tests/ImageSharp.Tests/Drawing/Text/DrawTextOnImageTests.cs

Faster 32-64 bit conversion, update references, and cleanup.

4eb8617

JimBobSquarePants added 2 commits June 21, 2018 17:16

Better tRNS coverage

5da4184

More tests

922d367

JimBobSquarePants added 4 commits June 22, 2018 10:50

Merge branch 'master' into js/16-bit-pngs

5672964

Merge branch 'master' into js/16-bit-pngs

fe3b473

Merge branch 'master' into js/16-bit-pngs

76a6819

Merge branch 'master' into js/16-bit-pngs

46df59f

JimBobSquarePants merged commit 0d179d6 into master Jun 23, 2018

JimBobSquarePants deleted the js/16-bit-pngs branch June 23, 2018 15:44

antonfirsov reviewed Jun 24, 2018

View reviewed changes

antonfirsov pushed a commit to antonfirsov/ImageSharp that referenced this pull request Nov 11, 2019

Merge pull request SixLabors#613 from SixLabors/js/16-bit-pngs

f96b9f6

Add 16 bit png support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 16 bit png support #613

Add 16 bit png support #613

JimBobSquarePants commented Jun 12, 2018 •

edited

Loading

codecov bot commented Jun 12, 2018 •

edited

Loading

JimBobSquarePants commented Jun 13, 2018 •

edited

Loading

JimBobSquarePants commented Jun 17, 2018

antonfirsov commented Jun 17, 2018

JimBobSquarePants commented Jun 17, 2018

JimBobSquarePants commented Jun 18, 2018

antonfirsov left a comment

antonfirsov Jun 18, 2018

antonfirsov Jun 18, 2018

JimBobSquarePants Jun 19, 2018

JimBobSquarePants Jun 21, 2018

antonfirsov Jun 18, 2018

JimBobSquarePants Jun 19, 2018 •

edited

Loading

antonfirsov Jun 18, 2018

JimBobSquarePants Jun 19, 2018

antonfirsov Jun 18, 2018

JimBobSquarePants Jun 19, 2018

antonfirsov Jun 18, 2018

JimBobSquarePants Jun 19, 2018

antonfirsov commented Jun 18, 2018 •

edited

Loading

antonfirsov commented Jun 19, 2018

dlemstra commented Jun 19, 2018

JimBobSquarePants commented Jun 19, 2018

antonfirsov commented Jun 19, 2018 •

edited

Loading

JimBobSquarePants commented Jun 20, 2018

antonfirsov commented Jun 20, 2018

JimBobSquarePants commented Jun 21, 2018

antonfirsov commented Jun 21, 2018

JimBobSquarePants commented Jun 23, 2018

antonfirsov Jun 24, 2018 •

edited

Loading

antonfirsov left a comment

antonfirsov Jun 24, 2018

antonfirsov Jun 24, 2018

antonfirsov Jun 24, 2018 •

edited

Loading

Add 16 bit png support #613

Add 16 bit png support #613

Conversation

JimBobSquarePants commented Jun 12, 2018 • edited Loading

Prerequisites

Description

codecov bot commented Jun 12, 2018 • edited Loading

Codecov Report

JimBobSquarePants commented Jun 13, 2018 • edited Loading

JimBobSquarePants commented Jun 17, 2018

antonfirsov commented Jun 17, 2018

JimBobSquarePants commented Jun 17, 2018

JimBobSquarePants commented Jun 18, 2018

antonfirsov 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

JimBobSquarePants Jun 19, 2018 • 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

antonfirsov commented Jun 18, 2018 • edited Loading

antonfirsov commented Jun 19, 2018

dlemstra commented Jun 19, 2018

JimBobSquarePants commented Jun 19, 2018

antonfirsov commented Jun 19, 2018 • edited Loading

JimBobSquarePants commented Jun 20, 2018

antonfirsov commented Jun 20, 2018

JimBobSquarePants commented Jun 21, 2018

antonfirsov commented Jun 21, 2018

JimBobSquarePants commented Jun 23, 2018

antonfirsov Jun 24, 2018 • edited Loading

Choose a reason for hiding this comment

antonfirsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov Jun 24, 2018 • edited Loading

Choose a reason for hiding this comment

JimBobSquarePants commented Jun 12, 2018 •

edited

Loading

codecov bot commented Jun 12, 2018 •

edited

Loading

JimBobSquarePants commented Jun 13, 2018 •

edited

Loading

JimBobSquarePants Jun 19, 2018 •

edited

Loading

antonfirsov commented Jun 18, 2018 •

edited

Loading

antonfirsov commented Jun 19, 2018 •

edited

Loading

antonfirsov Jun 24, 2018 •

edited

Loading

antonfirsov Jun 24, 2018 •

edited

Loading