#718 #730: Add Gray8 and Gray16 Pixel Formats and clean up IPixel by jongleur1983 · Pull Request #729 · SixLabors/ImageSharp

jongleur1983 · 2018-10-06T21:38:49Z

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

Implements Pixel formats Gray8 and Gray16.
Where I knew how to do it I provided optimized conversion methods, where not I used the "fallback" strategy to convert from and to Vector4, as it's done for many other implementations as well.

Unit tests are based on some of the other Pixel formats tests. Not sure this is seen as sufficient though, please tell me if not.

I guess I have to squash commits again, but for review the individual commits may help so I'm happy to do so after a first review.

… to t4 script

AND add unit tests AND fix Gray16 implementation (constructor takes ushort, not byte) AND fix Gray8 and Gray16 Pack (do not scale by sum)

JimBobSquarePants

I've started a review but I can see quite a lot of places where the maths are incorrect. It's probably quicker for me to push the changes to the code myself so am gonna jump in. Hope you don't mind!

JimBobSquarePants · 2018-10-08T15:34:28Z

+
+        /// <inheritdoc/>
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public void ToGray8(ref Gray8 dest) => dest.PackFromScaledVector4(this.ToScaledVector4());


This can be optimized.

JimBobSquarePants · 2018-10-08T15:36:42Z

