[libcamera-devel] [PATCH 2/2] test: camera: Camera reconfiguration and fd-leak test

[Umang Jain]

Hi Laurent,

On 8/17/21 6:34 PM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Tue, Aug 17, 2021 at 05:56:46PM +0530, Umang Jain wrote:
>> This tests basically checks for two things:
>> - Camera reconfigurations without stopping CameraManager
>> - Fd leaks across IPA IPC boundary [1]
>>
>> Currently, it uses vimc, but can be easily changed to using another
>> platform (for e.g. IPU3) by changing kCamId_ and kIpaProxyName_.
>>
>> The test performs kNumOfReconfigures_ (currently set to 10)
>> reconfigurations of the camera. Each reconfiguration runs
>> start(), capture(100ms), stop() of the camera. Hence, the test
>> runs approximately for 1 second.
>>
>> For checking the fd leaks, the test monitors the /proc/$PROXY_PID/fd
>> directory for open fds. It compares the number of open fds after each
>> run to the number of open fds before the first run. If those two are
>> found to be mis-matched, the test shall report failure.
>>
>> Since, the test needs to test the IPA IPC code paths, it is meant to
>> always run with LIBCAMERA_IPA_FORCE_ISOLATION environment variable.
>>
>> [1] https://bugs.libcamera.org/show_bug.cgi?id=63
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>>   test/camera/camera_reconfigure.cpp | 257 +++++++++++++++++++++++++++++
>>   test/camera/meson.build            |   1 +
>>   2 files changed, 258 insertions(+)
>>   create mode 100644 test/camera/camera_reconfigure.cpp
>>
>> diff --git a/test/camera/camera_reconfigure.cpp b/test/camera/camera_reconfigure.cpp
>> new file mode 100644
>> index 00000000..b32264b1
>> --- /dev/null
>> +++ b/test/camera/camera_reconfigure.cpp
>> @@ -0,0 +1,257 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2021, Google Inc.
>> + *
>> + * Test:
>> + * - Camera's multiple reconfigurations without stopping CameraManager
>> + * - leaking fds across IPA IPC boundary
>> + */
>> +
>> +#include <dirent.h>
>> +#include <iostream>
>> +#include <fstream>
>> +
>> +#include <libcamera/framebuffer_allocator.h>
>> +
>> +#include <libcamera/base/event_dispatcher.h>
>> +#include <libcamera/base/file.h>
>> +#include <libcamera/base/thread.h>
>> +#include <libcamera/base/timer.h>
>> +
>> +#include "camera_test.h"
>> +#include "test.h"
>> +
>> +using namespace std;
>> +
>> +namespace {
>> +
>> +class CameraReconfigure : public CameraTest, public Test
>> +{
>> +public:
>> +	CameraReconfigure()
>> +		: CameraTest(kCamId_, true)
>> +	{
>> +	}
>> +
>> +private:
>> +	static constexpr const char *kCamId_ = "platform/vimc.0 Sensor B";
>> +	static constexpr const char *kIpaProxyName_ = "vimc_ipa_proxy";
>> +	static constexpr unsigned int kNumOfReconfigures_ = 10;
>> +
>> +	void requestComplete(Request *request)
>> +	{
>> +		if (request->status() != Request::RequestComplete)
>> +			return;
>> +
>> +		const Request::BufferMap &buffers = request->buffers();
>> +
>> +		/* Create a new request. */
>> +		const Stream *stream = buffers.begin()->first;
>> +		FrameBuffer *buffer = buffers.begin()->second;
>> +
>> +		request->reuse();
>> +		request->addBuffer(stream, buffer);
>> +		camera_->queueRequest(request);
>> +	}
>> +
>> +	int startAndStop()
>> +	{
>> +		StreamConfiguration &cfg = config_->at(0);
>> +
>> +		if (camera_->acquire()) {
>> +			cerr << "Failed to acquire the camera" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		if (camera_->configure(config_.get())) {
>> +			cerr << "Failed to set default configuration" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		Stream *stream = cfg.stream();
>> +
>> +		if (!allocated_) {
>> +			int ret = allocator_->allocate(stream);
>> +			if (ret < 0)
>> +				return TestFail;
>> +			allocated_ = true;
>> +		}
>> +
>> +		for (const unique_ptr<FrameBuffer> &buffer : allocator_->buffers(stream)) {
>> +			unique_ptr<Request> request = camera_->createRequest();
>> +			if (!request) {
>> +				cerr << "Failed to create request" << endl;
>> +				return TestFail;
>> +			}
>> +
>> +			if (request->addBuffer(stream, buffer.get())) {
>> +				cerr << "Failed to associating buffer with request" << endl;
>> +				return TestFail;
>> +			}
>> +
>> +			requests_.push_back(move(request));
>> +		}
>> +
>> +		camera_->requestCompleted.connect(this, &CameraReconfigure::requestComplete);
>> +
>> +		if (camera_->start()) {
>> +			cerr << "Failed to start camera" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		for (unique_ptr<Request> &request : requests_) {
>> +			if (camera_->queueRequest(request.get())) {
>> +				cerr << "Failed to queue request" << endl;
>> +				return TestFail;
>> +			}
>> +		}
>> +
>> +		EventDispatcher *dispatcher = Thread::current()->eventDispatcher();
>> +
>> +		Timer timer;
>> +		timer.start(100);
>> +		while (timer.isRunning())
>> +			dispatcher->processEvents();
>> +
>> +		if (camera_->stop()) {
>> +			cerr << "Failed to stop camera" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		if (camera_->release()) {
>> +			cerr << "Failed to release camera" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		camera_->requestCompleted.disconnect(this, &CameraReconfigure::requestComplete);
>> +
>> +		requests_.clear();
>> +
>> +		return 0;
>> +	}
>> +
>> +	int fdsOpen(pid_t pid)
>> +	{
>> +		string proxyFdPath = "/proc/" + to_string(pid) + "/fd";
>> +		DIR *dir;
>> +		struct dirent *ptr;
>> +		unsigned int openFds = 0;
>> +
>> +		dir = opendir(proxyFdPath.c_str());
>> +		if (dir == nullptr) {
>> +			int err = errno;
>> +			cerr << "Error opening " << proxyFdPath << ": "
>> +			     << strerror(-err) << endl;
>> +			return 0;
>> +		}
>> +
>> +		while ((ptr = readdir(dir)) != nullptr) {
>> +			if ((strcmp(ptr->d_name, ".") == 0) ||
>> +			    (strcmp(ptr->d_name, "..") == 0))
>> +				continue;
>> +
>> +			openFds++;
>> +		}
>> +		closedir(dir);
>> +
>> +		return openFds;
>> +	}
>> +
>> +	pid_t findProxyPid()
>> +	{
>> +		string proxyPid;
>> +		string proxyName(kIpaProxyName_);
>> +		DIR *dir;
>> +		struct dirent *ptr;
>> +
>> +		dir = opendir("/proc");
>> +		while((ptr = readdir(dir)) != nullptr) {
>> +			if(ptr->d_type != DT_DIR)
>> +				continue;
>> +
>> +			string pname("/proc/" + string(ptr->d_name) + "/comm");
>> +			if(File::exists(pname.c_str())) {
> Missing space.


I failed to run checkstyle.py before sending, apologies. I hit -ENOSPC 
on my primary device few days agp and I am ended up working on a adhoc 
clone which didn't had the checkstyle.py as post-commit hook

>
> I may have missed it, but I haven't seen an answer to my review of the
> previous version that proposed checking the number of open file
> descriptors in the VIMC IPA module itself. That would test for fd leaks
> in all tests using vimc, and wouldn't require finding child processes.


Coming to your request, the reply sounded to me as an enhancement in the 
test-able VIMC in general, to increase our test coverage and IPC code 
paths. I am assuming you don't intend to dis-regard this particular test 
in favor of having it plumbed down to VIMC IPA, or is it? I am assuming 
on my part that both can(or should) co-exist, latter being developed 
along the lines and out of scope of this patch. Our priorities are 
well-addressed by this test for now, so maybe can I track it in bugzilla 
and come back to it in coming weeks?


>
>> +				ifstream pfile(pname.c_str());
>> +				string comm;
>> +				getline (pfile, comm);
>> +				pfile.close();
>> +
>> +				proxyPid = comm == proxyName ?
>> +					   string(ptr->d_name) :
>> +					   "";
>> +			}
>> +
>> +			if (!proxyPid.empty())
>> +				break;
>> +		}
>> +		closedir(dir);
>> +
>> +		if (!proxyPid.empty())
>> +			return atoi(proxyPid.c_str());
>> +
>> +		return -1;
>> +	}
>> +
>> +	int init() override
>> +	{
>> +		if (status_ != TestPass)
>> +			return status_;
>> +
>> +		config_ = camera_->generateConfiguration({ StreamRole::StillCapture });
>> +		if (!config_ || config_->size() != 1) {
>> +			cerr << "Failed to generate default configuration" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		allocator_ = make_unique<FrameBufferAllocator>(camera_);
>> +		allocated_ = false;
>> +
>> +		return TestPass;
>> +	}
>> +
>> +	int run() override
>> +	{
>> +		unsigned int openFdsAtStart = 0;
>> +		unsigned int openFds = 0;
>> +
>> +		pid_t proxyPid = findProxyPid();
>> +		if (proxyPid < 0) {
>> +			cerr << "Cannot find " << kIpaProxyName_
>> +			     << " pid, exiting" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		openFdsAtStart = fdsOpen(proxyPid);
>> +		for (unsigned int i = 0; i < kNumOfReconfigures_; i++) {
>> +			startAndStop();
>> +			openFds = fdsOpen(proxyPid);
>> +			if (openFds == 0) {
>> +				cerr << "No open fds found whereas "
>> +				     << "open fds at start: " << openFdsAtStart
>> +				     << endl;
>> +				return TestFail;
>> +			}
>> +
>> +			if (openFds != openFdsAtStart) {
>> +				cerr << "Leaking fds for " << kIpaProxyName_
>> +				     << " - Open fds: " << openFds << " vs "
>> +				     << "Open fds at start: " << openFdsAtStart
>> +				     << endl;
>> +				return TestFail;
>> +			}
>> +		}
>> +
>> +		return TestPass;
>> +	}
>> +
>> +	bool allocated_;
>> +
>> +	vector<unique_ptr<Request>> requests_;
>> +
>> +	unique_ptr<CameraConfiguration> config_;
>> +	unique_ptr<FrameBufferAllocator> allocator_;
>> +};
>> +
>> +} /* namespace */
>> +
>> +TEST_REGISTER(CameraReconfigure)
>> diff --git a/test/camera/meson.build b/test/camera/meson.build
>> index 002a87b5..668d5c03 100644
>> --- a/test/camera/meson.build
>> +++ b/test/camera/meson.build
>> @@ -8,6 +8,7 @@ camera_tests = [
>>       ['buffer_import',           'buffer_import.cpp'],
>>       ['statemachine',            'statemachine.cpp'],
>>       ['capture',                 'capture.cpp'],
>> +    ['camera_reconfigure',      'camera_reconfigure.cpp'],
>>   ]
>>   
>>   foreach t : camera_tests