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
1 change: 1 addition & 0 deletions ImageSharp.sln
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "issues", "issues", "{5C9B68
tests\Images\Input\Jpg\issues\issue750-exif-tranform.jpg = tests\Images\Input\Jpg\issues\issue750-exif-tranform.jpg
tests\Images\Input\Jpg\issues\Issue845-Incorrect-Quality99.jpg = tests\Images\Input\Jpg\issues\Issue845-Incorrect-Quality99.jpg
tests\Images\Input\Jpg\issues\issue855-incorrect-colorspace.jpg = tests\Images\Input\Jpg\issues\issue855-incorrect-colorspace.jpg
tests\Images\Input\Jpg\issues\issue-2067-comment.jpg = tests\Images\Input\Jpg\issues\issue-2067-comment.jpg
EndProjectSection
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "fuzz", "fuzz", "{516A3532-6AC2-417B-AD79-9BD5D0D378A0}"
Expand Down
20 changes: 19 additions & 1 deletion src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Buffers.Binary;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Text;
using SixLabors.ImageSharp.Common.Helpers;
using SixLabors.ImageSharp.Formats.Jpeg.Components;
using SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder;
Expand Down Expand Up @@ -481,7 +482,7 @@ internal void ParseStream(BufferedReadStream stream, SpectralConverter spectralC

case JpegConstants.Markers.APP15:
case JpegConstants.Markers.COM:
stream.Skip(markerContentByteSize);
this.ProcessComMarker(stream, markerContentByteSize);
break;

case JpegConstants.Markers.DAC:
Expand Down Expand Up @@ -515,6 +516,23 @@ public void Dispose()
this.scanDecoder = null;
}

/// <summary>
/// Assigns COM marker bytes to comment property
/// </summary>
/// <param name="stream">The input stream.</param>
/// <param name="markerContentByteSize">The remaining bytes in the segment block.</param>
private void ProcessComMarker(BufferedReadStream stream, int markerContentByteSize)
{
Span<byte> temp = stackalloc byte[markerContentByteSize];

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 should put a limit on when to use stackalloc, variable length can be dangerous. See the following for really useful advice.

https://vcsjones.dev/stackalloc/

char[] chars = new char[markerContentByteSize];
JpegMetadata metadata = this.Metadata.GetFormatMetadata(JpegFormat.Instance);

stream.Read(temp);
Encoding.ASCII.GetChars(temp, chars);

metadata.Comments.Add(chars);
}

/// <summary>
/// Returns encoded colorspace based on the adobe APP14 marker.
/// </summary>
Expand Down
45 changes: 45 additions & 0 deletions src/ImageSharp/Formats/Jpeg/JpegEncoderCore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#nullable disable

using System.Buffers.Binary;
using System.Text;
using SixLabors.ImageSharp.Common.Helpers;
using SixLabors.ImageSharp.Formats.Jpeg.Components;
using SixLabors.ImageSharp.Formats.Jpeg.Components.Encoder;
Expand Down Expand Up @@ -89,6 +90,9 @@ public void Encode<TPixel>(Image<TPixel> image, Stream stream, CancellationToken
// Write Exif, XMP, ICC and IPTC profiles
this.WriteProfiles(metadata, buffer);

// Write comments
this.WriteComment(jpegMetadata);

// Write the image dimensions.
this.WriteStartOfFrame(image.Width, image.Height, frameConfig, buffer);

Expand Down Expand Up @@ -167,6 +171,47 @@ private void WriteJfifApplicationHeader(ImageMetadata meta, Span<byte> buffer)
this.outputStream.Write(buffer, 0, 18);
}

/// <summary>
/// Writes comment
/// </summary>
/// <param name="metadata">The image metadata.</param>
private void WriteComment(JpegMetadata metadata)
{
if (metadata.Comments is { Count: 0 })

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can't we just use metadata.Comments.Length == 0?

{
return;
}

// Length (comment strings lengths) + (comments markers with payload sizes)
int commentsBytes = metadata.Comments.Sum(x => x.Length) + (metadata.Comments.Count * 4);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm against using LINQ at all. While this won't slow down jpeg codec overall it's slower than calculating sum yourself :)

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 shouldn't use Linq in the decoders/encoders. We also need to ensure the length of the comment does not exceed the maximum.

@br3aker br3aker Jan 24, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah yes, too big comment data must be scattered amongst multiple markers if I recall correctly.

int commentStart = 0;
Span<byte> commentBuffer = stackalloc byte[commentsBytes];

