Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed Color Image Mapping to be more accurate #48

Conversation

ChristopherRemde
Copy link

@ChristopherRemde ChristopherRemde commented Jul 2, 2020

Hi Marek,

while using Livescan, we noticed that it uses the color_image_to_depth_camera Transformation, which doesn't handle occlusion correctly. I think this was done for performance reasons?

Our team wanted to have more accuracy, so I implemented the depth_image_to_color_camera transformation. Here are the results:

Current version, which is using the color_image_to_depth_camera transformation:

snapshot01

This Pull Request, which is using the depth_image_to_color_camera transformation:

snapshot00

There are of course downsides to this.

This mapping technique is quite a bit slower than the original transformation, mainly because it produces a larger pointcloud, I implemented a color image resizer, which scales the color image down to the same height as the depth image, while preserving the aspect ratio of the color image. This produce a much smaller pointcloud.
I could also save some processing speed on the "StoreFrame" function by avoiding the use of "Vector.push_back".

But even using these tricks, this mapping has a higher performance cost, which could leed to a decrease in FPS on older systems.
However I think this is in a reasonable range, as I tested the software on two mid-tier processors (Desktop Ryzen 2600 and Mobile 8750H) and they held a stable 30 FPS while recording.

That said, I'm also quite a newbie regarding C++, so I hope I didn't do anything catastrophically wrong. I tested the software quite a bit and didn't encounter any errors/Memory leaks.

Christopher Remde added 2 commits July 2, 2020 13:50
…f the Color Image via OpenCV for improved performance and improved performance on the StoreFrame function by avoiding "vector.push_back"
@ChristopherRemde
Copy link
Author

Hi Marek,

Thanks, yes I can do that!
I'm on vacation in the next two weeks, so I'll provide the feature after that.

Best Regards!

Copy link
Owner

@MarekKowalski MarekKowalski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

Thanks a lot for submitting this, the results look great!
The PR looks good to me, though unfortunately I can't test it as I don't have access to my office where all the Kinects are. When you address the comments, I am fine merging this.

Thanks,

Marek

bin/LiveScanPlayer.exe.config Outdated Show resolved Hide resolved
include/LiveScanClient/azureKinectCapture.h Outdated Show resolved Hide resolved
src/LiveScanClient/azureKinectCapture.cpp Outdated Show resolved Hide resolved
cv::Mat cImg = cv::Mat(k4a_image_get_height_pixels(colorImage), k4a_image_get_width_pixels(colorImage), CV_8UC4, k4a_image_get_buffer(colorImage));

//Resize the k4a_image to the precalculated size. Takes quite along time, maybe there is a faster algorithm?
cv::resize(cImg, cImg, cv::Size(color_image_downscaled_width, color_image_downscaled_height), cv::INTER_NEAREST);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using INTER_NEAREST may lead to fairly poor quality. I would suggest using INTER_LINEAR as a reasonable speed/quality tradeoff.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How badly would changing to INTER_LINEAR impact the performance?

src/LiveScanClient/azureKinectCapture.cpp Outdated Show resolved Hide resolved
@MarekKowalski
Copy link
Owner

Hi Christopher,

I think you answered to my previous comment about making this optional, which I since deleted - sorry.
On second though I realized that it makes sense for this feature to be enabled in all cases, especially given it works at full framerate on a mid-tier CPUs.
Also, when you checked the performance, how many Kinects did you run on the same machine?

Thanks,

Marek

@ChristopherRemde
Copy link
Author

Hi Marek,

Thank you for looking through the code and the detailed comments! I'll try to sort them out after the vacation.

Regarding the performance:

Both of the PCs I used weren't able to run more than one Kinect at full frame rate, even with the standard Livescan( for Azure Kinect), but I don't know if this is due to processor or USB limitations. So I didn't test this pull request with two Kinects at the same machine.

I'll try to get a bit more detailed performance statistics, so that you can decide if this should be enabled by default or as a an option!

@ChristopherRemde
Copy link
Author

Hi Marek,

So I did a Performance Test today of both the original Azure Kinect branch and this fix... And I could not really detect any significant difference between the two. But I think my testing method is flawed. Currently I set one breakpoint at the start of the update loop in liveScanClient.cpp and then read out the time it took to update one frame (Visual studio shows this per default at the right end of the line at the breakpoint). This happens in Debug mode, so the performance is obiously distorted, compared to Release Build performance.

Do you know any way to better measure the performance, maybe even for a Release Build?

@MarekKowalski
Copy link
Owner

Hi Christopher,

Visual Studio has a great profiler, which allows for measuring the performance of your code. Here is the documentation:
https://docs.microsoft.com/en-us/visualstudio/profiling/?view=vs-2019

Marek

@ChristopherRemde
Copy link
Author

Hi Marek,

thanks for your tip! I used the tool and the results are kinda interesting for me. So here they are:

  • The default Azure Kinect Branch uses ~730ms/Frame while ideling and ~800ms/Frame while Recording
  • This Pull Request uses ~520ms/Frame while ideling and ~600ms/Frame while Recording

I expected the results to be the other way around, so that this pull request is slower. I tried to to find the reason for this, and it basically comes down to the "mfmjpegdec.dll" not being used in this pull request. I don't really know the reason for this, maybe this has something to do with the K4A-SDK? I tested the Azure Kinect Branch and this pull request both with SDK Version 1.4.1

Here are the detailed results if you are interested:

Azure Kinect Branch:

Performance_Original_Azure_Kinect_Branch

This pull request:

Performance_Original_Mapping_Fix_Branch

Regarding the performance of "INTER_LINEAR" and INTER_NEAREST": I couldn't really detect any difference between the two, so I changed the interpolation method to inter_linear.

Otherwise I fixed all the changes you requested with the latest commit.

@MarekKowalski
Copy link
Owner

Hi Christopher,

Thanks a lot for looking into this. Did you profile the app in Debug or Release mode? The times looks to be quite long which makes me think it might have been Debug. Would you be able to profile again in Release? Sorry I'm not able to help, but I do not have access to a Kinect at the moment.

If that looks good I am happy to accept the PR.

Marek

@ChristopherRemde
Copy link
Author

Hello Marek,

yes you were right, I did profile in Debug mode, my bad! I didn't realize you could also profile in Release mode.

I did profile in Release mode again, and here the numbers look a lot more coherent with what I'd expect:

  • The default Azure Kinect Branch uses ~27-29ms/Frame while ideling and 28-30~ms/Frame while Recording
    
  • This Pull Request uses ~31-33ms/Frame while ideling and ~35-40ms/Frame while Recording
    

So there is a performance penalty while using this pull request.
There seems to be some performance overhead while profiling though, because running the build without it, the FPS-Counter in the Client Window never drops under 29.72 FPS while recording (for this pull request) . So I'm not really sure which measure is to trust.

@MarekKowalski
Copy link
Owner

Thanks for running the test Christopher. I think these numbers are fine, I'll now accept the PR.

Best regards,

Marek

@MarekKowalski MarekKowalski merged commit 13e634f into MarekKowalski:AzureKinect Aug 4, 2020
@ChristopherRemde
Copy link
Author

Thanks a lot Marek for taking the time to review this PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants