Expose Span methods on LoadPixelData and SavePixelData by iamcarbon · Pull Request #567 · SixLabors/ImageSharp

iamcarbon · 2018-05-09T22:46:40Z

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

Expose the Span overloads in LoadImageData & SaveImageData. This allows image data to be copied directly to and from Memory.
Simplify palette access within the Png & Gif decoders and eliminate ByteExtensions.
Cleanup Gaurd.
Allow a leading period in extension when calling FindFormatByFileExtension

codecov · 2018-05-09T23:21:50Z

Codecov Report

Merging #567 into master will increase coverage by <.01%.
The diff coverage is 82.75%.

@@            Coverage Diff             @@
##           master     #567      +/-   ##
==========================================
+ Coverage   88.62%   88.62%   +<.01%     
==========================================
  Files         843      843              
  Lines       35549    35550       +1     
  Branches     2585     2585              
==========================================
+ Hits        31504    31506       +2     
  Misses       3268     3268              
+ Partials      777      776       -1

Impacted Files	Coverage Δ
src/ImageSharp/ImageFrame.LoadPixelData.cs	`83.33% <ø> (ø)`	⬆️
src/ImageSharp/Formats/Png/Filters/NoneFilter.cs	`100% <ø> (ø)`	⬆️
...ImageSharp.Tests/PixelFormats/ColorBuilderTests.cs	`100% <100%> (ø)`
src/ImageSharp/Formats/Gif/GifDecoderCore.cs	`77.77% <100%> (-0.12%)`	⬇️
src/ImageSharp/Image.LoadPixelData.cs	`75% <100%> (ø)`	⬆️
...arp.Tests/TestUtilities/TestEnvironment.Formats.cs	`90.62% <100%> (-0.29%)`	⬇️
tests/ImageSharp.Tests/Helpers/GuardTests.cs	`100% <100%> (ø)`	⬆️
src/ImageSharp/ImageExtensions.cs	`69.38% <100%> (ø)`	⬆️
src/ImageSharp/MetaData/ImageProperty.cs	`66.66% <100%> (ø)`	⬆️
src/ImageSharp/ImageFrameCollection.cs	`89.47% <50%> (-2.42%)`	⬇️
... and 6 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 5012861...05aa089. Read the comment docs.

…o memory-rc1

iamcarbon · 2018-05-10T00:06:00Z

Ready for review.

tocsoft

just the one change and a comment regards a subtle change in behavior. Everything else looks good to me.

tocsoft · 2018-05-10T09:47:01Z

