Skip to content

Conversation

@ironclownfish
Copy link
Contributor

@ironclownfish ironclownfish commented Mar 23, 2020

A new PR for speeding up image capture, better than my previous PR #2453.

  • Includes previous PR, which:

    • Locks the render target, reads the capture, then unlocks, instead of using ReadPixels().
    • Removes some unneeded complexity, like PNG compression and float-valued rgb
    • Removes extra mallocs/copies
    • Attempts to simplify the code, reduce looping and branching.
  • And adds the following

    • Implementation of a BufferPool, and application of it to reduce memory usage in image capture.
    • Removed all the remaining extra memcopies, as far as I can tell.
    • Cleaned up some of the sloppy parts of last PR, removed a couple unrelated commits, and re-instated simGetImages (the plural one)

@ironclownfish ironclownfish requested a review from madratman March 23, 2020 22:21
@madratman madratman mentioned this pull request Mar 24, 2020
@rajat2004
Copy link
Contributor

rajat2004 commented Mar 25, 2020

Still some compilation errors on Linux, when building Unreal project

/home/rajat/AirSim/Unreal/Environments/Blocks/Plugins/AirSim/Source/UnrealImageCapture.cpp:39:2: error: use of undeclared identifier '_BitScanReverse'
        _BitScanReverse(&log2, width - 1);
        ^
/home/rajat/AirSim/Unreal/Environments/Blocks/Plugins/AirSim/Source/UnrealImageCapture.cpp:41:30: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]
        response.image_data_uint8 = std::move(BufferPool_->GetBufferExactSize(height * stride * 4));
                                    ^
/home/rajat/AirSim/Unreal/Environments/Blocks/Plugins/AirSim/Source/UnrealImageCapture.cpp:41:30: note: remove std::move call here
        response.image_data_uint8 = std::move(BufferPool_->GetBufferExactSize(height * stride * 4));
                                    ^~~~~~~~~~                                                    ~

Will open PR to the branch if I'm able to fix these

Edit: Also, it would be great to have some measure of the improvement which can be expected from this, such as FPS of simGetImages before and after this PR!

@rajat2004
Copy link
Contributor

After modfiying the code a bit for compilation, more prblems occurred
Code:

#ifndef __GNUC__
	_BitScanReverse(&log2, width - 1);
#else
    log2 = __builtin_clz(width - 1) + 1;
#endif

	unsigned long long stride = 1ULL << (log2 + 1); //Round up to nearest power of 2.
	response.image_data_uint8 = BufferPool_->GetBufferExactSize(height * stride * 4);

Compilation successful, but error while starting UE

[2020.03.25-11.11.19:732][  0]Error: CDO Constructor (SimModeBase): Failed to find /AirSim/Blueprints/BP_PIPCamera.BP_PIPCamera_C

Multiple error messages with the same info, and then seg fault with the following trace

[2020.03.25-11.13.42:552][174]LogActor: Warning: BP_CameraDirector_C /Game/FlyingCPP/Maps/UEDPIE_0_FlyingExampleMap.FlyingExampleMap:PersistentLevel.CameraDirector has natively added scene component(s), but none of them were set as the actor's RootComponent - picking one arbitrarily
[2020.03.25-11.13.42:552][174]LogSpawn: Warning: SpawnActor failed because no class was specified
[2020.03.25-11.13.45:176][174]LogSpawn: Warning: SpawnActor failed because no class was specified
Signal 11 caught.
Malloc Size=131076 LargeMemoryPoolOffset=131092 
CommonLinuxCrashHandler: Signal=11
Malloc Size=65535 LargeMemoryPoolOffset=196655 
[2020.03.25-11.13.45:488][174]LogLinux: === Critical error: ===
Unhandled Exception: SIGSEGV: invalid attempt to read memory at address 0x0000000000000178

[2020.03.25-11.13.45:488][174]LogLinux: Fatal error!

