Switch all resize tests to bicubic interpolation#17
Conversation
| // until you write to an output file, buffer or memory | ||
| var memory = resized.WriteToMemory(); | ||
|
|
||
| return (resized.Width, resized.Height, memory); |
There was a problem hiding this comment.
Note that I returned the memory here to avoid dead code elimination: https://benchmarkdotnet.org/articles/guides/good-practices.html#avoid-dead-code-elimination
There was a problem hiding this comment.
This isn't necessary in this instance, because the JIT has no way of knowing whether WriteToMemory mutates resized. The code will always run, whether you capture the result in a variable or not -- or return it or not.
|
Huh the benchmarks look nice Kleis. It's great you get the same time for NetVips on linux and win now. |
|
@jcupitt This is the 'simple' resize benchmark results, here are the 'Load, Resize, Save' benchmark results: 'Load, Resize, Save' benchmark results on Linux'Load, Resize, Save' benchmark results on WindowsNetVips is still about 1.5 times slower on Windows than on Linux, but maybe we can't do much about it. The decrease in performance on Windows is also noticeable on the other image processing benchmarks. Note that this is without the 'fair-competition patch' (YCbCr vs RGB – Catmull-Rom vs Lanczos3) mentioned in #15, that's why MagicScaler differs from the rest. |
|
Ah, interesting, I wondered if file IO was in there too. I did notice a small performance issue yesterday with libpng read in libvips -- it's doing many more syscalls than it should. I don't suppose this is using libpng? |
|
Only jpeg images are tested with this benchmark, so this is using libjpeg-turbo. Regarding libpng, you might also be interested in this issue: lovell/sharp-libvips#25. See the relevant benchmarks: https://gist.github.com/kleisauke/879e3075a675c6f945dff3772d2ef0a3. Hopefully libspng will stabilize soon, it should be up to 35% faster than libpng(!). |
| using (var image = new ImageSharpImage(Width, Height)) | ||
| { | ||
| image.Mutate(i => i.Resize(ResizedWidth, ResizedHeight)); | ||
| image.Mutate(i => i.Resize(ResizedWidth, ResizedHeight, KnownResamplers.Bicubic)); |
There was a problem hiding this comment.
I'm a little uncomfortable with this and I'd prefer to hear @JimBobSquarePants ' opinion on this line change: I'm all in favor of comparing oranges to oranges, but we're comparing images that will have some aesthetic differences in the end even if we use bicubic everywhere. Benchmark numbers are not fully meaningful unless you also consider the visual quality of the results, which is harder to quantify. As a consequence, it's possible that by forcing all benchmarks to use bicubic, we may just be degrading performance without necessarily bringing significantly better picture quality. As such, I'm hesitant to take this change unless there's evidence that this brings more homogeneity in image quality across libraries.
There was a problem hiding this comment.
Bicubic is the default for ImageSharp, so there is no actual change on this code line. This explains why ImageSharp before/after values remained unchanged.
On the other hand, we need to investigate if the current comparison is fair. If output quality is different and/or not all libraries are capable to handle certain cases, that should be pointed out in benchmark (result) comments or the blog post. For example: do all libraries implement premultiplication to prevent alpha bleeding?
@JimBobSquarePants is planning to have a look soon and come back with more specific details.
There was a problem hiding this comment.
Benchmark numbers are not fully meaningful unless you also consider the visual quality of the results, which is harder to quantify.
I couldn't agree more here. Operations should consider far more than just the raw speed. Size, quality, correctness for example.
Here's a test image which demonstrates issues with several of the libraries. Only MagicScaler, ImageSharp, and System.Drawing get that right.
Note: I couldn't even get FreeImage to save an input png to jpeg with the current code so I'd consider that useless.
Input
MagicScaler
NetVips
SkiaSharpBitmap
SkiaSharpCanvas
SystemDrawing
ImageSharp
MagickNET
There was a problem hiding this comment.
Forgot to say, the System.Drawing code is using some sort of sharpening via InterpolationMode.HighQualityBicubic. It also doesn't all subsampling so image quality at 75 will appear sharper than many of the others.
There was a problem hiding this comment.
I think most of the differences here are due to the background colour.
This test PNG has transparent pixels in a checkerboard pattern, so when you make a JPG thumbnail, the colour you get depends on what background colour you set. NetVips defaults to black (like skia I guess), so it does look rather dark. If you set a white background, or thumbnail to PNG, NetVips and MagickNET look the same (I suppose skia would too).
NetVips has an option for linear light downsampling, but I don't think this particular test image is a great way to show that. It seems to have a surprise hidden in the RGB of the transparent pixels, so I think this image is probably for testing premultiplication.
|
Thanks for the contribution. I'm happy to take most of it, but I'd like @JimBobSquarePants ' opinion on the bicubic change to ImageSharp before I merge. |
There was a problem hiding this comment.
@kleisauke thanks for taking a look at this, and thanks for adding the NetVips benchmarks.
Can you give some explanation of the timing discrepancy between your version of the benchmark and mine? In my tests (on Windows 10), materializing only the original (black) image takes ~0.8ms. Materializing only the resized image takes ~1.5ms. Those timings were consistent whether I used CopyMemory or WriteMemory. It would seem to me that doing both should take ~2.3ms or less, but in fact my benchmark doing both takes ~5.2ms. That led me to believe that without materializing the original bitmap, the pixels don't exist and some shortcut is being taken internally. If you're confident the work is actually being done with the revised version, that's fine, but I'm not sure how the numbers can be that far off if that's the case.
For that matter, Skia also takes a shortcut in this benchmark. Because we start with a blank Canvas, it can create the blank image directly in video memory and do the scaling entirely in video, which is not possible with an actual decoded image. That's why its numbers are so low here (particularly in your Linux results) while being comparatively poor in the real-world tests. Skia is a poor choice for imaging tasks anyway, as explained in mono/SkiaSharp#520, but it would be nice if we could compare fairly.
And on the subject of fairness, I disagree with your characterization of the MagicScaler Y'CbCr JPEG processing as not fair. That falls under the category of "work smarter, not harder", and the output images prove that. It's not necessary to do the same work as long as the work that's done is correct.
And finally on that topic, it's worth pointing out that as of now, only System.Drawing, ImageSharp, and MagicScaler have correct output in the load/resize/save tests. All the others mangle the colors by dropping the embedded Adobe RGB ICC profiles in 10 of the 12 images. I see that libvips has the ability to do color profile conversions, and it would be nice to see its numbers when handling the colors correctly. I've been meaning to update the others as well. Skia and ImageMagick can also do color conversions -- not sure about FreeImage.
| // 'Catrom' is generally imprecisely known as 'BiCubic' interpolation | ||
| image.FilterType = FilterType.Catrom; | ||
|
|
There was a problem hiding this comment.
This comment doesn't make sense, as Catrom is most certainly a cubic filter. Also, ImageMagick has a cardinal cubic filter as you've specified in the other tests: FilterType.Cubic
There was a problem hiding this comment.
I'm not sure why I missed the FilterType.Cubic filter. Resolved in: 8fbc3c7.
Note that ImageMagick seems to alias BiCubic to Catrom:
After IM v6.7.7-7 'BiCubic' is simply an alias to 'Catrom', which is typically regarded as a good 'cubic interpolator' (b=0, c=1/2). You should use the name 'Catrom' rather than 'BiCubic' so as to be clear what you are using for interpolation.
There was a problem hiding this comment.
Ah, I see what you meant now :)
| // until you write to an output file, buffer or memory | ||
| var memory = resized.WriteToMemory(); | ||
|
|
||
| return (resized.Width, resized.Height, memory); |
There was a problem hiding this comment.
This isn't necessary in this instance, because the JIT has no way of knowing whether WriteToMemory mutates resized. The code will always run, whether you capture the result in a variable or not -- or return it or not.
| using (var original = NetVips.Image.Black(Width, Height).CopyMemory()) | ||
| using (var resized = original.Reduce(xFactor, yFactor, kernel: Enums.Kernel.Cubic).CopyMemory()) | ||
| using (var original = NetVips.Image.Black(Width, Height)) | ||
| using (var resized = original.Resize(1.0 / xFactor, vscale: 1.0 / yFactor, kernel: Enums.Kernel.Cubic)) |
There was a problem hiding this comment.
Why not just swap the numerator/denominator in the xFactor and yFactor calculations above?
| { | ||
| using (var original = new SKBitmap(Width, Height)) | ||
| using (var resized = original.Resize(new SKImageInfo(ResizedWidth, ResizedHeight), SKFilterQuality.High)) | ||
| using (var resized = original.Resize(new SKImageInfo(ResizedWidth, ResizedHeight), SKFilterQuality.Medium)) |
There was a problem hiding this comment.
I can't find any documentation that describes what the SKFilterQuality values mean in terms of resampling kernel. How did you determine Medium refers to a cubic kernel?
There was a problem hiding this comment.
You're right. It seems that SkiaSharp does not have a cardinal cubic filter available:
https://github.com/mono/SkiaSharp/blob/9d3d4f577f1e78c1c64e03f5c4b95e3976fb0856/binding/Binding/SKBitmap.cs#L22-L33
Switched back to SKFilterQuality.High in 8fbc3c7.
There was a problem hiding this comment.
Nice find. Those mappings are perplexing. It seems there's no way of knowing what you're actually going to get, regardless of the fact they mapped the old cubic kernels to SKFilterQuality.High. For sure, given the poor quality of Skia's image interpolation, anything other than their highest quality would be unacceptable to most people.
- ImageMagick has a cardinal cubic filter available, so use that instead of Catrom. - Use SKFilterQuality.High instead of SKFilterQuality.Medium. - Simplify the xFactor and yFactor calculations. - Just capture the result of WriteToMemory in a variable, there is no need to return it.
|
Based on mono/SkiaSharp#520 (comment) and the fact that the timing numbers and image quality are the same now, it seems that there is no longer any reason to have two versions of each Skia test. They appear to be doing the same thing internally at this point. |
|
Hi @saucecontrol, Thank you for your comments and code suggestions! I guess the shortcut what you're seeing is due to the operation cache. Every time you call an operation, libvips searches the cache for a previous call to the same operation with the same arguments. If it finds a match, you get the previous result again. You can disable this behavior with: NetVips.CacheSetMax(0);You could also try to disable the run-time vector code generation and set the the number of worker threads to 1: NetVips.VectorSet(false);
NetVips.ConcurrencySet(1);I should not call it 'fair-competition patch', sorry about that. I just wanted to explain why MagicScaler is a bit faster on Windows with the 'Load, Resize, Save' benchmark. The reason that libvips is slower here is because of the threading, thumbnailing to 150x150 pixels can be done quite quickly, so a large thread pool could slow down processing overall. Regarding the correct output in the load/resize/save tests, could you try this patch?: diff --git a/NetCore/LoadResizeSave.cs b/NetCore/LoadResizeSave.cs
index 1111111..2222222 100644
--- a/NetCore/LoadResizeSave.cs
+++ b/NetCore/LoadResizeSave.cs
@@ -187,9 +187,6 @@ namespace ImageProcessing
// Resize it to fit a 150x150 square
image.Resize(ThumbnailSize, ThumbnailSize);
- // Reduce the size of the file
- image.Strip();
-
// Set the quality
image.Quality = Quality;
@@ -331,7 +328,7 @@ namespace ImageProcessing
using (var thumb = NetVipsImage.Thumbnail(input, ThumbnailSize, ThumbnailSize))
{
// Save the results
- thumb.Jpegsave(OutputPath(input, NetVips), q: Quality, strip: true);
+ thumb.Jpegsave(OutputPath(input, NetVips), q: Quality);
}
}
}Note that this also increases the file size, because EXIF data, IPTC data, or XMP metadata are now also being saved. |
|
This is an interesting discussion, thank you! Re. the operation cache: all libvips operations are functional, that is, they have no side-effects. This means you can replace the result of any operation with the result of a previous call with the same arguments. By default, libvips aims to keep about 100mb of cached operations. This can give misleading benchmark results if you loop several times over the same image. I usually try to have N distinct input images rather than looping N times over the same image. |
|
Thanks for the info, guys! It sounds like from what @jcupitt said, the most correct thing to do is to call The worker thread issue is another matter, as I believe ImageSharp still does parallel processing internally as well. The load/resize/save parallel benchmark shows a severe per-image perf drop for NetVips, while ImageSharp suffers less, so the degree of internal parallel operation is definitely different. It would be good to normalize that. @kleisauke I tried disabling the metadata stripping feature, but that took the thumbnail size from 4-5KiB per image to 35-50KiB. Although that did preserve the color profile, carrying the XMP data with it isn't optimal. Is there a way to only preserve metadata necessary for proper image rendering? I believe that's how ImageSharp is currently handling it. I also recall @bleroy encountering a similar issue with ImageMagick, which was preserving all metadata by default. That code was modified to strip metadata as well, with the same result (incorrect colors). If it's not possible to preserve just the color profile, it would be preferable to convert to sRGB, which is what the System.Drawing sample does and what MagicScaler does by default. |
|
@saucecontrol there is no parallel processing in ImageSharp resize since SixLabors/ImageSharp#888. (Except with NearestNeighbour resampler, but it's not being used in this benchmark). |
|
Thanks, @antonfirsov, that's good to know! BTW, the ImageSharp performance gains since the first version of this benchmark project are really impressive. If I'm not mistaken, @JimBobSquarePants contributed this Resize benchmark in order to show that ImageSharp's resampling was faster than System.Drawing's even if it was a bit slower (at the time) in the end-to-end load/resize/save benchmark. I've always questioned the value of this specific benchmark given the many ways to cheat on it, and I'd personally be in favor of trashing it. The end-to-end processing is not only more representative of real-world scenarios, it's also easy to visualize the differences in processing output. Here -- just resizing empty pixels -- it's very difficult to tell what work is being done and what impact it would have on image quality, as evidenced by this thread. |
|
@bleroy Bicubic is our default sampler so that's fine by me.
I concur. It's not meaningful and barring exceptions the numbers are so small they should be considered "fast enough" for most users. Thanks for the compliment regarding performance gains also! We've come a long way and I believe that once we begin implementing the Runtime.Intrinsics APIs we will be able to get very close to the best performing libraries. |
|
[Counterargument] I think if we could follow a strict and systematic approach, we could ensure that the comparison is relatively fair:
With this approach, I would say there is nothing wrong with these benchmarks, and they actually communicate useful information to library users, even if the performance differences are small. |
|
It's tangential to this particular PR, but yes, redefining the set of images used is a good idea. The original set was from my own collection, selected because they were problematic in one way or another with System.Drawing at the time. That was years ago, and it's time to revise that. It would be good to use images that library authors use for their own testing to assert quality. And of course, the images must be public domain or under a friendly license. |
|
I totally agree this project could use a more diverse set of test images. In particular, I'd like to include a (maybe separate) suite of higher-resolution images more representative of today's cameras -- 8-12 megapixels for phones and 20+ megapixels for dedicated cameras. As far as this PR goes, this is my understanding of the changes:
|
|
Thanks for the contribution and for the good discussion. |
6 participants
This switches all resize tests to use bicubic interpolation. See the relevant discussion here: #15 (comment).
I've also slightly adapted the NetVips resize benchmark, it's now using the convenient
vips_resizeinstead of the low-levelvips_reduceoperation. TheCopyMemory()operations introduced in PR #16 doesn't seem quite right to me. I changed it toWriteToMemory()at the end of the chain to have libvips actually process pixels.Benchmark results on Linux
Runtime information
Before
After
Benchmark results on Windows
Runtime information
Before
After
/cc @jcupitt, because he might also be interested in the numbers above.