Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
99 changes: 74 additions & 25 deletions src/ImageSharp/Metadata/Profiles/IPTC/IptcProfile.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Text;
using SixLabors.ImageSharp.Metadata.Profiles.IPTC;

namespace SixLabors.ImageSharp.Metadata.Profiles.Iptc
{
Expand All @@ -20,6 +21,16 @@ public sealed class IptcProfile : IDeepCloneable<IptcProfile>

private const uint MaxStandardDataTagSize = 0x7FFF;

/// <summary>
/// 1:90 Coded Character Set.
/// </summary>
private const byte IptcEnvelopeCodedCharacterSet = 0x5A;

/// <summary>
/// This value marks that UTF-8 encoding is used in application records.
/// </summary>
private static readonly byte[] CodedCharacterSetUtf8Value = { 0x1B, 0x25, 0x47 };
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private static readonly byte[] CodedCharacterSetUtf8Value = { 0x1B, 0x25, 0x47 };
// Uses C#'s optimization to refer to the data segment in the assembly direclty, no allocation occurs.
private static ReadOnlySpan<byte> CodedCharacterSetUtf8Value => new byte[]{ 0x1B, 0x25, 0x47 };

And update WriteRecord (L294) to take a ROS.


/// <summary>
/// Initializes a new instance of the <see cref="IptcProfile"/> class.
/// </summary>
Expand Down Expand Up @@ -194,6 +205,17 @@ public void SetValue(IptcTag tag, Encoding encoding, string value, bool strict =
this.values.Add(new IptcValue(tag, encoding, value, strict));
}

/// <summary>
/// Sets the value of the specified tag.
/// </summary>
/// <param name="tag">The tag of the iptc value.</param>
/// <param name="value">The value.</param>
/// <param name="strict">
/// Indicates if length restrictions from the specification should be followed strictly.
/// Defaults to true.
/// </param>
public void SetValue(IptcTag tag, string value, bool strict = true) => this.SetValue(tag, Encoding.UTF8, value, strict);

/// <summary>
/// Makes sure the datetime is formatted according to the iptc specification.
/// <example>
Expand All @@ -219,17 +241,6 @@ public void SetDateTimeValue(IptcTag tag, DateTimeOffset dateTimeOffset)
this.SetValue(tag, Encoding.UTF8, formattedDate);
}

/// <summary>
/// Sets the value of the specified tag.
/// </summary>
/// <param name="tag">The tag of the iptc value.</param>
/// <param name="value">The value.</param>
/// <param name="strict">
/// Indicates if length restrictions from the specification should be followed strictly.
/// Defaults to true.
/// </param>
public void SetValue(IptcTag tag, string value, bool strict = true) => this.SetValue(tag, Encoding.UTF8, value, strict);

/// <summary>
/// Updates the data of the profile.
/// </summary>
Expand All @@ -241,12 +252,25 @@ public void UpdateData()
length += value.Length + 5;
}

bool hasValuesInUtf8 = this.HasValuesInUtf8();

if (hasValuesInUtf8)
{
// Additional length for UTF-8 Tag.
length += 5 + CodedCharacterSetUtf8Value.Length;
Copy link
Contributor

Choose a reason for hiding this comment

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

The cool thing is that with the above suggestion the JIT will treat CodedCharacterSetUtf8Value.Length as constant (3) here.

}

this.Data = new byte[length];
int offset = 0;
if (hasValuesInUtf8)
{
// Write Envelope Record.
offset = this.WriteRecord(offset, CodedCharacterSetUtf8Value, IptcRecordNumber.Envelope, IptcEnvelopeCodedCharacterSet);
}

int i = 0;
foreach (IptcValue value in this.Values)
{
// Standard DataSet Tag
// Write Application Record.
// +-----------+----------------+---------------------------------------------------------------------------------+
// | Octet Pos | Name | Description |
// +==========-+================+=================================================================================+
Expand All @@ -263,17 +287,24 @@ public void UpdateData()
// | | Octet Count | the following data field(32767 or fewer octets). Note that the value of bit 7 of|
// | | | octet 4(most significant bit) always will be 0. |
// +-----------+----------------+---------------------------------------------------------------------------------+
this.Data[i++] = IptcTagMarkerByte;
this.Data[i++] = 2;
this.Data[i++] = (byte)value.Tag;
this.Data[i++] = (byte)(value.Length >> 8);
this.Data[i++] = (byte)value.Length;
if (value.Length > 0)
{
Buffer.BlockCopy(value.ToByteArray(), 0, this.Data, i, value.Length);
i += value.Length;
}
offset = this.WriteRecord(offset, value.ToByteArray(), IptcRecordNumber.Application, (byte)value.Tag);
}
}