0x00007f4080dd006f FLinuxPlatformStackWalk::CaptureStackBackTrace(unsigned long long*, unsigned int, void*)
0x00007f4080c76445 FGenericPlatformStackWalk::StackWalkAndDump(char*, unsigned long, int, void*)
0x00007f4080d7aa51 FLinuxCrashContext::CaptureStackTrace()
0x00007f4075748360 CommonLinuxCrashHandler(FGenericCrashContext const&)
0x00007f4080d7c8f2 PlatformCrashHandler(int, siginfo_t*, void*)
0x00007f40819b0890 /lib/x86_64-linux-gnu/libpthread.so.0(+0x12890) [0x7f40819b0890]
0x00007f407cd4a0c5 AActor::AttachToComponent(USceneComponent*, FAttachmentTransformRules const&, FName)
0x00007f3f746951a3 ACarPawn::initializeForBeginPlay(bool)
0x00007f3f74690374 ASimModeBase::setupVehiclesAndCamera()
0x00007f3f7468c74e ASimModeBase::BeginPlay()
0x00007f3f7469a1a9 ASimModeCar::BeginPlay()
0x00007f407cd58d4e AActor::DispatchBeginPlay()
0x00007f407cd580c9 AActor::PostActorConstruction()
0x00007f407cd55c53 AActor::FinishSpawning(FTransform const&, bool, FComponentInstanceDataCache const*)
0x00007f407cd53a43 AActor::PostSpawnInitialize(FTransform const&, AActor*, APawn*, bool, bool, bool)
0x00007f407d66a45d UWorld::SpawnActor(UClass*, FTransform const*, FActorSpawnParameters const&)
0x00007f407d66b06c UWorld::SpawnActor(UClass*, FVector const*, FRotator const*, FActorSpawnParameters const&)
0x00007f3f7468880a ASimHUD::createSimMode()
0x00007f3f74687d75 ASimHUD::BeginPlay()
0x00007f407cd58d4e AActor::DispatchBeginPlay()
0x00007f407e0c50df AWorldSettings::NotifyBeginPlay()
0x00007f407d4e922c AGameStateBase::HandleBeginPlay()
0x00007f407e0af7d9 UWorld::BeginPlay()
0x00007f407d4c750c UGameInstance::StartPlayInEditorGameInstance(ULocalPlayer*, FGameInstancePIEParameters const&)
0x00007f4077535161 UEditorEngine::CreatePIEGameInstance(int, bool, bool, bool, bool, float)
0x00007f407752dbbb UEditorEngine::PlayInEditor(UWorld*, bool)
0x00007f40775203e8 UEditorEngine::StartQueuedPlayMapRequest()
0x00007f4076ea2980 UEditorEngine::Tick(float, bool)
0x00007f40778b5d06 UUnrealEdEngine::Tick(float, bool)
0x0000000000424ed7 FEngineLoop::Tick() [/home/rajat/UnrealEngine/Engine/Source/Runtime/Launch/Private/LaunchEngineLoop.cpp:3296]
0x000000000042dee3 GuardedMain(wchar_t const*) [/home/rajat/UnrealEngine/Engine/Source/Runtime/Launch/Private/Launch.cpp:62]
0x00007f4075749058 CommonLinuxMain(int, char**, int (*)(wchar_t const*))
0x00007f407455fb97 /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xe7) [0x7f407455fb97]
0x000000000041729a /home/rajat/UnrealEngine/Engine/Binaries/Linux/UE4Editor(_start+0x2a) [0x41729a]

@madratman madratman force-pushed the faster_image_capture branch from a23db70 to 4731e5e Compare March 25, 2020 23:36
@madratman
Copy link
Contributor

madratman commented Mar 25, 2020

Rebased onto current master as this had some merge conflicts wrt change in the simSwapTextureApi (noticed some space<->tab inconsistencies in swaptexture PR just now).
linux is now compiling with fixes by @ironclownfish

@madratman madratman force-pushed the faster_image_capture branch 2 times, most recently from 960c3a6 to de9cf5b Compare March 25, 2020 23:38
//msr::airlib::ClockBase* clock = msr::airlib::ClockFactory::get();

ImagesResult result;
/*ImagesResult result;
Copy link
Contributor

Choose a reason for hiding this comment

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

?

left_img[idx-counter] = result->response.at(0).image_data_uint8 [idx];
right_img[idx-counter] = result->response.at(1).image_data_uint8 [idx];
//left_img[idx-counter] = result->response.at(0).image_data_uint8 [idx];
//right_img[idx-counter] = result->response.at(1).image_data_uint8 [idx];
Copy link
Contributor

Choose a reason for hiding this comment

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

?

@madratman madratman force-pushed the faster_image_capture branch 2 times, most recently from 2e55e47 to 142f860 Compare March 26, 2020 03:18
def image_callback_benchmark_simGetImage(self):
self.image_benchmark_num_images += 1
iter_start_time = time.time()
image = self.airsim_client.simGetImage("fpv_cam", airsim.ImageType.Scene)
Copy link
Contributor

Choose a reason for hiding this comment

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

need to update simGetImage to return an ImageResponse instead of only raw data, for help in decoding


virtual void initialize() = 0;

virtual std::vector<ImageCaptureBase::ImageResponse> getImages(const std::vector<ImageCaptureBase::ImageRequest>& request) const = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated to this PR
but are the functions just above this on L40-44 needed anymore?

    virtual const ImageCaptureBase* getImageCapture() const = 0;
    virtual ImageCaptureBase* getImageCapture()
    {
        return const_cast<ImageCaptureBase*>(static_cast<const VehicleSimApiBase*>(this)->getImageCapture());
    }

@madratman
Copy link
Contributor

@rajat2004 agreed wrt before and after benchmark. We'll add share some benchmarks in the comments here.
I pushed a little benchmark script (has some hardcodes for 240x320 as of now).

@rajat2004
Copy link
Contributor

@madratman Great, and thanks for the script! Will test things on Linux and see if it's working

@rajat2004
Copy link
Contributor

I'm still getting the error from #2472 (comment), which I think is happening due to the .uasset file being updated

LogLinker: Warning: Unable to load package (../../../../AirSim/Unreal/Environments/Blocks/Plugins/AirSim/Content/Blueprints/BP_PIPCamera.uasset) PackageVersion 517, MaxExpected 514 : LicenseePackageVersion 0, MaxExpected 0.

@madratman
Copy link
Contributor

madratman commented Mar 26, 2020

weird. mind doing a full cleanup / asserting you're not missing any step in the following? I haven't seen that error (have compiled this PR branch on linux multiple times now).
I'd clear all junk (Saved/Intermediate/Binary directories)in the unreal/env/blocks folder (git clean -fdx should do that for you)

cd AirSim;
git checkout master; # update if not already: git pull upstream master --rebase
git clean -fdx;
git fetch upstream pull/2472/head:PR/2472; git checkout PR/2472;
./setup.sh && ./build.sh;
cd Unreal/Environments/Blocks; 
./update_from_git.sh
PATH_TO/UnrealEngine/Engine/Binaries/Linux/UE4Editor "${PWD}/Blocks.uproject" 

P.S. PR is not done yet. there some issues in decoding the image

@rajat2004
Copy link
Contributor

rajat2004 commented Mar 26, 2020

@madratman I ran the clean_rebuild.sh script, and also did all the steps you mentioned above, still the same error
The branch compiles, but errors while running the project. Which UE version were you using? Mine is 4.18
It might be the case that the asset was created with a later version of Unreal Engine, based on the error message above- PackageVersion 517, MaxExpected 514
Tried importing the asset in the Editor to try to rebuild it, but unable to import, not showing up in the Content explorer

@madratman
Copy link
Contributor

I tested on 4.22, 4.24. Any reason you're on 4.18? With this PR and static mesh one, it seems that we'll have to drop support for 4.18.
Unreal Engine refactored their mesh drawing pipeline, and I suspect that is one reason I had to do 09b22aa.
Yet to test backward compatibility of uasset change in this PR wrt 4.18.
In >=4.20, UE performance on linux was improved with vulkan. they have been adding more improvements / fixing vulkan related issues in 4.22, 4.24, and upcoming 4.25

@rajat2004
Copy link
Contributor

Just that I didn't have any specific reason to upgrade to 4.22. I'll install 4.24 and test, but since you've tested, should be good to go
Will need to update the docs to mention that >=4.22 is supported, and maybe mention the last commit which works on 4.18

@madratman madratman force-pushed the faster_image_capture branch from 458c023 to 220ad4b Compare March 27, 2020 20:33
@rajat2004
Copy link
Contributor

rajat2004 commented Mar 28, 2020

I updated my UE installation to 4.24(clang-8), now I'm getting compilation errors

In file included from /home/rajat/AirSim/Unreal/Environments/Blocks/Plugins/AirSim/Intermediate/Build/Linux/B4D820EA/UE4Editor/Development/AirSim/Module.AirSim.cpp:24:
In file included from /home/rajat/AirSim/Unreal/Environments/Blocks/Plugins/AirSim/Source/Recording/RecordingThread.cpp:7:
/home/rajat/AirSim/Unreal/Environments/Blocks/Plugins/AirSim/Source/RenderRequest.h:26:9: error: explicitly defaulted copy constructor is implicitly deleted [-Werror,-Wdefaulted-function-deleted]
        RenderResult(RenderResult&) = default;
        ^
/home/rajat/AirSim/Unreal/Environments/Blocks/Plugins/AirSim/Source/RenderRequest.h:34:91: note: copy constructor of 'RenderResult' is implicitly deleted because field 'pixels' has a deleted copy constructor
        std::unique_ptr<std::vector<uint8_t>, std::function<void(std::vector<uint8_t>*)>> pixels = nullptr;
                                                                                          ^
/home/rajat/UE_4.24/Engine/Source/ThirdParty/Linux/LibCxx/include/c++/v1/memory:2494:3: note: copy constructor is implicitly deleted because 'unique_ptr<std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >, std::__1::function<void (std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > *)> >' has a user-declared move constructor
  unique_ptr(unique_ptr&& __u) noexcept
  ^
In file included from /home/rajat/AirSim/Unreal/Environments/Blocks/Plugins/AirSim/Intermediate/Build/Linux/B4D820EA/UE4Editor/Development/AirSim/Module.AirSim.cpp:25:
/home/rajat/AirSim/Unreal/Environments/Blocks/Plugins/AirSim/Source/RenderRequest.cpp:13:57: error: member initializer 'buffer_pool_' does not name a non-static data member or base class
RenderRequest::RenderRequest(BufferPool *buffer_pool) : buffer_pool_(buffer_pool)
                                                        ^~~~~~~~~~~~~~~~~~~~~~~~~
/home/rajat/AirSim/Unreal/Environments/Blocks/Plugins/AirSim/Source/RenderRequest.cpp:27:46: error: binding value of type 'volatile RenderRequest::RenderResult' to reference to type 'RenderRequest::RenderResult' drops 'volatile' qualifier
            this->RenderThreadScreenshotTask(this->latest_result_);
                                             ^~~~~~~~~~~~~~~~~~~~
/home/rajat/AirSim/Unreal/Environments/Blocks/Plugins/AirSim/Source/RenderRequest.h:65:51: note: passing argument to parameter 'result' here
    void RenderThreadScreenshotTask(RenderResult &result);
                                                  ^
In file included from /home/rajat/AirSim/Unreal/Environments/Blocks/Plugins/AirSim/Intermediate/Build/Linux/B4D820EA/UE4Editor/Development/AirSim/Module.AirSim.cpp:25:
/home/rajat/AirSim/Unreal/Environments/Blocks/Plugins/AirSim/Source/RenderRequest.cpp:48:21: error: use of undeclared identifier 'buffer_pool_'
    result.pixels = buffer_pool_->GetBufferExactSize(height*stride);
                    ^
/home/rajat/AirSim/Unreal/Environments/Blocks/Plugins/AirSim/Source/RenderRequest.cpp:54:48: error: no viable overloaded 'operator->'
                FMemory::BigBlockMemcpy(latest_result_.pixels->data(), src, height * stride);
                                        ~~~~~~~~~~~~~~~~~~~~~^
/home/rajat/UE_4.24/Engine/Source/ThirdParty/Linux/LibCxx/include/c++/v1/memory:2620:11: note: candidate function not viable: 'this' argument has type 'volatile std::unique_ptr<std::vector<uint8_t>, std::function<void (std::vector<uint8_t> *)> >' (aka 'volatile unique_ptr<vector<unsigned char>, function<void (vector<unsigned char> *)> >'), but method is not marked volatile
  pointer operator->() const _NOEXCEPT {
          ^
In file included from /home/rajat/AirSim/Unreal/Environments/Blocks/Plugins/AirSim/Intermediate/Build/Linux/B4D820EA/UE4Editor/Development/AirSim/Module.AirSim.cpp:33:
/home/rajat/AirSim/Unreal/Environments/Blocks/Plugins/AirSim/Source/UnrealImageCapture.cpp:42:53: error: no viable overloaded 'operator->'
    int bytes = render_request.latest_result_.pixels->size();
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
/home/rajat/UE_4.24/Engine/Source/ThirdParty/Linux/LibCxx/include/c++/v1/memory:2620:11: note: candidate function not viable: 'this' argument has type 'volatile std::unique_ptr<std::vector<uint8_t>, std::function<void (std::vector<uint8_t> *)> >' (aka 'volatile unique_ptr<vector<unsigned char>, function<void (vector<unsigned char> *)> >'), but method is not marked volatile
  pointer operator->() const _NOEXCEPT {
          ^
In file included from /home/rajat/AirSim/Unreal/Environments/Blocks/Plugins/AirSim/Intermediate/Build/Linux/B4D820EA/UE4Editor/Development/AirSim/Module.AirSim.cpp:33:
/home/rajat/AirSim/Unreal/Environments/Blocks/Plugins/AirSim/Source/UnrealImageCapture.cpp:50:31: error: no viable overloaded '='
    response.image_data_uint8 = std::move(render_request.latest_result_.pixels);
    ~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/rajat/UE_4.24/Engine/Source/ThirdParty/Linux/LibCxx/include/c++/v1/memory:2401:28: note: candidate function (the implicit copy assignment operator) not viable: 1st argument ('typename remove_reference<volatile unique_ptr<vector<unsigned char, allocator<unsigned char> >, function<void (vector<unsigned char, allocator<unsigned char> > *)> > &>::type' (aka 'volatile std::__1::unique_ptr<std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >, std::__1::function<void (std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > *)> >')) would lose volatile qualifier
class _LIBCPP_TEMPLATE_VIS unique_ptr {
                           ^
/home/rajat/UE_4.24/Engine/Source/ThirdParty/Linux/LibCxx/include/c++/v1/memory:2517:15: note: candidate function not viable: 1st argument ('typename remove_reference<volatile unique_ptr<vector<unsigned char, allocator<unsigned char> >, function<void (vector<unsigned char, allocator<unsigned char> > *)> > &>::type' (aka 'volatile std::__1::unique_ptr<std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >, std::__1::function<void (std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > *)> >')) would lose volatile qualifier
  unique_ptr& operator=(unique_ptr&& __u) _NOEXCEPT {
              ^
/home/rajat/UE_4.24/Engine/Source/ThirdParty/Linux/LibCxx/include/c++/v1/memory:2609:15: note: candidate function not viable: no known conversion from 'typename remove_reference<volatile unique_ptr<vector<unsigned char, allocator<unsigned char> >, function<void (vector<unsigned char, allocator<unsigned char> > *)> > &>::type' (aka 'volatile std::__1::unique_ptr<std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >, std::__1::function<void (std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > *)> >') to 'std::nullptr_t' (aka 'nullptr_t') for 1st argument
  unique_ptr& operator=(nullptr_t) _NOEXCEPT {
              ^
/home/rajat/UE_4.24/Engine/Source/ThirdParty/Linux/LibCxx/include/c++/v1/memory:2528:15: note: candidate template ignored: deduced type 'unique_ptr<...>' of 1st parameter does not match adjusted type 'unique_ptr<...>' of argument [with _Up = std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >, _Ep = std::__1::function<void (std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > *)>, $2 = void, $3 = void]
  unique_ptr& operator=(unique_ptr<_Up, _Ep>&& __u) _NOEXCEPT {
              ^
/home/rajat/UE_4.24/Engine/Source/ThirdParty/Linux/LibCxx/include/c++/v1/memory:2599:7: note: candidate template ignored: could not match 'auto_ptr' against 'unique_ptr'
      operator=(auto_ptr<_Up> __p) {
      ^
7 errors generated.

Is anyone else also getting this? I ran the clean_rebuild.sh script also

#include "Async/Async.h"

RenderRequest::RenderRequest(std::vector<uint8_t>& rgba_output) : rgba_output_(&rgba_output)
RenderRequest::RenderRequest(BufferPool *buffer_pool) : buffer_pool_(buffer_pool)
Copy link
Contributor

@rajat2004 rajat2004 Mar 28, 2020

Choose a reason for hiding this comment

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

buffer_pool_ hasn't been declared as a variable in the class

@rajat2004
Copy link
Contributor

rajat2004 commented May 23, 2020

Hey, so some good news, I was back on Windows today after 2-3 weeks, and got this PR working!
Branch is here - https://github.com/rajat2004/AirSim/tree/pr/2472_rebased

FPS jumped by quite a bit, on master I was getting ~43, with the PR I'm getting ~69 on my laptop. This is with the Blocks env running in Editor. Need to test on Linux and see if it's working there, will mostly open a new PR after that

Re the .uasset change, it's currently using the master branch camera asset, so no change in that one, still not sure what was modified in the asset

One thing I noticed is that with the PR, we're getting 4 channels RGBA instead of 3 in master

Update: On Linux, Vulkan is causing the crash, OpenGL worked, but FPS is very low, ~29, need to compare to master.
Hmm, this is still much better than the current FPS of ~15 I'm getting on master, which seems too low to me. That's a huge difference between Win & Linux performance
Working! Getting ~36 FPS with Vulkan on Linux

Opened #2713 which replaces this one

@rajat2004 rajat2004 mentioned this pull request May 23, 2020
@rajat2004
Copy link
Contributor

rajat2004 commented May 24, 2020

@ironclownfish @madratman Thanks a lot for this awesome PR! I've opened #2713 which is upto date with master, and should work on Linux & Win. Any testing would be great!

Edit: It doesn't work properly right now, am getting random data in the image on Linux. Works with OpenGL, details in PR

@madratman madratman force-pushed the faster_image_capture branch from 9b74b56 to f56ace7 Compare June 22, 2020 20:00
@jonyMarino jonyMarino mentioned this pull request Sep 2, 2020
@rajat2004 rajat2004 mentioned this pull request Sep 13, 2020
5 tasks
@nightduck
Copy link

Could also link issues #3070 #2891 #2026 and #3297

@jonyMarino
Copy link
Collaborator

Closing for the sake of #3018

@jonyMarino jonyMarino closed this Feb 24, 2021
@sandilyasg
Copy link

@ironclownfish @rajat2004 would this work on Unity too?

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.

Too slow for recorded images on UE4.18

7 participants