Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Swap jpeg decoders #571

Merged
merged 22 commits into from
May 16, 2018
Merged

Swap jpeg decoders #571

merged 22 commits into from
May 16, 2018

Conversation

JimBobSquarePants
Copy link
Member

@JimBobSquarePants JimBobSquarePants commented May 11, 2018

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 switches out the old Golang based Jpeg decoder with the newer PdfJs decoder.

This gives us a robust decoder that produces 100% accurate spectral output. Performance matches the Golang decoder with less memory usage. There is excellent scope for further improving performance also.

We'll keep both decoders in the codebase for now as there might be ideas we can still pinch from the Golang decoder.

Fixes #517 and #518

I've also expanded the unit tests to better cover both decoders and reorganized the internal namespaces to clean up the code.

@codecov
Copy link

codecov bot commented May 11, 2018

Codecov Report

Merging #571 into master will decrease coverage by 0.01%.
The diff coverage is 90.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #571      +/-   ##
==========================================
- Coverage   88.79%   88.78%   -0.02%     
==========================================
  Files         851      854       +3     
  Lines       36037    36097      +60     
  Branches     2609     2607       -2     
==========================================
+ Hits        31999    32047      +48     
- Misses       3260     3274      +14     
+ Partials      778      776       -2
Impacted Files Coverage Δ
...harp/Formats/Jpeg/Components/Decoder/JFifMarker.cs 97.29% <ø> (ø)
...geSharp/Formats/Jpeg/Components/GenericBlock8x8.cs 84.61% <ø> (ø)
...s/Decoder/GolangJpegScanDecoder.ComputationData.cs 100% <ø> (ø)
...harp/Formats/Jpeg/Components/Encoder/HuffmanLut.cs 100% <ø> (ø)
...der/ColorConverters/JpegColorConverter.FromCmyk.cs 100% <ø> (ø)
...arp/Formats/Jpeg/Components/Encoder/HuffmanSpec.cs 100% <ø> (ø)
.../Jpg/Utils/ReferenceImplementations.AccurateDCT.cs 82.97% <ø> (ø) ⬆️
...lorConverters/JpegColorConverter.FromYCbCrBasic.cs 100% <ø> (ø)
src/ImageSharp/Formats/Jpeg/Components/ZigZag.cs 100% <ø> (ø)
...eferenceImplementationsTests.StandardIntegerDCT.cs 100% <ø> (ø) ⬆️
... and 80 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 dbe2b1b...711844b. 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.

In library code, all changes seem formal refactors to me, all looking good.

Managed to identify a few redundant tests however.

[Theory]
[WithFile(TestImages.Jpeg.Baseline.Calliphora, PixelTypes.Rgba32)]
[WithFile(TestImages.Jpeg.Baseline.Testorig420, PixelTypes.Rgba32)]
public void DoProcessorStepPdfJs<TPixel>(TestImageProvider<TPixel> provider)
Copy link
Member

Choose a reason for hiding this comment

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

VerifySpectralCorrectness_PdfJs/Golang should test the first step, here we only need to test JpegImagePostProcessor, so parsing input with both decoders doesn't add value here.
It's totally fine to run JpegImagePostProcessorTests with the default jpeg decoder only, which is PdfJsDecoderCore now.

@@ -80,9 +80,9 @@ internal partial struct Block8x8F
set => this[(y * 8) + x] = value;
}

public static Block8x8F operator *(Block8x8F block, float value)
public static Components.Block8x8F operator *(Components.Block8x8F block, float value)
Copy link
Member

@antonfirsov antonfirsov May 13, 2018

Choose a reason for hiding this comment

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

I'm getting "Qualifier is redundant" tips everywhere.
I wonder which analyzer added the namespace qualifiers?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was Resharper, probably due to the struct being a partial.

@antonfirsov
Copy link
Member

Did some refactor on JpegDecoderTests + added another test image for #159.

Everything LGTM now, feel free to merge!

@JimBobSquarePants JimBobSquarePants merged commit d566c27 into master May 16, 2018
@JimBobSquarePants JimBobSquarePants deleted the js/swap-jpeg-decoders branch May 16, 2018 01:00
@JimBobSquarePants JimBobSquarePants mentioned this pull request May 18, 2018
4 tasks
antonfirsov pushed a commit to antonfirsov/ImageSharp that referenced this pull request Nov 11, 2019
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.

2 participants