Addition plus division taking up all my CPU - Kinect depth denoising

Question

I have a method in my class that eats up something like 80% of my processor time while its running. In-fact one line within the method is responsible for the majority of this.

The idea behind this class is to stabilize the depth readings being returned by the Microsoft Kinect device by obtaining the average value of the last 4 frames plus the current one.

This is a the basic layout of the method concerned.

public event EventHandler<DenoisedDepthReadyEventArgs> DenoisedDepthReady;
private const int HistoricFrameCount = 4; 
private readonly KinectSensor _KinectSensor;
private List<DepthImagePixel[]> _LastFrames;
private FilteredDepthPixel[] _FilteredDepths;
private bool _ResolutionFilteredDepthsInitialized;
//This is invoked upto 30 times a second
private void KinectSensor_AllFramesReady(object sender, AllFramesReadyEventArgs e)
{
 using (DepthImageFrame depthFrame = e.OpenDepthImageFrame())
 {
 if (depthFrame != null)
 { 
 DepthImagePixel[] currentPixels = new DepthImagePixel[depthFrame.PixelDataLength]; //Note this is huge array, usually 307200
 depthFrame.CopyDepthImagePixelDataTo(currentPixels);
 this._LastFrames.Add(currentPixels);
 this.ReduceNoiseAndInvokeIfRequired(currentPixels, depthFrame.PixelDataLength, depthFrame.MaxDepth, depthFrame.MinDepth); 
 }
 }
} 
private void ReduceNoiseAndInvokeIfRequired(DepthImagePixel[] currentPixels, int totalPixels, int maxDepth, int minDepth)
{
 if (this._LastFrames.Count == HistoricFrameCount)
 {
 if (!this._ResolutionFilteredDepthsInitialized)
 {
 this._FilteredDepths = new FilteredDepthPixel[totalPixels];
 for (int i = 0; i < totalPixels; i++)
 {
 this._FilteredDepths[i] = new FilteredDepthPixel();
 }
 this._ResolutionFilteredDepthsInitialized = true;
 }
 for (int i = 0; i < this._LastFrames.Count; i++)
 {
 DepthImagePixel[] currentFrame = this._LastFrames[i];
 for (int p = 0; p < totalPixels; p++)
 {
 this._FilteredDepths[p].Depth = (this._FilteredDepths[p].Depth + currentFrame[p].Depth) / 2;
 }
 }
 for (int i = 0; i < currentPixels.Length; i++)
 {
 this._FilteredDepths[i].Depth = (this._FilteredDepths[i].Depth + currentPixels[i].Depth) / 2;
 this._FilteredDepths[i].IsKnownDepth = currentPixels[i].IsKnownDepth;
 this._FilteredDepths[i].PlayerIndex = currentPixels[i].PlayerIndex;
 }
 this._LastFrames.RemoveAt(this._LastFrames.Count - 1);
 RaiseOnDenoisedDepthEvent(this._FilteredDepths, maxDepth, minDepth);
 }
}

and here is what the performance profiler says about what I'm doing.

enter image description here

I'm trying to avoid threading and parallelization for the time being. I am also conscious that building a new collection each frame is probably silly too but not sure how to improve this given the immutability of the type I want to return.

Any input or guidance on alternative approaches would be highly appreciated.

Answer 1

@Jesse has already addressed the parallelization fun the way I would have addressed it. This answer merely adds a couple of coding style observations.

private readonly KinectSensor _KinectSensor;
private List<DepthImagePixel[]> _LastFrames;
private FilteredDepthPixel[] _FilteredDepths;
private bool _ResolutionFilteredDepthsInitialized;

These private fields don't follow the camelCase convention that's in order for private fields. Should be like this:

private readonly KinectSensor _kinectSensor;
private List<DepthImagePixel[]> _lastFrames;
private FilteredDepthPixel[] _filteredDepths;
private bool _resolutionFilteredDepthsInitialized;

