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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose Span methods on LoadPixelData and SavePixelData #567

Merged
merged 26 commits into from May 12, 2018

Conversation

Projects
None yet
4 participants
@iamcarbon
Copy link
Contributor

iamcarbon commented May 9, 2018

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

iamcarbon added some commits May 9, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented May 9, 2018

Codecov Report

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

Impacted file tree graph

@@            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.

iamcarbon added some commits May 9, 2018

@iamcarbon

This comment has been minimized.

Copy link
Contributor

iamcarbon commented May 10, 2018

Ready for review.

@tocsoft
Copy link
Member

tocsoft left a comment

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

@@ -76,7 +76,10 @@ public static TPixel FromRGBA(byte red, byte green, byte blue, byte alpha)
/// </returns>
private static string ToRgbaHex(string hex)
{
hex = hex.StartsWith("#") ? hex.Substring(1) : hex;
if (hex[0] == '#')

This comment has been minimized.

@tocsoft

tocsoft May 10, 2018

Member

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 comment has been minimized.

@iamcarbon

iamcarbon May 10, 2018

Contributor

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

@@ -853,7 +853,7 @@ private void ProcessScanlineFromPalette<TPixel>(ReadOnlySpan<byte> defilteredSca
where TPixel : struct, IPixel<TPixel>
{
ReadOnlySpan<byte> newScanline = ToArrayByBitsLength(defilteredScanline, this.bytesPerScanline, this.header.BitDepth);
byte[] pal = this.palette;
ReadOnlySpan<Rgb24> pal = MemoryMarshal.Cast<byte, Rgb24>(this.palette);

This comment has been minimized.

@tocsoft

tocsoft May 10, 2018

Member

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 comment has been minimized.

@iamcarbon

iamcarbon May 10, 2018

Contributor

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

@iamcarbon

This comment has been minimized.

Copy link
Contributor

iamcarbon commented May 10, 2018

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
@@ -33,7 +33,7 @@ public static Image<TPixel> LoadPixelData<TPixel>(TPixel[] data, int width, int
/// <param name="height">The height of the final image.</param>
/// <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)

This comment has been minimized.

@antonfirsov

antonfirsov May 10, 2018

Member

We should consume ReadOnlySpan in all overloads.

This comment has been minimized.

@antonfirsov

antonfirsov May 10, 2018

Member

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

This comment has been minimized.

@iamcarbon

iamcarbon May 10, 2018

Contributor

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

This comment has been minimized.

@iamcarbon

iamcarbon May 10, 2018

Contributor

Updated.

change not needed as caller is guarded

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

@iamcarbon

This comment has been minimized.

Copy link
Contributor

iamcarbon commented May 10, 2018

@JimBobSquarePants @antonfirsov

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

@antonfirsov

This comment has been minimized.

Copy link
Member

antonfirsov commented May 10, 2018

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

This comment has been minimized.

Copy link
Contributor

iamcarbon commented May 10, 2018

@JimBobSquarePants Ready for your final review.

@JimBobSquarePants
Copy link
Member

JimBobSquarePants left a comment

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

@@ -57,7 +57,7 @@ public static Image<TPixel> LoadPixelData<TPixel>(byte[] data, int width, int he
/// <param name="height">The height of the final image.</param>
/// <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)

This comment has been minimized.

@JimBobSquarePants

JimBobSquarePants May 11, 2018

Member

ReadOnlySpan<byte>

where TPixel : struct, IPixel<TPixel>
=> source.Frames.RootFrame.SavePixelData(MemoryMarshal.Cast<byte, TPixel>(buffer));

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

This comment has been minimized.

@JimBobSquarePants

JimBobSquarePants May 11, 2018

Member

Target buffer is of type TPixel

@@ -42,6 +42,12 @@ internal ImageFrameCollection(Image<TPixel> parent, IEnumerable<ImageFrame<TPixe
this.ValidateFrame(f);
this.frames.Add(f);
}

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

This comment has been minimized.

@JimBobSquarePants

JimBobSquarePants May 11, 2018

Member

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

This comment has been minimized.

@iamcarbon

iamcarbon May 11, 2018

Contributor

No count on IEnumerable.

This comment has been minimized.

@JimBobSquarePants

JimBobSquarePants May 12, 2018

Member

Legit, forgot about IEnumerable.

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

This comment has been minimized.

@JimBobSquarePants

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

This comment has been minimized.

@JimBobSquarePants

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

This comment has been minimized.

@JimBobSquarePants
@@ -85,6 +85,13 @@ public void AddImageFormat(IImageFormat format)
/// <returns>The <see cref="IImageFormat"/> if found otherwise null</returns>
public IImageFormat FindFormatByFileExtension(string extension)
{
Guard.NotNullOrWhiteSpace(extension, nameof(extension));

if (extension[0] == '.')

This comment has been minimized.

@JimBobSquarePants

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

4 of 5 checks passed

codecov/patch 82.75% of diff hit (target 88.62%)
Details
codecov/project 88.62% (+<.01%) compared to 5012861
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment