-
-
Notifications
You must be signed in to change notification settings - Fork 896
Add JPEG COM marker support #2641
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.
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
Changes from 5 commits
bb9ed65
b3a8452
d616590
e78db37
e259e35
5127202
9260be9
dc0484e
c10863f
d9169c5
d225128
6542476
8af9a80
13625ce
a78aab1
d8484da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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); | ||
|
|
||
|
|
@@ -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 }) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we just use |
||
| { | ||
| return; | ||
| } | ||
|
|
||
| // Length (comment strings lengths) + (comments markers with payload sizes) | ||
| int commentsBytes = metadata.Comments.Sum(x => x.Length) + (metadata.Comments.Count * 4); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ public class JpegMetadata : IDeepCloneable | |
| /// </summary> | ||
| public JpegMetadata() | ||
| { | ||
| this.Comments = new List<Memory<char>>(); | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -25,6 +26,7 @@ private JpegMetadata(JpegMetadata other) | |
| { | ||
| this.ColorType = other.ColorType; | ||
|
|
||
| this.Comments = other.Comments; | ||
| this.LuminanceQuality = other.LuminanceQuality; | ||
| this.ChrominanceQuality = other.ChrominanceQuality; | ||
| } | ||
|
|
@@ -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; } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 On that note we could use a specific struct 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @br3aker What do you think?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
True, if I recall correctly jpeg specification doesn't specify anything about comment section contents.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
|
||
| /// <inheritdoc/> | ||
| public IDeepCloneable DeepClone() => new JpegMetadata(this); | ||
| } | ||
| 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; | ||
|
|
||
|
|
@@ -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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need such methods if comments are exposed as
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)] | ||
|
|
||
There was a problem hiding this comment.
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/