private int WriteRecord(int offset, byte[] recordData, IptcRecordNumber recordNumber, byte recordBinaryRepresentation)
{
this.Data[offset++] = IptcTagMarkerByte;
this.Data[offset++] = (byte)recordNumber;
this.Data[offset++] = recordBinaryRepresentation;
this.Data[offset++] = (byte)(recordData.Length >> 8);
this.Data[offset++] = (byte)recordData.Length;
Copy link
Contributor

Choose a reason for hiding this comment

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

Here the JIT needs to introduce a bound check for each access to Data. These can be avoided by

Span<byte> data = this.Data.AsSpan(offset, 5);
data[0] = IptcTagMarkerByte;
data[1] = recordNumber;
data[2] = recordBinaryRepresentation;
data[3] = (byte)(recordData.Length >> 8);
data[4] = (byte)recordData.Length;
offset += 5;

So if the AsSpan succeeds the JIT knows that all index accesses are within bounds, so no bounds check needs to be emitted (tested with .NET 6).

if (recordData.Length > 0)
{
Buffer.BlockCopy(recordData, 0, this.Data, offset, recordData.Length);
Copy link
Contributor

Choose a reason for hiding this comment

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

This will become

Suggested change
Buffer.BlockCopy(recordData, 0, this.Data, offset, recordData.Length);
recordData.CopyTo(this.Data.AsSpan(offset));

offset += recordData.Length;
}

return offset;
}

private void Initialize()
Expand All @@ -298,6 +329,7 @@ private void Initialize()
bool isValidRecordNumber = recordNumber is >= 1 and <= 9;
var tag = (IptcTag)this.Data[offset++];
bool isValidEntry = isValidTagMarker && isValidRecordNumber;
bool isApplicationRecord = recordNumber == (byte)IptcRecordNumber.Application;

uint byteCount = BinaryPrimitives.ReadUInt16BigEndian(this.Data.AsSpan(offset, 2));
offset += 2;
Expand All @@ -307,15 +339,32 @@ private void Initialize()
break;
}

if (isValidEntry && byteCount > 0 && (offset <= this.Data.Length - byteCount))
if (isValidEntry && isApplicationRecord && byteCount > 0 && (offset <= this.Data.Length - byteCount))
{
var iptcData = new byte[byteCount];
byte[] iptcData = new byte[byteCount];
Buffer.BlockCopy(this.Data, offset, iptcData, 0, (int)byteCount);
this.values.Add(new IptcValue(tag, iptcData, false));
}

offset += (int)byteCount;
}
}

/// <summary>
/// Gets if any value has UTF-8 encoding.
/// </summary>
/// <returns>true if any value has UTF-8 encoding.</returns>
private bool HasValuesInUtf8()
{
foreach (IptcValue value in this.values)
{
if (value.Encoding == Encoding.UTF8)
{
return true;
}
}

return false;
}
}
}
21 changes: 21 additions & 0 deletions src/ImageSharp/Metadata/Profiles/IPTC/IptcRecordNumber.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

namespace SixLabors.ImageSharp.Metadata.Profiles.IPTC
{
/// <summary>
/// Enum for the different record types of a IPTC value.
/// </summary>
internal enum IptcRecordNumber : byte
{
/// <summary>
/// A Envelope Record.
/// </summary>
Envelope = 0x01,

/// <summary>
/// A Application Record.
/// </summary>
Application = 0x02
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,10 @@ public void Encode_PreservesIptcProfile()
{
// arrange
using var input = new Image<Rgba32>(1, 1);
input.Metadata.IptcProfile = new IptcProfile();
input.Metadata.IptcProfile.SetValue(IptcTag.Byline, "unit_test");
var expectedProfile = new IptcProfile();
expectedProfile.SetValue(IptcTag.Country, "ESPAÑA");
expectedProfile.SetValue(IptcTag.City, "unit-test-city");
input.Metadata.IptcProfile = expectedProfile;

// act
using var memStream = new MemoryStream();
Expand All @@ -50,7 +52,7 @@ public void Encode_PreservesIptcProfile()
using var output = Image.Load<Rgba32>(memStream);
IptcProfile actual = output.Metadata.IptcProfile;
Assert.NotNull(actual);
IEnumerable<IptcValue> values = input.Metadata.IptcProfile.Values;
IEnumerable<IptcValue> values = expectedProfile.Values;
Assert.Equal(values, actual.Values);
}

Expand Down
20 changes: 18 additions & 2 deletions tests/ImageSharp.Tests/Metadata/Profiles/IPTC/IptcProfileTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ namespace SixLabors.ImageSharp.Tests.Metadata.Profiles.IPTC
{
public class IptcProfileTests
{
private static JpegDecoder JpegDecoder => new JpegDecoder() { IgnoreMetadata = false };
private static JpegDecoder JpegDecoder => new() { IgnoreMetadata = false };

private static TiffDecoder TiffDecoder => new TiffDecoder() { IgnoreMetadata = false };
private static TiffDecoder TiffDecoder => new() { IgnoreMetadata = false };

public static IEnumerable<object[]> AllIptcTags()
{
Expand All @@ -27,6 +27,22 @@ public static IEnumerable<object[]> AllIptcTags()
}
}

[Fact]
public void IptcProfile_WithUtf8Data_WritesEnvelopeRecord_Works()
{
// arrange
var profile = new IptcProfile();
profile.SetValue(IptcTag.City, "ESPAÑA");
profile.UpdateData();
byte[] expectedEnvelopeData = { 28, 1, 90, 0, 3, 27, 37, 71 };

// act
byte[] profileBytes = profile.Data;

// assert
Assert.True(profileBytes.AsSpan(0, 8).SequenceEqual(expectedEnvelopeData));
}

[Theory]
[MemberData(nameof(AllIptcTags))]
public void IptcProfile_SetValue_WithStrictEnabled_Works(IptcTag tag)
Expand Down