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.

Sign up for GitHub

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

Jump to bottom

Expose Span methods on LoadPixelData and SavePixelData #567

Merged
merged 26 commits into from
May 12, 2018
Merged

Expose Span methods on LoadPixelData and SavePixelData #567

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
92ab411
Expose LoadPixelData overloads accepting Span<T>
iamcarbon May 9, 2018
cddab85
Cleanup ByteExtensions
iamcarbon May 9, 2018
54791bf
Remove trailing whitespace
iamcarbon May 9, 2018
c6c9ae8
Simplify png decoder
iamcarbon May 9, 2018
1653a93
Simplify GifDecoder
iamcarbon May 9, 2018
ea38b24
Remove ByteExtensions
iamcarbon May 9, 2018
bddc20a
Simplfiy ToRgbaHex
iamcarbon May 9, 2018
1c79b08
Merge branch 'master' into memory-rc1
iamcarbon May 9, 2018
5156890
👮
iamcarbon May 9, 2018
15494b4
Merge branch 'memory-rc1' of https://github.com/carbon/ImageSharp int…
iamcarbon May 9, 2018
00af2df
Rename Gaurd.NotNullOrEmpty to NotNullOrWhiteSpace to match behavior …
iamcarbon May 10, 2018
eb318a3
Don't use Linq to check if IEnumerable is empty.
iamcarbon May 10, 2018
fd766a5
React to Gaurd changes
iamcarbon May 10, 2018
017c7fb
Verify ColorBuilder throws on null and empty
iamcarbon May 10, 2018
fda2d1f
Optimize ColorBuilder
iamcarbon May 10, 2018
4f779ca
Improve ColorBuilder test coverage
iamcarbon May 10, 2018
46e0a85
Allow leading period in FindFormatByFileExtension
iamcarbon May 10, 2018
202e472
Use ReadOnlySpans in LoadPixelData
iamcarbon May 10, 2018
45e5d5b
Allow pixel data to be saved directly to a span
iamcarbon May 10, 2018
09a77e4
Merge branch 'master' into memory-rc1
antonfirsov May 10, 2018
a05b618
Update Span ->ReadOnlySpan
iamcarbon May 11, 2018
92f353b
Tidy up ImageExtension docs
iamcarbon May 11, 2018
1695b59
Update three more spans to ReadOnly
iamcarbon May 11, 2018
63dc698
Merge branch 'memory-rc1' of https://github.com/carbon/ImageSharp int…
iamcarbon May 11, 2018
93c44f4
React to LoadPixelData change
iamcarbon May 11, 2018
05aa089
Merge branch 'master' into memory-rc1
JimBobSquarePants May 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 0 additions & 55 deletions src/ImageSharp/Common/Extensions/ByteExtensions.cs
View file
Open in desktop

This file was deleted.

24 changes: 8 additions & 16 deletions src/ImageSharp/Formats/Gif/GifDecoderCore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,6 @@ internal sealed class GifDecoderCore
/// </summary>
private IManagedByteBuffer globalColorTable;

/// <summary>
/// The global color table length
/// </summary>
private int globalColorTableLength;

