Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
4e8de95
Refactored byte[] array in ZigZag type
Sergio0694 Feb 27, 2020
c83d066
Refactored byte[] array in DeflaterHuffman type
Sergio0694 Feb 27, 2020
aa65526
Refactored byte[] array in PngConstants type
Sergio0694 Feb 27, 2020
da79756
Refactored byte[] array in ExifConstants type
Sergio0694 Feb 27, 2020
4dbec4c
Refactored byte[] array in Octree type
Sergio0694 Feb 27, 2020
714c960
Refactored byte[] arrays in GifConstants type
Sergio0694 Feb 27, 2020
bb7b041
Refactored byte[] arrays in ProfileResolver type
Sergio0694 Feb 27, 2020
f907f90
Code style tweaks
Sergio0694 Feb 27, 2020
8ad8a59
Fixed a build error
Sergio0694 Feb 27, 2020
b251c00
Minor code changes to improve clarity
Sergio0694 Feb 28, 2020
0577690
Added input validation to DeflaterHuffman unsafe offsetting
Sergio0694 Feb 28, 2020
68387a7
Fixed a test in the DeflaterHuffman type
Sergio0694 Feb 28, 2020
f3f6d9c
Merge branch 'master' into sp/text-segment-initialization
Sergio0694 Feb 28, 2020
5b7ac75
Refactored DeflaterHuffman.BitLengthOrder to ReadOnlySpan<byte>
Sergio0694 Feb 28, 2020
70ed21e
Reintroduced some bounds checks for additional security
Sergio0694 Feb 28, 2020
4b0dfd1
Minor micro-optimization in DeflaterHuffman type
Sergio0694 Feb 28, 2020
719b5a3
Merge branch 'master' into sp/text-segment-initialization
JimBobSquarePants Feb 29, 2020
99b88c0
Merge branch 'master' into sp/text-segment-initialization
JimBobSquarePants Mar 4, 2020
b43a66c
tem remove submodule to reset dirty
JimBobSquarePants Mar 6, 2020
cbd1872
Fix module, import configs, automate T4 builds
JimBobSquarePants Mar 6, 2020
20bf5fa
Manually include compiled file in source via auto copy.
JimBobSquarePants Mar 6, 2020
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
4 changes: 3 additions & 1 deletion src/ImageSharp/Common/Extensions/StreamExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@
// Licensed under the Apache License, Version 2.0.

using System;
using System.Buffers;
using System.IO;
using SixLabors.ImageSharp.Memory;
#if !SUPPORTS_SPAN_STREAM
using System.Buffers;
#endif

namespace SixLabors.ImageSharp
{
Expand Down
31 changes: 21 additions & 10 deletions src/ImageSharp/Formats/Gif/GifConstants.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) Six Labors and contributors.
// Licensed under the Apache License, Version 2.0.

using System;
using System.Collections.Generic;
using System.Text;

Expand All @@ -21,11 +22,6 @@ internal static class GifConstants
/// </summary>
public const string FileVersion = "89a";

/// <summary>
/// The ASCII encoded bytes used to identify the GIF file.
/// </summary>
internal static readonly byte[] MagicNumber = Encoding.ASCII.GetBytes(FileType + FileVersion);

/// <summary>
/// The extension block introducer <value>!</value>.
/// </summary>
Expand All @@ -51,11 +47,6 @@ internal static class GifConstants
/// </summary>
public const string NetscapeApplicationIdentification = "NETSCAPE2.0";

/// <summary>
/// The ASCII encoded application identification bytes.
/// </summary>
internal static readonly byte[] NetscapeApplicationIdentificationBytes = Encoding.ASCII.GetBytes(NetscapeApplicationIdentification);

/// <summary>
/// The Netscape looping application sub block size.
/// </summary>
Expand Down Expand Up @@ -110,5 +101,25 @@ internal static class GifConstants
/// The collection of file extensions that equate to a Gif.
/// </summary>
public static readonly IEnumerable<string> FileExtensions = new[] { "gif" };

/// <summary>
/// Gets the ASCII encoded bytes used to identify the GIF file (combining <see cref="FileType"/> and <see cref="FileVersion"/>).
/// </summary>
internal static ReadOnlySpan<byte> MagicNumber => new[]
{
(byte)'G', (byte)'I', (byte)'F',
(byte)'8', (byte)'9', (byte)'a'
};