foreach (Memory<char> comment in metadata.Comments)
{
int totalComLength = comment.Length + 4;

Span<byte> commentData = commentBuffer.Slice(commentStart, totalComLength);
Span<byte> markers = commentData.Slice(0, 2);
Span<byte> payloadSize = commentData.Slice(2, 2);
Span<byte> payload = commentData.Slice(4, comment.Length);

// Beginning of comment ff fe
markers[0] = JpegConstants.Markers.XFF;
markers[1] = JpegConstants.Markers.COM;

// Write payload size
BinaryPrimitives.WriteInt16BigEndian(payloadSize, (short)(commentData.Length - 2));

Encoding.ASCII.GetBytes(comment.Span, payload);

// Indicate begin of next comment in buffer
commentStart += totalComLength;
}

this.outputStream.Write(commentBuffer, 0, commentBuffer.Length);
}

/// <summary>
/// Writes the Define Huffman Table marker and tables.
/// </summary>
Expand Down
7 changes: 7 additions & 0 deletions src/ImageSharp/Formats/Jpeg/JpegMetadata.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ public class JpegMetadata : IDeepCloneable
/// </summary>
public JpegMetadata()
{
this.Comments = new List<Memory<char>>();
}

/// <summary>
Expand All @@ -25,6 +26,7 @@ private JpegMetadata(JpegMetadata other)
{
this.ColorType = other.ColorType;

this.Comments = other.Comments;
this.LuminanceQuality = other.LuminanceQuality;
this.ChrominanceQuality = other.ChrominanceQuality;
}
Expand Down Expand Up @@ -101,6 +103,11 @@ public int Quality
/// </remarks>
public bool? Progressive { get; internal set; }

/// <summary>
/// Gets the comments.
/// </summary>
public ICollection<Memory<char>>? Comments { get; }

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 don't think this should be nullable. Collections should always have a backing value in a publi API. I would use IList also to prevent all the casting and maintain consistency with the PngMetadata.TextDataProperty

On that note we could use a specific struct JpegComData which would contain the backing memory and can have static FromString(string value)

public readonly struct JpegComData
{
	public static JpegComData FromString(string value) => new(value.AsMemory());

	public JpegComData(ReadOnlyMemory<char> value) 
		=> this.Value = value;
	
	
	public ReadOnlyMemory<char> Value { get; }

	public override string ToString() => this.Value.ToString();
}

This would also remove the need for any extension methods.

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.

@br3aker What do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi!
I'm in favor of an empty collection instead of a null one - it's better from a user perspective.
Not so sure about JpegComData, do we really need to expose comments as ReadOnlyMemory<char> instead of a plain string?

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 thought we couldn't guarantee that the data was always an ASCII string and that's why we had a collection of char?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I thought we couldn't guarantee that the data was always an ASCII string and that's why we had a collection of char?

True, if I recall correctly jpeg specification doesn't specify anything about comment section contents.
Why would we need FromString(string value) method though?

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.

Sorry @br3aker I was confinced I'd replied to this!!

I'm just covering the most common use case, ASCII strings, in a nice manner. Saves using String.AsMemory() which feels a little clunky.


/// <inheritdoc/>
public IDeepCloneable DeepClone() => new JpegMetadata(this);
}
34 changes: 34 additions & 0 deletions src/ImageSharp/Formats/Jpeg/MetadataExtensions.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

using System.Text;
using SixLabors.ImageSharp.Formats.Jpeg;
using SixLabors.ImageSharp.Metadata;

Expand All @@ -17,4 +18,37 @@ public static partial class MetadataExtensions
/// <param name="metadata">The metadata this method extends.</param>
/// <returns>The <see cref="JpegMetadata"/>.</returns>
public static JpegMetadata GetJpegMetadata(this ImageMetadata metadata) => metadata.GetFormatMetadata(JpegFormat.Instance);

/// <summary>
/// Sets the comment in <see cref="JpegMetadata"/>
/// </summary>
/// <param name="metadata">The metadata this method extends.</param>
/// <param name="index">The index of comment to be inserted to.</param>
/// <param name="comment">The comment string.</param>
public static void SetComment(this JpegMetadata metadata, int index, string comment)

@br3aker br3aker Jan 24, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we really need such methods if comments are exposed as IList?
IList has all these methods so users can do whatever they want.

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.

My thoughts exactly.

{
if (metadata.Comments == null)
{
return;
}

ASCIIEncoding encoding = new();
byte[] bytes = encoding.GetBytes(comment);
List<Memory<char>>? comments = metadata.Comments as List<Memory<char>>;
comments?.Insert(index, encoding.GetChars(bytes));
}

/// <summary>
/// Gets the comments from <see cref="JpegMetadata"/>
/// </summary>
/// <param name="metadata">The metadata this method extends.</param>
/// <param name="index">The index of comment.</param>
/// <returns>The IEnumerable string of comments.</returns>
public static string? GetComment(this JpegMetadata metadata, int index) => metadata.Comments?.ElementAtOrDefault(index).ToString();

/// <summary>
/// Clears comments
/// </summary>
/// <param name="metadata">The <see cref="JpegMetadata"/>.</param>
public static void ClearComments(this JpegMetadata metadata) => metadata.Comments?.Clear();
}
15 changes: 15 additions & 0 deletions tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Metadata.cs
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,21 @@ public void EncodedStringTags_Read()
VerifyEncodedStrings(exif);
}