+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public void PackFromRgba32(Rgba32 source)
+        {
+            this.PackedValue = Pack(source.R, source.G, source.B);


This and others is incorrect. Will lead to a packed value of 255.

JimBobSquarePants · 2018-10-08T15:37:11Z

+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public void ToRgb48(ref Rgb48 dest)
+        {
+            ushort gray = (ushort)(this.PackedValue * 255);


JimBobSquarePants · 2018-10-08T15:37:49Z

+        /// <param name="b">Blue value of the color to pack.</param>
+        /// <returns>The <see cref="byte"/> containing the packed value.</returns>
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private static byte Pack(float r, float g, float b)


This only works with Vector4 but is being used for Rgba32 etc.

JimBobSquarePants · 2018-10-08T18:01:38Z

I'm gonna do #730 at the same time otherwise we're writing code we'll immediately delete.

JimBobSquarePants · 2018-10-09T23:05:54Z

@jongleur1983 Ok. So I've fixed up all the pixels formats and commented out all the old tests.

I need to do two things now to complete this PR:

Add a new suite of tests to cover IPixel functionality
Add optimized overloads for the PixelOperations methods.

JimBobSquarePants · 2018-10-09T23:27:58Z

Got four tests failing due to ToString change in Rgba32 as the reference image path depended on original name. We should update that. Perhaps hex?

https://ci.appveyor.com/project/six-labors/imagesharp/builds/19386791/job/x9ru3njufhn9ec6j/tests

antonfirsov · 2018-10-09T23:46:08Z

Yeah hex is better for this purpose

…d fix conversion

codecov · 2018-10-15T17:16:25Z

Codecov Report

Merging #729 into master will decrease coverage by 0.95%.
The diff coverage is 83.6%.

@@            Coverage Diff             @@
##           master     #729      +/-   ##
==========================================
- Coverage    89.3%   88.34%   -0.96%     
==========================================
  Files         973      982       +9     
  Lines       42976    41592    -1384     
  Branches     3047     3126      +79     
==========================================
- Hits        38381    36746    -1635     
- Misses       3913     4129     +216     
- Partials      682      717      +35

Impacted Files	Coverage Δ
...geSharp.Tests/PixelFormats/NormalizedByte4Tests.cs	`100% <ø> (ø)`	⬆️
tests/ImageSharp.Tests/PixelFormats/Short2Tests.cs	`100% <ø> (ø)`	⬆️
...eSharp.Tests/PixelFormats/NormalizedShort2Tests.cs	`100% <ø> (ø)`	⬆️
tests/ImageSharp.Tests/PixelFormats/Argb32Tests.cs	`100% <ø> (ø)`	⬆️
tests/ImageSharp.Tests/PixelFormats/Bgr565Tests.cs	`100% <ø> (ø)`	⬆️
.../ImageSharp/PixelFormats/Rgba32.PixelOperations.cs	`100% <ø> (ø)`	⬆️
tests/ImageSharp.Tests/PixelFormats/Short4Tests.cs	`100% <ø> (ø)`	⬆️
...mageSharp.Tests/PixelFormats/UnPackedPixelTests.cs	`100% <ø> (ø)`	⬆️
...ests/Processing/Transforms/AffineTransformTests.cs	`98.48% <ø> (-0.04%)`	⬇️
tests/ImageSharp.Tests/Numerics/RationalTests.cs	`100% <ø> (ø)`	⬆️
... and 134 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 6f5ebbb...cd543b2. Read the comment docs.

JimBobSquarePants · 2018-10-15T17:21:01Z

Much happier with this now. The structure has much greater conformity and all IPixel operations are covered in both singular and bulk forms.

Gotta fix up the ToString() methods to use invariant culture but I'll wait til the other PR's come in first to drop NetStandard 1.1

antonfirsov · 2018-10-17T22:46:22Z

Ok, actually this might be good, I haven't noticed the new optimized bulk implementations!

So never mind my comment regarding dp.PackFromScaledVector4(sp.ToScaledVector4());! Gonna have a second look on Thursday.

JimBobSquarePants · 2018-10-18T00:04:25Z

@antonfirsov Yeah, all the IPixel conversions have optimized bulk implementations so there should be no performance loss. It should actually be faster in some cases than before.

antonfirsov · 2018-10-18T21:27:02Z

@JimBobSquarePants in this benchmark I emulated a scenario which is similar to the ToRgba32() stuff happening in PNG encoder, and certain processors (eg. Binarization, Dithering).

It turns out, that even in these cases the retval version is significantly slower.

Also benchmarked ToVector4() - same stuff, with less difference. I can't really explain this, but we should change to a CopyTo(ref ...) style API in both cases.

The question is when? We have two options:

Change it in this PR, open issue for ToVector4(). Milestone: RC1 (API change)
Merge this PR, open issue for both ToRgba32() and ToVector4(). Milestone: RC1 (API change, perf regression!)

JimBobSquarePants · 2018-10-18T21:38:11Z

@antonfirsov Those results are extraordinary, I'm very surprised!

Let's do it here now. A new PR will be another very large one again probably too large to digest. We know where we are here and can review knowing exactly what we are looking at having recently worked on the files.

ScaledVector4 will have to be updated also.

antonfirsov · 2018-10-18T21:45:34Z

One possible explanation is that JIT is not capable for Return value optimization in most cases.

~~Do you mean also finishing up the Vector4 stuff here?`~~ Should/can we split the work somehow?

Edit:
Just noticed your edit on Vector4

JimBobSquarePants · 2018-10-18T21:51:18Z

I was tempted to do it all here but for sanities sake lets do Rgba32 here and immediately raise an issue for next priority for Vector4. We don't want to be blocking any open PR's

antonfirsov · 2018-10-21T22:08:50Z

@JimBobSquarePants my feeling is that it would be much simpler to merge this as is, right after #742 and carry out all my proposed API changes together in a new PR.

JimBobSquarePants · 2018-10-21T22:13:25Z

@antonfirsov works for me! 👍

# Conflicts: # src/ImageSharp/PixelFormats/Rgba32.PixelOperations.cs # tests/ImageSharp.Tests/PixelFormats/PixelOperationsTests.cs

antonfirsov · 2018-10-22T00:30:59Z

Is something wrong with GitHub at the moment?

The status checks are not being updated + the git changelog I see with my client is out of sync with the stuff presented on GitHub web.

antonfirsov · 2018-10-22T00:31:36Z

Is something wrong with GitHub at the moment?

The status checks are not being updated + the git changelog I see with my client is out of sync with the stuff presented on GitHub web.

antonfirsov · 2018-10-22T13:14:56Z

@JimBobSquarePants any idea how to ping the CLA assistant? Pressing "Recheck PR-s" on it's UI did not help.

antonfirsov · 2018-10-22T13:23:56Z

OK, it seem there are still issues with service availability, let's check again after a few hours.

JimBobSquarePants · 2018-10-22T15:06:21Z

@antonfirsov Github's having all sorts of issues just now.

SixLabors#718 SixLabors#730: Add Gray8 and Gray16 Pixel Formats and clean up IPixel

jongleur1983 and others added 8 commits October 6, 2018 23:16

fix whitespacing in generated code

0abb99b

SixLabors#718: add pixel types Gray8 and Gray16

9720fea

SixLabors#718: fix merge conflicts, adapt changes from PR SixLabors#727…

0354f32

… to t4 script

SixLabors#718: fix Gray8Tests

904fcd5

SixLabors#718: extend IPixel interface and implement it everywhere, ...

58a5232

AND add unit tests AND fix Gray16 implementation (constructor takes ushort, not byte) AND fix Gray8 and Gray16 Pack (do not scale by sum)

SixLabors#718: add missing inheritdoc comments

5d35bcd

SixLabors#718: add missing documentation

c8efad0

Merge branch 'master' into 718-gray8-gray16

8de1769

JimBobSquarePants requested changes Oct 8, 2018

View reviewed changes

Fix Gray8

1533c4c

JimBobSquarePants changed the title ~~#718: Add Gray8 and Gray16 Pixel Formats~~ WIP #718 #730: Add Gray8 and Gray16 Pixel Formats and clean up IPixel Oct 9, 2018

JimBobSquarePants added 2 commits October 9, 2018 23:57

Update and normalise pixel format + disable out of date tests

2950219

Merge branch 'master' into 718-gray8-gray16

486ca25

JimBobSquarePants added 6 commits October 10, 2018 08:58

Fix linear gradient brush test.

d4327c5

Merge remote-tracking branch 'upstream/master' into 718-gray8-gray16

891a1c5

Fix up Gray8 and Gary16 tests

c349c2b

Merge remote-tracking branch 'upstream/master' into 718-gray8-gray16

c85c739

Fix up pixel format tests

1f7df77

Generate bulk pixel operations for all standard packing operations an…

1c2aee0

…d fix conversion

JimBobSquarePants approved these changes Oct 15, 2018

View reviewed changes

JimBobSquarePants added 4 commits October 15, 2018 19:53

Fix sandbox tests

d4bda60

Fix JIT issue on 32bit framework

b14c35f

Merge branch 'master' into 718-gray8-gray16

c23dbcd

Merge remote-tracking branch 'upstream/master' into 718-gray8-gray16

e97ced4

antonfirsov approved these changes Oct 17, 2018

View reviewed changes

antonfirsov added 3 commits October 18, 2018 22:45

common fixtures for PixelConversion* benchmarks

9b19631

PixelConversion_ConvertToRgba32_AsPartOfCompositeOperation

3051d71

benchmarks for TPixel -> Vector4 conversion

9ab574c

Merge branch 'master' into 718-gray8-gray16

21853e6

antonfirsov mentioned this pull request Oct 21, 2018

Clean up and optimize byte<->float and Rgba32 <-> Vector4 conversion #742

Merged

4 tasks done

antonfirsov added 5 commits October 22, 2018 00:30

Merge branch 'af/simd-conversion' into 718-gray8-gray16

170a00c

# Conflicts: # src/ImageSharp/PixelFormats/Rgba32.PixelOperations.cs # tests/ImageSharp.Tests/PixelFormats/PixelOperationsTests.cs

fix PixelOperationsTests

876f230

Merge remote-tracking branch 'origin/master' into 718-gray8-gray16

de2d037

wakeup message to the git status-checks

5a070ea

Something's wrong with the status checks

6ca596d

CLA assistant, please wake up!

ed55a1f

Bump build now Github should be returned to normal.

cd543b2

JimBobSquarePants merged commit d65cf67 into SixLabors:master Oct 23, 2018

JimBobSquarePants mentioned this pull request Oct 23, 2018

Addition of Breadley AdaptiveThreshold #725

Merged

3 tasks done

antonfirsov mentioned this pull request Oct 26, 2018

Introduce optimized conversion methods for RGBA-like types #751

Merged

4 tasks done

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

Merge pull request SixLabors#729 from jongleur1983/718-gray8-gray16

ba6bacf

SixLabors#718 SixLabors#730: Add Gray8 and Gray16 Pixel Formats and clean up IPixel

Conversation

jongleur1983 commented Oct 6, 2018

Prerequisites

Description

Uh oh!

JimBobSquarePants left a comment

Choose a reason for hiding this comment

Uh oh!

JimBobSquarePants Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

JimBobSquarePants Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

JimBobSquarePants Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

JimBobSquarePants Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

JimBobSquarePants commented Oct 8, 2018

Uh oh!

JimBobSquarePants commented Oct 9, 2018

Uh oh!

JimBobSquarePants commented Oct 9, 2018

Uh oh!

antonfirsov commented Oct 9, 2018

Uh oh!

codecov Bot commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JimBobSquarePants commented Oct 15, 2018

Uh oh!

antonfirsov commented Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JimBobSquarePants commented Oct 18, 2018

Uh oh!

antonfirsov commented Oct 18, 2018

Uh oh!

JimBobSquarePants commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antonfirsov commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JimBobSquarePants commented Oct 18, 2018

Uh oh!

antonfirsov commented Oct 21, 2018

Uh oh!

JimBobSquarePants commented Oct 21, 2018

Uh oh!

antonfirsov commented Oct 22, 2018

Uh oh!

antonfirsov commented Oct 22, 2018

Uh oh!

antonfirsov commented Oct 22, 2018

Uh oh!

antonfirsov commented Oct 22, 2018

Uh oh!

JimBobSquarePants commented Oct 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Oct 15, 2018 •

edited

Loading

antonfirsov commented Oct 17, 2018 •

edited

Loading

JimBobSquarePants commented Oct 18, 2018 •

edited

Loading

antonfirsov commented Oct 18, 2018 •

edited

Loading