Skip to content

Throw NotSupportedException for arithmetic coding and lossless jpeg's #1742

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
merged 6 commits into from
Aug 23, 2021
Merged

Throw NotSupportedException for arithmetic coding and lossless jpeg's #1742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
59a8449
Throw NotSupportedException for arithmetic coding and lossless jpeg's
brianpopow Aug 17, 2021
1ab5185
Add NotSupportedException to the Image.Load overload documentation
brianpopow Aug 17, 2021
27cbafe
Add SOF5 and SOF6 to the unsupported jpeg markers
brianpopow Aug 17, 2021
10b0557
Move arithmetic_progressive.jpg to progressive folder
brianpopow Aug 17, 2021
1000dfb
Add NotSupportedException to the Image.Load FromStream overload docum…
brianpopow Aug 17, 2021
d1fe136
Merge branch 'master' into bp/unsupportedjpegs
brianpopow Aug 22, 2021
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
55 changes: 55 additions & 0 deletions src/ImageSharp/Formats/Jpeg/JpegConstants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,11 @@ internal static class Markers
/// </summary>
public const byte APP15 = 0xEF;

/// <summary>
/// Define arithmetic coding conditioning marker.
/// </summary>
public const byte DAC = 0xCC;

/// <summary>
/// The text comment marker
/// </summary>
Expand Down Expand Up @@ -173,6 +178,56 @@ internal static class Markers
/// </summary>
public const byte SOF2 = 0xC2;

/// <summary>
/// Start of Frame marker, non differential lossless, Huffman coding.
/// </summary>
public const byte SOF3 = 0xC3;

Copy link
Collaborator Author

@brianpopow brianpopow Aug 17, 2021

Choose a reason for hiding this comment

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

note: there are also 0xC5 and 0xC6 StartOfFrame markers for Differential progressive DCT and Differential sequential DCT. I could not find any real life examples of those, so I was unsure if I should add those here too.

Copy link
Contributor

@br3aker br3aker Aug 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Afaik arithmetic coding is not very common (at least for general internet use case) as it's harder to compute and storage gain is not really better than DCT one. Adding yet another rare format just for 'why-not-ness' won't break anything as it's internal or kill the performance. So, why not?

Copy link
Member

@JimBobSquarePants JimBobSquarePants Aug 17, 2021

Choose a reason for hiding this comment

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

If anyone gets bored they could copy implementations of the missing decoder functionality from here.

https://github.com/yigolden/JpegLibrary

They copied a bunch of stuff from us so all fair.

/// <summary>
/// Start of Frame marker, differential, Huffman coding, Differential sequential DCT.
/// </summary>
public const byte SOF5 = 0xC5;

/// <summary>
/// Start of Frame marker, differential, Huffman coding, Differential progressive DCT.
/// </summary>
public const byte SOF6 = 0xC6;

/// <summary>
/// Start of Frame marker, differential lossless, Huffman coding.
/// </summary>
public const byte SOF7 = 0xC7;

/// <summary>
/// Start of Frame marker, non-differential, arithmetic coding, Extended sequential DCT.
/// </summary>
public const byte SOF9 = 0xC9;

/// <summary>
/// Start of Frame marker, non-differential, arithmetic coding, Progressive DCT.
/// </summary>
public const byte SOF10 = 0xCA;

/// <summary>
/// Start of Frame marker, non-differential, arithmetic coding, Lossless (sequential).
/// </summary>
public const byte SOF11 = 0xCB;

/// <summary>
/// Start of Frame marker, differential, arithmetic coding, Differential sequential DCT.
/// </summary>
public const byte SOF13 = 0xCD;

/// <summary>
/// Start of Frame marker, differential, arithmetic coding, Differential progressive DCT.
/// </summary>
public const byte SOF14 = 0xCE;

/// <summary>
/// Start of Frame marker, differential, arithmetic coding, Differential lossless (sequential).
/// </summary>
public const byte SOF15 = 0xCF;

/// <summary>
/// Define Huffman Table(s)
/// <remarks>
Expand Down
26 changes: 26 additions & 0 deletions src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,28 @@ internal void ParseStream(BufferedReadStream stream, HuffmanScanDecoder scanDeco
this.ProcessStartOfFrameMarker(stream, remaining, fileMarker, metadataOnly);
break;

case JpegConstants.Markers.SOF5:
JpegThrowHelper.ThrowNotSupportedException("Decoding jpeg files with differential sequential DCT is not supported.");
break;

case JpegConstants.Markers.SOF6:
JpegThrowHelper.ThrowNotSupportedException("Decoding jpeg files with differential progressive DCT is not supported.");
break;

case JpegConstants.Markers.SOF3:
case JpegConstants.Markers.SOF7:
JpegThrowHelper.ThrowNotSupportedException("Decoding lossless jpeg files is not supported.");
break;

case JpegConstants.Markers.SOF9:
case JpegConstants.Markers.SOF10:
case JpegConstants.Markers.SOF11:
case JpegConstants.Markers.SOF13:
case JpegConstants.Markers.SOF14:
case JpegConstants.Markers.SOF15:
JpegThrowHelper.ThrowNotSupportedException("Decoding jpeg files with arithmetic coding is not supported.");
break;

case JpegConstants.Markers.SOS:
if (!metadataOnly)
{
Expand Down Expand Up @@ -326,6 +348,10 @@ internal void ParseStream(BufferedReadStream stream, HuffmanScanDecoder scanDeco
case JpegConstants.Markers.COM:
stream.Skip(remaining);
break;

case JpegConstants.Markers.DAC:
JpegThrowHelper.ThrowNotSupportedException("Decoding jpeg files with arithmetic coding is not supported.");
break;
}
}

Expand Down
7 changes: 7 additions & 0 deletions src/ImageSharp/Formats/Jpeg/JpegThrowHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,13 @@ namespace SixLabors.ImageSharp.Formats.Jpeg
{
internal static class JpegThrowHelper
{
/// <summary>
/// Cold path optimization for throwing <see cref="NotSupportedException"/>'s.
/// </summary>
/// <param name="errorMessage">The error message for the exception.</param>
[MethodImpl(InliningOptions.ColdPath)]
public static void ThrowNotSupportedException(string errorMessage) => throw new NotSupportedException(errorMessage);
Copy link
Member

@antonfirsov antonfirsov Aug 17, 2021

Choose a reason for hiding this comment

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

Do we throw ImageFormatException or NotSupportedException in other cases we don't support decoding a valid input?

Ideally, we need to make sure to document all exceptions we throw on Image.Load overloads, although I'm less strict on this since I know that the BCL is also full of undocumented exceptions :)

Copy link
Collaborator Author

@brianpopow brianpopow Aug 17, 2021

Choose a reason for hiding this comment

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

we throw ImageFormatException in cases when the image data is not as expected, e.g. not as the specification suggest.
Those cases here the jpeg files are valid, so i think NotSupportedException is the correct Exception to throw.

Copy link
Contributor

@br3aker br3aker Aug 17, 2021

Choose a reason for hiding this comment

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

+1 for NotSupportedException because corrupted image and not supported image are very different scenarios :)

Copy link
Member

@antonfirsov antonfirsov Aug 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Great to see the docs fixed in Image.FromFile.cs and Image.FromBytes.cs ! 👍

I would also change the docs in Image.FromStream.cs:

/// <exception cref="NotSupportedException">The stream is not readable. -or- The image format is not supported.</exception>


/// <summary>
/// Cold path optimization for throwing <see cref="InvalidImageContentException"/>'s.
/// </summary>
Expand Down
Loading