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

21437 diff mbox series

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@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(-)

Hi Paul

On Tue, Oct 01, 2024 at 02:09:06AM GMT, Paul Elder wrote:
> 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.

Which needs to be documented :)

>
> Signed-off-by: Paul Elder <paul.elder@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);
> +

To define the expected configuration format, I would make 2/2 precede 1/2

>  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_;
> +
>  	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_);

Can't you pass in the CameraConfiguration which containts the
StreamConfigurations and the the Orientation already ?

> +
>  	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);

I was really expecting to be operating on File instances

> +
> +	/*
> +	 * 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";
> +	dumpMetadata_->write(str.c_str(), str.size());

and use the libyaml emitter for this instead of raw writes

Have you considered it and decided it was better to use raw writes ?

Thanks
  j

> +	dumpMetadata_->flush();
>  }
>
>  PipelineHandler::~PipelineHandler()
>  {
>  	for (std::shared_ptr<MediaDevice> media : mediaDevices_)
>  		media->release();
> +
> +	if (dumpCaptureScript_)
> +		delete dumpCaptureScript_;
> +
> +	if (dumpMetadata_)
> +		delete dumpMetadata_;
>  }
>
>  /**
> @@ -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());
> +
> +	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
> --
> 2.39.2
>

Hi Paul,
   one more question

On Tue, Oct 01, 2024 at 02:09:06AM GMT, Paul Elder wrote:
> 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@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_;
> +
>  	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";
> +	dumpMetadata_->write(str.c_str(), str.size());
> +	dumpMetadata_->flush();

Do we automatically dump metadata as well, or should we rather use two
distinct environment variables to control dumping of controls and
metadata ? Dumping metadata for each frame might be expensive, maybe
we should enable it separately ?


>  }
>
>  PipelineHandler::~PipelineHandler()
>  {
>  	for (std::shared_ptr<MediaDevice> media : mediaDevices_)
>  		media->release();
> +
> +	if (dumpCaptureScript_)
> +		delete dumpCaptureScript_;
> +
> +	if (dumpMetadata_)
> +		delete dumpMetadata_;
>  }
>
>  /**
> @@ -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());
> +
> +	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
> --
> 2.39.2
>

On Wed, Oct 02, 2024 at 08:28:26AM +0200, Jacopo Mondi wrote:
> Hi Paul
> 
> On Tue, Oct 01, 2024 at 02:09:06AM GMT, Paul Elder wrote:
> > 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.
> 
> Which needs to be documented :)

I... opened the tab ready to write it then forgot to write it :)

> 
> >
> > Signed-off-by: Paul Elder <paul.elder@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);
> > +
> 
> To define the expected configuration format, I would make 2/2 precede 1/2

Indeed 2/2 going first doesn't actually break anything. Sure.

> 
> >  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_;
> > +
> >  	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_);
> 
> Can't you pass in the CameraConfiguration which containts the
> StreamConfigurations and the the Orientation already ?

CameraConfiguration can't be copied because it has virtual functions.
And CameraConfiguration is only available in Camera::configure(). So, no.

> 
> > +
> >  	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);
> 
> I was really expecting to be operating on File instances

Oh ig we could do that too. I copied a few ideas from the logger.

> 
> > +
> > +	/*
> > +	 * 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";
> > +	dumpMetadata_->write(str.c_str(), str.size());
> 
> and use the libyaml emitter for this instead of raw writes
> 
> Have you considered it and decided it was better to use raw writes ?

No I hadn't. I'll keep it raw for now though until you make the
YamlEmitter :)


Thanks,

Paul

> 
> > +	dumpMetadata_->flush();
> >  }
> >
> >  PipelineHandler::~PipelineHandler()
> >  {
> >  	for (std::shared_ptr<MediaDevice> media : mediaDevices_)
> >  		media->release();
> > +
> > +	if (dumpCaptureScript_)
> > +		delete dumpCaptureScript_;
> > +
> > +	if (dumpMetadata_)
> > +		delete dumpMetadata_;
> >  }
> >
> >  /**
> > @@ -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());
> > +
> > +	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
> > --
> > 2.39.2
> >

On Wed, Oct 02, 2024 at 11:59:21AM +0200, Jacopo Mondi wrote:
> Hi Paul,
>    one more question
> 
> On Tue, Oct 01, 2024 at 02:09:06AM GMT, Paul Elder wrote:
> > 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@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_;
> > +
> >  	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";
> > +	dumpMetadata_->write(str.c_str(), str.size());
> > +	dumpMetadata_->flush();
> 
> Do we automatically dump metadata as well, or should we rather use two
> distinct environment variables to control dumping of controls and
> metadata ? Dumping metadata for each frame might be expensive, maybe
> we should enable it separately ?

Good point yeah maybe we should.


Paul

> 
> 
> >  }
> >
> >  PipelineHandler::~PipelineHandler()
> >  {
> >  	for (std::shared_ptr<MediaDevice> media : mediaDevices_)
> >  		media->release();
> > +
> > +	if (dumpCaptureScript_)
> > +		delete dumpCaptureScript_;
> > +
> > +	if (dumpMetadata_)
> > +		delete dumpMetadata_;
> >  }
> >
> >  /**
> > @@ -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());
> > +
> > +	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
> > --
> > 2.39.2
> >

Hi

2024. szeptember 30., hétfő 19:09 keltezéssel, Paul Elder <paul.elder@ideasonboard.com> í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@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`?


> +
>  	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.


> +
> +	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
> --
> 2.39.2
> 


Regards,
Barnabás Pőcze

Hi Paul,

Thank you for the patch.

On Wed, Oct 02, 2024 at 07:20:48PM +0900, Paul Elder wrote:
> On Wed, Oct 02, 2024 at 08:28:26AM +0200, Jacopo Mondi wrote:
> > On Tue, Oct 01, 2024 at 02:09:06AM GMT, Paul Elder wrote:
> > > 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.
> > 
> > Which needs to be documented :)
> 
> I... opened the tab ready to write it then forgot to write it :)
> 
> > > Signed-off-by: Paul Elder <paul.elder@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;

You should include orientation.h, the compiler will need to to determine
the size of the enum, to determine the layout of the Camera class. It's
included indirectly at the moment, which explains why you don't get a
compilation error.

> > > +
> > >  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);

Pass it by value, it's an enum.

> > > +
> > 
> > To define the expected configuration format, I would make 2/2 precede 1/2
> 
> Indeed 2/2 going first doesn't actually break anything. Sure.
> 
> > >  protected:
> > >  	void registerCamera(std::shared_ptr<Camera> camera);
> > >  	void hotplugMediaDevice(MediaDevice *media);
> > > @@ -81,6 +87,11 @@ protected:
> > >  	CameraManager *manager_;
> > >
> > >  private:
> > > +	enum DumpMode {

Make it an enum class, you already prefix the enumerators with the enum
name when you use them.

> > > +		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_;
> > > +
> > >  	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_);
> > 
> > Can't you pass in the CameraConfiguration which containts the
> > StreamConfigurations and the the Orientation already ?
> 
> CameraConfiguration can't be copied because it has virtual functions.
> And CameraConfiguration is only available in Camera::configure(). So, no.
> 
> > 
> > > +
> > >  	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 */

