Added: ability to skip unneeded chunks for optimization mode #1012
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1012 +/- ##
==========================================
+ Coverage 82.58% 82.61% +0.02%
==========================================
Files 694 694
Lines 30047 30085 +38
Branches 3397 3409 +12
==========================================
+ Hits 24814 24854 +40
Misses 4535 4535
+ Partials 698 696 -2
Continue to review full report at Codecov.
|
|
Good idea! I think we should make our chunk preservation more granular though, perhaps an enum so we can choose whether to preserve exif? Incidentally deleting the relevant metadata will achieve e same result, but this could be a nice shortcut |
|
@JimBobSquarePants split up optimization into a flagged enum so should let users choose what they would like to do + fixes |
|
In decoders, we have an |
There was a problem hiding this comment.
The most critical issue is the lack of test coverage for the functionality introduced by the PR. We need to define positive & negative tests for all the flags.
I'm also suggesting a different definition & naming for the flag semantics, but we also need @JimBobSquarePants 's approval for that.
| /// Provides enumeration of available PNG optimization methods. | ||
| /// </summary> | ||
| [Flags] | ||
| public enum PngOptimizeMethod |
There was a problem hiding this comment.
As I mentioned on gitter, it's better if our naming should reflect functional semantics rather than a specific use case (optimization). In that case "positive" flags may be easier than negative ones expressing the suppression. "positive" flags may be better than negative ones, but I'm not sure about this, need @JimBobSquarePants 's advice.
Suggestion Positive flags:
public enum PngChunkFilter
{
ExcludeAll = 0,
IncludePhysicalChunk = 1 << 0,
IncludeGammaChunk = 1 << 1,
IncludeExifChunk = 1 << 2,
IncludeTextChunks = 1 << 3,
// Default:
IncludeAll = IncludePhysicalChunk | IncludeGammaChunk | IncludeExifChunk | IncludeTextChunks
}Negative flags:
public enum PngChunkFilter
{
// Default:
IncludeAll = 0,
ExcludePhysicalChunk = 1 << 0,
ExcludeGammaChunk = 1 << 1,
ExcludeExifChunk = 1 << 2,
ExcludeTextChunks = 1 << 3,
ExcludeAll = ExcludePhysicalChunk | ExcludeGammaChunk | ExcludeExifChunk | ExcludeTextChunks
}
There was a problem hiding this comment.
I much prefer the suggested naming definition.
The overall metadata API is designed to preserve as much metadata as possible so I think we should adopt the negative flags. This to me as a consumer would cognitively reflect that intention as I'm building a rule for what to exclude.
There was a problem hiding this comment.
this is changed now to negative flags
| /// <summary> | ||
| /// Make funlly transparent pixels black. | ||
| /// </summary> | ||
| MakeTransparentBlack = 16, |
There was a problem hiding this comment.
This one is really different from the other ones, I would rather place this one into a separate property inside PngEncoder.
There was a problem hiding this comment.
Agreed. It doesn't seem to fit. Is the naming accurate also? It's not black, it's empty.
There was a problem hiding this comment.
i think MakeTransparentBlack is clear in what it does, but im open for other suggestions
| /// <summary> | ||
| /// All possible optimizations. | ||
| /// </summary> | ||
| All = 31, |
There was a problem hiding this comment.
Defining the union of the flags manually is more prone to mistakes when changed in future.
# Conflicts: # src/ImageSharp/Formats/Png/PngEncoderCore.cs # tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs
|
I like the idea, i want to continue on this. |
| /// <summary> | ||
| /// Enum indicating how the transparency should be handled on encoding. | ||
| /// </summary> | ||
| public enum PngTransparentColorBehavior |
There was a problem hiding this comment.
My bad, let's call it PngTransparentColorMode to keep it in line with othernaming.
5 participants
Prerequisites
Description
This part of work on creating an alternative to TinyPNG / PNGCrush using ImageSharp.