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

Use double precision to calculate variance. This prevents early escape from color count calculation. Fix #866

Also cleaned up a little.

@codecov
Copy link

codecov bot commented Apr 4, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@cb18952). Click here to learn what that means.
The diff coverage is 88.92%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #874   +/-   ##
=========================================
  Coverage          ?   88.96%           
=========================================
  Files             ?     1018           
  Lines             ?    44578           
  Branches          ?     3235           
=========================================
  Hits              ?    39661           
  Misses            ?     4196           
  Partials          ?      721
Impacted Files Coverage Δ
tests/ImageSharp.Tests/TestImages.cs 100% <ø> (ø)
...ssing/Processors/Quantization/QuantizeProcessor.cs 0% <0%> (ø)
.../ImageSharp.Tests/Quantization/WuQuantizerTests.cs 100% <100%> (ø)
...cessors/Quantization/FrameQuantizerBase{TPixel}.cs 98% <100%> (ø)
src/ImageSharp/Formats/Png/PngEncoderCore.cs 93.53% <100%> (ø)
src/ImageSharp/Formats/Gif/GifEncoderCore.cs 93.9% <75%> (ø)
...rocessors/Quantization/WuFrameQuantizer{TPixel}.cs 91.86% <85.02%> (ø)

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 cb18952...b42d93e. Read the comment docs.

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

Can we introduce a test that fails with the old code and is passing using the updated one? Most straightforward: PNG test based on the image in #866. (Not sure if it's the best.)

@JimBobSquarePants
Copy link
Member Author

JimBobSquarePants commented Apr 4, 2019

I guess... means writing a histogram implementation though a reference image might be subject to change as the quantisation+encoding process is almost as much about aesthetics as anything.

I've added the unit tests from the original implementation.

@antonfirsov
Copy link
Member

I will check again as soon as I get home.

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

@JimBobSquarePants I re-executed the tests after replacing the WuFrameQuantizer<T> implementation with the code before the change, and all tests passed. This means that the improvement introduced by this PR is not covered!

I think there is an easy-to-code, implementation-agnostic approach to test IQuantizer implementations:

  1. Create an Image<T> from QuantizedFrame<T> representing the quantization result
  2. Compare the quantized image against the original one using a tolerance

With this approach the old code should lead to a test failure using the image from #866!

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

2 colors before VS 48 colors after. Looks pretty good. Great that the you also added the QuantizedFrame<T> refactor. Let's merge this in!

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.

Default PNG quantizer introduces jaggedness

2 participants