/// <summary>
/// Gets the ASCII encoded application identification bytes (representing <see cref="NetscapeApplicationIdentification"/>).
/// </summary>
internal static ReadOnlySpan<byte> NetscapeApplicationIdentificationBytes => new[]
{
(byte)'N', (byte)'E', (byte)'T',
(byte)'S', (byte)'C', (byte)'A',
(byte)'P', (byte)'E',
(byte)'2', (byte)'.', (byte)'0'
};
}
}
2 changes: 1 addition & 1 deletion src/ImageSharp/Formats/Gif/GifEncoderCore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ private int GetTransparentIndex<TPixel>(QuantizedFrame<TPixel> quantized)
/// </summary>
/// <param name="stream">The stream to write to.</param>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
private void WriteHeader(Stream stream) => stream.Write(GifConstants.MagicNumber, 0, GifConstants.MagicNumber.Length);
private void WriteHeader(Stream stream) => stream.Write(GifConstants.MagicNumber);

/// <summary>
/// Writes the logical screen descriptor to the stream.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public int WriteTo(Span<byte> buffer)
buffer[0] = GifConstants.ApplicationBlockSize;

// Write NETSCAPE2.0
GifConstants.NetscapeApplicationIdentificationBytes.AsSpan().CopyTo(buffer.Slice(1, 11));
GifConstants.NetscapeApplicationIdentificationBytes.CopyTo(buffer.Slice(1, 11));

// Application Data ----
buffer[12] = 3; // Application block length (always 3)
Expand Down
35 changes: 24 additions & 11 deletions src/ImageSharp/Formats/Jpeg/Components/Decoder/ProfileResolver.cs
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
// Copyright (c) Six Labors and contributors.
// Copyright (c) Six Labors and contributors.
// Licensed under the Apache License, Version 2.0.

using System;
using System.Text;

namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder
{
Expand All @@ -12,24 +11,38 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder
internal static class ProfileResolver
{
/// <summary>
/// Describes the JFIF specific markers.
/// Gets the JFIF specific markers.
/// </summary>
public static readonly byte[] JFifMarker = Encoding.ASCII.GetBytes("JFIF\0");
public static ReadOnlySpan<byte> JFifMarker => new[]
{
(byte)'J', (byte)'F', (byte)'I', (byte)'F', (byte)'\0'
};

/// <summary>
/// Describes the ICC specific markers.
/// Gets the ICC specific markers.
/// </summary>
public static readonly byte[] IccMarker = Encoding.ASCII.GetBytes("ICC_PROFILE\0");
public static ReadOnlySpan<byte> IccMarker => new[]
{
(byte)'I', (byte)'C', (byte)'C', (byte)'_',
(byte)'P', (byte)'R', (byte)'O', (byte)'F',
(byte)'I', (byte)'L', (byte)'E', (byte)'\0'
};

/// <summary>
/// Describes the EXIF specific markers.
/// Gets the EXIF specific markers.
/// </summary>
public static readonly byte[] ExifMarker = Encoding.ASCII.GetBytes("Exif\0\0");
public static ReadOnlySpan<byte> ExifMarker => new[]
{
(byte)'E', (byte)'x', (byte)'i', (byte)'f', (byte)'\0', (byte)'\0'
};

/// <summary>
/// Describes Adobe specific markers <see href="http://www.sno.phy.queensu.ca/~phil/exiftool/TagNames/JPEG.html#Adobe"/>.
/// Gets the Adobe specific markers <see href="http://www.sno.phy.queensu.ca/~phil/exiftool/TagNames/JPEG.html#Adobe"/>.
/// </summary>
public static readonly byte[] AdobeMarker = Encoding.ASCII.GetBytes("Adobe");
public static ReadOnlySpan<byte> AdobeMarker => new[]
{
(byte)'A', (byte)'d', (byte)'o', (byte)'b', (byte)'e'
};

/// <summary>
/// Returns a value indicating whether the passed bytes are a match to the profile identifier.
Expand All @@ -43,4 +56,4 @@ public static bool IsProfile(ReadOnlySpan<byte> bytesToCheck, ReadOnlySpan<byte>
&& bytesToCheck.Slice(0, profileIdentifier.Length).SequenceEqual(profileIdentifier);
}
}
}
}
16 changes: 9 additions & 7 deletions src/ImageSharp/Formats/Jpeg/Components/ZigZag.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,11 @@ internal unsafe struct ZigZag
public fixed byte Data[Size];

/// <summary>
/// Unzig maps from the zigzag ordering to the natural ordering. For example,
/// unzig[3] is the column and row of the fourth element in zigzag order. The
/// value is 16, which means first column (16%8 == 0) and third row (16/8 == 2).
/// Gets the unzigs map, which maps from the zigzag ordering to the natural ordering.
/// For example, unzig[3] is the column and row of the fourth element in zigzag order.
/// The value is 16, which means first column (16%8 == 0) and third row (16/8 == 2).
/// </summary>
private static readonly byte[] Unzig =
new byte[Size]
private static ReadOnlySpan<byte> Unzig => new byte[]
{
0, 1, 8, 16, 9, 2, 3, 10,
17, 24, 32, 25, 18, 11, 4, 5,
Expand Down Expand Up @@ -75,8 +74,11 @@ public byte this[int idx]
public static ZigZag CreateUnzigTable()
{
ZigZag result = default;
byte* unzigPtr = result.Data;
Marshal.Copy(Unzig, 0, (IntPtr)unzigPtr, Size);
ref byte sourceRef = ref MemoryMarshal.GetReference(Unzig);
ref byte destinationRef = ref Unsafe.AsRef<byte>(result.Data);

Unzig.CopyTo(new Span<byte>(result.Data, Size));

Comment thread
Sergio0694 marked this conversation as resolved.
return result;
}

Expand Down
31 changes: 16 additions & 15 deletions src/ImageSharp/Formats/Png/PngConstants.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) Six Labors and contributors.
// Licensed under the Apache License, Version 2.0.

