Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 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
32 changes: 32 additions & 0 deletions src/ImageSharp/Formats/Jpeg/JpegComData.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

namespace SixLabors.ImageSharp.Formats.Jpeg;

/// <summary>
/// Contains JPEG comment
/// </summary>
public readonly struct JpegComData
{
/// <summary>
/// Converts string to <see cref="JpegComData"/>
/// </summary>
/// <param name="value">The comment string.</param>
/// <returns>The <see cref="JpegComData"/></returns>
public static JpegComData FromString(string value) => new(value.AsMemory());

/// <summary>
/// Initializes a new instance of the <see cref="JpegComData"/> struct.
/// </summary>
/// <param name="value">The comment ReadOnlyMemory of chars.</param>
public JpegComData(ReadOnlyMemory<char> value)
=> this.Value = value;

public ReadOnlyMemory<char> Value { get; }

/// <summary>
/// Converts Value to string
/// </summary>
/// <returns>The comment string.</returns>
public override string ToString() => this.Value.ToString();
}
19 changes: 18 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,22 @@ 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 = new byte[markerContentByteSize];
JpegMetadata metadata = this.Metadata.GetFormatMetadata(JpegFormat.Instance);

stream.Read(temp);
string comment = Encoding.ASCII.GetString(temp);

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're assuming here that the input is always ASCII when we do not actually know that. I would create a char[] cast it to byte[], read the stream and pass it to the JpegComData constructor.


metadata.Comments.Add(JpegComData.FromString(comment));
}

/// <summary>
/// Returns encoded colorspace based on the adobe APP14 marker.
/// </summary>
Expand Down
52 changes: 52 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,54 @@ 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)
{
int maxCommentLength = 65533;

if (metadata.Comments.Count == 0)
{
return;
}

for (int i = 0; i < metadata.Comments.Count; i++)
{
string comment = metadata.Comments[i].ToString();

if (comment.Length > maxCommentLength)
{
string splitComment = comment.Substring(maxCommentLength, comment.Length - maxCommentLength);
metadata.Comments.Insert(i + 1, JpegComData.FromString(splitComment));

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 not be changing the metadata here. Encoding should leave the image unchanged.


// We don't want to keep the extra bytes
comment = comment.Substring(0, maxCommentLength);
}

int commentLength = comment.Length + 4;

Span<byte> commentSpan = new byte[commentLength];

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.

You should be able to avoid the allocation here by using a single buffer of the maximum length plus markers outside of the loop and slicing.

Span<byte> markers = commentSpan.Slice(0, 2);
Span<byte> payloadSize = commentSpan.Slice(2, 2);
Span<byte> payload = commentSpan.Slice(4, comment.Length);

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

// Write payload size
int comWithoutMarker = commentLength - 2;
payloadSize[0] = (byte)((comWithoutMarker >> 8) & 0xFF);
payloadSize[1] = (byte)(comWithoutMarker & 0xFF);

Encoding.ASCII.GetBytes(comment, payload);

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 don't know it's ASCII. I would cast and write.


this.outputStream.Write(commentSpan, 0, commentSpan.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<JpegComData>();
}

/// <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 IList<JpegComData> Comments { get; }

/// <inheritdoc/>
public IDeepCloneable DeepClone() => new JpegMetadata(this);
}
1 change: 1 addition & 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 Down
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.Comments.ElementAtOrDefault(0).ToString());
image.DebugSave(provider);
image.CompareToOriginal(provider);
}

private static void VerifyEncodedStrings(ExifProfile exif)
{
Assert.NotNull(exif);
Expand Down
46 changes: 45 additions & 1 deletion tests/ImageSharp.Tests/Formats/Jpg/JpegEncoderTests.Metadata.cs
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ public void Encode_PreserveRatio(string imagePath, int xResolution, int yResolut
[MemberData(nameof(QualityFiles))]
public void Encode_PreservesQuality(string imagePath, int quality)
{
var testFile = TestFile.Create(imagePath);
TestFile testFile = TestFile.Create(imagePath);
using (Image<Rgba32> input = testFile.CreateRgba32Image())
{
using (var memStream = new MemoryStream())
Expand All @@ -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 Image<TPixel> input = provider.GetImage(JpegDecoder.Instance);
using var memStream = new MemoryStream();

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

// assert
memStream.Position = 0;
using Image<Rgba32> 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.ElementAtOrDefault(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.Comments.Add(JpegComData.FromString("First comment"));
meta.Comments.Add(JpegComData.FromString("Second Comment"));
input.Save(memStream, JpegEncoder);

// assert
memStream.Position = 0;
using Image<Rgba32> output = Image.Load<Rgba32>(memStream);
JpegMetadata actual = output.Metadata.GetJpegMetadata();
Assert.NotEmpty(actual.Comments);
Assert.Equal(2, actual.Comments.Count);
Assert.Equal(meta.Comments.ElementAtOrDefault(0).ToString(), actual.Comments.ElementAtOrDefault(0).ToString());
Assert.Equal(meta.Comments.ElementAtOrDefault(1).ToString(), actual.Comments.ElementAtOrDefault(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<JpegComData>().SequenceEqual(meta.Comments));
}

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

var meta = new JpegMetadata();
meta.Comments.Add(JpegComData.FromString(comment));

Assert.Equal(1, meta.Comments.Count);
Assert.True(expectedCollection.FirstOrDefault() == 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.