\todo

or better, address it.

> > > +	const char *file = utils::secure_getenv("LIBCAMERA_DUMP_CAPTURE_SCRIPT");
> > > +	if (!file)
> > > +		return;
> > > +
> > > +	dumpCaptureScript_ = new std::ofstream(file);
> > 
> > I was really expecting to be operating on File instances
> 
> Oh ig we could do that too. I copied a few ideas from the logger.
> 
> > > +
> > > +	/*
> > > +	 * 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";
> > > +	dumpMetadata_->write(str.c_str(), str.size());
> > 
> > and use the libyaml emitter for this instead of raw writes
> > 
> > Have you considered it and decided it was better to use raw writes ?
> 
> No I hadn't. I'll keep it raw for now though until you make the
> YamlEmitter :)
> 
> > > +	dumpMetadata_->flush();
> > >  }
> > >
> > >  PipelineHandler::~PipelineHandler()
> > >  {
> > >  	for (std::shared_ptr<MediaDevice> media : mediaDevices_)
> > >  		media->release();
> > > +
> > > +	if (dumpCaptureScript_)
> > > +		delete dumpCaptureScript_;
> > > +
> > > +	if (dumpMetadata_)
> > > +		delete dumpMetadata_;
> > >  }
> > >
> > >  /**
> > > @@ -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 */

\todo

Same below.

> > > +
> > > +	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());

I wonder if you could write

	*dumpCaptureScript_ << ss.rdbuf();

But you could use dumpCaptureScript_ directly with the << operator
above, no need for an intermediate stringstream.

> > > +
> > > +	std::string str = "frames:\n";
> > > +	dumpCaptureScript_->write(str.c_str(), str.size());

	*dumpCaptureScript_ << "frames:\n";

> > > +	dumpCaptureScript_->flush();
> > > +}
> > > +
> > > +void PipelineHandler::dumpRequest(Request *request, DumpMode mode)
> > > +{
> > > +	ControlList &controls =

const

> > > +		mode == DumpMode::Controls ? request->controls()
> > > +					   : request->metadata();
> > > +	std::ostream *output =
> > > +		mode == DumpMode::Controls ? dumpCaptureScript_
> > > +					   : dumpMetadata_;
> > > +
> > > +	if (!output || controls.empty())
> > > +		return;
> > > +
> > > +	std::stringstream ss;

Use output directly, don't create an intermediate stringstream.

> > > +	/* 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()) */

Could you elaborate on this for my information ?

> > > +		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

I'm not too sure about that, I think we'll see some types of
applications (or frameworks) setting controls in every request. We need
to be ready for it.

> > > +	 * 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

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@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

On Thu, Oct 03, 2024 at 02:41:27AM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Wed, Oct 02, 2024 at 07:20:48PM +0900, Paul Elder wrote:
> > On Wed, Oct 02, 2024 at 08:28:26AM +0200, Jacopo Mondi wrote:
> > > On Tue, Oct 01, 2024 at 02:09:06AM GMT, Paul Elder wrote:
> > > > 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.
> > > 
> > > Which needs to be documented :)
> > 
> > I... opened the tab ready to write it then forgot to write it :)
> > 
> > > > Signed-off-by: Paul Elder <paul.elder@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;
> 
> You should include orientation.h, the compiler will need to to determine
> the size of the enum, to determine the layout of the Camera class. It's
> included indirectly at the moment, which explains why you don't get a
> compilation error.
> 
> > > > +
> > > >  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);
> 
> Pass it by value, it's an enum.
> 
> > > > +
> > > 
> > > To define the expected configuration format, I would make 2/2 precede 1/2
> > 
> > Indeed 2/2 going first doesn't actually break anything. Sure.
> > 
> > > >  protected:
> > > >  	void registerCamera(std::shared_ptr<Camera> camera);
> > > >  	void hotplugMediaDevice(MediaDevice *media);
> > > > @@ -81,6 +87,11 @@ protected:
> > > >  	CameraManager *manager_;
> > > >
> > > >  private:
> > > > +	enum DumpMode {
> 
> Make it an enum class, you already prefix the enumerators with the enum
> name when you use them.
> 
> > > > +		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_;
> > > > +
> > > >  	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_);
> > > 
> > > Can't you pass in the CameraConfiguration which containts the
> > > StreamConfigurations and the the Orientation already ?
> > 
> > CameraConfiguration can't be copied because it has virtual functions.
> > And CameraConfiguration is only available in Camera::configure(). So, no.
> > 
> > > 
> > > > +
> > > >  	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 */
> 
> \todo
> 
> or better, address it.
> 
> > > > +	const char *file = utils::secure_getenv("LIBCAMERA_DUMP_CAPTURE_SCRIPT");
> > > > +	if (!file)
> > > > +		return;
> > > > +
> > > > +	dumpCaptureScript_ = new std::ofstream(file);
> > > 
> > > I was really expecting to be operating on File instances
> > 
> > Oh ig we could do that too. I copied a few ideas from the logger.
> > 
> > > > +
> > > > +	/*
> > > > +	 * 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";
> > > > +	dumpMetadata_->write(str.c_str(), str.size());
> > > 
> > > and use the libyaml emitter for this instead of raw writes
> > > 
> > > Have you considered it and decided it was better to use raw writes ?
> > 
> > No I hadn't. I'll keep it raw for now though until you make the
> > YamlEmitter :)
> > 
> > > > +	dumpMetadata_->flush();
> > > >  }
> > > >
> > > >  PipelineHandler::~PipelineHandler()
> > > >  {
> > > >  	for (std::shared_ptr<MediaDevice> media : mediaDevices_)
> > > >  		media->release();
> > > > +
> > > > +	if (dumpCaptureScript_)
> > > > +		delete dumpCaptureScript_;
> > > > +
> > > > +	if (dumpMetadata_)
> > > > +		delete dumpMetadata_;
> > > >  }
> > > >
> > > >  /**
> > > > @@ -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 */
> 
> \todo
> 
> Same below.
> 
> > > > +
> > > > +	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());
> 
> I wonder if you could write
> 
> 	*dumpCaptureScript_ << ss.rdbuf();
> 
> But you could use dumpCaptureScript_ directly with the << operator
> above, no need for an intermediate stringstream.
> 

ack

> > > > +
> > > > +	std::string str = "frames:\n";
> > > > +	dumpCaptureScript_->write(str.c_str(), str.size());
> 
> 	*dumpCaptureScript_ << "frames:\n";
> 
> > > > +	dumpCaptureScript_->flush();
> > > > +}
> > > > +
> > > > +void PipelineHandler::dumpRequest(Request *request, DumpMode mode)
> > > > +{
> > > > +	ControlList &controls =
> 
> const
> 
> > > > +		mode == DumpMode::Controls ? request->controls()
> > > > +					   : request->metadata();
> > > > +	std::ostream *output =
> > > > +		mode == DumpMode::Controls ? dumpCaptureScript_
> > > > +					   : dumpMetadata_;
> > > > +
> > > > +	if (!output || controls.empty())
> > > > +		return;
> > > > +
> > > > +	std::stringstream ss;
> 
> Use output directly, don't create an intermediate stringstream.
> 
> > > > +	/* 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()) */
> 
> Could you elaborate on this for my information ?

This will print enums just as numbers, so you'd get "AeMeteringMode: 0"
instead of "AeMeteringMode: MeteringCentreWeighted".

> 
> > > > +		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
> 
> I'm not too sure about that, I think we'll see some types of
> applications (or frameworks) setting controls in every request. We need
> to be ready for it.
> 

Good point. Although I'm not sure how else to determine how often to
flush. Would it be good enough to add another environment variable to
set every how many frames to dump? And default to a value > 1? Or count
how often it's getting queued and adjust the dump rate dynamically (with
an environment variable to override to "dump every frame" for debugging
purposes)?

In either case ig it's time to investigate the actual overhead :/


Paul

> > > > +	 * 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

On Thu, Oct 03, 2024 at 09:46:38AM +0900, Paul Elder wrote:
> On Thu, Oct 03, 2024 at 02:41:27AM +0300, Laurent Pinchart wrote:
> > On Wed, Oct 02, 2024 at 07:20:48PM +0900, Paul Elder wrote:
> > > On Wed, Oct 02, 2024 at 08:28:26AM +0200, Jacopo Mondi wrote:
> > > > On Tue, Oct 01, 2024 at 02:09:06AM GMT, Paul Elder wrote:
> > > > > 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.
> > > > 
> > > > Which needs to be documented :)
> > > 
> > > I... opened the tab ready to write it then forgot to write it :)
> > > 
> > > > > Signed-off-by: Paul Elder <paul.elder@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;
> > 
> > You should include orientation.h, the compiler will need to to determine
> > the size of the enum, to determine the layout of the Camera class. It's
> > included indirectly at the moment, which explains why you don't get a
> > compilation error.
> > 
> > > > > +
> > > > >  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);
> > 
> > Pass it by value, it's an enum.
> > 
> > > > > +
> > > > 
> > > > To define the expected configuration format, I would make 2/2 precede 1/2
> > > 
> > > Indeed 2/2 going first doesn't actually break anything. Sure.
> > > 
> > > > >  protected:
> > > > >  	void registerCamera(std::shared_ptr<Camera> camera);
> > > > >  	void hotplugMediaDevice(MediaDevice *media);
> > > > > @@ -81,6 +87,11 @@ protected:
> > > > >  	CameraManager *manager_;
> > > > >
> > > > >  private:
> > > > > +	enum DumpMode {
> > 
> > Make it an enum class, you already prefix the enumerators with the enum
> > name when you use them.
> > 
> > > > > +		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_;
> > > > > +
> > > > >  	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_);
> > > > 
> > > > Can't you pass in the CameraConfiguration which containts the
> > > > StreamConfigurations and the the Orientation already ?
> > > 
> > > CameraConfiguration can't be copied because it has virtual functions.
> > > And CameraConfiguration is only available in Camera::configure(). So, no.
> > > 
> > > > 
> > > > > +
> > > > >  	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 */
> > 
> > \todo
> > 
> > or better, address it.
> > 
> > > > > +	const char *file = utils::secure_getenv("LIBCAMERA_DUMP_CAPTURE_SCRIPT");
> > > > > +	if (!file)
> > > > > +		return;
> > > > > +
> > > > > +	dumpCaptureScript_ = new std::ofstream(file);
> > > > 
> > > > I was really expecting to be operating on File instances
> > > 
> > > Oh ig we could do that too. I copied a few ideas from the logger.
> > > 
> > > > > +
> > > > > +	/*
> > > > > +	 * 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";
> > > > > +	dumpMetadata_->write(str.c_str(), str.size());
> > > > 
> > > > and use the libyaml emitter for this instead of raw writes
> > > > 
> > > > Have you considered it and decided it was better to use raw writes ?
> > > 
> > > No I hadn't. I'll keep it raw for now though until you make the
> > > YamlEmitter :)
> > > 
> > > > > +	dumpMetadata_->flush();
> > > > >  }
> > > > >
> > > > >  PipelineHandler::~PipelineHandler()
> > > > >  {
> > > > >  	for (std::shared_ptr<MediaDevice> media : mediaDevices_)
> > > > >  		media->release();
> > > > > +
> > > > > +	if (dumpCaptureScript_)
> > > > > +		delete dumpCaptureScript_;
> > > > > +
> > > > > +	if (dumpMetadata_)
> > > > > +		delete dumpMetadata_;
> > > > >  }
> > > > >
> > > > >  /**
> > > > > @@ -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 */
> > 
> > \todo
> > 
> > Same below.
> > 
> > > > > +
> > > > > +	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());
> > 
> > I wonder if you could write
> > 
> > 	*dumpCaptureScript_ << ss.rdbuf();
> > 
> > But you could use dumpCaptureScript_ directly with the << operator
> > above, no need for an intermediate stringstream.
> > 
> 
> ack
> 
> > > > > +
> > > > > +	std::string str = "frames:\n";
> > > > > +	dumpCaptureScript_->write(str.c_str(), str.size());
> > 
> > 	*dumpCaptureScript_ << "frames:\n";
> > 
> > > > > +	dumpCaptureScript_->flush();
> > > > > +}
> > > > > +
> > > > > +void PipelineHandler::dumpRequest(Request *request, DumpMode mode)
> > > > > +{
> > > > > +	ControlList &controls =
> > 
> > const
> > 
> > > > > +		mode == DumpMode::Controls ? request->controls()
> > > > > +					   : request->metadata();
> > > > > +	std::ostream *output =
> > > > > +		mode == DumpMode::Controls ? dumpCaptureScript_
> > > > > +					   : dumpMetadata_;
> > > > > +
> > > > > +	if (!output || controls.empty())
> > > > > +		return;
> > > > > +
> > > > > +	std::stringstream ss;
> > 
> > Use output directly, don't create an intermediate stringstream.
> > 
> > > > > +	/* 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()) */
> > 
> > Could you elaborate on this for my information ?
> 
> This will print enums just as numbers, so you'd get "AeMeteringMode: 0"
> instead of "AeMeteringMode: MeteringCentreWeighted".

The latter would be more readable indeed. At the cost of more CPU cycles
to output the value, as well as to parse the capture script. We can't
have our cake and eat it :(

> > > > > +		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
> > 
> > I'm not too sure about that, I think we'll see some types of
> > applications (or frameworks) setting controls in every request. We need
> > to be ready for it.
> 
> Good point. Although I'm not sure how else to determine how often to
> flush. Would it be good enough to add another environment variable to
> set every how many frames to dump? And default to a value > 1? Or count
> how often it's getting queued and adjust the dump rate dynamically (with
> an environment variable to override to "dump every frame" for debugging
> purposes)?

Let's not make it overly complicated, you can start with a constant
flush interval expressed as a number of requests. Store it in a static
constexpr, and set it to 1 to start with.

> In either case ig it's time to investigate the actual overhead :/

Having numbers will certainly help discussions. No need to get the
numbers to post v2 though.

> > > > > +	 * 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