src/ImageSharp/PixelFormats/ColorBuilder{TPixel}.cs

        private static string ToRgbaHex(string hex)
        {
-            hex = hex.StartsWith("#") ? hex.Substring(1) : hex;
+            if (hex[0] == '#')


this will fail if hex is an empty string. we should be checking the length > 0 too. Probably should be defensive over nulls as well.

This methods only caller is guarding against null and empty. I added some tests to verify this behavior.

tocsoft · 2018-05-10T09:52:22Z

src/ImageSharp/Formats/Png/PngDecoderCore.cs

        {
            ReadOnlySpan<byte> newScanline = ToArrayByBitsLength(defilteredScanline, this.bytesPerScanline, this.header.BitDepth);
-            byte[] pal = this.palette;
+            ReadOnlySpan<Rgb24> pal = MemoryMarshal.Cast<byte, Rgb24>(this.palette);


won't this fail if this.palette isn't the correct length? just raising this as a subtle change of behavior that might introduce exceptions that previously (potentially incorrectly) allowed.

note: not asking for a change just pointing it out incase someone else thinks its important.

This should only throw if the we try indexing into the palette outside it's bounds.

…& cleanup messages. Also remove the unused message arguments.

iamcarbon · 2018-05-10T15:49:24Z

Haven't seen this error before or able to reproduce locally. Will force another build to see if it's transient.

/home/travis/build/SixLabors/ImageSharp/tests/ImageSharp.Tests/Image/ImageCloneTests.cs(48,0): at SixLabors.ImageSharp.Tests.ImageCloneTests.CloneAs_ToBgr24(TestImageProvider`1 provider)
Failed   CloneAs_ToBgr24(provider: TestPattern9x9[Rgba32])
Error Message:
 Assert.Equal() Failure
Expected: 0
Actual:   211
Stack Trace:
   at SixLabors.ImageSharp.Tests.ImageCloneTests.CloneAs_ToBgr24(TestImageProvider`1 provider) in /home/travis/build/SixLabors/ImageSharp/tests/ImageSharp.Tests/Image/ImageCloneTests.cs:line 48

antonfirsov · 2018-05-10T20:28:51Z

src/ImageSharp/Image.LoadPixelData.cs

        /// <typeparam name="TPixel">The pixel format.</typeparam>
        /// <returns>A new <see cref="Image{TPixel}"/>.</returns>
-        private static Image<TPixel> LoadPixelData<TPixel>(Span<TPixel> data, int width, int height)
+        public static Image<TPixel> LoadPixelData<TPixel>(Span<TPixel> data, int width, int height)


We should consume ReadOnlySpan in all overloads.

My mistake that I also forgot about this in #565 :)

Agreed. I'll update the PR shortly. Also need to figure out why that test is failing.

change not needed as caller is guarded

iamcarbon · 2018-05-10T21:02:05Z

@JimBobSquarePants @antonfirsov

Should we remove the LoadFrom / SaveTo array overloads and just expose Span to simplify the API surface? Any downsides?

antonfirsov · 2018-05-10T21:45:31Z

Not everyone knows about Span<T>, and there are only a few who really understand it. I would expect many users looking for the good-old byte[] overload, so maybe it's better to keep it.

An other donwside is loosing binary compatiblity, but I don't mind that, because no one cares about binary compatiblity in .NET Core world these days 😆 (At least not before reaching RC.)

iamcarbon · 2018-05-10T21:52:42Z

@JimBobSquarePants Ready for your final review.

JimBobSquarePants

Good stuff... One change and a small doco error though please..

JimBobSquarePants · 2018-05-11T01:40:11Z

src/ImageSharp/Image.LoadPixelData.cs

        /// <typeparam name="TPixel">The pixel format.</typeparam>
        /// <returns>A new <see cref="Image{TPixel}"/>.</returns>
-        private static Image<TPixel> LoadPixelData<TPixel>(Span<byte> data, int width, int height)
+        public static Image<TPixel> LoadPixelData<TPixel>(Span<byte> data, int width, int height)


ReadOnlySpan<byte>

JimBobSquarePants · 2018-05-11T01:42:47Z

src/ImageSharp/ImageExtensions.cs


        /// <summary>
-        /// Saves the raw image to the given bytes.
+        /// Saves the raw image to the given byte buffer.


Target buffer is of type TPixel

JimBobSquarePants · 2018-05-11T01:45:19Z

src/ImageSharp/ImageFrameCollection.cs

            }
+
+            // Ensure at least 1 frame was added to the frames collection
+            if (this.frames.Count == 0)


Would we not check this before iterating. Guard.IsTrue(frames.Count > 0)?

No count on IEnumerable.

Legit, forgot about IEnumerable.

JimBobSquarePants · 2018-05-11T01:46:51Z

src/ImageSharp/PixelFormats/ColorBuilder{TPixel}.cs

-                (byte)(packedValue >> 16),
-                (byte)(packedValue >> 8),
-                (byte)(packedValue >> 0));
+            var rgba = new Rgba32(BinaryPrimitives.ReverseEndianness(packedValue));


JimBobSquarePants · 2018-05-11T01:47:06Z

src/ImageSharp/PixelFormats/ColorBuilder{TPixel}.cs

+            char a = hex.Length == 3 ? 'F' : hex[3];

-            return string.Concat(red, red, green, green, blue, blue, alpha, alpha);
+            return new string(new[] { r, r, g, g, b, b, a, a });


JimBobSquarePants · 2018-05-11T01:47:31Z

