Skip to content
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

Jump to bottom

Add exception type and update xml docs. #1179

Merged
merged 5 commits into from
Apr 22, 2020
Merged

Add exception type and update xml docs. #1179

merged 5 commits into from
Apr 22, 2020

Conversation

JimBobSquarePants
Copy link
Member

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

  • Adds new InvalidImageContentException type.
  • Documents exceptions and adds guards on all loading mechanisms.
  • Documents exceptions and adds guards on all saving mechanisms.
  • Minor cleanup and performance tweaks.

Fixes. #1110

@JimBobSquarePants JimBobSquarePants added this to the 1.0.0-rc1 milestone Apr 21, 2020
@JimBobSquarePants JimBobSquarePants requested a review from a team April 21, 2020 20:01
@codecov
Copy link

codecov bot commented Apr 21, 2020

Codecov Report

Merging #1179 into master will increase coverage by 0.00%.
The diff coverage is 74.54%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1179   +/-   ##
=======================================
  Coverage   82.47%   82.47%           
=======================================
  Files         688      690    +2     
  Lines       29891    29916   +25     
  Branches     3378     3379    +1     
=======================================
+ Hits        24653    24674   +21     
- Misses       4538     4541    +3     
- Partials      700      701    +1
Flag Coverage Δ
#unittests 82.47% <74.54%> (+<0.01%) ⬆️
Impacted Files Coverage Δ
src/ImageSharp/Formats/Bmp/BmpInfoHeader.cs 82.51% <0.00%> (ø)
src/ImageSharp/Formats/Gif/GifDecoderCore.cs 83.76% <0.00%> (ø)
src/ImageSharp/Formats/Gif/GifThrowHelper.cs 0.00% <0.00%> (ø)
...olorConverters/JpegColorConverter.FromYCbCrSimd.cs 86.79% <0.00%> (ø)
...harp/Formats/Jpeg/Components/Decoder/JFifMarker.cs 86.84% <0.00%> (ø)
src/ImageSharp/Formats/Tga/TgaDecoderCore.cs 91.26% <0.00%> (ø)
src/ImageSharp/Formats/Tga/TgaThrowHelper.cs 0.00% <0.00%> (ø)
...harp/Processing/Extensions/ProcessingExtensions.cs 100.00% <ø> (ø)
src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs 94.44% <20.00%> (ø)
src/ImageSharp/Formats/Png/PngThrowHelper.cs 16.66% <20.00%> (ø)
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c25d6c...c32cc6e. Read the comment docs.

antonfirsov
antonfirsov reviewed Apr 21, 2020
View reviewed changes
Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

Great job! Will have another one in a few days.

src/ImageSharp/Image.FromBytes.cs
/// <exception cref="NotSupportedException">Thrown if the stream is not readable.</exception>
/// <exception cref="ArgumentNullException">The configuration is null.</exception>
/// <exception cref="ArgumentNullException">The data is null.</exception>
/// <exception cref="NotSupportedException">The data is not readable.</exception>
/// <returns>
/// The <see cref="IImageInfo"/> or null if suitable info detector is not found.
/// </returns>
public static IImageInfo Identify(Configuration configuration, byte[] data, out IImageFormat format)
Copy link
Member

Choose a reason for hiding this comment

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

Can decoders throw InvalidImageContentException in Identify?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, they can hit a bad value in the image header.

@JimBobSquarePants
Copy link
Member Author

Great job! Will have another one in a few days.

Quick as possible if you can. I'm hoping to ship in less than a week.

@JimBobSquarePants JimBobSquarePants merged commit 6df81b3 into master Apr 22, 2020
@JimBobSquarePants JimBobSquarePants deleted the js/fix-1110 branch April 22, 2020 06:52
@brianpopow
Copy link
Collaborator

@JimBobSquarePants there are still some places left with a TODO: use InvalidImageContentException here, if we decide to define it

For example

ImageSharp/src/ImageSharp/Formats/Gif/GifDecoder.cs

Line 43 in 5639bab

throw new ImageFormatException($"Can not decode image. Failed to allocate buffers for possibly degenerate dimensions: {dims.Width}x{dims.Height}.", ex);
.

Was this intentional or should we change those, too?

@JimBobSquarePants
Copy link
Member Author

@brianpopow I missed that. Yeah we should update.

@brianpopow
Copy link
Collaborator

@brianpopow I missed that. Yeah we should update.

@JimBobSquarePants ok i will do that

@brianpopow brianpopow mentioned this pull request Apr 28, 2020
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antonfirsov antonfirsov antonfirsov left review comments

Assignees
No one assigned
Labels
API
Projects
None yet
Milestone
1.0.0-rc1
Development

Successfully merging this pull request may close these issues.
3 participants
@JimBobSquarePants @brianpopow @antonfirsov