Actually the last one, named like this, could be a delegate of some kind. I prefer using is or has as a prefix, it makes it easier to find the Boolean flags in the autocomplete dropdown:

private bool _isResolutionFilteredDepthsInitialized;

You're over-qualifying your member variables:

this._lastFrames.Add(currentPixels);
this.ReduceNoiseAndInvokeIfRequired(currentPixels, depthFrame.PixelDataLength, depthFrame.MaxDepth, depthFrame.MinDepth);

The this qualifier is redundant here, and is adding noise that's not needed. Ironic when you consider that the method invoked is called ReduceNoise ;)

_lastFrames.Add(currentPixels);
ReduceNoiseAndInvokeIfRequired(currentPixels, depthFrame.PixelDataLength, depthFrame.MaxDepth, depthFrame.MinDepth); 

In terms of readability, I find it particularly rewarding with cases like this:

if (!this._ResolutionFilteredDepthsInitialized)

That turn into this:

if (!_isResolutionFilteredDepthsInitialized)

But then again, your usage of this is consistent all across, so this is basically a matter of personal preference - I prefer only qualifying with this when it's needed.

One thing though, is that if you're going to keep all the this qualifiers, then the _ underscore prefix should be dropped on the private fields.

---

Your event here:

public event EventHandler<DenoisedDepthReadyEventArgs> DenoisedDepthReady;

Appears to be raised by this method call:

RaiseOnDenoisedDepthEvent(this._FilteredDepths, maxDepth, minDepth);

Convention would be to raise the event in a method like this:

public void OnDenoisedDepthReady(DenoisedDepthReadyEventArgs args)
{
 if (DenoisedDepthReady == null) return;
 DenoisedDepthReady(this, args);
}

Or (and?):

public void OnDenoisedDepthReady(FilteredDepthPixel[] depths, int minDepth, int maxDepth)
{
 if (DenoisedDepthReady == null) return;
 var args = new DenoisedDepthReadyEventArgs(depths, minDepth, maxDepth); // assuming constructor
 DenoisedDepthReady(this, args); // or pass args to above overload
}

• Maxim Gershkovich: Thanks very much for suggestions. I agree I end up with a bit of redundant code but I've always done it and its kinda become a never ending loop of wanting to stay consistent. Either way might be time for a change. One thing though, your example of invoking an event is a bit off. Check this stackoverflow post out stackoverflow.com/questions/786383/…
• Mathieu Guindon: @Maxim indeed, I should have mentioned that invoking events like this wasn't thread-safe.
• Maxim Gershkovich: Appreciate all the thought in your post, lovely to have someone review my code like that cause I work alone...

Answer 2

Why are you avoiding parallelization? This type of problem is known as "embarrassingly parallel" because each loop iteration is independent of any other. Quick replacement makes it happen:

for (int i = 0; i < this._LastFrames.Count; i++)
{
 DepthImagePixel[] currentFrame = this._LastFrames[i];
 Parallel.For(0, totalPixels, p =>
 {
 this._FilteredDepths[p].Depth = (this._FilteredDepths[p].Depth + currentFrame[p].Depth) / 2;
 });
}
Parallel.For(0, currentPixels.Length, i =>
{
 this._FilteredDepths[i].Depth = (this._FilteredDepths[i].Depth + currentPixels[i].Depth) / 2;
 this._FilteredDepths[i].IsKnownDepth = currentPixels[i].IsKnownDepth;
 this._FilteredDepths[i].PlayerIndex = currentPixels[i].PlayerIndex;
});

As to the answer to your question, where do you expect your code to be taking the bulk of its time? The rest of the code is initialization, flow control, etc. The hot code is looped calculation. That's a CPU-bound operation that can really only break free with parallelization. You might be able to speed it up by doing a shift right by 1 bit rather than the arithmetic division by 2, but that seems a hit hinkey.