/// <summary>
/// The area to restore.
/// </summary>
Expand Down Expand Up @@ -333,8 +328,8 @@ private void ReadFrame<TPixel>(ref Image<TPixel> image, ref ImageFrame<TPixel> p
indices = this.configuration.MemoryManager.AllocateManagedByteBuffer(imageDescriptor.Width * imageDescriptor.Height, true);

this.ReadFrameIndices(imageDescriptor, indices.Span);
IManagedByteBuffer colorTable = localColorTable ?? this.globalColorTable;
this.ReadFrameColors(ref image, ref previousFrame, indices.Span, colorTable.Span, imageDescriptor);
ReadOnlySpan<Rgb24> colorTable = MemoryMarshal.Cast<byte, Rgb24>((localColorTable ?? this.globalColorTable).Span);
this.ReadFrameColors(ref image, ref previousFrame, indices.Span, colorTable, imageDescriptor);

// Skip any remaining blocks
this.Skip(0);
Expand Down Expand Up @@ -370,7 +365,7 @@ private void ReadFrameIndices(in GifImageDescriptor imageDescriptor, Span<byte>
/// <param name="indices">The indexed pixels.</param>
/// <param name="colorTable">The color table containing the available colors.</param>
/// <param name="descriptor">The <see cref="GifImageDescriptor"/></param>
private void ReadFrameColors<TPixel>(ref Image<TPixel> image, ref ImageFrame<TPixel> previousFrame, Span<byte> indices, Span<byte> colorTable, in GifImageDescriptor descriptor)
private void ReadFrameColors<TPixel>(ref Image<TPixel> image, ref ImageFrame<TPixel> previousFrame, Span<byte> indices, ReadOnlySpan<Rgb24> colorTable, in GifImageDescriptor descriptor)
where TPixel : struct, IPixel<TPixel>
{
ref byte indicesRef = ref MemoryMarshal.GetReference(indices);
Expand Down Expand Up @@ -458,11 +453,8 @@ private void ReadFrameColors<TPixel>(ref Image<TPixel> image, ref ImageFrame<TPi
if (this.graphicsControlExtension.TransparencyFlag == false ||
this.graphicsControlExtension.TransparencyIndex != index)
{
int indexOffset = index * 3;

ref TPixel pixel = ref Unsafe.Add(ref rowRef, x);
rgba.Rgb = colorTable.GetRgb24(indexOffset);

rgba.Rgb = colorTable[index];
pixel.PackFromRgba32(rgba);
}

Expand Down Expand Up @@ -534,12 +526,12 @@ private void ReadLogicalScreenDescriptorAndGlobalColorTable(Stream stream)

if (this.logicalScreenDescriptor.GlobalColorTableFlag)
{
this.globalColorTableLength = this.logicalScreenDescriptor.GlobalColorTableSize * 3;
int globalColorTableLength = this.logicalScreenDescriptor.GlobalColorTableSize * 3;

this.globalColorTable = this.MemoryManager.AllocateManagedByteBuffer(this.globalColorTableLength, true);
this.globalColorTable = this.MemoryManager.AllocateManagedByteBuffer(globalColorTableLength, true);

// Read the global color table from the stream
stream.Read(this.globalColorTable.Array, 0, this.globalColorTableLength);
// Read the global color table data from the stream
stream.Read(this.globalColorTable.Array, 0, globalColorTableLength);
}
}
}
Expand Down
16 changes: 6 additions & 10 deletions src/ImageSharp/Formats/Png/PngDecoderCore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@iamcarbon iamcarbon May 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

var color = default(TPixel);

var rgba = default(Rgba32);
Expand All @@ -865,10 +865,9 @@ private void ProcessScanlineFromPalette<TPixel>(ReadOnlySpan<byte> defilteredSca
for (int x = 0; x < this.header.Width; x++)
{
int index = newScanline[x];
int pixelOffset = index * 3;

rgba.A = this.paletteAlpha.Length > index ? this.paletteAlpha[index] : (byte)255;
rgba.Rgb = pal.GetRgb24(pixelOffset);
rgba.Rgb = pal[index];

color.PackFromRgba32(rgba);
row[x] = color;
Expand All @@ -881,9 +880,8 @@ private void ProcessScanlineFromPalette<TPixel>(ReadOnlySpan<byte> defilteredSca
for (int x = 0; x < this.header.Width; x++)
{
int index = newScanline[x];
int pixelOffset = index * 3;

rgba.Rgb = pal.GetRgb24(pixelOffset);
rgba.Rgb = pal[index];

color.PackFromRgba32(rgba);
row[x] = color;
Expand Down Expand Up @@ -946,6 +944,7 @@ private void ProcessInterlacedDefilteredScanline<TPixel>(ReadOnlySpan<byte> defi

ReadOnlySpan<byte> newScanline = ToArrayByBitsLength(scanlineBuffer, this.bytesPerScanline, this.header.BitDepth);
var rgba = default(Rgba32);
Span<Rgb24> pal = MemoryMarshal.Cast<byte, Rgb24>(this.palette);

if (this.paletteAlpha != null && this.paletteAlpha.Length > 0)
{
Expand All @@ -954,10 +953,9 @@ private void ProcessInterlacedDefilteredScanline<TPixel>(ReadOnlySpan<byte> defi
for (int x = pixelOffset, o = 0; x < this.header.Width; x += increment, o++)
{
int index = newScanline[o];
int offset = index * 3;

rgba.A = this.paletteAlpha.Length > index ? this.paletteAlpha[index] : (byte)255;
rgba.Rgb = this.palette.GetRgb24(offset);
rgba.Rgb = pal[index];

color.PackFromRgba32(rgba);
rowSpan[x] = color;
Expand All @@ -970,10 +968,8 @@ private void ProcessInterlacedDefilteredScanline<TPixel>(ReadOnlySpan<byte> defi
for (int x = pixelOffset, o = 0; x < this.header.Width; x += increment, o++)
{
int index = newScanline[o];
int offset = index * 3;

rgba.Rgb = this.palette.GetRgb24(offset);

rgba.Rgb = pal[index];
color.PackFromRgba32(rgba);
rowSpan[x] = color;
}
Expand Down
8 changes: 4 additions & 4 deletions src/ImageSharp/Image.LoadPixelData.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

@antonfirsov antonfirsov May 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We should consume ReadOnlySpan in all overloads.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

where TPixel : struct, IPixel<TPixel>
=> LoadPixelData(Configuration.Default, data, width, height);

Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

ReadOnlySpan<byte>

where TPixel : struct, IPixel<TPixel>
=> LoadPixelData<TPixel>(Configuration.Default, data, width, height);

Expand All @@ -83,7 +83,7 @@ public static Image<TPixel> LoadPixelData<TPixel>(Configuration config, byte[] d
/// <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>(Configuration config, Span<byte> data, int width, int height)
public static Image<TPixel> LoadPixelData<TPixel>(Configuration config, Span<byte> data, int width, int height)
where TPixel : struct, IPixel<TPixel>
=> LoadPixelData(config, MemoryMarshal.Cast<byte, TPixel>(data), width, height);

Expand Down Expand Up @@ -111,7 +111,7 @@ public static Image<TPixel> LoadPixelData<TPixel>(Configuration config, TPixel[]
/// <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>(Configuration config, Span<TPixel> data, int width, int height)
public static Image<TPixel> LoadPixelData<TPixel>(Configuration config, Span<TPixel> data, int width, int height)
where TPixel : struct, IPixel<TPixel>
{
int count = width * height;
Expand Down
5 changes: 4 additions & 1 deletion src/ImageSharp/PixelFormats/ColorBuilder{TPixel}.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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] == '#')
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@iamcarbon iamcarbon May 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

{
hex = hex.Substring(1);
}

if (hex.Length == 8)
{
Expand Down