Throw NotSupportedException for arithmetic coding and lossless jpeg's #1742
Conversation
| /// Start of Frame marker, non differential lossless, Huffman coding. | ||
| /// </summary> | ||
| public const byte SOF3 = 0xC3; | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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> | ||
| /// <param name="errorMessage">The error message for the exception.</param> | ||
| [MethodImpl(InliningOptions.ColdPath)] | ||
| public static void ThrowNotSupportedException(string errorMessage) => throw new NotSupportedException(errorMessage); |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
+1 for NotSupportedException because corrupted image and not supported image are very different scenarios :)
There was a problem hiding this comment.
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>
|
|
||
| [Theory] | ||
| [WithFile(TestImages.Jpeg.Baseline.ArithmeticCoding, PixelTypes.Rgba32)] | ||
| [WithFile(TestImages.Jpeg.Baseline.ArithmeticCodingProgressive, PixelTypes.Rgba32)] |
There was a problem hiding this comment.
Progressive image should be placed to Jpeg/Progressive folder.
Codecov Report
@@ Coverage Diff @@
## master #1742 +/- ##
==========================================
- Coverage 84.46% 84.44% -0.02%
==========================================
Files 836 836
Lines 36737 36748 +11
Branches 4306 4306
==========================================
+ Hits 31029 31032 +3
- Misses 4876 4883 +7
- Partials 832 833 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Prerequisites
Description
Currently the jpeg decoder will fail with an object null exception with jpegs which are not supported yet.
This PR changes the jpeg decoder to throw a NotSupportedException instead.