using System;
using System.Collections.Generic;
using System.Text;

Expand Down Expand Up @@ -36,21 +37,6 @@ internal static class PngConstants
/// </summary>
public static readonly IEnumerable<string> FileExtensions = new[] { "png" };

/// <summary>
/// The header bytes identifying a Png.
/// </summary>
public static readonly byte[] HeaderBytes =
{
0x89, // Set the high bit.
0x50, // P
0x4E, // N
0x47, // G
0x0D, // Line ending CRLF
0x0A, // Line ending CRLF
0x1A, // EOF
0x0A // LF
};

/// <summary>
/// The header bytes as a big-endian coded ulong.
/// </summary>
Expand All @@ -77,5 +63,20 @@ internal static class PngConstants
/// The minimum length of a keyword in a text chunk is 1 byte.
/// </summary>
public const int MinTextKeywordLength = 1;

/// <summary>
/// Gets the header bytes identifying a Png.
/// </summary>
public static ReadOnlySpan<byte> HeaderBytes => new byte[]
{
0x89, // Set the high bit.
0x50, // P
0x4E, // N
0x47, // G
0x0D, // Line ending CRLF
0x0A, // Line ending CRLF
0x1A, // EOF
0x0A // LF
};
}
}
2 changes: 1 addition & 1 deletion src/ImageSharp/Formats/Png/PngEncoderCore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ public void Encode<TPixel>(Image<TPixel> image, Stream stream)
QuantizedFrame<TPixel> quantized = PngEncoderOptionsHelpers.CreateQuantizedFrame(this.options, image);
this.bitDepth = PngEncoderOptionsHelpers.CalculateBitDepth(this.options, image, quantized);

stream.Write(PngConstants.HeaderBytes, 0, PngConstants.HeaderBytes.Length);
stream.Write(PngConstants.HeaderBytes);

this.WriteHeaderChunk(stream);
this.WritePaletteChunk(stream, quantized);
Expand Down
29 changes: 23 additions & 6 deletions src/ImageSharp/Formats/Png/Zlib/DeflaterHuffman.cs
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,6 @@ internal sealed unsafe class DeflaterHuffman : IDisposable
// probability, to avoid transmitting the lengths for unused bit length codes.
private static readonly int[] BitLengthOrder = { 16, 17, 18, 0, 8, 7, 9, 6, 10, 5, 11, 4, 12, 3, 13, 2, 14, 1, 15 };

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We might well be better off using the ReadOnlySpan<byte> here also and casting.

I know we do that in the jpeg encoder for uint and BitCountLut with, as I recall, a net gain in performance.

@Sergio0694 Sergio0694 Feb 28, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done in 5b7ac75.
Should I add some bounds check here, or are all those accesses guaranteed to be safe?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I couldn't tell you for certain I'm afraid. That port was an act of desperation over understanding.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I see. Should I just switch those accesses to a direct Span<T>[int] access then? 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I get what you're saying now, yeah 👍
I'm not sure I understand where you're seeing that with Bit4Reverse though.
Here's where that is used:

int toReverseRightShiftBy12 = toReverse >> 12;
Guard.MustBeLessThanOrEqualTo<uint>((uint)toReverseRightShiftBy12, 15, nameof(toReverse));
ref byte bit4ReverseRef = ref MemoryMarshal.GetReference(Bit4Reverse);
return (short)(Unsafe.Add(ref bit4ReverseRef, toReverse & 0xF) << 12
| Unsafe.Add(ref bit4ReverseRef, (toReverse >> 4) & 0xF) << 8
| Unsafe.Add(ref bit4ReverseRef, (toReverse >> 8) & 0xF) << 4
| Unsafe.Add(ref bit4ReverseRef, toReverseRightShiftBy12));

All the indexing values there are int already 🤔
I mean, those are actually the same indices that were being used before as well. I'm not sure I'm following here, where is the byte offsetting peformed?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ahh sorry, I was wrong the values for Bit4Reverse are not used for indexing into data. This is the data that gets indexed.

We should deal with Guard however to make sure we don't regress. Do you plan to fix it globally in SixLabors/SharedInfrastructure, or will you replace it in this PR with a manual check + ThrowHelper calls?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ahahah no problem, glad we could figure this out, I was wondering what was I missing there 😄

As for Guard, I'd say that's an unrelated improvement, since it's not something that will impact the functionality in any way, it's just a matter of further optimizations. I guess the best thing would be to just make another PR and fix the entire Guard class, with all the included APIs. Sounds good?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you can do it, that would be the best!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sure, always happy to bring more of my discoveries and optimizations to ImageSharp! 😄


private static readonly byte[] Bit4Reverse = { 0, 8, 4, 12, 2, 10, 6, 14, 1, 9, 5, 13, 3, 11, 7, 15 };

private static readonly short[] StaticLCodes;
private static readonly byte[] StaticLLength;
private static readonly short[] StaticDCodes;
Expand Down Expand Up @@ -128,6 +126,8 @@ public DeflaterHuffman(MemoryAllocator memoryAllocator)
this.pinnedLiteralBuffer = (short*)this.literalBufferHandle.Pointer;
}

private static ReadOnlySpan<byte> Bit4Reverse => new byte[] { 0, 8, 4, 12, 2, 10, 6, 14, 1, 9, 5, 13, 3, 11, 7, 15 };

/// <summary>
/// Gets the pending buffer to use.
/// </summary>
Expand Down Expand Up @@ -363,10 +363,27 @@ public bool TallyDist(int distance, int length)
[MethodImpl(InliningOptions.ShortMethod)]
public static short BitReverse(int toReverse)
{
return (short)(Bit4Reverse[toReverse & 0xF] << 12
| Bit4Reverse[(toReverse >> 4) & 0xF] << 8
| Bit4Reverse[(toReverse >> 8) & 0xF] << 4
| Bit4Reverse[toReverse >> 12]);
/* Use unsafe offsetting and manually validate the input index to reduce the
* total number of conditional branches. There are two main cases to test here:
* 1. In the first 3, the input value (or some combination of it) is combined
* with & 0xF, which results in a maximum value of 0xF no matter what the
* input value was. That is 15, which is always in range for the target span.
* As a result, no input validation is needed at all in this case.
* 2. There are two cases where the input value might cause an invalid access:
* when it is either negative, or greater than 15 << 12. We can test both
* conditions in a single pass by casting the input value to uint, and checking
* whether that value is lower than 15 << 12. If it was a negative value (2-complement),
* the test will fail as the uint cast will result in a much larger value.
* If the value was simply too high, the test will fail as expected.
* Doing this reduces the total number of index checks from 4 down to just 1. */
Guard.MustBeLessThanOrEqualTo<uint>((uint)toReverse, 15 << 12, nameof(toReverse));

ref byte bit4ReverseRef = ref MemoryMarshal.GetReference(Bit4Reverse);

return (short)(Unsafe.Add(ref bit4ReverseRef, toReverse & 0xF) << 12
| Unsafe.Add(ref bit4ReverseRef, (toReverse >> 4) & 0xF) << 8
| Unsafe.Add(ref bit4ReverseRef, (toReverse >> 8) & 0xF) << 4
| Unsafe.Add(ref bit4ReverseRef, toReverse >> 12));
}

/// <inheritdoc/>
Expand Down
10 changes: 6 additions & 4 deletions src/ImageSharp/Metadata/Profiles/Exif/ExifConstants.cs
Original file line number Diff line number Diff line change
@@ -1,24 +1,26 @@
// Copyright (c) Six Labors and contributors.
// Copyright (c) Six Labors and contributors.
// Licensed under the Apache License, Version 2.0.

using System;

namespace SixLabors.ImageSharp.Metadata.Profiles.Exif
{
internal static class ExifConstants
{
public static readonly byte[] LittleEndianByteOrderMarker =
public static ReadOnlySpan<byte> LittleEndianByteOrderMarker => new byte[]
{
(byte)'I',
(byte)'I',
0x2A,
0x00,
};

public static readonly byte[] BigEndianByteOrderMarker =
public static ReadOnlySpan<byte> BigEndianByteOrderMarker => new byte[]
{
(byte)'M',
(byte)'M',
0x00,
0x2A
};
}
}
}
2 changes: 1 addition & 1 deletion src/ImageSharp/Metadata/Profiles/Exif/ExifWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ public byte[] GetData()
int i = 0;

// The byte order marker for little-endian, followed by the number 42 and a 0
ExifConstants.LittleEndianByteOrderMarker.AsSpan().CopyTo(result.AsSpan(start: i));
ExifConstants.LittleEndianByteOrderMarker.CopyTo(result.AsSpan(start: i));
i += ExifConstants.LittleEndianByteOrderMarker.Length;

uint ifdOffset = ((uint)i - startIndex) + 4U;
Expand Down
Loading