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

Jpeg decoder mcu size fix #1845

Merged
merged 5 commits into from
Nov 21, 2021

Conversation

br3aker
Copy link
Contributor

@br3aker br3aker commented Nov 21, 2021

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 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 fixes a bug described in #1816 (comment).

Basically, it removes dependency on the 'first component' during decoding because jpegs can have lower luma sampling than chroma. This bug can be spotted with image from current test image base - jpeg422.jpg. This is a decoded image from current master code:
jpeg422_MASTER_BUG

Current PR produces almost ideal image - diff is 0.0013% compared to PhotoShop png reference:

MASTER
Total difference: 19,7988%
[Δ(0,-20560,0,0) @ (0,0)];
[Δ(0,-20560,0,0) @ (1,0)];
[Δ(0,-20303,-257,0) @ (2,0)];
[Δ(0,-20046,0,0) @ (3,0)];
[Δ(0,-19789,-257,0) @ (4,0)]...
PR
Total difference: 0,0013%
[Δ(0,-257,257,0) @ (18,2)];
[Δ(0,-257,0,0) @ (19,3)];
[Δ(0,-257,0,0) @ (23,7)];
[Δ(0,257,0,0) @ (32,8)];
[Δ(0,0,-257,0) @ (56,8)]...

I've also added this image to the decoder test suit.

@@ -65,26 +62,26 @@ public override void InjectFrameData(JpegFrame frame, IRawJpegData jpegData)
MemoryAllocator allocator = this.configuration.MemoryAllocator;

// iteration data
IJpegComponent c0 = frame.Components[0];
int majorBlockWidth = frame.Components.Max((component) => component.SizeInBlocks.Width);
Copy link
Member

Choose a reason for hiding this comment

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

How’s the perf using Max here? We’ve avoided Linq everywhere in the codecs AFAIK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considering it's from 1 to 4 elements and it executes only once I don't think it will matter but I will run benchmarks just to be sure.

@@ -151,6 +151,7 @@ private void ParseBaselineData()
if (this.componentsCount == this.frame.ComponentCount)
{
this.ParseBaselineDataInterleaved();
this.spectralConverter.CommitConversion();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before this PR spectral converter did some magic to determine if image was converted based on number off processed rows. But someone wise once said: "explicit is better than implicit" so I've decided to create a separate method to 'seal' conversion. It doesn't look so OOPish but at least it's more readable IMO.

@codecov
Copy link

codecov bot commented Nov 21, 2021

Codecov Report

Merging #1845 (2a182d7) into master (a8de2ec) will increase coverage by 0%.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1845   +/-   ##
======================================
  Coverage      87%     87%           
======================================
  Files         937     937           
  Lines       48420   48423    +3     
  Branches     6054    6056    +2     
======================================
+ Hits        42222   42225    +3     
  Misses       5192    5192           
  Partials     1006    1006           
Flag Coverage Δ
unittests 87% <100%> (+<1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...mats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs 93% <100%> (+<1%) ⬆️
...rmats/Jpeg/Components/Decoder/SpectralConverter.cs 100% <100%> (ø)
...eg/Components/Decoder/SpectralConverter{TPixel}.cs 100% <100%> (ø)

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 a8de2ec...2a182d7. Read the comment docs.

@br3aker
Copy link
Contributor Author

br3aker commented Nov 21, 2021

Fresh benchmarks

Master

Method Mean Error StdDev
'Baseline 4:4:4 Interleaved' 11.712 ms 0.0454 ms 0.0355 ms
'Baseline 4:2:0 Interleaved' 8.931 ms 0.0513 ms 0.0455 ms
'Baseline 4:0:0 (grayscale)' 1.639 ms 0.0045 ms 0.0037 ms
'Progressive 4:2:0 Non-Interleaved' 13.554 ms 0.0711 ms 0.0665 ms

PR

Method Mean Error StdDev
'Baseline 4:4:4 Interleaved' 11.781 ms 0.0737 ms 0.0654 ms
'Baseline 4:2:0 Interleaved' 8.688 ms 0.0345 ms 0.0306 ms
'Baseline 4:0:0 (grayscale)' 1.643 ms 0.0092 ms 0.0086 ms
'Progressive 4:2:0 Non-Interleaved' 13.770 ms 0.0928 ms 0.0823 ms

While progressive 420 is indeed slower by almost 1.5% baseline 420 is faster by 2.7% without any particular reasons. I believe it's a noise, performance shouldn't be affected by LINQ calls, especially by 4 element array.

Edit:
I have no idea why this benchmark is a lot faster than the one from closed decoder optimization PR, updated results.

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

LGTM. Your additional comments make sense

@JimBobSquarePants JimBobSquarePants merged commit 2c5c9f1 into SixLabors:master Nov 21, 2021
@br3aker br3aker deleted the jpeg-decoder-size-fix branch November 21, 2021 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants