[PATCH 1/2] pipeline: Add support for dumping capture script and metadata

[Laurent Pinchart]

Hi Barnabás,

On Wed, Oct 02, 2024 at 01:28:51PM +0000, Barnabás Pőcze wrote:
> 2024. szeptember 30., hétfő 19:09 keltezéssel, Paul Elder írta:
> 
> > Add support for dumping capture scripts and metadata. The capture
> > scripts can then be fed into the cam application and a capture can thus
> > be "replayed". Metadata can also be dumped.
> > 
> > Camera configuration is also dumped to the capture script. The cam
> > application currently does not support loading configuration from the
> > capture script, but support for that will be added in a subsequent
> > patch.
> > 
> > These can be enabled by a new environment variable.
> > 
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > ---
> >  include/libcamera/internal/camera.h           |  3 +
> >  include/libcamera/internal/pipeline_handler.h | 16 ++++
> >  src/libcamera/camera.cpp                      | 13 +++
> >  src/libcamera/pipeline_handler.cpp            | 93 ++++++++++++++++++-
> >  4 files changed, 124 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h
> > index 0add0428bb5d..a42f03d4c755 100644
> > --- a/include/libcamera/internal/camera.h
> > +++ b/include/libcamera/internal/camera.h
> > @@ -19,6 +19,8 @@
> > 
> >  namespace libcamera {
> > 
> > +enum class Orientation;
> > +
> >  class CameraControlValidator;
> >  class PipelineHandler;
> >  class Stream;
> > @@ -65,6 +67,7 @@ private:
> >  	std::string id_;
> >  	std::set<Stream *> streams_;
> >  	std::set<const Stream *> activeStreams_;
> > +	Orientation orientation_;
> > 
> >  	bool disconnected_;
> >  	std::atomic<State> state_;
> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > index 0d38080369c5..fb3914185a01 100644
> > --- a/include/libcamera/internal/pipeline_handler.h
> > +++ b/include/libcamera/internal/pipeline_handler.h
> > @@ -9,6 +9,7 @@
> > 
> >  #include <memory>
> >  #include <queue>
> > +#include <set>
> >  #include <string>
> >  #include <sys/types.h>
> >  #include <vector>
> > @@ -20,6 +21,8 @@
> > 
> >  namespace libcamera {
> > 
> > +enum class Orientation;
> > +
> >  class Camera;
> >  class CameraConfiguration;
> >  class CameraManager;
> > @@ -68,6 +71,9 @@ public:
> > 
> >  	CameraManager *cameraManager() const { return manager_; }
> > 
> > +	void dumpConfiguration(const std::set<const Stream *> &streams,
> > +			       const Orientation &orientation);
> > +
> >  protected:
> >  	void registerCamera(std::shared_ptr<Camera> camera);
> >  	void hotplugMediaDevice(MediaDevice *media);
> > @@ -81,6 +87,11 @@ protected:
> >  	CameraManager *manager_;
> > 
> >  private:
> > +	enum DumpMode {
> > +		Controls,
> > +		Metadata,
> > +	};
> > +
> >  	void unlockMediaDevices();
> > 
> >  	void mediaDeviceDisconnected(MediaDevice *media);
> > @@ -89,6 +100,8 @@ private:
> >  	void doQueueRequest(Request *request);
> >  	void doQueueRequests();
> > 
> > +	void dumpRequest(Request *request, DumpMode mode);
> > +
> >  	std::vector<std::shared_ptr<MediaDevice>> mediaDevices_;
> >  	std::vector<std::weak_ptr<Camera>> cameras_;
> > 
> > @@ -97,6 +110,9 @@ private:
> >  	const char *name_;
> >  	unsigned int useCount_;
> > 
> > +	std::ostream *dumpCaptureScript_;
> > +	std::ostream *dumpMetadata_;
> 
> Have you looked at using `std::optional` here instead? Or `std::unique_ptr`?

Avoiding a manual delete is certainly a good idea.

> > +
> >  	friend class PipelineHandlerFactoryBase;
> >  };
> > 
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index a86f552a47bc..1282f99d839b 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -1215,6 +1215,9 @@ int Camera::configure(CameraConfiguration *config)
> >  		d->activeStreams_.insert(stream);
> >  	}
> > 
> > +	/* TODO Save sensor configuration for dumping it to capture script */
> > +	d->orientation_ = config->orientation;
> > +
> >  	d->setState(Private::CameraConfigured);
> > 
> >  	return 0;
> > @@ -1356,6 +1359,16 @@ int Camera::start(const ControlList *controls)
> > 
> >  	ASSERT(d->requestSequence_ == 0);
> > 
> > +	/*
> > +	 * Invoke method in blocking mode to avoid the risk of writing after
> > +	 * streaming has started.
> > +	 * This needs to be here as PipelineHandler::start is a virtual function
> > +	 * so it is impractical to add the dumping there.
> > +	 * TODO Pass the sensor configuration, once it is supported
> > +	 */
> > +	d->pipe_->invokeMethod(&PipelineHandler::dumpConfiguration,
> > +			       ConnectionTypeBlocking, d->activeStreams_, d->orientation_);
> > +
> >  	ret = d->pipe_->invokeMethod(&PipelineHandler::start,
> >  				     ConnectionTypeBlocking, this, controls);
> >  	if (ret)
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index e5940469127e..7002b4323bdd 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -8,6 +8,7 @@
> >  #include "libcamera/internal/pipeline_handler.h"
> > 
> >  #include <chrono>
> > +#include <fstream>
> >  #include <sys/stat.h>
> >  #include <sys/sysmacros.h>
> > 
> > @@ -68,14 +69,36 @@ LOG_DEFINE_CATEGORY(Pipeline)
> >   * through the PipelineHandlerFactoryBase::create() function.
> >   */
> >  PipelineHandler::PipelineHandler(CameraManager *manager)
> > -	: manager_(manager), useCount_(0)
> > +	: manager_(manager), useCount_(0),
> > +	  dumpCaptureScript_(nullptr), dumpMetadata_(nullptr)
> >  {
> > +	/* TODO Print notification that we're dumping capture script */
> > +	const char *file = utils::secure_getenv("LIBCAMERA_DUMP_CAPTURE_SCRIPT");
> > +	if (!file)
> > +		return;
> > +
> > +	dumpCaptureScript_ = new std::ofstream(file);
> > +
> > +	/*
> > +	 * Metadata needs to go into a separate file because otherwise it'll
> > +	 * flood the capture script
> > +	 */
> > +	dumpMetadata_ = new std::ofstream(std::string(file) + ".metadata");
> > +	std::string str = "frames:\n";
> 
> I think `std::string_view` is sufficient for this.
> 
> > +	dumpMetadata_->write(str.c_str(), str.size());
> > +	dumpMetadata_->flush();
> >  }
> > 
> >  PipelineHandler::~PipelineHandler()
> >  {
> >  	for (std::shared_ptr<MediaDevice> media : mediaDevices_)
> >  		media->release();
> > +
> > +	if (dumpCaptureScript_)
> > +		delete dumpCaptureScript_;
> > +
> > +	if (dumpMetadata_)
> > +		delete dumpMetadata_;
> 
> `delete` already checks for `nullptr`, so if you want to go the raw ptr way,
> then there is no need for an extra check. But as I mentioned I think `std::optional`
> or `std::unique_ptr` would probably be preferable.
> 
> >  }
> > 
> >  /**
> > @@ -464,6 +487,8 @@ void PipelineHandler::doQueueRequest(Request *request)
> > 
> >  	request->_d()->sequence_ = data->requestSequence_++;
> > 
> > +	dumpRequest(request, DumpMode::Controls);
> > +
> >  	if (request->_d()->cancelled_) {
> >  		completeRequest(request);
> >  		return;
> > @@ -555,6 +580,8 @@ void PipelineHandler::completeRequest(Request *request)
> > 
> >  	request->_d()->complete();
> > 
> > +	dumpRequest(request, DumpMode::Metadata);
> > +
> >  	Camera::Private *data = camera->_d();
> > 
> >  	while (!data->queuedRequests_.empty()) {
> > @@ -758,6 +785,70 @@ void PipelineHandler::disconnect()
> >   * \return The CameraManager for this pipeline handler
> >   */
> > 
> > +void PipelineHandler::dumpConfiguration(const std::set<const Stream *> &streams,
> > +					const Orientation &orientation)
> > +{
> > +	if (!dumpCaptureScript_)
> > +		return;
> > +
> > +	std::stringstream ss;
> > +	ss << "configuration:" << std::endl;
> > +	ss << "  orientation: " << orientation << std::endl;
> > +
> > +	/* TODO Dump Sensor configuration */
> > +
> > +	ss << "  streams:" << std::endl;
> > +	for (const auto &stream : streams) {
> > +		const StreamConfiguration &streamConfig = stream->configuration();
> > +		ss << "    - pixelFormat: " << streamConfig.pixelFormat << std::endl;
> > +		ss << "      size: " << streamConfig.size << std::endl;
> > +		ss << "      stride: " << streamConfig.stride << std::endl;
> > +		ss << "      frameSize: " << streamConfig.frameSize << std::endl;
> > +		ss << "      bufferCount: " << streamConfig.bufferCount << std::endl;
> > +		if (streamConfig.colorSpace)
> > +			ss << "      colorSpace: " << streamConfig.colorSpace->toString() << std::endl;
> > +	}
> > +
> > +	dumpCaptureScript_->write(ss.str().c_str(), ss.str().size());
> 
> It's probably best to avoid calling `std::stringstream::str()` twice since it
> returns a copy of the string twice in this case. I believe you might as well do
> `auto str = std::move(ss).str()`, which should avoid the copy altogether.

Even better, we shouldn't use a stringstream in the first place, we can
output to dumpCaptureScript_ directly.

> > +
> > +	std::string str = "frames:\n";
> > +	dumpCaptureScript_->write(str.c_str(), str.size());
> > +	dumpCaptureScript_->flush();
> > +}
> > +
> > +void PipelineHandler::dumpRequest(Request *request, DumpMode mode)
> > +{
> > +	ControlList &controls =
> > +		mode == DumpMode::Controls ? request->controls()
> > +					   : request->metadata();
> > +	std::ostream *output =
> > +		mode == DumpMode::Controls ? dumpCaptureScript_
> > +					   : dumpMetadata_;
> > +
> > +	if (!output || controls.empty())
> > +		return;
> > +
> > +	std::stringstream ss;
> > +	/* TODO Figure out PFC */
> > +	ss << "  - " << request->sequence() << ":" << std::endl;
> > +
> > +	const ControlIdMap *idMap = controls.idMap();
> > +	for (const auto &pair : controls) {
> > +		const ControlId *ctrlId = idMap->at(pair.first);
> > +		/* TODO Prettify enums (probably by upgrading ControlValue::toString()) */
> > +		ss << "      " << ctrlId->name() << ": " << pair.second.toString() << std::endl;
> > +	}
> > +
> > +	/*
> > +	 * TODO Investigate the overhead of flushing this frequently
> > +	 * Controls aren't going to be queued too frequently so it should be
> > +	 * fine to dump controls every frame. Metadata on the other hand needs
> > +	 * to be investigated.
> > +	 */
> > +	output->write(ss.str().c_str(), ss.str().size());
> > +	output->flush();
> > +}
> > +
> >  /**
> >   * \class PipelineHandlerFactoryBase
> >   * \brief Base class for pipeline handler factories

-- 
Regards,

Laurent Pinchart