Imageprocessor: Image now takes 4x memory size
The pixels in ImageBase is represented by float[]. Each pixel is represented by 4 floats and requires 16 bytes. When pixels are represented using byte[], each pixel only takes 4 bytes. This effectively takes 4x memory, a big concern on low memory devices like phones.
yufeih
All 95 comments
True,
This allows us much more accurate calculations though (colorspace, gamma to linear and reverse, etc) and potential for HDR support. It does also however cause issues at points also with increased alpha bleeding potential.
What would you suggest as a compromise to keep memory low but allow accurate conversion?
JimBobSquarePants
on 3 Jan 2016
Is this an observed issue or just something we assume will become problematic in the future?
christopherbauer
on 3 Jan 2016
@christopherbauer I was looking at using ImageProcessor (over Nine.Imaging) for one of my mobile apps. On android, we have very limited amount of memory to consume, this is an important concern.
yufeih
on 4 Jan 2016
@JimBobSquarePants
I suggest represent image pixels using standard byte[], make Color backed by 32bit uint and provide explicit conversions between Color and Vector3/Vector4.
When you do filtering, you first cast a pixel to Vector4, then do the calculation in floating point precision with SIMD provided by Vector4, then cast back to Color to set the pixel. (https://github.com/JimBobSquarePants/ImageProcessor/issues/286)
Unless you are dealing with HDR images, 32bit color has enough precision for common usage. Plus, I don't think the current encoders/decoders support HDR colors.
As for color spaces like Hsl, YCbCr, I don't see them been used anywhere in the library, filters like Hue is using a dedicated conversion algorithm rather then casting the color to Hsl. So I didn't consider that part.
yufeih
on 4 Jan 2016
@yufeih A lot to think about there...
If I use byte with a backing uint I might as well drop the Bgra32 class as I will be moving back to using the 0-255 range. That will certainly hurt should anyone wishing to develop an encoder/decoder for something like Radiance HDR which is certainly doable (converting from this for example)
Casting to/from Vector4 would solve some if not all of the accuracy issues.
I don't use the other color spaces Hsl, YCbCr, etc yet but I want other developers to. I want a single location for accurate colorspace conversion so other developers can expand on the library. I use the dedicated algorithm in Hue as it's faster than casting then adjusting each pixel.
Floating point pixels has caused me issues for sure and I would like to keep memory usage to the absolute minimum. I just don't want to cripple the library like so many others are.
I'm gonna do a shoutout on Twitter and see if I can get some input from experts out there. I'm sorely tempted to take up your suggestion but wanna be sure we do the right thing.
JimBobSquarePants
on 4 Jan 2016
Yeah, I also like to hear more inputs here. Ideally, images and filtering algorithms should be independent of the underlying pixel type, so the same algorithm works with both Bgra32, BgraFloat and variations like Rgba32, Argb32.
This leads me to thinking about a generic Image<T> where T can be the color format of choice. But the problem is it's really hard to write such Image
yufeih
on 4 Jan 2016
That's an interesting idea! I know of one project DotImaging that does something very similar.
We'd have to do a fair amount of work refactoring colors etc to ensure methods like Color.Compand and the inverse were possible but it could be possible and performant.
Do you have any time to give it a go?
JimBobSquarePants
on 4 Jan 2016
I'd like to wait for more inputs and mature the design.
yufeih
on 4 Jan 2016
No worries, we'll see if anyone chips in.
JimBobSquarePants
on 4 Jan 2016
Sorry to barge in, but if you are using multiple (for example) chained filters, isn't the conversion byte -> float(operate) -> byte -> float(operate) -> byte going to cause a compounding loss of accuracy?
rubensr
on 4 Jan 2016
Don't be sorry, The more input the better! :smile: That's one thing that does bother me and will require testing to see if it is an issue.
JimBobSquarePants
on 4 Jan 2016
@yufeih Had a wee play around last night with a generic image/color format. Without templating support in C# this is going to yield some pretty awful looking code. I haven't done enough to be able to test performance yet. There's a generic-image branch uploaded anyway that I'm experimenting in (doesn't build)
public static Color<T> operator +(Color<T> left, Color<T> right)
{
if (typeof(T) == typeof(byte))
{
return (Color<T>)(object)new Color<byte>(
(byte)((byte)(object)left.R + (byte)(object)right.R),
(byte)((byte)(object)left.G + (byte)(object)right.G),
(byte)((byte)(object)left.B + (byte)(object)right.B),
(byte)((byte)(object)left.A + (byte)(object)right.A));
}
// etc
}
How about something like this (I'm using the beta Numerics package from Nuget):
namespace ConsoleApplication2
{
class Program
{
static void Main(string[] args)
{
var left = new Color<Single>(10, 10, 10, 1);
var right = new Color<Single>(10, 10, 10, 1);
var sum = left + right;
System.Diagnostics.Debug.WriteLine(sum);
}
}
public class Color<T> where T : struct
{
Vector<T> backingStore;
public T R { get { return backingStore[0]; } }
public T G { get { return backingStore[1]; } }
public T B { get { return backingStore[2]; } }
public T A { get { return backingStore[3]; } }
public Color(Vector<T> comp) : this(comp[0], comp[1], comp[2], comp[3]) { }
public Color(T r, T g, T b, T a)
{
backingStore = new Vector<T>(new T[4] { r, g, b, a });
}
public Color() : this(default(T), default(T), default(T), default(T)) { }
public override string ToString()
{
return $"R: {R} G: {G} B: {B} A: {A}";
}
public static Color<T> operator +(Color<T> left, Color<T> right)
{
return new Color<T>(left.backingStore + right.backingStore);
}
}
}
@rubensr That certainly looks much nicer than my current implementation. Any idea if there are any performance implications involved in operating in the individual array items? Ideally I'd like my RGBA values to be read/writeable for performance (My knowledge of the vector types is nil)
JimBobSquarePants
on 5 Jan 2016
@JimBobSquarePants As far as I understand it, to take advantage of the SIMD feature, you'd need to perform all of your calculations using the operators defined within the Vector types like I did in the '+' operator above - as soon as you access the R, G, B, A properties I created and then use them individually for calculations, you lose the performance improvements.
But no I don't have any testing to back that up :)
rubensr
on 5 Jan 2016
@rubensr Cheers. There wouldn't have been any SIMD benefits from the original property accessors against Vector4 anyway. We can certainly take advantage though by allowing conversion to Vector
JimBobSquarePants
on 5 Jan 2016
@JimBobSquarePants Yeah I think for example operations like the Compand and InverseCompand could benefit greatly from using Vectors and Matrices.
rubensr
on 5 Jan 2016
That's a super tricky one actually. I had one of the MS guys give it a go.
JimBobSquarePants
on 5 Jan 2016
Yes fair enough I take that back :) Can't argue against 4s vs 53s !
rubensr
on 5 Jan 2016
@yufeih I had a go at transforming to using Image
JimBobSquarePants
on 13 Jan 2016
I have 2 proposals in mind:
1. Image and HdrImage
Image is backed by bytes, and is the default pixel format. HdrImage is backed by Vector4. Image and HdrImage can be converted back and forth.
Most formats will just import to Image, except for specialized ones that works with Hdr formats.
Filters needs to support both Image and HdrImage, that means every filter will have 2 methods, one to work with byte[] pixels, one to work with Vector4[] pixels.
2. Image< T >
Add Image<T>, but instead of turning Color into Color<byte> / Color<float>, turn Image into Image<Bgra> / <Image<Argb>> / Image<BgraFloat>. T is any color fomat that can be converted between Vector4.
I am thinking about the following conversion process, which eliminate boxing and convert pixels in batches:
public interface IPixelConverter<T>
{
void ToVector4(T[] pixels, int startIndex, int length, Vector4[] destinationPixels, int destinationStartIndex);
void FromVector4(Vector4[] pixels, int startIndex, int length, T[] pixels, int destinationStartIndex);
}
For filters
public void Hue<T>
{
private IPixelConverter<T> _converter = //...
public void Process(Image<T> src, Image<T> dest)
{
var pixels = new Vector4[1024];
_converter.ToVector4(src.Pixels, 0, 1024, pixels, 0);
// doing the compute
converter.FromVector4(pixels, 0, 1024, dest.Pixels, 0);
}
}
1 is simple
2 is more extensible
yufeih
on 13 Jan 2016
I would much rather the second version but I've got a couple of questions, forgive me if I'm being daft...
- If I'm converting to and from Vector4[] for each operation am I not going to be ultimately requiring the same memory as using the current float[] format.
- How would you handle down-casting of float to byte as T in a manner to avoid overflow exceptions?
JimBobSquarePants
on 14 Jan 2016
- Indeed. The thinking was based on really simple cases like Hue, where processing a pixel does not sample surrounding pixels :+1: An alternative is to have the pixel converter do per pixel conversion like:
Vector4 ToVector4<T>(ref T color) where T struct, but this adds a virtual function call overhead for each pixel access. - I don't quite follow the concern, shouldn't
clamp(float * 255, 0, 255)be enough?
yufeih
on 15 Jan 2016
For 2. I'm having issues casting down to a byte from a float when I don't know it's a byte at runtime.
JimBobSquarePants
on 15 Jan 2016
@JimBobSquarePants have you seen this? http://www.codeproject.com/Articles/23323/A-Generic-Clamp-Function-for-C
public static T Clamp<T>(T value, T max, T min)
where T : System.IComparable<T> {
T result = value;
if (value.CompareTo(max) > 0)
result = max;
if (value.CompareTo(min) < 0)
result = min;
return result;
}
although on second thought it may not help with your down-casting issue; haven't tested it.
robertjf
on 15 Jan 2016
@robertjf I actually use that method in V2; the issue is further down the wire. despite the two types being what they are I get an InvalidCastException.
public static T To<T>(this object obj)
{
Type destinationType = typeof(T);
Type sourceType = obj.GetType();
if (destinationType == typeof(float) && sourceType == typeof(byte))
{
// This throws a wobbler.
return (T)obj;
}
// blah
throw new NotSupportedException("Type not supported");
}
@JimBobSquarePants What if you use Convert.ToByte() and friends? You'd need to catch OverflowException of course, but could use that to clamp the values...
robertjf
on 15 Jan 2016
My original thought was to use virtual dispatch like this: https://gist.github.com/yufeih/ee77890dbd1eed0cc88d
There is a virtual method call overhead each time we retrieve or store a pixel, but you don't have to deal with casting objects of unknown type and pay for the boxing overhead.
Performance wise, I am more towards having just a single Image with fixed pixel type. If there is a need for HdrImage, then have both Image and HdrImage with filtering for both. Yes we ended up with duplicated code, but there is really no way to do compile time generics and zero cost abstractions in c#.
yufeih
on 15 Jan 2016
While I work out in my head a better solution may I suggest T4? https://msdn.microsoft.com/en-us/library/bb126445.aspx
SamuelEnglard
on 21 Jan 2016
John Skeet has a neat library called MiscUtils that allow you to write generic maths with unexpectedly fabulous preformance. I don't know the platform constraints as it makes use of compiled expressions.
This may help unify the code and types that use different underlying pixel types.
HelloKitty
on 14 Mar 2016
Thanks @HelloKitty, I'll check it out. If you have time yourself would you be able to do so. I'm under the cosh just now time-wise.
JimBobSquarePants
on 14 Mar 2016
This issue still irks me; I really wish I could find a decent solution.
There's a big part of me that thinks I should revert back to byte but it would be a truly enormous amount of work and definitely a loss of flexibility.
JimBobSquarePants
on 11 Apr 2016
Just a heads up to say I'm still playing around with different pixels formats here
JimBobSquarePants
on 30 Jun 2016
As I mentioned to @JimBobSquarePants on slack, and possible solution would be having the actual backing data type be opaque. To show how this could be done I've created a simple prototype of Number on gist https://gist.github.com/SamuelEnglard/0c2a10ea13dae8a58fd7007289f84b60
SamuelEnglard
on 3 Jul 2016
While I don't personally do any image processing did the potential solution I suggested earlier not pan out?
The idea is outlined by Marc Gravell and implemented in Jon Skeet's MiscUtil lib. It should allow for you to create performant generic pixels like Pixel{T} and have T switch between byte, short, int and whatever you'd like.
This may be what you want http://www.yoda.arachsys.com/csharp/genericoperators.html generic math, generic operators and etc.
HelloKitty
on 4 Jul 2016
@HelloKitty What I and @JimBobSquarePants have been working on is actually based on Marc and Jon's work there! (I spend a lot of time looking over their code while working on my Number type)
SamuelEnglard
on 4 Jul 2016
@HelloKitty @SamuelEnglard Yeah, I think we might be onto something and would absolutely love to crack it. Need to take some time to look over it properly though.
Toughest issue is probably talking to decoders/encoders in a simple generic way I would imagine.
JimBobSquarePants
on 4 Jul 2016
@JimBobSquarePants Want to hop on Slack to talk about the gist? Or do it here?
SamuelEnglard
on 4 Jul 2016
@SamuelEnglard Will hop on but we should make sure all comms are also public.
JimBobSquarePants
on 4 Jul 2016
I've updated the Gist. Main differences are that I've made improvements to memory usage, I've cut down on repeated code a lot, I added code to make it useable with unmanaged memory, and I started added methods to supplement the Math.* methods. I really think my Number idea could be the answer
SamuelEnglard
on 4 Jul 2016
@SamuelEnglard I agree; the progress you have made is excellent. If you are keen I'd like to give you direct access to the repository so we can work together on a branch that would implement this. What do you think?
JimBobSquarePants
on 5 Jul 2016
@JimBobSquarePants Did you consider support platforms where dynamic expression compilation is not available? like Xamarin iOS.
yufeih
on 5 Jul 2016
@yufeih I'm only planning on supporting anything that runs .NET Core.
According to this IOS and Android both should be possible.
https://msdn.microsoft.com/en-us/library/dn878908(v=vs.110).aspx
Is there something specific you have flagged?
https://developer.xamarin.com/guides/ios/advanced_topics/limitations/
JimBobSquarePants
on 5 Jul 2016
In the ios limitations page:
Since the iPhone's kernel prevents an application from generating code dynamically Mono on the iPhone does not support any form of dynamic code generation. These include:
The System.Reflection.Emit is not available.
No support for System.Runtime.Remoting.
No support for creating types dynamically (no Type.GetType ("MyType`1")), although looking up existing types (Type.GetType ("System.String") for example, works just fine).Reverse callbacks must be registered with the runtime at compile time.
The System.Reflection.Emit is not available, which means dynamic expression compilation technique described at http://www.yoda.arachsys.com/csharp/genericoperators.html is not likely to work on an iOS device.
yufeih
on 5 Jul 2016
OK, after looking at @SamuelEnglard 's gist, it does not use expression compilation, which solve the problem of running everywhere. Performance wise, it might be slow.
yufeih
on 5 Jul 2016
@yufeih @SamuelEnglard Yeah we'll need to benchmark the hell out of it to see if it is usable.
JimBobSquarePants
on 5 Jul 2016
@yufeih I would note that on platforms where dynamic expression compilation isn't available, it would fall back to interpolation. So the code would still work, just slower.
While I admit I have not run benchmarks on it (mostly from lack of any idea what a good test would be) it should perform well. Biggest issue it would have is that currently the way I store the actual backing type isn't great. I need to find a better way but I haven't thought of one yet.
SamuelEnglard
on 5 Jul 2016
Updated the Gist again. I continued performance work I started before and I think I solved the issue of how I'm storing the backing data.
SamuelEnglard
on 6 Jul 2016
Excellent. I've got another Idea I'm playing with that uses packed vectors to store the image data. I think it might work also and will share asap. I'm killing myself bit-shifting just now though.
JimBobSquarePants
on 6 Jul 2016
@SamuelEnglard Before you go too far down the rabbit hole I'd like you and @yufeih to have another look at this. https://github.com/JimBobSquarePants/ImageProcessor/tree/e8d174508130e99cb9152e3c6dad690232425075/GenericImage
I've made some good progress and would now be able to convert to and from bytes fairly easily in a generic manner which would allow the existing formats to communicate with them. With the buffer stored as the actual type performance should be pretty good also.
JimBobSquarePants
on 6 Jul 2016
@JimBobSquarePants The idea looks sound, though I don't see a way to do byte based pixels in it
SamuelEnglard
on 6 Jul 2016
@SamuelEnglard The Rgba32 format packs the 4 bytes into a uint. You can pack them using PackBytes(x,y,z,w) and recover the byte array using ToBytes(). Manipulating each component can be done via ToVector4() which is what we would use in the various algorithms.
Does that make sense?
JimBobSquarePants
on 6 Jul 2016
@JimBobSquarePants Ok, now I see it. The setting via floats threw me off. So how would you allow float based images?
SamuelEnglard
on 6 Jul 2016
@SamuelEnglard This is one thing I'm trying to figure out.... I don't know how to pack 4 floats and retrieve them without first clamping the value. A loss of precision is acceptable but a loss of range is not.
Once I crack that I can support HDR image formats also.
All file formats I know of use bytes for actual storage which is why I added those other methods.
JimBobSquarePants
on 6 Jul 2016
Maybe do it how I did in my Number type? I just "store" the internal bytes that make up the floats
SamuelEnglard
on 6 Jul 2016
That might work yeah.... I have been told that GPU's support the concept of half-floats 16bit ones so my money is that there is something within MonoGame we can steal.
In fact....
JimBobSquarePants
on 6 Jul 2016
@JimBobSquarePants That might work, though you're outside my skill set now :)
SamuelEnglard
on 6 Jul 2016
@SamuelEnglard @yufeih I have a working example of a truly generic image proof-of-concept that could potentially handle any pixel format you want to throw at it. I invite you both to check it out. I've just configure it to use Bgra32 for now.
The downside is that at the moment it is twice as slow as System.Drawing and our current float based implementation when resizing an image . If either of you have any spare time I'd love to see if we can speed things up and make this a real possibility.
JimBobSquarePants
on 8 Jul 2016
@JimBobSquarePants I'll give it a look. Do you know why it slows down?
SamuelEnglard
on 8 Jul 2016
@SamuelEnglard Cheers, Yeah it looks like it is the getters and setters for accessing pixels.
I think shifting to/from the packed values and creating the Vector4 instance is the main cause. Math.Round makes very little difference and is required anyway.
I benchmarked directly assigning a concrete Bgra32 value against a pixel and it is blisteringly fast taking only 8% as long as the System.Drawing Get/Set code.
JimBobSquarePants
on 8 Jul 2016
@JimBobSquarePants
return *(this.pixelsBase + ((y * this.Width) + x)); this line turns a struct into an interface, which boxes every pixel.
yufeih
on 8 Jul 2016
@JimBobSquarePants as @yufeih said, that is what is most likely killing perf. You'll note in my code I took create pains to prevent any boxing. Also note that interface dispatching will be slower too since it isn't direct.
SamuelEnglard
on 8 Jul 2016
Thanks both of you. Thought it would be that... Was by far the weakest link in the code. So how can we fix that? Is there something we can combine from your gist or a better way to allow a concrete approach?
I think we're super close to something awesome.
JimBobSquarePants
on 8 Jul 2016
@JimBobSquarePants A quick thought would be to replace the IPackedVector with a "special" struct that hides what's in it
SamuelEnglard
on 8 Jul 2016
@SamuelEnglard Could you elaborate on that?
I tried IPackedVector<TPacked> which would have prevented the boxing but I couldn't cast to that from my concrete BgraPixelAcessor in Image<TPacked>.Lock().
JimBobSquarePants
on 8 Jul 2016
@JimBobSquarePants I just had a crazy thought, let me try some things
SamuelEnglard
on 8 Jul 2016
@JimBobSquarePants I would like to know where you want to see your library? Something easy and light weight mostly for web services maybe with a handful of advanced algorithms or a full fleshed computer vision library like OpenCV? I think that, especially for scientific computing or machine vision a good generic image support is essential, where you should be able to specify the depth and the color.
I know that this is a lot of work, but the current approach would disqualify the library in many use cases where resources (CPU, Memory) are restricted. E.g. if you always need a 16 byte per pixel array when you just want to do some operations on a 1 byte per pixel greyscale image that would disqualify the use of this library. 1bpp grayscale images should get processed noticeably faster while having only 1/16th of the memory footprint.
If you want to supply a library mainly for web services where nearly all images are BGR anyways you may get away with it, but 16 bytes per pixel is still a massive burden on memory. In most cases you could propably get away with 3 bytes per pixel (BGR) and only convert the image to float if explicitly needed for an algorithm.
In my opionion two options are viable:
Image< TColor, TDepth > or just Image with two properties that specify the Color and the Depth. The first one produces more readable code and compile time safety while the second one would provide more flexibility. Maybe you could take a look at EmguCV (.net wrapper around OpenCV) and OpenCV to get an better idea of what you want (or don't want). EmguCV used the Image< TColor, TDepth > approach first but is now transitioning to the property based approach.
legg33
on 8 Jul 2016
Hi @Grebe-M good question.
Initially I was aiming to get the library working on the server only, everything was going to be 16bit and it would have been easy but the more I have researched, the more I am driven to produce something that will allow me to work with multiple pixel formats and tightly control memory usage. Something more suitable for scientific work as well as capable on the server.
This has led to my Core-Flava branch that I am working on. I currently have a POC that will allow me to pass structs of varying size containing packed Vectors values.
Ultimately I now have Image<TPackedVector>.
These packed vectors are based tightly on the ones found in MonoGame but with a couple of extra properties that allow converting to and from byte arrays to allow encoding/decoding.
In principle this should work well but I now need some help benchmarking my new code as I've just realised my laptop has CPU throttling that kicks in whenever I try to benchmark my new build.
I've only written the Bgra32 struct so far but adding the others should be fairly trivial based on the existing code. If you have time could you run the benchmark tests on that version?
@SamuelEnglard Could you also pull my latest changes allowing IPixelAccesor<TPackedVector> and benchmark that before attempting something crazy?
JimBobSquarePants
on 8 Jul 2016
Benchmark results for the Core-Flava branch:
BenchmarkDotNet=v0.9.7.0
OS=Microsoft Windows NT 6.1.7601 Service Pack 1
Processor=Intel(R) Xeon(R) CPU E3-1231 v3 3.40GHz, ProcessorCount=8
Frequency=3320332 ticks, Resolution=301.1747 ns, Timer=TSC
HostCLR=MS.NET 4.0.30319.42000, Arch=64-bit RELEASE [RyuJIT]
JitModules=clrjit-v4.6.1076.0
Type=Resize Mode=Throughput
Image size: 400x400, Resize size: 100x100
| Method | Median | StdDev | Scaled |
| --- | --- | --- | --- |
| System.Drawing Resize | 2.2854 ms | 0.0258 ms | 1.00 |
| ImageProcessorCore Resize | 5.8513 ms | 0.1032 ms | 2.56 |
Image size: 4000x4000, Resize size: 1000x1000
| Method | Median | StdDev | Scaled |
| --- | --- | --- | --- |
| System.Drawing Resize | 150.5100 ms | 0.3307 ms | 1.00 |
| ImageProcessorCore Resize | 510.0908 ms | 14.8342 ms | 3.39 |
The cpu load using ImageProcessor was 6-7 times higher, too:
legg33
on 8 Jul 2016
Bugger... That's awful. I somehow managed to make it even slower.
JimBobSquarePants
on 8 Jul 2016
The cpu load will be down the the parallel processing no?
JimBobSquarePants
on 8 Jul 2016
Did you mean:
The cpu load will be due to the parallel processing no?
If so: yes it is.
But I see a problem that may come into play when the library gets used in real world scenarios eventually. When using System.Drawing the CPU still has lots of reserves to process multiple images in parallel, while the CPU is struggling with a single image when using ImageProcessor. The resulting nested parallel loops may hurt performance further. At the same time parallel processing is often the only option to gain some real performance in C#.
So I think it should get tested if the nested parallel loops hurt performance. If thats the case it may be preferable to leave certain algorithms single threaded, if they don't gain very much performance by using parallel processing. This would leave room for the end user to process images in parallell, which may be a lot faster then, because a single image doesn't already block all CPU cores.
This is just a general idea I wanted to communicate and not specific to the previous benchmark.
legg33
on 8 Jul 2016
I would add to this that Multi-threading + GDI is a bad combo
SamuelEnglard
on 8 Jul 2016
Sorry guys was at the cinema.
I think there might be a wee bit of confusion going on. There's no nested parallel processing going on in the library. There used to be but I stripped it out a while back. There's certainly no GDI also.
@Grebe-M System.Drawing has a system wide process lock when using the Graphics object so it can't do any level of parallelism. That said, that graph scares me a little, I don't want to kill servers. What did you use to generate that btw?
To me it seems that the the CPU usage is higher in the Core-Flava version and I'm wondering why. I was able to run the benchmarks fine enough before.
@SamuelEnglard What was this crazy idea you were on about. I'm stumped how to improve performance now. I thought switching to IPixelAccesor<TPackedVector> would help but it hasn't.
JimBobSquarePants
on 8 Jul 2016
@JimBobSquarePants No problem, I'm mult-tasking here too.
I only brought up GDI as that's what System.Drawing uses so they are really the same thing.
He used https://github.com/PerfDotNet/BenchmarkDotNet to create it. I've been using it too.
As to my crazy thinking, it was wondering if using C# "Unions" would help, though I'm not sure if they would really
SamuelEnglard
on 8 Jul 2016
Yes in library itself does no nested parallel processing, but if I as the end user now uses the library and does something like:
public class Program
{
public static void Main(string[] args)
{
var images = new List<Image>
{
new Image(1000, 1000),
new Image(1000, 1000),
new Image(1000, 1000),
new Image(1000, 1000),
};
Parallel.ForEach(images, image =>
{
image.Crop(100, 100);
});
}
}
Then you have nested parallel loops. I just think that this is a very common real world scenario to batch process a bunch of images and that ImageProcessor should keep that in mind. Therefore it might be beneficial to not use parallel processing internally where the performance gain in doing so is small, thus leaving more resources for the end user. EDIT: I am not saying that this is necessarily the case, just a thing to keep in mind and to do some test for maybe.
The System.Drawing comparison was not very good, my bad. I mainly used OpenCV as of now and using Parallel.ForEach with it to process multiple images resulted in immense performance boosts, because internally (all?) of OpenCV is single threaded. But maybe I should stop comparing apples and oranges, because the goals of OpenCV and ImageProcessor are different (which is good!).
@JimBobSquarePants When using Parallel.For for image manipulation it is to be expected that the CPU gets nearly maxed out if any kind of significant calculation is going on. If you don't want to max out severs you may need to allow the user to specify the degree of parallelism so he can limit the resource usage.
The graph is just a crop from "ProcessExplorer" which is kind of a task manager on steroids.
legg33
on 8 Jul 2016
@JimBobSquarePants Actually, @Grebe-M brings up a good point. We may want to make the library single threaded and make multi-threading at the users discretion. After all, as a library we don't know what their threading needs are
SamuelEnglard
on 8 Jul 2016
@SamuelEnglard @Grebe-M That's a very good point. I've been looking at this on such a microscopic level for so long that I'd forgotten about how people would actually use it. I think testing each processor would definitely be a good idea. We could set a max degrees of parallelism in Bootstrapper also.
Back to Image<T> if we can. I guess what is confusing me now is that I thought I had removed the boxing by using the T TypeParam. I'm beginning to feel like I do not understand programming at all...
JimBobSquarePants
on 8 Jul 2016
Imho in you current implementation is way to much casting, converting and memory allocation going on. A main problem is the Vector class. It's nice and all, but currently I don't see a way how it's gonna fit in very well. You will always end up with awkward casts back and forth that eventuall kill performance. Imo you either you have all your data in Vectors from the beginning and leave it there or you are better off with using standard value types and if you want to support bytes as your depth but always cast to Vector first you gained nothing regarding memory usage.
Furthermore I really think you need a depth parameter if you want to support generic images. Simple example you declare BGRA to have bytes as the depth. Now i want to average several images, but I can't do so safely, because it will likely overflow if I add 10 images and then divide by 10.
So what can I do? Well if various depth parameters are supported I create a < BGRA, float > image and add all images there. Then i divide by 10 and convert back to < BGRA, byte >.
I know this is a lot of work, but it's the base of a potent and performant imaging library.
legg33
on 8 Jul 2016
@Grebe-M I'm going to have to see an example of how you would set that up if that's ok? The idea was that I would have Bgra32, Bgr32, BgraF but that's obviously not going to cut it since I need that second type parameter for performant operations.
JimBobSquarePants
on 9 Jul 2016
Well I tinkered a bit and an image now would have a TColorDepth[width, height, channels] that one could address nicely or pointer to with a fixed() statement. TColorDepth is the depth of each pixel component (byte, float, double etc.) The problem is that C# has no restriction for generics that allow only value types. So you get a problem that I don't know how to solve elegantly.
var image = new Image<Bgra, byte>(10, 10);
var scaled = image[0, 0, 0] * 1.5f; // Error: Operator '*' cannot be applied to operands of type 'TColorDepth' and 'float'
So you'd have to cast the image array fist, but that obviously sucks because then you get something like the following with a lot of duplicate code:
if (image.Pixels is byte[,,]) {
...
}
else if (image.Pixels is float[,,]) {
...
}
Just wanted to pop in with some notes from my on going "offline" research.
Did some testing on my Number type. Yes it works but it is SLOW. on average we're talking 10x. I know the reason and I'm not sure how to fix it though.
In doing some more thinking I was wondering if the solution is to do something like Shaders on graphics cards. The thinking being very dynamic code. Need to flesh out the idea more though.
SamuelEnglard
on 11 Jul 2016
Quick question... If I wanted to perform maths on packed values would I have to expand the value, perform the maths then contract or is there something smarter I can do to make it a single operation?
JimBobSquarePants
on 12 Jul 2016
Yes, you have to expand them first to get usable results.
legg33
on 12 Jul 2016
A quick thing to note: While bit shifting isn't slow instead we could use C# Unions:
[StructLayout(LayoutKind.Explicit)]
public struct RGBA32
{
[FieldOffset(0)]
public long All;
[FieldOffset(0)]
public byte R;
[FieldOffset(1)]
public byte G;
[FieldOffset(2)]
public byte B;
[FieldOffset(3)]
public byte A;
}
At the very least they're easier on the eyes
SamuelEnglard
on 12 Jul 2016
Ok.... So I'm now within 10% speed-wise of System.Drawing with my generic image approach. Some very sneaky Interface trickery has allowed me to remove the boxing and do maths.
I need someone to help me out and cast an eye over what I have done to my Resize method though. The unit test is returning Nearest Neighbor and Triangle correctly but Bicubic and others are returning images with error. I'm assuming it's something silly but I'm far too tired and have been staring at the code for far too long to figure out what is going wrong. I think it might be that my resamplers should be using doubles.
Could someone have a look and also benchmark the latest code.
Beware, the code is not as tidy as I would normally produce.
JimBobSquarePants
on 13 Jul 2016
Managed to solve the output bugs but may have slowed it down too much. Will need benchmarking on a machine that isn't mine.
JimBobSquarePants
on 13 Jul 2016
A very quick sample of an idea: in short I'm still abstracting away the underlying datatype of the pixels but only pulling the "number" into managed land when you do an operation.
#pragma warning disable CC0001
#pragma warning disable CC0031
#pragma warning disable CC0105
#pragma warning disable CC0068
#pragma warning disable HeapAnalyzerExplicitNewObjectRule
#pragma warning disable HeapAnalyzerExplicitNewArrayRule
#pragma warning disable RECS0145
#pragma warning disable RECS0091
namespace CrazyIdeas
{
public struct MemoryProcessor
{
private readonly bool isFloat;
private readonly byte[] stream;
private readonly int stride;
public MemoryProcessor(byte[] stream, bool isFloat, int stride)
{
this.stream = stream;
this.isFloat = isFloat;
this.stride = stride;
}
public DataProcessor this[int x, int y]
{
get
{
return new DataProcessor(this, x, y);
}
}
public struct DataProcessor
{
private readonly MemoryProcessor processor;
internal DataProcessor(MemoryProcessor processor, int x, int y)
{
this.processor = processor;
X = x;
Y = y;
}
public int X
{
get;
}
public int Y
{
get;
}
public unsafe void Add(DataProcessor other)
{
fixed(byte* thisStream = processor.stream, otherStream = other.processor.stream)
{
byte* thisPixel = thisStream + (Y * processor.stride * 4 * (processor.isFloat ? 4 : 1));
byte* otherPixel = otherStream + (other.Y * other.processor.stride * 4 * (other.processor.isFloat ? 4 : 1));
if (processor.isFloat && other.processor.isFloat)
{
*((float*)thisPixel) += *((float*)otherPixel);
}
else if (processor.isFloat && !other.processor.isFloat)
{
*((float*)thisPixel) += *otherPixel;
}
else if (!processor.isFloat && other.processor.isFloat)
{
*thisPixel += (byte)*((float*)otherPixel);
}
else
{
*thisPixel += *otherPixel;
}
}
}
}
}
}
Admittedly this sample doesn't really work with "pixels" but the basic idea can be seen.
SamuelEnglard
on 14 Jul 2016
@Grebe-M @SamuelEnglard @yufeih
I think we can all collectively sigh in relief. The latest build will allow us to use signature compatible IPackedVector<T> types that can be consumed by the likes of MonoGame and is FASTER than System.Drawing!
That means I can now make a Color and ColorF set of structs which will allow both levels of memory usage and possible future HDR support. Developers will have Image and ImageF to work with. Other IPackedVector<T> implementations can be easily ported across.
I am so happy with this :smile:
Here's my benchmark scaling a 2000x2000 image to 400x400.
BenchmarkDotNet=v0.9.7.0
OS=Microsoft Windows NT 6.2.9200.0
Processor=Intel(R) Core(TM) i7-6700HQ CPU 2.60GHz, ProcessorCount=8
Frequency=2531245 ticks, Resolution=395.0625 ns, Timer=TSC
HostCLR=MS.NET 4.0.30319.42000, Arch=64-bit RELEASE [RyuJIT]
JitModules=clrjit-v4.6.1080.0
Type=Resize Mode=Throughput
Method | Median | StdDev | Scaled |
-------------------------- |----------- |---------- |------- |
System.Drawing Resize | 77.5405 ms | 4.4940 ms | 1.00 |
ImageProcessorCore Resize | 60.6195 ms | 4.0986 ms | 0.78 |
The plan will be now to introduce and test the other processors into this branch and eventually merge it all into the default Core branch. Once I've done that I'll close this issue.
JimBobSquarePants
on 15 Jul 2016
This is awesome results! Thanks @JimBobSquarePants for pushing this to happen 馃嵎
yufeih
on 15 Jul 2016
That's great! @JimBobSquarePants, what's the branch again? I really want to see what you did
SamuelEnglard
on 15 Jul 2016
It's on the "Core-Flava" Branch.
@JimBobSquarePants Nice work getting the time down!
Theres is something I don't quite get though. Why did you make the packed value generic? I get that Image
Additionaly: How would I define a BGRA image with float or worse, double components now? Image< Bgra64, ???? > Image< Bgra128, ???? > Image< Bgra256, ???? > Image< Gray8, byte> Image< Gray16, ushort> ImageF< Gray32, float > Image< Gray32, uint> and so on can't be the solution. I hope I am missing something obivious?
Additionaly: Would Image and ImageF be compatible in operations like add?
legg33
on 15 Jul 2016
Thanks @Grebe-M
I'm going to continue my research, mostly for fun at this point 馃槃
SamuelEnglard
on 15 Jul 2016
@SamuelEnglard Yeah Core-Flava, you know, cos of all the flavas of pixel formats (I am sooo coool :wink: )
Why did you make the packed value generic?
@Grebe-M Because it is exposed as a generic PackedValue property in MonoGame API as IPackedVector<T>.PackedValue which in-turn implements the Microsoft XNA Framework 4 API and I needed to ensure compatibility.
Note. I'm actually exposing that as a method since it allowed me to make Color a blittable type which helps makes things much faster.
You can see the list here.
https://github.com/mono/MonoGame/tree/develop/MonoGame.Framework/Graphics/PackedVector
As far as I could figure, I have to pass the second generic TP parameter from the very top level to ensure that I was actually dealing with packed vector as a a type rather than an interface. Ensuring this was key to getting the time down.
I know Image<Color, uint> its clunky but its the best I can do. It's also possible for us to wrap that in another class as I have which means most developers don't need to know or care. By default they would use the Image class.
People who know what they are doing would use things like Image<Alpha8, byte> or we can even write wrapper classes for them also - ImageAlpha8 maybe?
I'm going to have to put some thought into the ColorF struct as it looks like that would essentially be an implementation of HalfVector4, this would mean that it can't actually have RGBA fields or properties like Color since they take up more space than the packed value. (I actually don't know what existing structure you could sit 4 floats within without reducing accuracy). I guess accessing components would have to be done in the vector space.
I've removed the dedicated Add, Subtract, etc methods from the interface as unpacking toVector4 and doing the operations there will allow automatic conversion once the vector is repacked.
Does this make more sense?
JimBobSquarePants
on 16 Jul 2016
Gonna close this off now as I've just merged the new code. We now use 75% less memory, run faster than previous builds on my test machine, and also have tight control over ParallelOptions via the Bootstrapper class.
JimBobSquarePants
on 30 Jul 2016
Related issues
x2764tech
路
7Comments
skttl
路
5Comments
Bartmax
路
8Comments
clarkd
路
8Comments
mortenbock
路
3Comments