Improve CQ10 #783

iamcarbon · 2018-12-05T17:37:50Z

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

Update BenchmarkDotNet and Moq Libs
Use unmanaged constraint on internal api
Remove redundant package references
~~Use new HashHelper overloads~~ Use new HashCode.Combine polyfill
Cleanup the shared assembly properties (preferring auto generation)

iamcarbon · 2018-12-05T23:07:21Z

Ready for review.

dlemstra · 2018-12-06T11:35:52Z

src/ImageSharp/MetaData/Profiles/ICC/TagDataEntries/IccLut16TagDataEntry.cs

-            this.InputValues = inputValues;
-            this.ClutValues = clutValues;
-            this.OutputValues = outputValues;
+            this.InputValues = inputValues ?? throw new ArgumentNullException(nameof(inputValues));


I really prefer using Guard instead of the null-coalescing operator.

They look nicer, but are they worth the expense of an extra call and a convoluted stack that shows that the exception occurred within the Guard method instead of the actual callsite? I would advocate that we eventually replace all Guard methods with methods that throw inline so the user knows exactly where the problem is.

dlemstra · 2018-12-06T11:46:46Z

src/ImageSharp/MetaData/Profiles/ICC/Curves/IccResponseCurve.cs

-                return hashCode;
-            }
+            return HashHelpers.Combine(
+                (int)this.CurveType,


CurveType.GetHashCode()?

dlemstra · 2018-12-06T11:47:30Z

src/ImageSharp/MetaData/Profiles/ICC/TagDataEntries/IccChromaticityTagDataEntry.cs

-            }
+            return HashHelpers.Combine(
+                base.GetHashCode(),
+                (int)this.ColorantType,


this.ColorantType.GetHashCode()?

dlemstra · 2018-12-06T11:48:41Z

src/ImageSharp/MetaData/Profiles/ICC/TagDataEntries/IccLut16TagDataEntry.cs

-                hashCode = (hashCode * 397) ^ (this.InputValues?.GetHashCode() ?? 0);
-                hashCode = (hashCode * 397) ^ (this.ClutValues?.GetHashCode() ?? 0);
-                hashCode = (hashCode * 397) ^ (this.OutputValues?.GetHashCode() ?? 0);
+                hashCode = (hashCode * 397) ^ this.InputValues.GetHashCode();


Why are we not using HashHelpers.Combine here?

dlemstra · 2018-12-06T11:49:05Z

src/ImageSharp/MetaData/Profiles/ICC/TagDataEntries/IccMultiProcessElementsTagDataEntry.cs

-            }
+            return HashHelpers.Combine(
+                base.GetHashCode(),
+                this.InputChannelCount,


this.InputChannelCount.GetHashCode()?

Was on the fence whether or not to update all of these to use an integer or call GetHashCode and left as is for the PR.

dlemstra · 2018-12-06T11:49:50Z

src/ImageSharp/MetaData/Profiles/ICC/TagDataEntries/IccViewingConditionsTagDataEntry.cs

+                base.GetHashCode(),
+                this.IlluminantXyz.GetHashCode(),
+                this.SurroundXyz.GetHashCode(),
+                (int)this.Illuminant);


this.Illuminant.GetHashCode()?

This is happening in other places also, now starting to wonder if you should not use GetHashCode for an int?

Going to update these all to using GetHashCode() for consistency with the rest of the codebase.

codecov · 2018-12-06T16:57:26Z

Codecov Report

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

@@            Coverage Diff            @@
##             master     #783   +/-   ##
=========================================
  Coverage          ?   88.68%           
=========================================
  Files             ?     1012           
  Lines             ?    43253           
  Branches          ?     3144           
=========================================
  Hits              ?    38359           
  Misses            ?     4192           
  Partials          ?      702

Impacted Files	Coverage Δ
.../ImageSharp/Common/ParallelUtils/ParallelHelper.cs	`100% <ø> (ø)`
...MetaData/Profiles/ICC/Various/IccResponseNumber.cs	`57.14% <0%> (ø)`
...es/ICC/TagDataEntries/IccFix16ArrayTagDataEntry.cs	`50% <0%> (ø)`
...les/ICC/TagDataEntries/IccSignatureTagDataEntry.cs	`50% <0%> (ø)`
.../Implementation/WorkingSpaces/GammaWorkingSpace.cs	`35.71% <0%> (ø)`
...Profiles/ICC/TagDataEntries/IccTextTagDataEntry.cs	`50% <0%> (ø)`
...ntries/IccProfileSequenceIdentifierTagDataEntry.cs	`50% <0%> (ø)`
...s/ICC/TagDataEntries/IccUInt16ArrayTagDataEntry.cs	`50% <0%> (ø)`
...mageSharp/MetaData/Profiles/ICC/Various/IccClut.cs	`42.66% <0%> (ø)`
...Profiles/ICC/TagDataEntries/IccDataTagDataEntry.cs	`45.45% <0%> (ø)`
... and 53 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 dce781d...31c2d4a. Read the comment docs.

codecov · 2018-12-06T16:57:26Z

Codecov Report

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

@@           Coverage Diff            @@
##             master    #783   +/-   ##
========================================
  Coverage          ?   88.7%           
========================================
  Files             ?    1012           
  Lines             ?   43248           
  Branches          ?    3127           
========================================
  Hits              ?   38363           
  Misses            ?    4181           
  Partials          ?     704

Impacted Files	Coverage Δ
.../ImageSharp/Common/ParallelUtils/ParallelHelper.cs	`100% <ø> (ø)`
...mageSharp/MetaData/Profiles/ICC/IccTagDataEntry.cs	`46.66% <0%> (ø)`
...lementation/RGBPrimariesChromaticityCoordinates.cs	`58.33% <0%> (ø)`
...les/ICC/TagDataEntries/IccSignatureTagDataEntry.cs	`50% <0%> (ø)`
.../Implementation/WorkingSpaces/GammaWorkingSpace.cs	`29.41% <0%> (ø)`
...ntries/IccProfileSequenceIdentifierTagDataEntry.cs	`50% <0%> (ø)`
...mageSharp/MetaData/Profiles/ICC/Various/IccClut.cs	`42.66% <0%> (ø)`
...Profiles/ICC/TagDataEntries/IccDataTagDataEntry.cs	`45.45% <0%> (ø)`
...s/ICC/TagDataEntries/IccUInt32ArrayTagDataEntry.cs	`50% <0%> (ø)`
...DataEntries/IccMultiProcessElementsTagDataEntry.cs	`60% <0%> (ø)`
... and 68 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 9446900...4568a4b. Read the comment docs.

iamcarbon · 2018-12-06T16:59:03Z

@dlemstra Thanks for the feedback. Addressed the PR with one comment. Let me know your thoughts on throws. I've been letting them slowly creep in in my prior PR's -- we should make a call.

iamcarbon · 2018-12-07T18:17:53Z

@JimBobSquarePants PING.

JimBobSquarePants · 2018-12-07T19:09:57Z

@iamcarbon Just landed back in Oz. Will review properly when over my jet lag.

Two things I’d like to see/discuss though.

Potential use of Type forwarded HashCode from CoreFx. The API for that is lush.
What does a stack trace for Guard vs non Guard look like?

iamcarbon · 2018-12-08T02:56:52Z

@JimBobSquarePants I submitted a PR in core to polyfill HashCode. Stacktraces coming soon.

JimBobSquarePants · 2018-12-08T09:57:36Z

Legend!

iamcarbon · 2018-12-08T20:37:23Z

Here are examples of the stack traces.

For trivial argument validation on public methods, I believe we should prefer inline throws to improve the stack traces (brevity + clarity). This also eliminates code generation for the generic function and a call.

THROWING USING GUARD HELPER

E:\ImageSharp\src\ImageSharp\Common\Helpers\Guard.cs(294,0): at SixLabors.ImageSharp.Guard.ThrowArgumentNullException(String parameterName)
E:\ImageSharp\src\ImageSharp\Common\Helpers\Guard.cs(29,0): at SixLabors.ImageSharp.Guard.NotNull[T](T value, String parameterName)
E:\ImageSharp\src\ImageSharp\Formats\ImageFormatManager.cs(127,0): at SixLabors.ImageSharp.Formats.ImageFormatManager.SetEncoder(IImageFormat imageFormat, IImageEncoder encoder)
E:\ImageSharp\tests\ImageSharp.Tests\Formats\ImageFormatManagerTests.cs(34,0): at SixLabors.ImageSharp.Tests.ImageFormatManagerTests.Stack()

THROWING DIRECTLY

E:\ImageSharp\src\ImageSharp\Formats\ImageFormatManager.cs(129,0): at SixLabors.ImageSharp.Formats.ImageFormatManager.SetEncoder(IImageFormat imageFormat, IImageEncoder encoder)
E:\ImageSharp\tests\ImageSharp.Tests\Formats\ImageFormatManagerTests.cs(34,0): at SixLabors.ImageSharp.Tests.ImageFormatManagerTests.Stack()

iamcarbon · 2018-12-12T01:48:35Z

Updated to use HashCode.Combine

src/ImageSharp/MetaData/Profiles/ICC/TagDataEntries/IccLutBToATagDataEntry.cs

JimBobSquarePants · 2018-12-12T02:05:12Z

tests/ImageSharp.Sandbox46/ImageSharp.Sandbox46.csproj

@@ -18,19 +18,13 @@
    <ProjectReference Include="..\..\src\ImageSharp\ImageSharp.csproj" />
  </ItemGroup>
  <ItemGroup>
-    <PackageReference Include="BitMiracle.LibJpeg.NET" Version="1.4.280" />
    <PackageReference Include="Magick.NET-Q16-AnyCPU" Version="7.9.0.1" />
    <PackageReference Include="xunit" Version="2.3.1" />


Are you able to shed any light into what issues are blocking us upgrading xunit?

I spent 30 minutes on this before. I'll take another stab in CQ11.

Good man! 👍

iamcarbon · 2018-12-12T02:40:15Z

@JimBobSquarePants -- what other rad .NETCORE stuff can we polyfill. :)

