Adds support for encoding 8-bit bitmaps #906

brianpopow · 2019-05-07T18:12:54Z

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 PR add the support for encoding 8-Bit Bitmaps.

codecov · 2019-05-09T18:35:26Z

Codecov Report

Merging #906 into master will decrease coverage by 0.08%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #906      +/-   ##
==========================================
- Coverage   89.55%   89.46%   -0.09%     
==========================================
  Files        1031     1031              
  Lines       46115    46137      +22     
  Branches     3261     3264       +3     
==========================================
- Hits        41297    41278      -19     
- Misses       4108     4111       +3     
- Partials      710      748      +38

Impacted Files	Coverage Δ
tests/ImageSharp.Tests/TestImages.cs	`100% <ø> (ø)`	⬆️
src/ImageSharp/Formats/Bmp/BmpEncoder.cs	`100% <ø> (ø)`	⬆️
.../Processors/Quantization/QuantizedFrame{TPixel}.cs	`94.11% <ø> (ø)`	⬆️
src/ImageSharp/Formats/Bmp/BmpEncoderCore.cs	`94.4% <100%> (+1.19%)`	⬆️
...ts/ImageSharp.Tests/Formats/Bmp/BmpEncoderTests.cs	`98.24% <100%> (ø)`	⬆️
...xelFormats/PixelOperations{TPixel}.PixelBenders.cs	`89.9% <0%> (-10.1%)`	⬇️
...Processing/Processors/Transforms/TransformUtils.cs	`87.64% <0%> (-5.62%)`	⬇️
src/ImageSharp/Common/Helpers/ImageMaths.cs	`81.81% <0%> (-5.2%)`	⬇️
...ocessing/Processors/Dithering/ErrorDiffuserBase.cs	`93.75% <0%> (-3.13%)`	⬇️
...ormats/Jpeg/Components/Decoder/QualityEvaluator.cs	`94.04% <0%> (-2.39%)`	⬇️
... and 18 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 89f3de4...6ce4efe. Read the comment docs.

codecov · 2019-05-09T18:35:26Z

Codecov Report

Merging #906 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #906      +/-   ##
==========================================
+ Coverage    89.6%   89.61%   +0.01%     
==========================================
  Files        1060     1060              
  Lines       46542    46586      +44     
  Branches     3268     3272       +4     
==========================================
+ Hits        41703    41750      +47     
+ Misses       4128     4125       -3     
  Partials      711      711

Impacted Files	Coverage Δ
tests/ImageSharp.Tests/TestImages.cs	`100% <ø> (ø)`	⬆️
.../Processors/Quantization/QuantizedFrame{TPixel}.cs	`94.11% <ø> (ø)`	⬆️
src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs	`86.97% <100%> (+0.02%)`	⬆️
...cessing/Processors/Quantization/OctreeQuantizer.cs	`100% <100%> (+11.11%)`	⬆️
...ts/ImageSharp.Tests/Formats/Bmp/BmpEncoderTests.cs	`98.63% <100%> (+0.38%)`	⬆️
src/ImageSharp/Formats/Bmp/BmpEncoder.cs	`100% <100%> (ø)`	⬆️
src/ImageSharp/Formats/Bmp/BmpEncoderCore.cs	`94.48% <100%> (+1.28%)`	⬆️
...geSharp/PixelFormats/PixelImplementations/Gray8.cs	`81.63% <0%> (+2.04%)`	⬆️

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 25b3f75...b1d2947. Read the comment docs.

brianpopow · 2019-05-10T19:26:30Z

@JimBobSquarePants: i have some trouble with unit tests failing for net472 and net462. I cannot not reproduce it locally.
My dotnet info output looks like this:

> dotnet --info
.NET Core SDK (gemäß "global.json"):
 Version:   2.2.203
 Commit:    e5bab63eca

Laufzeitumgebung:
 OS Name:     Windows
 OS Version:  10.0.17763
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\2.2.203\

Host (useful for support):
  Version: 2.2.4
  Commit:  f95848e524

As far as i can tell, this matches the appveyor configuration which is failing. Do you have an idea how i can reproduce this issue? The tests work fine with net462 and net472 when i run them locally with:

dotnet test --filter "FullyQualifiedName=SixLabors.ImageSharp.Tests.Formats.Bmp.BmpEncoderTests.Encode_8Bit_WithV4Header_Works" tests/ImageSharp.Tests/ImageSharp.Tests.csproj -c "Release" -f "net472"

antonfirsov · 2019-05-10T19:39:51Z

@brianpopow I think hardware-specific floating point inaccuracies are trolling us again.

Which PixelOperations<TPixel>.Instance operation do the failing tests trigger in BmpEncoder? What is TPixel? Is the operation implemented using Vector4 for the given pixel type?

brianpopow · 2019-05-11T09:35:53Z

@antonfirsov TPixel is Gray8. Im using the OctreeFrameQuantizer to downscale the number of colors from 32 bit to 8 bit, it uses Vector3 in ConstructPalette(). The quantization is the main part of encoding 8 bit bitmaps, so i suspect the issue is maybe related to it, but i cannot really tell how. Especially because im unable to reproduce it.

antonfirsov · 2019-05-11T11:02:40Z

@brianpopow for quantization, inconsistencies across platforms are acceptable. You just need to pass a tolerant comparer inEncode_8Bit_WithV3Header_Works and Encode_8Bit_WithV4Header_Works when pixel type is Gray8 (maybe splitting the theories further?).
The reported error is 0.0064%, so I would suggest to use ImageComparer.TolerantPercentage(0.01f).

brianpopow · 2019-05-11T15:41:24Z

Ok i have changed the test for the one gray image to have a tolerance of 0.01f, as Anton suggested. I think this is now ready for review.

The OctreeFrameQuantizer works pretty good btw. Here is an example output with just 256 different colors (converted to png, because github does not like bmp):

antonfirsov · 2019-05-11T19:37:02Z

src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs

@@ -1022,7 +1022,8 @@ private void ReadInfoHeader()
            this.bmpMetadata.InfoHeaderType = infoHeaderType;

            // We can only encode at these bit rates so far.
-            if (bitsPerPixel.Equals((short)BmpBitsPerPixel.Pixel16)
+            if (bitsPerPixel.Equals((short)BmpBitsPerPixel.Pixel8)


Are there other options?

yes, 4 bit and 1 bit

antonfirsov

A few minor change requests and questions, otherwise looks very good!

antonfirsov · 2019-05-11T19:39:24Z

src/ImageSharp/Formats/Bmp/BmpEncoderCore.cs

+            where TPixel : struct, IPixel<TPixel>
+        {
+#if NETCOREAPP2_1
+            Span<byte> colorPalette = stackalloc byte[ColorPaletteSize8Bit];


1024 byte is too much for stackallock, we can remove this.

What is the limit? I can never find an explicit instruction.

I've read something in Joe Duffy's original material suggesting 128 bytes, based on their benchmarks.

Ah ok. I use 257 bytes in the HuffmanTable but I'm gonna leave that as-is.

antonfirsov · 2019-05-11T19:41:15Z

src/ImageSharp/Formats/Bmp/BmpEncoderCore.cs

+#endif
+
+            var quantizer = new OctreeQuantizer(dither: true, maxColors: 256);
+            QuantizedFrame<TPixel> quantized = quantizer.CreateFrameQuantizer<TPixel>(this.configuration).QuantizeFrame(image);


FrameQuantizer<T> and QuantizedFrame<T> are IDisposable!

Also:
IQuantizer should be configuratble by the user as an Encoder parameter, like we do it in PngEncoder!

antonfirsov · 2019-05-11T19:50:55Z

src/ImageSharp/Formats/Bmp/BmpEncoderCore.cs

+            var color = default(Rgba32);
+            foreach (TPixel quantizedColor in quantized.Palette)
+            {
+                quantizedColor.ToRgba32(ref color);


Might be a stupid idea and lack of understanding, but:
What if we convert to Rgba32 first, and quantize in Rgba32 space?

Yeah, in Octree we're working in the Rgba32 space then converting to/from so I see where you are coming from. However, by converting before quantization we end up allocating a new buffer to house the image. That's probably a lot more costly.

Totally forgot that it will lead to another allocation, so let's keep it for now.

(An idea is starting to formulate in my mind, but of course it would break IQuantizer API again 😄.)

antonfirsov · 2019-05-11T19:52:55Z

src/ImageSharp/Formats/Bmp/BmpEncoderCore.cs

+#if NETCOREAPP2_1
+            Span<byte> colorPalette = stackalloc byte[ColorPaletteSize8Bit];
+#else
+            byte[] colorPalette = new byte[ColorPaletteSize8Bit];


I would suggest to use MemoryAllocator.AllocateManagedByteBuffer() for this size.

We don't actually need the #ifdef anymore. It's supported in latest c# versions.

I mean replacing GC array allocation.

antonfirsov · 2019-05-11T19:57:40Z

tests/ImageSharp.Tests/Formats/Bmp/BmpEncoderTests.cs

            where TPixel : struct, IPixel<TPixel> => TestBmpEncoderCore(provider, bitsPerPixel, supportTransparency: true);

+        [Theory]
+        [WithFile(Bit8Gs, PixelTypes.Gray8, BmpBitsPerPixel.Pixel8)]
+        public void Encode_8BitGray_WithV3Header_Works<TPixel>(TestImageProvider<TPixel> provider, BmpBitsPerPixel bitsPerPixel)


I can't say enough times, how much I ❤️ this kind of extensive and explanatory test coverage! This is an important enabler for us to do optimizations and refactors.
For #907 stuff I'm literally spending days to add missing tests, so I can change the code safely.

antonfirsov

Thanks!
I'm merging, to resolve conflicts with #908.

antonfirsov · 2019-05-14T21:41:02Z

@brianpopow I've been thinking on the quantizer inaccuracies. Do we really need execute the quantization when when TPixel is Gray8? Can't we just use the 255 colors of Gray8 as a palette to get rid of the inconsistent behavior?

brianpopow · 2019-05-15T17:24:49Z

@antonfirsov no you are right, when its Gray8, we do not actually need the quantization. I will change that.

…coding Adds support for encoding 8-bit bitmaps

brianpopow and others added 2 commits May 7, 2019 20:08

Add support for encoding 8-bit bitmaps

3431ad3

Merge branch 'master' into feature/Bitmap8BitEncoding

7cfb560

brianpopow changed the title ~~Adds support for encoding 8-bit bitmaps~~ WIP: Adds support for encoding 8-bit bitmaps May 7, 2019

brianpopow added 2 commits May 7, 2019 20:43

Changed WuQuantizer to OctreeQuantizer

6401344

Trying MagickReferenceDecoder for the 8bit greyscale image encoder tests

6ce4efe

brianpopow and others added 3 commits May 9, 2019 22:07

Setting dither to false in the OctreeQuantizer

b5df1c7

Merge branch 'master' into feature/Bitmap8BitEncoding

4233bd2

Switched back to the default reference decoder

e0afde9

brianpopow and others added 3 commits May 11, 2019 13:52

Using tolerant comparer for the Bit8Gs image

e869573

Merge branch 'master' into feature/Bitmap8BitEncoding

9a6698f

Enabled dither again

7b8f9ef

brianpopow changed the title ~~WIP: Adds support for encoding 8-bit bitmaps~~ Adds support for encoding 8-bit bitmaps May 11, 2019

antonfirsov reviewed May 11, 2019

View reviewed changes

antonfirsov requested changes May 11, 2019

View reviewed changes

JimBobSquarePants added enhancement formats:bmp labels May 12, 2019

JimBobSquarePants added this to the 1.0.0-rc1 milestone May 12, 2019

Review changes

b1d2947

antonfirsov approved these changes May 12, 2019

View reviewed changes

antonfirsov merged commit 2461da8 into SixLabors:master May 12, 2019

JimBobSquarePants mentioned this pull request May 12, 2019

Implement 8-bit grayscale bitmap encoding #896

Closed

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

Merge pull request SixLabors#906 from brianpopow/feature/Bitmap8BitEn…

a7bd41e

…coding Adds support for encoding 8-bit bitmaps

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for encoding 8-bit bitmaps #906

Adds support for encoding 8-bit bitmaps #906

brianpopow commented May 7, 2019

codecov bot commented May 9, 2019

codecov bot commented May 9, 2019 •

edited

Loading

brianpopow commented May 10, 2019

antonfirsov commented May 10, 2019

brianpopow commented May 11, 2019

antonfirsov commented May 11, 2019

brianpopow commented May 11, 2019

antonfirsov May 11, 2019

brianpopow May 11, 2019

antonfirsov left a comment

antonfirsov May 11, 2019

JimBobSquarePants May 12, 2019

antonfirsov May 12, 2019

JimBobSquarePants May 12, 2019

antonfirsov May 11, 2019 •

edited

Loading

antonfirsov May 11, 2019 •

edited

Loading

antonfirsov May 11, 2019 •

edited

Loading

JimBobSquarePants May 12, 2019

antonfirsov May 12, 2019

antonfirsov May 11, 2019

JimBobSquarePants May 12, 2019

antonfirsov May 12, 2019

antonfirsov May 11, 2019

antonfirsov left a comment •

edited

Loading

antonfirsov commented May 14, 2019

brianpopow commented May 15, 2019

Adds support for encoding 8-bit bitmaps #906

Adds support for encoding 8-bit bitmaps #906

Conversation

brianpopow commented May 7, 2019

Prerequisites

Description

codecov bot commented May 9, 2019

Codecov Report

codecov bot commented May 9, 2019 • edited Loading

Codecov Report

brianpopow commented May 10, 2019

antonfirsov commented May 10, 2019

brianpopow commented May 11, 2019

antonfirsov commented May 11, 2019

brianpopow commented May 11, 2019

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov May 11, 2019 • edited Loading

Choose a reason for hiding this comment

antonfirsov May 11, 2019 • edited Loading

Choose a reason for hiding this comment

antonfirsov May 11, 2019 • 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 left a comment • edited Loading

Choose a reason for hiding this comment

antonfirsov commented May 14, 2019

brianpopow commented May 15, 2019

codecov bot commented May 9, 2019 •

edited

Loading

antonfirsov May 11, 2019 •

edited

Loading

antonfirsov May 11, 2019 •

edited

Loading

antonfirsov May 11, 2019 •

edited

Loading

antonfirsov left a comment •

edited

Loading