Skip to content

[fix]: Fixed Unknown App0 Marker add profileResolver missing #2566

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.

Sign up for GitHub

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

Merged
JimBobSquarePants merged 4 commits into from
Nov 29, 2023
+16 −4
Merged

[fix]: Fixed Unknown App0 Marker add profileResolver missing #2566

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c7f3aa4
[fix]: Fixed Unknown App0 Marker add profileResolver missing
JoseEliasSantos Oct 19, 2023
412e451
Merge branch 'main' into MakerProfileResolver
JoseEliasSantos Nov 6, 2023
ca1fd46
Merge remote-tracking branch 'upstream/main' into MakerProfileResolver
JimBobSquarePants Nov 29, 2023
48cecd3
Make JFIF marker optional
JimBobSquarePants Nov 29, 2023
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
3 changes: 2 additions & 1 deletion src/ImageSharp/Formats/Jpeg/Components/Decoder/JFifMarker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ public static bool TryParse(ReadOnlySpan<byte> bytes, out JFifMarker marker)
{
// Some images incorrectly use JFXX as the App0 marker (Issue 2478)
if (ProfileResolver.IsProfile(bytes, ProfileResolver.JFifMarker)
|| ProfileResolver.IsProfile(bytes, ProfileResolver.JFxxMarker))
|| ProfileResolver.IsProfile(bytes, ProfileResolver.JFxxMarker)
|| ProfileResolver.IsProfile(bytes, ProfileResolver.OSQidMaker))
Copy link
Member

@JimBobSquarePants JimBobSquarePants Oct 23, 2023

Choose a reason for hiding this comment

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

While this works, I'm not sure it's the correct fix.

I think we might be able to simply remove the throw when the marker doesn't match a JFIF one and skip the remaining bytes.

What is OSQidMaker? Can you please supply more information?

Copy link
Contributor Author

@JoseEliasSantos JoseEliasSantos Oct 25, 2023

Choose a reason for hiding this comment

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

In fact, in my research I didn't find markers that specified such a conversion starting with these bytes, but I have images of a camera that always has these bytes at the beginning. I also had success in all tests when ignoring this if but I don't know what more problems this could cause. I'm running a fork of the lib in my application and I don't get any more errors

Copy link
Member

@JimBobSquarePants JimBobSquarePants Oct 25, 2023

Choose a reason for hiding this comment

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

The JFIF JFXX headers contain metadata and we can actually operate without them. Your images actually contain a JFIF header in an APP0 block just after the camera junk.

I can’t accept the fix as-is because the values are non-standard so we should try a more robust solution as suggested

{
byte majorVersion = bytes[5];
byte minorVersion = bytes[6];
Expand Down
8 changes: 8 additions & 0 deletions src/ImageSharp/Formats/Jpeg/Components/Decoder/ProfileResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,14 @@ internal static class ProfileResolver
(byte)'J', (byte)'F', (byte)'X', (byte)'X', (byte)'\0'
};

/// <summary>
/// Gets the \n[ID or 10 91 73 68 32 specific markers.
/// </summary>
public static ReadOnlySpan<byte> OSQidMaker => new[]
{
(byte)'\n', (byte)'[', (byte)'I', (byte)'D', (byte)' '
};
Copy link
Contributor

@gfoidl gfoidl Oct 25, 2023

Choose a reason for hiding this comment

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

Suggested change
public static ReadOnlySpan<byte> OSQidMaker => new[]
{
(byte)'\n', (byte)'[', (byte)'I', (byte)'D', (byte)' '
};
public static ReadOnlySpan<byte> OSQidMaker => new[] { (byte)'\n', (byte)'[', (byte)'I', (byte)'D', (byte)' ' };

I'd like to have it written as one-liner, because when we are able to use ut8-literals it's easier to find & replace these.

Copy link
Contributor Author

@JoseEliasSantos JoseEliasSantos Oct 25, 2023

Choose a reason for hiding this comment

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

It's a good suggestion, but the other properties of the class already follow this same pattern, so I chose to keep it.


/// <summary>
/// Gets the ICC specific markers.
/// </summary>
Expand Down
11 changes: 11 additions & 0 deletions tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,17 @@ public void Issue2478_DecodeWorks<TPixel>(TestImageProvider<TPixel> provider)
image.CompareToOriginal(provider);
}

// https://github.com/SixLabors/ImageSharp/discussions/2564
[Theory]
[WithFile(TestImages.Jpeg.Issues.Issue2564, PixelTypes.Rgba32)]
public void Issue2564_DecodeWorks<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel>
{
using Image<TPixel> image = provider.GetImage(JpegDecoder.Instance);
image.DebugSave(provider);
image.CompareToOriginal(provider);
}

[Theory]
[WithFile(TestImages.Jpeg.Issues.HangBadScan, PixelTypes.L8)]
public void DecodeHang<TPixel>(TestImageProvider<TPixel> provider)
Expand Down
1 change: 1 addition & 0 deletions tests/ImageSharp.Tests/TestImages.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ public static class Issues
public const string Issue2334_NotEnoughBytesA = "Jpg/issues/issue-2334-a.jpg";
public const string Issue2334_NotEnoughBytesB = "Jpg/issues/issue-2334-b.jpg";
public const string Issue2478_JFXX = "Jpg/issues/issue-2478-jfxx.jpg";
public const string Issue2564 = "Jpg/issues/issue-2564.jpg";
public const string HangBadScan = "Jpg/issues/Hang_C438A851.jpg";

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