Make sure magick.net and imagesharp use the same configuration for webp encoding #1808
Conversation
| NearLossless = 100 | ||
| }; | ||
|
|
||
| this.webpMagick.Settings.SetDefine(MagickFormat.WebP, "exact", false); |
There was a problem hiding this comment.
@dlemstra the exact and quality parameters seem to be missing in WebPWriteDefines. Should they maybe be added to WebPWriteDefines? If you think that makes sense, i could open a PR at Magick.Net.
There was a problem hiding this comment.
Not sure if we need to exact option. You can use this.webpMagick.Settings.Quality to set the quality. Setting the quality too 100 might enable this?
There was a problem hiding this comment.
If i dont set it, the libwebp default will be used, right? The default is false.
There was a problem hiding this comment.
The IMagickSettings interface does not seem to have a Quality Property.
There was a problem hiding this comment.
Sorry my bad, that should be webpMagick.Quality instead. And I just added the Exact property to the WebPWriteDefines. This will become available in the next release. And setting the Quality to 100 will make it lossless.
There was a problem hiding this comment.
Sorry my bad, that should be
webpMagick.Qualityinstead. And I just added theExactproperty to theWebPWriteDefines. This will become available in the next release. And setting theQualityto100will make it lossless.
No problem, thanks for adding exact to WebPWriteDefines
Codecov Report
@@ Coverage Diff @@
## master #1808 +/- ##
==========================================
- Coverage 87.30% 87.30% -0.01%
==========================================
Files 936 936
Lines 48015 48015
Branches 6034 6034
==========================================
- Hits 41921 41920 -1
- Misses 5095 5096 +1
Partials 999 999
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
@brianpopow does this affect the benchmarks? |
As I already said in #1786, the benchmarks there cannot be trusted and I need to re-run them again at the commit we merged webp into master. I first wanted to make sure we agree on the settings here. The settings i have choosen are the default values we are using. Most of them should be also the default settings for libwebp, but i wanted to make it explicit. Maybe we decide to change the defaults at some point. Then the benchmarks should not change. Here are the current benchmark results. Encode: Decode: |
|
I wonder if Magick.NET should be changed to |
|
Thanks @brianpopow I misread, I thought you had reran the benchmarks and were making configuration changes upon the result to ensure that the parameters matched. I was wondering if the changes you had made affected before and after benchmarks for the current state. |
|
@JimBobSquarePants I think the benchmarks in the other PR's are comparable, but to really make sure we should also re-run those with these settings.
If no other benchmark is relying on Q16, we probably should consider changing to Q8. |
Prerequisites
Description
This PR makes sure magick.net and imagesharp use the same configuration for encoding benchmarks, so we comparing the same thing.
Related to #1786