Faster Image Capture #2713
Faster Image Capture #2713
Conversation
…more. Buffer pool might be useful for other things.
…er pool to the render thread so it can ask for the correct size of dest buffer when src buffer is known.
…e-0 buffer back into the response's image_data_uint8, which then returns that useless buffer to the buffer pool. Since RPC can't take over the unique_ptr that manages our buffer, we should send RPC a copy.
… Vulkan" With this modification, garbage data is returned This reverts commit e85fee6.
|
Update: The PR has been tested to be working correctly on Windows, on Linux, with Linux testing has been done with both 4.24 & 4.25 Any suggestions & testing would be great! Post on UE4 forums - https://answers.unrealengine.com/questions/961968/view.html, https://forums.unrealengine.com/development-discussion/c-gameplay-programming/1767926-rhilocktexture2d-returns-garbage-data-with-vulkan-on-linux |
|
This is amazing. Thank you so much for this amazing work <3 At our repo we have been trying to get this to work as well. We are on linux with vulkan, commented out the assert and are using |
|
I tested this out. Performance does seem amazing now from a first glance. However I noticed a few issues. Do note I'm on a pretty old version of AirSim (I'd say the version from mid 2019 for Unreal 4.18) and am using 4.22 atm. In the old version the RenderRequest functionality used BGR instead of RGB, which I always thought was odd (some CV2 specific choice?) so I changed it to RGB by changing the following: to RGB. This made my ROS code a lot easier to use. I couldn't not find in this PR how it was done now. Would be great if you could let me know if I could change it as well to RGB. I also experienced a crash almost on every run now now. Again, I'm on 4.22 with a pretty old version of Airsim with many custom changes so not sure how relevant this crash report is. Was running from Editor. I'll let you know if I get more information on it. Using this request setup: Got a breakpoint crash here: the exception: callstack: I also get a lot of warnings related to |
|
@WouterJansen Thanks for the detailed testing!
When testing this PR, you're on the latest master (well, even more cutting edge). Testing with 4.22 is great since I haven't done that
OpenCV uses BGR as it's default order, so probably yes
This currently just copies the entire block of pixels rendered by UE, see this. So even I'm not sure how to do that, need to look into that. But should be possible to write a wrapper code after calling the API to convert to desired format.
I'm not seeing these on Linux at least, and been some time since I tested on Win. Maybe was introduced by the recent revert to the asset which was disabling the flag? Also, could the crash be happening due to a specific image type? If possible, can you check if commenting out, say the Depth image doesn't cause it to crash? |
|
I'm not on the master, I'm on a older version of Airsim with just this PR implemented on top of it :) The RGB/BGR thing was solved by just changing the encoding in my ROS publish of the image. Not a big deal just need to alter my ROS node. I'll do some more testing to see if it's a particular image type that causes the crash. I did not test before turning I did notice another issue. Seems like turning on Edit: seems no matter what I do, compress is always |
|
Was doing another test. Now with only a scene capture (request:scene, compress=false,pixels_as_float=false). Got a new crash in
|
Oh ok, that makes sense
I think it might just be due to some pieces being missed out during implementing this, such as this, float will need some looking into. |
This seems to be somewhat common crash, like this, could be due to 4.22? Not sure |
Ok that makes sense. Well I could work around the float not being implemented yet, but would be good to have back at some point. Did some more testing, doesn't really seem to matter which camera type I use. Scene seems to make the crashes happen easily, just like all the others. Turning on and off Capture Every Frame and Always Persist Rendering State also made no difference. It's relatively random. It's more easy to trigger when looping it instead of the occasional request: while True:
requests = []
requests.append(airsimpy.ImageRequest("front_center", airsim.ImageType.Scene, False, False))
responses = client.simGetImages(requests)
time.sleep(0.1)I'll keep an eye out on this PR. If these crashes are only an issue that I am having I guess it's worth a full update of my Airsim to the latest versions :) |
|
So am doing a bit of testing today, but yeah, am able to trigger crash similar to this - #2713 (comment) @WouterJansen regarding |
If I recall correctly, in my old version of AirSim |
|
Worked on the float part a bit, my testing branch is here - https://github.com/rajat2004/AirSim/tree/faster_img_cap_test, compare against this branch can be seen here I'm still confused about the
For visualising the Depth image, it has to be called with In my testing branch, right now the Depth images are of type |
|
Is there currently any recommended branch or commit to use if I want to take a look at a working 'faster' version? |
|
@petergerten You could try out this or even https://github.com/rajat2004/AirSim/tree/faster_img_cap_test branch, the latter one has some more commits, but |
|
@madratman @saihv I was working on the |
|
I think we should move BufferPool to the unreal/plugins part of the code, as it's only used in the Unreal part. |
|
Any luck with the crashes? |
|
@WouterJansen No, it's still crashing. It seems to be more frequent on Windows than Linux, people have tried out 15 min runs with the binary without crashing, the editor does crash more. Also if it crashes once, then it mostly crashes on the next run as well. There might be some deeper issue at hand here, since it's crashing where the campers is activated. |
|
This is by far the current biggest bottleneck for me to use AirSim to the full potential. As soon as I add more then 1 camera to my If any additional testing or help is required here I would love to assist. |
|
I'll maybe try to work on this over the weekend, anyone is more than welcome to try it out and figure out the problems, everything is open-source. But fixing the crashes and making it work on Vulkan seems to be very difficult to me right now, any ideas or suggestions are welcome. |
H rajat2004. At least from my side the ParallelFor method sounds interesting. One of my issues is that I am testing Stereo Cameras in Unreal, and the lack of synchronisation between multiple cameras is a major issue. |
|
I did try out the |
|
|
||
| // Disable camera after capturing image, this reduces resource consumption when images are not being taken | ||
| // Particulary when a high-resolution camera is used occasionally | ||
| camera->setCameraTypeEnabled(image_type, false); |
There was a problem hiding this comment.
@rajat2004 Hi Rajat. Thanks for your hard work on this pull request. I've been looking at this issue for a little, and so far, after commenting out this line, everything works pretty much perfectly (please verify this, I made a few other modifications so it's possible I made a change somewhere else). I ran it twice for around 50 minutes each without a single crash. My guess is this line is causing a race condition somewhere. Maybe it needs to be run on a different thread? Please let me know if that change also works for you.
Best,
Tom
There was a problem hiding this comment.
Awesome, thanks for testing and figuring out the problems! I'll also try it out, if it works out then can be moved into a separate API call which the user calls the disable cameras when not needed
|
Here's a Linux Blocks binary with the suggested fix by @cthoma20-arc - https://drive.google.com/file/d/1sK-S0CX6cqGtuMeQLqz2OhAVVg6dZAq1/view?usp=sharing |
| class BufferCollection | ||
| { | ||
| public: | ||
| BufferCollection(size_t size) : Size(size) {} |
There was a problem hiding this comment.
There should probably be a destructor defined for this class to avoid memory leaks
There was a problem hiding this comment.
I'm not very sure about this, will need to think more and check if memory leak is happening
If destructor is defined, then copy constructor, assignment might also be required, atleast from what I understand
Any pointers, suggestions and discussions on this would be great!
There was a problem hiding this comment.
Please correct me if I'm wrong, but I believe a memory leak will occur since we are allocating Buffers using new but the buffers are never deleted. We make unique_ptrs using the allocated buffers, but we use a custom deleter so they aren't deleted. This isn't really a problem at the moment since a single static BufferPool is being used, but it could be an issue in the future if someone uses more BufferPool objects. The destructor should probably delete the pointers in AvailableBuffers so that that memory is freed. I think it would probably make sense to delete the copy constructor and assignment operator, since it doesn't really make sense to copy this kind of object.
Thanks again :)
| params_[i]->render_component->CaptureSceneDeferred(); | ||
| } | ||
| UAirBlueprintLib::RunCommandOnGameThread([this]() { | ||
| fast_cap_done_ = false; |
There was a problem hiding this comment.
Should this line be moved outside RunCommandOnGameThread, since the condition is checked later in the current thread?
There was a problem hiding this comment.
Yup, done, see rajat2004@901bd01
| //Try to wait just long enough for the render thread to finish the capture. | ||
| //Start with a long wait, then check for completion more frequently. | ||
| //TODO: Optimize these numbers. | ||
| for (int best_guess_cap_time_microseconds = 500; !fast_cap_done_; best_guess_cap_time_microseconds = 200) |
There was a problem hiding this comment.
Would it be possible to use a condition variable and a mutex here to block until the rendering is done instead of polling fast_cap_done_?
There was a problem hiding this comment.
Changed to mutex, condition_variable, see rajat2004@901bd01
Didn't notice any major difference, not expected also
|
Sorry for the late reply, I think I'll mostly open a new PR with the other branch, there are still major points which need to be fixed, but can be discussed on that PR |
|
Opened #3018 which replaces this, have added the major problems right now in the description there, any testing, reviews would be great! |
Rebased and working version of #2472. Needs more testing
See some benchmarks on my system here - #2472 (comment)
One thing I noticed is that with the PR, we're getting 4 channels RGBA instead of 3 in master, this now becomes inline with Unity which is also giving 4 channels
My later commits might be a bit haphazard, will clean up later
Binaries - https://drive.google.com/open?id=14t9rmTGR3Au0R5V8GoF5DfYKYcLOJSAU
Linux -
-openglgives correct images, default Vulkan gives scrambled data, see comment belowOn Linux, with binary, I'm getting about 2x improvement from ~15 (latest release) to ~35 FPS on my system
Image types
Scene,Segmentation,Normalswork correctly, howeverDepthimages is giving 8 channels somehow?If someone wants to test using this PR from source, use this branch - https://github.com/rajat2004/AirSim/tree/faster_img_cap_test (It's updated to latest master, and will be pushing any commits there first)