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.

Sign up for GitHub

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

Jump to bottom

Reimplemented Jpeg postprocessor logic + fixed bugs in stream parsing #322

Merged
merged 81 commits into from
Sep 3, 2017
Merged

Reimplemented Jpeg postprocessor logic + fixed bugs in stream parsing #322

merged 81 commits into from
Sep 3, 2017

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Sep 2, 2017 •
edited
Loading

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

Should fix stability and memory issues #224 #214 #178 #159 #151 and provide better accuracy (#245 #155) without significant performance drop. For some images it produces almost identical results to libjpeg-turbo (run CompareJpegDecoders_Baseline() for comparison results).

@antonfirsov
refactor OldComponent: it's class again + got moved in some logic
a25ecc9
@antonfirsov
CopyColorsTo() no longer uses MutableSpan
f3b1114
@antonfirsov
goodby MutableSpan!
492c32e
@antonfirsov
removed DecodedBlock.bx, DecodedBlock.by
9528e9d
@antonfirsov
removed DecodedBlock
96ccdae
@antonfirsov
Block8x8: the UInt16 Jpeg block
20288a8
@antonfirsov
ToString(), CopyTo(), ToArray() for Block8x8
5eea0e0
@antonfirsov
open up OldJpegDecoderCore API for testing
69d46d7
@antonfirsov
spectral testing
43bf873
@antonfirsov
reference original code in TransformIDCT() docs
dd05aed
@antonfirsov
moved CreateOutputDirectory() into TestEnvironment
0e1459e
@antonfirsov
removed TestBase
b1ecd66
@antonfirsov
more jpeg testing
eb2fd0e
@antonfirsov
reading libjpeg component dump
443f88d
@antonfirsov
robust spectral verification
9012fbf
@antonfirsov
better tuple usage
4f3095a
@antonfirsov
clean up and organize Jpeg test utilities
02bf454
@antonfirsov
move jpeg SpanExtensions into test utils
4c9e945
@antonfirsov
fix Sandbox46 build
ec38e7f
@antonfirsov
organizing DCT code
959fb45
@antonfirsov
playing with DCT implementations
d6d1720
@antonfirsov
managed to figure out everything regarding IDCT reference implementat…
41eff8c 
…ions
@antonfirsov
DCT.cs -> FastFloatingPointDCT.cs
905931f
@antonfirsov
moved FastFloatingPointDCT.cs
40c73bc
@antonfirsov
better constants in ReferenceImplementations.FastFloatingPointDCT
15e9213
@antonfirsov
better IDCT constants, but it did not help
7c89ea4
@antonfirsov
Postfixing: Orig*** instead of Old** + introducing PdfJs*** again
fe3d725
@antonfirsov
rename files to match types
5ab1644
@antonfirsov
UnZigAndQuantize --> QuantizeBlock (the block itself is not zigged here)
523eb6c
@antonfirsov
well ... that LLM implementation is actually NOT inaccurate
ba0c8b8
JimBobSquarePants and others added 21 commits August 30, 2017 22:42
@JimBobSquarePants
Merge branch 'jpeg-lab' of https://github.com/SixLabors/ImageSharp in…
0258885 
…to jpeg-lab
@JimBobSquarePants
GrayScale & RGB + Tests
92750fd
@antonfirsov
deleted all deprecated channel processing code
6e17898
@antonfirsov
fixed #214
f11fc03
@antonfirsov
introduced test for #178
b18cbb4
@JimBobSquarePants
Add Cmyk and Ycck converter tests
629d543
@antonfirsov
fixed #178
a743fc9
@antonfirsov
Merge branch 'jpeg-lab' of https://github.com/SixLabors/ImageSharp in…
e8c4568 
…to jpeg-lab
@antonfirsov
test case for #159
1aae264
@antonfirsov
ErrorCode is now a state of InputProcessor
b75481b
@antonfirsov
fixed #159 for the progressive case
bc27c83
@antonfirsov
fixed #159 for the other known case, test image is too big though, wo…
e696d99 
…n't add it
@antonfirsov
skipping Jpeg420Small + PdfJs spectral tests
c6176d3
@tocsoft
expose registered ImageFormats publicly
fceb6f8
@antonfirsov
formatting + cleanup
baf2da2
@antonfirsov
docs
99b0b15
@antonfirsov
switching jpeg decoders back
4b39251
@antonfirsov
Merge remote-tracking branch 'origin/master' into jpeg-lab
b9d61f5
@antonfirsov
Merge remote-tracking branch 'origin/beta-1' into jpeg-lab
79630a1
@antonfirsov
xml merge tool, why are you doing this to me?
07d99c9
@antonfirsov
run Windows-only tests only on Windows
9d75055
tocsoft
tocsoft approved these changes Sep 2, 2017
View reviewed changes
Copy link
Member

@tocsoft tocsoft left a comment

Choose a reason for hiding this comment

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

looking good to me, just a tiny (not worth blocking) thing around const vs readonly static

src/ImageSharp/Formats/Jpeg/Common/FastFloatingPointDCT.cs Outdated
{
#pragma warning disable SA1310 // FieldNamesMustNotContainUnderscore
private static readonly float C_1_175876 = 1.175876f;
private static readonly float C_1_175876 = 1.175875602f;
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't theses be better as const values instead of readonly static ones?

Copy link
Member Author

Choose a reason for hiding this comment

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

Wow, worked with this code a lot but haven't spotted this all the time! I think this is because these values were originally defined as Vector4.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we can fix that as we optimize post beta.

@JimBobSquarePants
Copy link
Member

Very happy with this as is. Well done @antonfirsov on this and in getting real tests in there.

Will help out post beta to introduce more constants into the decoding process. E X I F, A d o b e etc and any optimization I can get my head around.

@JimBobSquarePants
Copy link
Member 

BenchmarkDotNet=v0.10.9, OS=Windows 10 Redstone 2 (10.0.15063)
Processor=Intel Core i7-6600U CPU 2.60GHz (Skylake), ProcessorCount=4
Frequency=2742191 Hz, Resolution=364.6719 ns, Timer=TSC
.NET Core SDK=2.0.0
  [Host]     : .NET Core 1.1.2 (Framework 4.6.25211.01), 64bit RyuJIT
  DefaultJob : .NET Core 1.1.2 (Framework 4.6.25211.01), 64bit RyuJIT


            Method |     Mean |    Error |   StdDev | Allocated |
------------------ |---------:|---------:|---------:|----------:|
 'ImageSharp Jpeg' | 143.0 ms | 2.778 ms | 3.307 ms |   7.87 KB |

Just leaving the above so we have a known reference point to optimize from.

@antonfirsov antonfirsov merged commit 1799cd4 into beta-1 Sep 3, 2017
@tocsoft tocsoft deleted the jpeg-lab branch September 14, 2017 18:23
antonfirsov added a commit to antonfirsov/ImageSharp that referenced this pull request Nov 11, 2019
@antonfirsov
Merge pull request SixLabors#322 from SixLabors/jpeg-lab
3438c7d 
Reimplemented Jpeg postprocessor logic + fixed bugs in stream parsing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JimBobSquarePants JimBobSquarePants JimBobSquarePants approved these changes

@tocsoft tocsoft tocsoft approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@antonfirsov @JimBobSquarePants @tocsoft