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

NVIDIA VisionWorks and stereo_image_proc #380

Open
wants to merge 2 commits into
base: melodic
Choose a base branch
from

Conversation

zlacelle
Copy link

@zlacelle zlacelle commented Feb 4, 2019

Changes and additions from Zach LaCelle at The MITRE Corporation to add NVIDIA VisionWorks support for stereo algorithm acceleration in ROS stereo_image_proc.

This includes:

  • Adding a library for visionworks functionality, linked against NVIDIA libovx and libnvx
  • Adding supporting calls in processor.h/processor.cpp to switch to this functionality
  • Editing dynamic-reconfigure options
  • Adding supporting documentation

This software should auto-detect the presence of installed VisionWorks code and, if detected, compile and link against said code. If the requirements are not met, then stereo_image_proc will not link in the new code and will continue to run in the traditional stereo_image_proc method.

…dd NVIDIA VisionWorks support for stereo algorithm acceleration in ROS stereo_image_proc.

This includes:
* Adding a library for visionworks functionality, linked against NVIDIA libovx and libnvx
* Adding supporting calls in processor.h/processor.cpp to switch to this functionality
* Editing dynamic-reconfigure options
* Adding supporting documentation

This software should auto-detect the presence of installed VisionWorks code and, if detected, compile and link against said code. If the requirements are not met, then stereo_image_proc will not link in the new code and will continue to run in the traditional stereo_image_proc method.
@SteveMacenski
Copy link
Member

@zlacelle hi, we're getting to all this. This is fabulous, great work! 2 asks before review:

  • we need this rebased against melodic for CI to work
  • we would like this as a separate nodelet from stereo proc so that a user can consciously and with intent chose to use this

@zlacelle
Copy link
Author

@SteveMacenski OK, can do. I'll rebase and give it a try.

For the rebase, I'll just re-branch off of melodic and create a new pull request. Is there another way you'd like it done?

For the separate nodelet, my plan is to just copy the entire stereo_image_proc nodelet code as is--I'm not going to be able to test it in hardware with any substantial changes. In the future, with this approach, I think it would be good to remove all non-VisionWorks code and have the nodelet exclusively make use of visionworks.

@SteveMacenski
Copy link
Member

SteveMacenski commented Apr 10, 2019

Hi,
For rebasing, you can look at the rebase command, but yes rebranching off melodic and submitting a new PR will do the job as well. However you like.

I'm not sure what you mean by not being able to test - has this not been validated? I think to give you some background on my motivation: stereo image proc is used by a bunch of people and works on robust APIs that themselves haven't changed in a long time. I wouldn't want to break anything for anyone, or have it magically start using GPU resources without asking, I'd like it to be an intentional and deliberate action for someone to say "use GPU", which could be done via a parameter, however that would involve changing the core stereo image proc with a pretty new GPU-vendor specific implementation. I'd much rather see a separate nodelet with just the GPU capabilities that has the same API so someone just changes a name and poof it works. Then once we're all comfortable with it working as expected, we might reintegrate it into the main stereo image proc as a parameter, but likely it'll live as a separate vw_stereo_image_proc

I'm also not certain how this CMakeLists work will scale with deploying debians from the build farm, I think it tries to link at compile time to vision works rather than at run time, but you'd have to inform me your expected behavior.

@zlacelle
Copy link
Author

Hi,

Yes, everything has been validated on a TX2 with a Bumblebee. What I mean to say is that the hardware I was using to test is no longer set up and readily available, so any major code changes (like fully removing non-GPU code from the GPU nodelet) would make me uncomfortable. But the existing code is tested.

I understand completely not wanting to modify the existing nodelet and APIs. I will make the separate nodelet you're mentioning, which should be just build system changes.

@zlacelle zlacelle changed the base branch from indigo to melodic April 15, 2019 19:57
@zlacelle
Copy link
Author

@SteveMacenski Rebased, will work on separate module for stereo_image_proc and associated libraries.

@JWhitleyWork
Copy link
Collaborator

@zlacelle This PR was made prior to me taking over maintainership of this repo. Is this still something you need? If so, could you please rebase to the latest melodic? If not, or you can no longer work on it, I would be happy to close the PR.

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

Successfully merging this pull request may close these issues.

4 participants