[Theory]
[WithFile(TestImages.Jpeg.Issues.Issue2067_CommentMarker, PixelTypes.Rgba32)]
public void JpegDecoder_DecodeMetadataComment<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel>
{
string expectedComment = "TEST COMMENT";
using Image<TPixel> image = provider.GetImage(JpegDecoder.Instance);
JpegMetadata metadata = image.Metadata.GetJpegMetadata();

Assert.Equal(1, metadata.Comments?.Count);
Assert.Equal(expectedComment, metadata.GetComment(0));
image.DebugSave(provider);
image.CompareToOriginal(provider);
}

private static void VerifyEncodedStrings(ExifProfile exif)
{
Assert.NotNull(exif);
Expand Down
44 changes: 44 additions & 0 deletions tests/ImageSharp.Tests/Formats/Jpg/JpegEncoderTests.Metadata.cs
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,50 @@ public void Encode_PreservesQuality(string imagePath, int quality)
}
}

[Theory]
[WithFile(TestImages.Jpeg.Issues.Issue2067_CommentMarker, PixelTypes.Rgba32)]
public void Encode_PreservesComments<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel>
{
// arrange
using var input = provider.GetImage(JpegDecoder.Instance);

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.

Please favour explicit types and target typed new expressions

using var memStream = new MemoryStream();

// act
input.Save(memStream, JpegEncoder);

// assert
memStream.Position = 0;
using var output = Image.Load<Rgba32>(memStream);
JpegMetadata actual = output.Metadata.GetJpegMetadata();
Assert.NotEmpty(actual.Comments);
Assert.Equal(1, actual.Comments.Count);
Assert.Equal("TEST COMMENT", actual.Comments.ElementAt(0).ToString());
}

[Fact]
public void Encode_SavesMultipleComments()
{
// arrange
using var input = new Image<Rgba32>(1, 1);
JpegMetadata meta = input.Metadata.GetJpegMetadata();
using var memStream = new MemoryStream();

// act
meta.SetComment(0, "First comment");
meta.SetComment(1, "Second Comment");
input.Save(memStream, JpegEncoder);

// assert
memStream.Position = 0;
using var output = Image.Load<Rgba32>(memStream);
JpegMetadata actual = output.Metadata.GetJpegMetadata();
Assert.NotEmpty(actual.Comments);
Assert.Equal(2, actual.Comments?.Count);
Assert.Equal(meta.Comments?.ElementAt(0).ToString(), actual.Comments?.ElementAt(0).ToString());
Assert.Equal(meta.Comments?.ElementAt(1).ToString(), actual.Comments?.ElementAt(1).ToString());
}

[Theory]
[WithFile(TestImages.Jpeg.Baseline.Floorplan, PixelTypes.Rgb24, JpegEncodingColor.Luminance)]
[WithFile(TestImages.Jpeg.Baseline.Jpeg444, PixelTypes.Rgb24, JpegEncodingColor.YCbCrRatio444)]
Expand Down
22 changes: 22 additions & 0 deletions tests/ImageSharp.Tests/Formats/Jpg/JpegMetadataTests.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

using System.Collections.ObjectModel;
using SixLabors.ImageSharp.Formats.Jpeg;

namespace SixLabors.ImageSharp.Tests.Formats.Jpg;
Expand Down Expand Up @@ -57,4 +58,25 @@ public void Quality_ReturnsMaxQuality()

Assert.Equal(meta.Quality, qualityLuma);
}

[Fact]
public void Comment_EmptyComment()
{
var meta = new JpegMetadata();

Assert.True(Array.Empty<Memory<char>>().SequenceEqual(meta.Comments));
}

[Fact]
public void Comment_OnlyComment()
{
string comment = "test comment";
var expectedCollection = new Collection<Memory<char>> { new(comment.ToCharArray()) };

var meta = new JpegMetadata();
meta.Comments?.Add(comment.ToCharArray());

Assert.Equal(1, meta.Comments?.Count);
Assert.True(expectedCollection.FirstOrDefault().ToString() == meta.Comments?.FirstOrDefault().ToString());
}
}
1 change: 1 addition & 0 deletions tests/ImageSharp.Tests/TestImages.cs
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,7 @@ public static class Issues
public const string Issue2564 = "Jpg/issues/issue-2564.jpg";
public const string HangBadScan = "Jpg/issues/Hang_C438A851.jpg";
public const string Issue2517 = "Jpg/issues/issue2517-bad-d7.jpg";
public const string Issue2067_CommentMarker = "Jpg/issues/issue-2067-comment.jpg";

public static class Fuzz
{
Expand Down
3 changes: 3 additions & 0 deletions tests/Images/Input/Jpg/issues/issue-2067-comment.jpg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.