Refactored the decoder and encoder options. by dlemstra · Pull Request #113 · SixLabors/ImageSharp

dlemstra · 2017-02-21T21:20:46Z

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 practise as demonstrated in the repository. These follow strict Stylecop rules 👮.
I have provided test coverage for my change (where applicable)

Description

This pull requests adds the features requested in #26:

Create Options classes to pass to the encoders and decoders to reduce parameter count.
Create overload for Image<TColor, TPacked> that allows passing a specific decoder or decoders with options.

This was already changed in my previous pull request (#99):

Remove Quality property from Image<TColor, TPacked>

This pull request also adds support for skipping reading/writing metadata as requested in #11.

Fixes #26
Fixes #11

codecov-io · 2017-02-21T21:34:01Z

Codecov Report

Merging #113 into master will increase coverage by 0.07%.
The diff coverage is 88.69%.

@@            Coverage Diff            @@
##           master    #113      +/-   ##
=========================================
+ Coverage   88.12%   88.2%   +0.07%     
=========================================
  Files         429     441      +12     
  Lines       20042   20364     +322     
  Branches     1431    1448      +17     
=========================================
+ Hits        17663   17963     +300     
- Misses       1972    1995      +23     
+ Partials      407     406       -1

Impacted Files	Coverage Δ
src/ImageSharp.Formats.Bmp/BmpDecoder.cs	`100% <ø> (ø)`	✅
...ts/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs	`100% <ø> (ø)`
...Sharp.Tests/Formats/Jpg/JpegProfilingBenchmarks.cs	`0% <ø> (ø)`	✅
tests/ImageSharp.Tests/TestFile.cs	`94.82% <100%> (+0.28%)`	✅
src/ImageSharp.Formats.Png/PngDecoderOptions.cs	`100% <100%> (ø)`
src/ImageSharp.Formats.Gif/GifEncoder.cs	`100% <100%> (ø)`	✅
...harp.Tests/TestUtilities/ImagingTestCaseUtility.cs	`95.23% <100%> (ø)`	✅
src/ImageSharp.Formats.Png/ImageExtensions.cs	`100% <100%> (ø)`	✅
...s/ImageSharp.Tests/Formats/Jpg/JpegEncoderTests.cs	`100% <100%> (ø)`	✅
src/ImageSharp.Formats.Gif/GifDecoder.cs	`100% <100%> (ø)`	✅
... and 44 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 b334834...ffe7f96. Read the comment docs.

JimBobSquarePants

This is a great addition, really cleaning the the mess that was the codec options!

Just a few questions and a minor XML comment change.

JimBobSquarePants · 2017-02-21T22:17:52Z

+        /// <returns>The options for the <see cref="BmpEncoder"/>.</returns>
+        internal static IBmpEncoderOptions Create(IEncoderOptions options)
+        {
+            IBmpEncoderOptions bmpOptions = options as IBmpEncoderOptions;


Couldn't this be

return options as IBmpEncoderOptions ?? new BmpEncoderOptions(options);

Yeah I could and should have done that.

JimBobSquarePants · 2017-02-21T22:18:44Z

+        /// <summary>
+        /// Gets the default encoding to use when reading comments.
+        /// </summary>
+        public static Encoding DefaultEncoding { get; } = Encoding.GetEncoding("ASCII");


Amazed that the specs don't restrict this. Good catch.

JimBobSquarePants · 2017-02-21T22:20:25Z

+        /// </summary>
+        /// <param name="options">The decoder options.</param>
+        public GifDecoderCore(IGifDecoderOptions options)
+        {


When is it possible to call new GifDecoderCore(null) ?

This will happen when you call the constructor of Image without options (options=null). The GifDecoder will just forward the value.

JimBobSquarePants · 2017-02-21T22:22:27Z

+        /// </summary>
+        /// <typeparam name="TColor">The pixel format.</typeparam>
+        /// <param name="image">The <see cref="ImageBase{TColor}"/> to be encoded.</param>
+        /// <param name="writer">The stream to write to.</param>


Nice addition!

JimBobSquarePants · 2017-02-21T22:24:03Z

+        /// <summary>
+        /// Gets or sets the transparency threshold.
+        /// </summary>
+        public byte Threshold { get; set; } = 128;


Thinking about that we should probably reduce the value now we have dithering. Our output should be much cleaner. Do you know of any other encoders that offer this option? We should follow the most common default.

I can take a look at what we do inside ImageMagick but I think we always use dithering.

JimBobSquarePants · 2017-02-21T22:25:02Z

-                        => source.Save(stream, new GifEncoder { Quality = quality });
+        {
+            GifEncoder encoder = new GifEncoder();
+            encoder.Encode(source, stream, options);


Well that answers my question earlier 😄

JimBobSquarePants · 2017-02-21T22:27:05Z

-        /// <value>The quality of the jpg image from 0 to 100.</value>
-        public int Quality
+        /// <inheritdoc/>
+        public void Encode<TColor>(Image<TColor> image, Stream stream, IEncoderOptions options)


So much cleaner!

JimBobSquarePants · 2017-02-21T22:28:43Z

-            }
+            quality = quality.Clamp(1, 100);
+
+            this.outputStream = stream;


I think we have that marked as 80 and below for 420 in the quality XML comments, 91 is the correct value for switching so we should say 90.

I updated the comments.

JimBobSquarePants · 2017-02-21T22:29:15Z

+        /// index must be between 0 and 100 (compression from max to min).
+        /// </summary>
+        /// <remarks>
+        /// If the quality is less than or equal to 80, the subsampling ratio will switch to <see cref="JpegSubsample.Ratio420"/>


Yup, should be 90 here.

Fixed and removed the remarks from the interface.

JimBobSquarePants · 2017-02-21T22:31:56Z

            }

-            if (this.Quantizer == null)
+            if (this.quantizer == null)


Is this something we should be handling in the options?

Yeah IPngEncoderOptions.Quantizer is not used in PngEncoderCore!

It looks like a mistake that I forgot to use the quantizer from the options. Thanks @antonfirsov! I could not set it inside the options due to the {TColor} restriction.

antonfirsov · 2017-02-22T00:33:20Z

Well done, seems OK to me!

tocsoft · 2017-02-22T07:43:42Z

My only concern is the usage of optional arguments.

We shouldn't be using them on out public API's it makes them a lot harder to version in the future. See this blog post for details http://haacked.com/archive/2010/08/10/versioning-issues-with-optional-arguments.aspx/

I understand there are more instances of this thru-out the codebase so its probably not worth bothering to fix it in this PR and we should instead creating a separate issue/PR fixing them all.

Apart from the optional arguments this looks good to me.

dlemstra · 2017-02-22T08:27:04Z

I also prefer to use overloads instead of optional arguments. The current code base already uses a lot of optional arguments so that is why I used that. I can change it inside this pull request to use overloads instead of optional arguments. Your thought @JimBobSquarePants?

JimBobSquarePants · 2017-02-22T08:28:04Z

@dlemstra @tocsoft We probably should change any to not use them. I've actually been burnt by that issue in the past.

dlemstra · 2017-02-22T17:13:06Z

Just pushed the last change. I think we are ready to merge.

tocsoft

much better with the overloads instead of the optional args. 👍

dlemstra · 2017-02-22T17:20:27Z

We should probably create a new issue/pull request to remove all the optional arguments.

tocsoft · 2017-02-22T17:43:38Z

I've added an issue to track the required work.

JimBobSquarePants · 2017-02-22T21:27:08Z

Great. @dlemstra do you want to do the honours?

Dirk Lemstra added 22 commits February 21, 2017 22:11

Added options for the decoder.

614aa59

Implemented the IDecoderOptions inside the gif decoder.

984027b

Implemented the IDecoderOptions inside the jpeg decoder.

c165183

Use constructor instead of property to get the default decoder options.

86a77fa

Implemented the IDecoderOptions inside the png decoder.

683bd87

Also allow specification of the text encoding inside the gif decoder.

339319c

StyleCop fix.

8fada14

Added options for the encoder.

302d62a

Added overloads to the decoder that use the coder specific options.

5d00464

Nicer initialization of the options.

96482d8

Minor comment change.

11536b0

Added extra overload for TestFile.CreateImage and used it in the tests.

c2d2fff

Added options argument to the filePath overloads.

8aa5202

Implemented the IEncoderOptions inside the bmp encoder.

769a413

Comment fixes.

7d4221d

Options should be readonly.

2fe2405

Implemented the IEncoderOptions inside the gif encoder.

9700b3b

Build fixes due to re-base.

0c28b31

Added extra test for trimming comments.

42c9f93

Implemented the IEncoderOptions inside the jpeg encoder.

5510346

Renamed test classes.

0314cef

Implemented the IEncoderOptions inside the png encoder.

fda87b2

dlemstra requested review from JimBobSquarePants, antonfirsov, olivif and tocsoft February 21, 2017 21:35

JimBobSquarePants reviewed Feb 21, 2017

View reviewed changes

JimBobSquarePants added the area:formats label Feb 22, 2017

JimBobSquarePants added this to the Beta 1 milestone Feb 22, 2017

Dirk Lemstra added 3 commits February 22, 2017 17:24

Refactored the Create method for the encoder/decoder options.

17aa888

Fixed text in comments.

04d3b13

Added missing initialization of the quantizer field.

2b74ab7

dlemstra force-pushed the decoder-encoder-options branch from 9db6823 to a739109 Compare February 22, 2017 17:14

tocsoft approved these changes Feb 22, 2017

View reviewed changes

Changed optional arguments into overloads.

ffe7f96

dlemstra force-pushed the decoder-encoder-options branch from a739109 to ffe7f96 Compare February 22, 2017 17:19

tocsoft mentioned this pull request Feb 22, 2017

Migrate all usages of optional method arguments to overloads #114

Closed

dlemstra merged commit 0853c66 into SixLabors:master Feb 22, 2017

Conversation

dlemstra commented Feb 21, 2017

Prerequisites

Description

Uh oh!

codecov-io commented Feb 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JimBobSquarePants left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlemstra Feb 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov commented Feb 22, 2017

Uh oh!

tocsoft commented Feb 22, 2017

Uh oh!

dlemstra commented Feb 22, 2017

Uh oh!

JimBobSquarePants commented Feb 22, 2017

Uh oh!

dlemstra commented Feb 22, 2017

Uh oh!

tocsoft left a comment

Choose a reason for hiding this comment

Uh oh!

dlemstra commented Feb 22, 2017

Uh oh!

tocsoft commented Feb 22, 2017

Uh oh!

JimBobSquarePants commented Feb 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Feb 21, 2017 •

edited

Loading

dlemstra Feb 22, 2017 •

edited

Loading

JimBobSquarePants commented Feb 22, 2017 •

edited

Loading