JimBobSquarePants · 2018-12-12T23:35:50Z

Pulling this in now. Thanks again!!

Improve CQ10

iamcarbon added 8 commits November 20, 2018 08:50

Remove redundant package references

92db6af

Update BenchmarkDotNet to 0.11.3

d3c717a

Update Moq to 4.10.0

5f4e66f

Use unmanaged constraint on internal api

ab5ded8

Remove linq use in JpegDecoderCode

af7d239

Ensure exifData is set

7eb0c3c

Merge remote-tracking branch 'upstream/master' into cq10

3a533f4

Use new HashHelper overloads

ac22630

iamcarbon changed the title ~~Cq10~~ Improve CQ10 Dec 5, 2018

Replace == null with is null

c78fc1d

dlemstra reviewed Dec 6, 2018

View reviewed changes

iamcarbon added 3 commits December 6, 2018 08:39

Update (hashCode * 397) pattern to use HashHelpers

d0f78c2

Remove indirection getting HashCode on ExifValue

44edf7d

Use GetHashCode when combining int values

31c2d4a

iamcarbon added 3 commits December 6, 2018 12:42

Cleanup assembly properties

18a75f4

Remove unused BitMiracle.LibJpeg.NET depedency

196b748

Remove unused Microsoft.CSharp dependency

690c551

iamcarbon added 2 commits December 10, 2018 16:18

Merge branch 'master' into cq10

db841b7

Replace HashHelpers with HashCode.Combine

77c5466

JimBobSquarePants reviewed Dec 12, 2018

View reviewed changes

src/ImageSharp/MetaData/Profiles/ICC/TagDataEntries/IccLutBToATagDataEntry.cs Outdated Show resolved Hide resolved

JimBobSquarePants reviewed Dec 12, 2018

View reviewed changes

Use HashCode.Add when combining more than 8 hashcodes

c0db8f1

Merge branch 'master' into cq10

4568a4b

JimBobSquarePants merged commit 590609a into SixLabors:master Dec 12, 2018

iamcarbon deleted the cq10 branch December 13, 2018 16:19

qdraw mentioned this pull request Apr 14, 2019

JpegEncoder producing incorrect output on linux-arm (since Beta006) #871

Closed

5 tasks

antonfirsov pushed a commit to antonfirsov/ImageSharp that referenced this pull request Nov 11, 2019

Merge pull request SixLabors#783 from carbon/cq10

47c7d11

Improve CQ10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve CQ10 #783

Improve CQ10 #783

iamcarbon commented Dec 5, 2018 •

edited

Loading

iamcarbon commented Dec 5, 2018

dlemstra Dec 6, 2018

iamcarbon Dec 6, 2018 •

edited

Loading

dlemstra Dec 6, 2018

iamcarbon Dec 6, 2018

dlemstra Dec 6, 2018

iamcarbon Dec 6, 2018

dlemstra Dec 6, 2018

dlemstra Dec 6, 2018

iamcarbon Dec 6, 2018 •

edited

Loading

dlemstra Dec 6, 2018

iamcarbon Dec 6, 2018 •

edited

Loading

codecov bot commented Dec 6, 2018

codecov bot commented Dec 6, 2018 •

edited

Loading

iamcarbon commented Dec 6, 2018

iamcarbon commented Dec 7, 2018

JimBobSquarePants commented Dec 7, 2018

iamcarbon commented Dec 8, 2018

JimBobSquarePants commented Dec 8, 2018

iamcarbon commented Dec 8, 2018 •

edited

Loading

iamcarbon commented Dec 12, 2018

JimBobSquarePants Dec 12, 2018

iamcarbon Dec 12, 2018

JimBobSquarePants Dec 12, 2018

iamcarbon commented Dec 12, 2018

JimBobSquarePants commented Dec 12, 2018

Improve CQ10 #783

Improve CQ10 #783

Conversation

iamcarbon commented Dec 5, 2018 • edited Loading

Prerequisites

Description

iamcarbon commented Dec 5, 2018

Choose a reason for hiding this comment

iamcarbon Dec 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamcarbon Dec 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamcarbon Dec 6, 2018 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Dec 6, 2018

Codecov Report

codecov bot commented Dec 6, 2018 • edited Loading

Codecov Report

iamcarbon commented Dec 6, 2018

iamcarbon commented Dec 7, 2018

JimBobSquarePants commented Dec 7, 2018

iamcarbon commented Dec 8, 2018

JimBobSquarePants commented Dec 8, 2018

iamcarbon commented Dec 8, 2018 • edited Loading

iamcarbon commented Dec 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamcarbon commented Dec 12, 2018

JimBobSquarePants commented Dec 12, 2018

iamcarbon commented Dec 5, 2018 •

edited

Loading

iamcarbon Dec 6, 2018 •

edited

Loading

iamcarbon Dec 6, 2018 •

edited

Loading

iamcarbon Dec 6, 2018 •

edited

Loading

codecov bot commented Dec 6, 2018 •

edited

Loading

iamcarbon commented Dec 8, 2018 •

edited

Loading