Skip to content

Conversation

@JimBobSquarePants
Copy link
Member

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

Replaces the .NET DeflateStream implementation used by the png encoder with a fully managed one ported from SharpZipLib and heavily modified.

This fixes #1027 with a 6x improvement in compression with the supplied image.

Benchmarks are.... ok I guess.... I managed to make the ported DeflaterOuputStream and accompanying classes around 40% faster but we've lost a fair amount of performance compared to the native SIMD accelerated version used by .NET Core. I think the massive compression gains are worth it though but if anyone fancies a crack at further optimization please give it a go.

Before

Method Job Runtime LargeImage Mean Error StdDev Ratio RatioSD Gen 0 Gen 1 Gen 2 Allocated
'System.Drawing Png' Clr Clr False 16.00 ms 1.429 ms 0.0783 ms 1.00 0.00 500.0000 218.7500 218.7500 2.03 MB
'ImageSharp Png' Clr Clr False 31.80 ms 2.094 ms 0.1148 ms 1.99 0.01 500.0000 500.0000 500.0000 2.25 MB
'System.Drawing Png' Core Core False 16.85 ms 10.899 ms 0.5974 ms 1.00 0.00 500.0000 218.7500 218.7500 2.03 MB
'ImageSharp Png' Core Core False 21.19 ms 7.654 ms 0.4195 ms 1.26 0.07 562.5000 531.2500 531.2500 2.24 MB

After

Method Job Runtime LargeImage Mean Error StdDev Ratio RatioSD Gen 0 Gen 1 Gen 2 Allocated
'System.Drawing Png' Clr Clr False 18.15 ms 4.199 ms 0.2302 ms 1.00 0.00 500.0000 218.7500 218.7500 2.03 MB
'ImageSharp Png' Clr Clr False 43.28 ms 1.944 ms 0.1066 ms 2.38 0.03 416.6667 416.6667 416.6667 2.3 MB
'System.Drawing Png' Core Core False 18.33 ms 2.518 ms 0.1380 ms 1.00 0.00 500.0000 218.7500 218.7500 2.03 MB
'ImageSharp Png' Core Core False 43.05 ms 5.185 ms 0.2842 ms 2.35 0.03 416.6667 416.6667 416.6667 2.3 MB

@codecov
Copy link

codecov bot commented Nov 14, 2019

Codecov Report

Merging #1054 into master will decrease coverage by 0.03%.
The diff coverage is 86.77%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1054      +/-   ##
=========================================
- Coverage   89.93%   89.9%   -0.04%     
=========================================
  Files        1124    1131       +7     
  Lines       49772   50654     +882     
  Branches     3513    3717     +204     
=========================================
+ Hits        44764   45539     +775     
- Misses       4287    4357      +70     
- Partials      721     758      +37
Impacted Files Coverage Δ
.../ImageSharp/Formats/Png/Zlib/DeflateThrowHelper.cs 0% <0%> (ø)
src/ImageSharp/Formats/Png/Zlib/Crc32.cs 92.75% <100%> (+0.1%) ⬆️
src/ImageSharp/Formats/Png/Zlib/Adler32.cs 79.31% <100%> (+0.73%) ⬆️
...ts/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs 99.51% <100%> (ø) ⬆️
src/ImageSharp/Formats/Png/PngEncoderCore.cs 96.91% <100%> (ø) ⬆️
...c/ImageSharp/Formats/Png/Zlib/DeflaterConstants.cs 100% <100%> (ø)
...c/ImageSharp/Formats/Png/Zlib/ZlibDeflateStream.cs 66.66% <46.66%> (+3.5%) ⬆️
...mageSharp/Formats/Png/Zlib/DeflaterOutputStream.cs 58% <58%> (ø)
src/ImageSharp/Formats/Png/Zlib/Deflater.cs 58.62% <58.62%> (ø)
src/ImageSharp/Formats/Png/Zlib/DeflaterEngine.cs 86.62% <86.62%> (ø)
... and 9 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 2010afa...a92e7e7. Read the comment docs.

/// </summary>
/// <param name="curMatch">The current match.</param>
/// <returns>True if a match greater than the minimum length is found</returns>
private bool FindLongestMatch(int curMatch)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be a hotspot according to dotTrace.

image

@JimBobSquarePants
Copy link
Member Author

I've done my best. Get stuck in please.

@JimBobSquarePants
Copy link
Member Author

I think I'm going to go ahead and merge this. It isn't slow and we get good compression. Any other gains we can make down the line.

@JimBobSquarePants JimBobSquarePants merged commit ec4eca1 into master Nov 27, 2019
@JimBobSquarePants JimBobSquarePants deleted the js/managed-zlib branch November 27, 2019 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PNG Encoder produces too huge images and different sizes in Linux and Windows

2 participants