tests/ImageSharp.Tests/PixelFormats/ColorBuilderTests.cs

+
+namespace SixLabors.ImageSharp.Tests.Colors
+{
+    public class ColorBuilderTests


JimBobSquarePants · 2018-05-11T01:48:16Z

src/ImageSharp/Formats/ImageFormatManager.cs

        {
+            Guard.NotNullOrWhiteSpace(extension, nameof(extension));
+
+            if (extension[0] == '.')


…o memory-rc1

iamcarbon added 2 commits May 9, 2018 15:42

Expose LoadPixelData overloads accepting Span<T>

92ab411

Cleanup ByteExtensions

cddab85

iamcarbon added 8 commits May 9, 2018 16:22

Remove trailing whitespace

54791bf

Simplify png decoder

c6c9ae8

Simplify GifDecoder

1653a93

Remove ByteExtensions

ea38b24

Simplfiy ToRgbaHex

bddc20a

Merge branch 'master' into memory-rc1

1c79b08

👮

5156890

Merge branch 'memory-rc1' of https://github.com/carbon/ImageSharp int…

15494b4

…o memory-rc1

tocsoft previously requested changes May 10, 2018

View reviewed changes

iamcarbon added 6 commits May 10, 2018 07:59

Rename Gaurd.NotNullOrEmpty to NotNullOrWhiteSpace to match behavior …

00af2df

…& cleanup messages. Also remove the unused message arguments.

Don't use Linq to check if IEnumerable is empty.

eb318a3

React to Gaurd changes

fd766a5

Verify ColorBuilder throws on null and empty

017c7fb

Optimize ColorBuilder

fda2d1f

Improve ColorBuilder test coverage

4f779ca

Allow leading period in FindFormatByFileExtension

46e0a85

antonfirsov requested changes May 10, 2018

View reviewed changes

iamcarbon added 2 commits May 10, 2018 13:47

Use ReadOnlySpans in LoadPixelData

202e472

Allow pixel data to be saved directly to a span

45e5d5b

iamcarbon changed the title ~~Expose LoadPixelData overloads accepting Span<T>~~ Expose Span methods on LoadPixelData and SavePixelData May 10, 2018

antonfirsov approved these changes May 10, 2018

View reviewed changes

Merge branch 'master' into memory-rc1

09a77e4

JimBobSquarePants requested changes May 11, 2018

View reviewed changes

iamcarbon added 5 commits May 11, 2018 10:31

Update Span ->ReadOnlySpan

a05b618

Tidy up ImageExtension docs

92f353b

Update three more spans to ReadOnly

1695b59

Merge branch 'memory-rc1' of https://github.com/carbon/ImageSharp int…

63dc698

…o memory-rc1

React to LoadPixelData change

93c44f4

JimBobSquarePants approved these changes May 12, 2018

View reviewed changes

Merge branch 'master' into memory-rc1

05aa089

JimBobSquarePants merged commit 61da705 into SixLabors:master May 12, 2018

This was referenced May 26, 2018

System.Memory API integration #565

Closed

Image.Load(Span<byte> data) overload please! #489

Closed

dependabot bot mentioned this pull request Feb 6, 2026

Bump the nuget group with 3 updates haoyunZhou/Csharp-Data-Visualization#1

Open

Uh oh!

Conversation

iamcarbon commented May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Prerequisites

Description

Uh oh!

codecov bot commented May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

iamcarbon commented May 10, 2018

Uh oh!

tocsoft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamcarbon May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamcarbon May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamcarbon commented May 10, 2018

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamcarbon commented May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

iamcarbon commented May 10, 2018

Uh oh!

JimBobSquarePants left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iamcarbon commented May 9, 2018 •

edited

Loading

codecov bot commented May 9, 2018 •

edited

Loading

iamcarbon May 10, 2018 •

edited

Loading

iamcarbon May 10, 2018 •

edited

Loading

antonfirsov May 10, 2018 •

edited

Loading

iamcarbon commented May 10, 2018 •

edited

Loading

antonfirsov commented May 10, 2018 •

edited

Loading