[{"id":31516,"web_url":"https://patchwork.libcamera.org/comment/31516/","msgid":"","date":"2024-10-02T06:28:26","subject":"Re: [PATCH 1/2] pipeline: Add support for dumping capture script and\n\tmetadata","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Paul\n\nOn Tue, Oct 01, 2024 at 02:09:06AM GMT, Paul Elder wrote:\n> Add support for dumping capture scripts and metadata. The capture\n> scripts can then be fed into the cam application and a capture can thus\n> be \"replayed\". Metadata can also be dumped.\n>\n> Camera configuration is also dumped to the capture script. The cam\n> application currently does not support loading configuration from the\n> capture script, but support for that will be added in a subsequent\n> patch.\n>\n> These can be enabled by a new environment variable.\n\nWhich needs to be documented :)\n\n>\n> Signed-off-by: Paul Elder \n> ---\n> include/libcamera/internal/camera.h | 3 +\n> include/libcamera/internal/pipeline_handler.h | 16 ++++\n> src/libcamera/camera.cpp | 13 +++\n> src/libcamera/pipeline_handler.cpp | 93 ++++++++++++++++++-\n> 4 files changed, 124 insertions(+), 1 deletion(-)\n>\n> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> index 0add0428bb5d..a42f03d4c755 100644\n> --- a/include/libcamera/internal/camera.h\n> +++ b/include/libcamera/internal/camera.h\n> @@ -19,6 +19,8 @@\n>\n> namespace libcamera {\n>\n> +enum class Orientation;\n> +\n> class CameraControlValidator;\n> class PipelineHandler;\n> class Stream;\n> @@ -65,6 +67,7 @@ private:\n> \tstd::string id_;\n> \tstd::set streams_;\n> \tstd::set activeStreams_;\n> +\tOrientation orientation_;\n>\n> \tbool disconnected_;\n> \tstd::atomic state_;\n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 0d38080369c5..fb3914185a01 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -9,6 +9,7 @@\n>\n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -20,6 +21,8 @@\n>\n> namespace libcamera {\n>\n> +enum class Orientation;\n> +\n> class Camera;\n> class CameraConfiguration;\n> class CameraManager;\n> @@ -68,6 +71,9 @@ public:\n>\n> \tCameraManager *cameraManager() const { return manager_; }\n>\n> +\tvoid dumpConfiguration(const std::set &streams,\n> +\t\t\t const Orientation &orientation);\n> +\n\nTo define the expected configuration format, I would make 2/2 precede 1/2\n\n> protected:\n> \tvoid registerCamera(std::shared_ptr camera);\n> \tvoid hotplugMediaDevice(MediaDevice *media);\n> @@ -81,6 +87,11 @@ protected:\n> \tCameraManager *manager_;\n>\n> private:\n> +\tenum DumpMode {\n> +\t\tControls,\n> +\t\tMetadata,\n> +\t};\n> +\n> \tvoid unlockMediaDevices();\n>\n> \tvoid mediaDeviceDisconnected(MediaDevice *media);\n> @@ -89,6 +100,8 @@ private:\n> \tvoid doQueueRequest(Request *request);\n> \tvoid doQueueRequests();\n>\n> +\tvoid dumpRequest(Request *request, DumpMode mode);\n> +\n> \tstd::vector> mediaDevices_;\n> \tstd::vector> cameras_;\n>\n> @@ -97,6 +110,9 @@ private:\n> \tconst char *name_;\n> \tunsigned int useCount_;\n>\n> +\tstd::ostream *dumpCaptureScript_;\n> +\tstd::ostream *dumpMetadata_;\n> +\n> \tfriend class PipelineHandlerFactoryBase;\n> };\n>\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index a86f552a47bc..1282f99d839b 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -1215,6 +1215,9 @@ int Camera::configure(CameraConfiguration *config)\n> \t\td->activeStreams_.insert(stream);\n> \t}\n>\n> +\t/* TODO Save sensor configuration for dumping it to capture script */\n> +\td->orientation_ = config->orientation;\n> +\n> \td->setState(Private::CameraConfigured);\n>\n> \treturn 0;\n> @@ -1356,6 +1359,16 @@ int Camera::start(const ControlList *controls)\n>\n> \tASSERT(d->requestSequence_ == 0);\n>\n> +\t/*\n> +\t * Invoke method in blocking mode to avoid the risk of writing after\n> +\t * streaming has started.\n> +\t * This needs to be here as PipelineHandler::start is a virtual function\n> +\t * so it is impractical to add the dumping there.\n> +\t * TODO Pass the sensor configuration, once it is supported\n> +\t */\n> +\td->pipe_->invokeMethod(&PipelineHandler::dumpConfiguration,\n> +\t\t\t ConnectionTypeBlocking, d->activeStreams_, d->orientation_);\n\nCan't you pass in the CameraConfiguration which containts the\nStreamConfigurations and the the Orientation already ?\n\n> +\n> \tret = d->pipe_->invokeMethod(&PipelineHandler::start,\n> \t\t\t\t ConnectionTypeBlocking, this, controls);\n> \tif (ret)\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index e5940469127e..7002b4323bdd 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -8,6 +8,7 @@\n> #include \"libcamera/internal/pipeline_handler.h\"\n>\n> #include \n> +#include \n> #include \n> #include \n>\n> @@ -68,14 +69,36 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> * through the PipelineHandlerFactoryBase::create() function.\n> */\n> PipelineHandler::PipelineHandler(CameraManager *manager)\n> -\t: manager_(manager), useCount_(0)\n> +\t: manager_(manager), useCount_(0),\n> +\t dumpCaptureScript_(nullptr), dumpMetadata_(nullptr)\n> {\n> +\t/* TODO Print notification that we're dumping capture script */\n> +\tconst char *file = utils::secure_getenv(\"LIBCAMERA_DUMP_CAPTURE_SCRIPT\");\n> +\tif (!file)\n> +\t\treturn;\n> +\n> +\tdumpCaptureScript_ = new std::ofstream(file);\n\nI was really expecting to be operating on File instances\n\n> +\n> +\t/*\n> +\t * Metadata needs to go into a separate file because otherwise it'll\n> +\t * flood the capture script\n> +\t */\n> +\tdumpMetadata_ = new std::ofstream(std::string(file) + \".metadata\");\n> +\tstd::string str = \"frames:\\n\";\n> +\tdumpMetadata_->write(str.c_str(), str.size());\n\nand use the libyaml emitter for this instead of raw writes\n\nHave you considered it and decided it was better to use raw writes ?\n\nThanks\n j\n\n> +\tdumpMetadata_->flush();\n> }\n>\n> PipelineHandler::~PipelineHandler()\n> {\n> \tfor (std::shared_ptr media : mediaDevices_)\n> \t\tmedia->release();\n> +\n> +\tif (dumpCaptureScript_)\n> +\t\tdelete dumpCaptureScript_;\n> +\n> +\tif (dumpMetadata_)\n> +\t\tdelete dumpMetadata_;\n> }\n>\n> /**\n> @@ -464,6 +487,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n>\n> \trequest->_d()->sequence_ = data->requestSequence_++;\n>\n> +\tdumpRequest(request, DumpMode::Controls);\n> +\n> \tif (request->_d()->cancelled_) {\n> \t\tcompleteRequest(request);\n> \t\treturn;\n> @@ -555,6 +580,8 @@ void PipelineHandler::completeRequest(Request *request)\n>\n> \trequest->_d()->complete();\n>\n> +\tdumpRequest(request, DumpMode::Metadata);\n> +\n> \tCamera::Private *data = camera->_d();\n>\n> \twhile (!data->queuedRequests_.empty()) {\n> @@ -758,6 +785,70 @@ void PipelineHandler::disconnect()\n> * \\return The CameraManager for this pipeline handler\n> */\n>\n> +void PipelineHandler::dumpConfiguration(const std::set &streams,\n> +\t\t\t\t\tconst Orientation &orientation)\n> +{\n> +\tif (!dumpCaptureScript_)\n> +\t\treturn;\n> +\n> +\tstd::stringstream ss;\n> +\tss << \"configuration:\" << std::endl;\n> +\tss << \" orientation: \" << orientation << std::endl;\n> +\n> +\t/* TODO Dump Sensor configuration */\n> +\n> +\tss << \" streams:\" << std::endl;\n> +\tfor (const auto &stream : streams) {\n> +\t\tconst StreamConfiguration &streamConfig = stream->configuration();\n> +\t\tss << \" - pixelFormat: \" << streamConfig.pixelFormat << std::endl;\n> +\t\tss << \" size: \" << streamConfig.size << std::endl;\n> +\t\tss << \" stride: \" << streamConfig.stride << std::endl;\n> +\t\tss << \" frameSize: \" << streamConfig.frameSize << std::endl;\n> +\t\tss << \" bufferCount: \" << streamConfig.bufferCount << std::endl;\n> +\t\tif (streamConfig.colorSpace)\n> +\t\t\tss << \" colorSpace: \" << streamConfig.colorSpace->toString() << std::endl;\n> +\t}\n> +\n> +\tdumpCaptureScript_->write(ss.str().c_str(), ss.str().size());\n> +\n> +\tstd::string str = \"frames:\\n\";\n> +\tdumpCaptureScript_->write(str.c_str(), str.size());\n> +\tdumpCaptureScript_->flush();\n> +}\n> +\n> +void PipelineHandler::dumpRequest(Request *request, DumpMode mode)\n> +{\n> +\tControlList &controls =\n> +\t\tmode == DumpMode::Controls ? request->controls()\n> +\t\t\t\t\t : request->metadata();\n> +\tstd::ostream *output =\n> +\t\tmode == DumpMode::Controls ? dumpCaptureScript_\n> +\t\t\t\t\t : dumpMetadata_;\n> +\n> +\tif (!output || controls.empty())\n> +\t\treturn;\n> +\n> +\tstd::stringstream ss;\n> +\t/* TODO Figure out PFC */\n> +\tss << \" - \" << request->sequence() << \":\" << std::endl;\n> +\n> +\tconst ControlIdMap *idMap = controls.idMap();\n> +\tfor (const auto &pair : controls) {\n> +\t\tconst ControlId *ctrlId = idMap->at(pair.first);\n> +\t\t/* TODO Prettify enums (probably by upgrading ControlValue::toString()) */\n> +\t\tss << \" \" << ctrlId->name() << \": \" << pair.second.toString() << std::endl;\n> +\t}\n> +\n> +\t/*\n> +\t * TODO Investigate the overhead of flushing this frequently\n> +\t * Controls aren't going to be queued too frequently so it should be\n> +\t * fine to dump controls every frame. Metadata on the other hand needs\n> +\t * to be investigated.\n> +\t */\n> +\toutput->write(ss.str().c_str(), ss.str().size());\n> +\toutput->flush();\n> +}\n> +\n> /**\n> * \\class PipelineHandlerFactoryBase\n> * \\brief Base class for pipeline handler factories\n> --\n> 2.39.2\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A1189C3257\n\tfor ;\n\tWed, 2 Oct 2024 06:28:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 656B063523;\n\tWed, 2 Oct 2024 08:28:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2556C6350B\n\tfor ;\n\tWed, 2 Oct 2024 08:28:34 +0200 (CEST)","from ideasonboard.com (unknown [5.77.88.238])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6C8A43EA;\n\tWed, 2 Oct 2024 08:27:00 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"JC+wroy8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727850421;\n\tbh=qOTHPyk1rdGVzLGjJpUAzKKvwx9n6H9XzzWli+7lHMY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JC+wroy8lGXcM1wq+k5QpPSZNA+fdG6EAmFYnQ+dI9W8OUYEJo87PkkzxwQvVidba\n\t+Ke64L6zL1xhDYg1IjGjB+Xp4lpAkLnLnnofwhVJeVMgPjCEp58t6//MDkqdhiu9zE\n\t1Qp9wRnRFXRxfwJz+BBj4C/zuX6vixBAcKpUhvhg=","Date":"Wed, 2 Oct 2024 08:28:26 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org, laurent.pinchart@ideasonboard.com","Subject":"Re: [PATCH 1/2] pipeline: Add support for dumping capture script and\n\tmetadata","Message-ID":"","References":"<20240930170907.1404844-1-paul.elder@ideasonboard.com>\n\t<20240930170907.1404844-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240930170907.1404844-2-paul.elder@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31518,"web_url":"https://patchwork.libcamera.org/comment/31518/","msgid":"","date":"2024-10-02T09:59:21","subject":"Re: [PATCH 1/2] pipeline: Add support for dumping capture script and\n\tmetadata","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Paul,\n one more question\n\nOn Tue, Oct 01, 2024 at 02:09:06AM GMT, Paul Elder wrote:\n> Add support for dumping capture scripts and metadata. The capture\n> scripts can then be fed into the cam application and a capture can thus\n> be \"replayed\". Metadata can also be dumped.\n>\n> Camera configuration is also dumped to the capture script. The cam\n> application currently does not support loading configuration from the\n> capture script, but support for that will be added in a subsequent\n> patch.\n>\n> These can be enabled by a new environment variable.\n>\n> Signed-off-by: Paul Elder \n> ---\n> include/libcamera/internal/camera.h | 3 +\n> include/libcamera/internal/pipeline_handler.h | 16 ++++\n> src/libcamera/camera.cpp | 13 +++\n> src/libcamera/pipeline_handler.cpp | 93 ++++++++++++++++++-\n> 4 files changed, 124 insertions(+), 1 deletion(-)\n>\n> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> index 0add0428bb5d..a42f03d4c755 100644\n> --- a/include/libcamera/internal/camera.h\n> +++ b/include/libcamera/internal/camera.h\n> @@ -19,6 +19,8 @@\n>\n> namespace libcamera {\n>\n> +enum class Orientation;\n> +\n> class CameraControlValidator;\n> class PipelineHandler;\n> class Stream;\n> @@ -65,6 +67,7 @@ private:\n> \tstd::string id_;\n> \tstd::set streams_;\n> \tstd::set activeStreams_;\n> +\tOrientation orientation_;\n>\n> \tbool disconnected_;\n> \tstd::atomic state_;\n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 0d38080369c5..fb3914185a01 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -9,6 +9,7 @@\n>\n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -20,6 +21,8 @@\n>\n> namespace libcamera {\n>\n> +enum class Orientation;\n> +\n> class Camera;\n> class CameraConfiguration;\n> class CameraManager;\n> @@ -68,6 +71,9 @@ public:\n>\n> \tCameraManager *cameraManager() const { return manager_; }\n>\n> +\tvoid dumpConfiguration(const std::set &streams,\n> +\t\t\t const Orientation &orientation);\n> +\n> protected:\n> \tvoid registerCamera(std::shared_ptr camera);\n> \tvoid hotplugMediaDevice(MediaDevice *media);\n> @@ -81,6 +87,11 @@ protected:\n> \tCameraManager *manager_;\n>\n> private:\n> +\tenum DumpMode {\n> +\t\tControls,\n> +\t\tMetadata,\n> +\t};\n> +\n> \tvoid unlockMediaDevices();\n>\n> \tvoid mediaDeviceDisconnected(MediaDevice *media);\n> @@ -89,6 +100,8 @@ private:\n> \tvoid doQueueRequest(Request *request);\n> \tvoid doQueueRequests();\n>\n> +\tvoid dumpRequest(Request *request, DumpMode mode);\n> +\n> \tstd::vector> mediaDevices_;\n> \tstd::vector> cameras_;\n>\n> @@ -97,6 +110,9 @@ private:\n> \tconst char *name_;\n> \tunsigned int useCount_;\n>\n> +\tstd::ostream *dumpCaptureScript_;\n> +\tstd::ostream *dumpMetadata_;\n> +\n> \tfriend class PipelineHandlerFactoryBase;\n> };\n>\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index a86f552a47bc..1282f99d839b 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -1215,6 +1215,9 @@ int Camera::configure(CameraConfiguration *config)\n> \t\td->activeStreams_.insert(stream);\n> \t}\n>\n> +\t/* TODO Save sensor configuration for dumping it to capture script */\n> +\td->orientation_ = config->orientation;\n> +\n> \td->setState(Private::CameraConfigured);\n>\n> \treturn 0;\n> @@ -1356,6 +1359,16 @@ int Camera::start(const ControlList *controls)\n>\n> \tASSERT(d->requestSequence_ == 0);\n>\n> +\t/*\n> +\t * Invoke method in blocking mode to avoid the risk of writing after\n> +\t * streaming has started.\n> +\t * This needs to be here as PipelineHandler::start is a virtual function\n> +\t * so it is impractical to add the dumping there.\n> +\t * TODO Pass the sensor configuration, once it is supported\n> +\t */\n> +\td->pipe_->invokeMethod(&PipelineHandler::dumpConfiguration,\n> +\t\t\t ConnectionTypeBlocking, d->activeStreams_, d->orientation_);\n> +\n> \tret = d->pipe_->invokeMethod(&PipelineHandler::start,\n> \t\t\t\t ConnectionTypeBlocking, this, controls);\n> \tif (ret)\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index e5940469127e..7002b4323bdd 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -8,6 +8,7 @@\n> #include \"libcamera/internal/pipeline_handler.h\"\n>\n> #include \n> +#include \n> #include \n> #include \n>\n> @@ -68,14 +69,36 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> * through the PipelineHandlerFactoryBase::create() function.\n> */\n> PipelineHandler::PipelineHandler(CameraManager *manager)\n> -\t: manager_(manager), useCount_(0)\n> +\t: manager_(manager), useCount_(0),\n> +\t dumpCaptureScript_(nullptr), dumpMetadata_(nullptr)\n> {\n> +\t/* TODO Print notification that we're dumping capture script */\n> +\tconst char *file = utils::secure_getenv(\"LIBCAMERA_DUMP_CAPTURE_SCRIPT\");\n> +\tif (!file)\n> +\t\treturn;\n> +\n> +\tdumpCaptureScript_ = new std::ofstream(file);\n> +\n> +\t/*\n> +\t * Metadata needs to go into a separate file because otherwise it'll\n> +\t * flood the capture script\n> +\t */\n> +\tdumpMetadata_ = new std::ofstream(std::string(file) + \".metadata\");\n> +\tstd::string str = \"frames:\\n\";\n> +\tdumpMetadata_->write(str.c_str(), str.size());\n> +\tdumpMetadata_->flush();\n\nDo we automatically dump metadata as well, or should we rather use two\ndistinct environment variables to control dumping of controls and\nmetadata ? Dumping metadata for each frame might be expensive, maybe\nwe should enable it separately ?\n\n\n> }\n>\n> PipelineHandler::~PipelineHandler()\n> {\n> \tfor (std::shared_ptr media : mediaDevices_)\n> \t\tmedia->release();\n> +\n> +\tif (dumpCaptureScript_)\n> +\t\tdelete dumpCaptureScript_;\n> +\n> +\tif (dumpMetadata_)\n> +\t\tdelete dumpMetadata_;\n> }\n>\n> /**\n> @@ -464,6 +487,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n>\n> \trequest->_d()->sequence_ = data->requestSequence_++;\n>\n> +\tdumpRequest(request, DumpMode::Controls);\n> +\n> \tif (request->_d()->cancelled_) {\n> \t\tcompleteRequest(request);\n> \t\treturn;\n> @@ -555,6 +580,8 @@ void PipelineHandler::completeRequest(Request *request)\n>\n> \trequest->_d()->complete();\n>\n> +\tdumpRequest(request, DumpMode::Metadata);\n> +\n> \tCamera::Private *data = camera->_d();\n>\n> \twhile (!data->queuedRequests_.empty()) {\n> @@ -758,6 +785,70 @@ void PipelineHandler::disconnect()\n> * \\return The CameraManager for this pipeline handler\n> */\n>\n> +void PipelineHandler::dumpConfiguration(const std::set &streams,\n> +\t\t\t\t\tconst Orientation &orientation)\n> +{\n> +\tif (!dumpCaptureScript_)\n> +\t\treturn;\n> +\n> +\tstd::stringstream ss;\n> +\tss << \"configuration:\" << std::endl;\n> +\tss << \" orientation: \" << orientation << std::endl;\n> +\n> +\t/* TODO Dump Sensor configuration */\n> +\n> +\tss << \" streams:\" << std::endl;\n> +\tfor (const auto &stream : streams) {\n> +\t\tconst StreamConfiguration &streamConfig = stream->configuration();\n> +\t\tss << \" - pixelFormat: \" << streamConfig.pixelFormat << std::endl;\n> +\t\tss << \" size: \" << streamConfig.size << std::endl;\n> +\t\tss << \" stride: \" << streamConfig.stride << std::endl;\n> +\t\tss << \" frameSize: \" << streamConfig.frameSize << std::endl;\n> +\t\tss << \" bufferCount: \" << streamConfig.bufferCount << std::endl;\n> +\t\tif (streamConfig.colorSpace)\n> +\t\t\tss << \" colorSpace: \" << streamConfig.colorSpace->toString() << std::endl;\n> +\t}\n> +\n> +\tdumpCaptureScript_->write(ss.str().c_str(), ss.str().size());\n> +\n> +\tstd::string str = \"frames:\\n\";\n> +\tdumpCaptureScript_->write(str.c_str(), str.size());\n> +\tdumpCaptureScript_->flush();\n> +}\n> +\n> +void PipelineHandler::dumpRequest(Request *request, DumpMode mode)\n> +{\n> +\tControlList &controls =\n> +\t\tmode == DumpMode::Controls ? request->controls()\n> +\t\t\t\t\t : request->metadata();\n> +\tstd::ostream *output =\n> +\t\tmode == DumpMode::Controls ? dumpCaptureScript_\n> +\t\t\t\t\t : dumpMetadata_;\n> +\n> +\tif (!output || controls.empty())\n> +\t\treturn;\n> +\n> +\tstd::stringstream ss;\n> +\t/* TODO Figure out PFC */\n> +\tss << \" - \" << request->sequence() << \":\" << std::endl;\n> +\n> +\tconst ControlIdMap *idMap = controls.idMap();\n> +\tfor (const auto &pair : controls) {\n> +\t\tconst ControlId *ctrlId = idMap->at(pair.first);\n> +\t\t/* TODO Prettify enums (probably by upgrading ControlValue::toString()) */\n> +\t\tss << \" \" << ctrlId->name() << \": \" << pair.second.toString() << std::endl;\n> +\t}\n> +\n> +\t/*\n> +\t * TODO Investigate the overhead of flushing this frequently\n> +\t * Controls aren't going to be queued too frequently so it should be\n> +\t * fine to dump controls every frame. Metadata on the other hand needs\n> +\t * to be investigated.\n> +\t */\n> +\toutput->write(ss.str().c_str(), ss.str().size());\n> +\toutput->flush();\n> +}\n> +\n> /**\n> * \\class PipelineHandlerFactoryBase\n> * \\brief Base class for pipeline handler factories\n> --\n> 2.39.2\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5121ABD80A\n\tfor ;\n\tWed, 2 Oct 2024 09:59:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 16C706351F;\n\tWed, 2 Oct 2024 11:59:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 402BD60534\n\tfor ;\n\tWed, 2 Oct 2024 11:59:26 +0200 (CEST)","from ideasonboard.com (unknown [37.159.124.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8733D3EA;\n\tWed, 2 Oct 2024 11:57:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LsSAlk99\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727863073;\n\tbh=d0z0E/4a0OhH2kVkRnw8CAGrMHUHL/AZzoie8dq9N3c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LsSAlk99d4eJNZllMyOCXk72v6VEIqaclQD7NrH4dsO4+jSYOgcGVHTxGi/kPwSuS\n\tFulCIv+qC43r9h3ZoBQWcp1veNwH8Nq+oR8b49bnQLtyE8MmjLK/XqeIx0/lDRARQC\n\tJRZ37iGwoRFgrx1MHdhbECFfSpaA04/bUxXbiCJQ=","Date":"Wed, 2 Oct 2024 11:59:21 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org, laurent.pinchart@ideasonboard.com","Subject":"Re: [PATCH 1/2] pipeline: Add support for dumping capture script and\n\tmetadata","Message-ID":"","References":"<20240930170907.1404844-1-paul.elder@ideasonboard.com>\n\t<20240930170907.1404844-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240930170907.1404844-2-paul.elder@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31520,"web_url":"https://patchwork.libcamera.org/comment/31520/","msgid":"","date":"2024-10-02T10:20:48","subject":"Re: [PATCH 1/2] pipeline: Add support for dumping capture script and\n\tmetadata","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Wed, Oct 02, 2024 at 08:28:26AM +0200, Jacopo Mondi wrote:\n> Hi Paul\n> \n> On Tue, Oct 01, 2024 at 02:09:06AM GMT, Paul Elder wrote:\n> > Add support for dumping capture scripts and metadata. The capture\n> > scripts can then be fed into the cam application and a capture can thus\n> > be \"replayed\". Metadata can also be dumped.\n> >\n> > Camera configuration is also dumped to the capture script. The cam\n> > application currently does not support loading configuration from the\n> > capture script, but support for that will be added in a subsequent\n> > patch.\n> >\n> > These can be enabled by a new environment variable.\n> \n> Which needs to be documented :)\n\nI... opened the tab ready to write it then forgot to write it :)\n\n> \n> >\n> > Signed-off-by: Paul Elder \n> > ---\n> > include/libcamera/internal/camera.h | 3 +\n> > include/libcamera/internal/pipeline_handler.h | 16 ++++\n> > src/libcamera/camera.cpp | 13 +++\n> > src/libcamera/pipeline_handler.cpp | 93 ++++++++++++++++++-\n> > 4 files changed, 124 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> > index 0add0428bb5d..a42f03d4c755 100644\n> > --- a/include/libcamera/internal/camera.h\n> > +++ b/include/libcamera/internal/camera.h\n> > @@ -19,6 +19,8 @@\n> >\n> > namespace libcamera {\n> >\n> > +enum class Orientation;\n> > +\n> > class CameraControlValidator;\n> > class PipelineHandler;\n> > class Stream;\n> > @@ -65,6 +67,7 @@ private:\n> > \tstd::string id_;\n> > \tstd::set streams_;\n> > \tstd::set activeStreams_;\n> > +\tOrientation orientation_;\n> >\n> > \tbool disconnected_;\n> > \tstd::atomic state_;\n> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > index 0d38080369c5..fb3914185a01 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -9,6 +9,7 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -20,6 +21,8 @@\n> >\n> > namespace libcamera {\n> >\n> > +enum class Orientation;\n> > +\n> > class Camera;\n> > class CameraConfiguration;\n> > class CameraManager;\n> > @@ -68,6 +71,9 @@ public:\n> >\n> > \tCameraManager *cameraManager() const { return manager_; }\n> >\n> > +\tvoid dumpConfiguration(const std::set &streams,\n> > +\t\t\t const Orientation &orientation);\n> > +\n> \n> To define the expected configuration format, I would make 2/2 precede 1/2\n\nIndeed 2/2 going first doesn't actually break anything. Sure.\n\n> \n> > protected:\n> > \tvoid registerCamera(std::shared_ptr camera);\n> > \tvoid hotplugMediaDevice(MediaDevice *media);\n> > @@ -81,6 +87,11 @@ protected:\n> > \tCameraManager *manager_;\n> >\n> > private:\n> > +\tenum DumpMode {\n> > +\t\tControls,\n> > +\t\tMetadata,\n> > +\t};\n> > +\n> > \tvoid unlockMediaDevices();\n> >\n> > \tvoid mediaDeviceDisconnected(MediaDevice *media);\n> > @@ -89,6 +100,8 @@ private:\n> > \tvoid doQueueRequest(Request *request);\n> > \tvoid doQueueRequests();\n> >\n> > +\tvoid dumpRequest(Request *request, DumpMode mode);\n> > +\n> > \tstd::vector> mediaDevices_;\n> > \tstd::vector> cameras_;\n> >\n> > @@ -97,6 +110,9 @@ private:\n> > \tconst char *name_;\n> > \tunsigned int useCount_;\n> >\n> > +\tstd::ostream *dumpCaptureScript_;\n> > +\tstd::ostream *dumpMetadata_;\n> > +\n> > \tfriend class PipelineHandlerFactoryBase;\n> > };\n> >\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index a86f552a47bc..1282f99d839b 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -1215,6 +1215,9 @@ int Camera::configure(CameraConfiguration *config)\n> > \t\td->activeStreams_.insert(stream);\n> > \t}\n> >\n> > +\t/* TODO Save sensor configuration for dumping it to capture script */\n> > +\td->orientation_ = config->orientation;\n> > +\n> > \td->setState(Private::CameraConfigured);\n> >\n> > \treturn 0;\n> > @@ -1356,6 +1359,16 @@ int Camera::start(const ControlList *controls)\n> >\n> > \tASSERT(d->requestSequence_ == 0);\n> >\n> > +\t/*\n> > +\t * Invoke method in blocking mode to avoid the risk of writing after\n> > +\t * streaming has started.\n> > +\t * This needs to be here as PipelineHandler::start is a virtual function\n> > +\t * so it is impractical to add the dumping there.\n> > +\t * TODO Pass the sensor configuration, once it is supported\n> > +\t */\n> > +\td->pipe_->invokeMethod(&PipelineHandler::dumpConfiguration,\n> > +\t\t\t ConnectionTypeBlocking, d->activeStreams_, d->orientation_);\n> \n> Can't you pass in the CameraConfiguration which containts the\n> StreamConfigurations and the the Orientation already ?\n\nCameraConfiguration can't be copied because it has virtual functions.\nAnd CameraConfiguration is only available in Camera::configure(). So, no.\n\n> \n> > +\n> > \tret = d->pipe_->invokeMethod(&PipelineHandler::start,\n> > \t\t\t\t ConnectionTypeBlocking, this, controls);\n> > \tif (ret)\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index e5940469127e..7002b4323bdd 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -8,6 +8,7 @@\n> > #include \"libcamera/internal/pipeline_handler.h\"\n> >\n> > #include \n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -68,14 +69,36 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> > * through the PipelineHandlerFactoryBase::create() function.\n> > */\n> > PipelineHandler::PipelineHandler(CameraManager *manager)\n> > -\t: manager_(manager), useCount_(0)\n> > +\t: manager_(manager), useCount_(0),\n> > +\t dumpCaptureScript_(nullptr), dumpMetadata_(nullptr)\n> > {\n> > +\t/* TODO Print notification that we're dumping capture script */\n> > +\tconst char *file = utils::secure_getenv(\"LIBCAMERA_DUMP_CAPTURE_SCRIPT\");\n> > +\tif (!file)\n> > +\t\treturn;\n> > +\n> > +\tdumpCaptureScript_ = new std::ofstream(file);\n> \n> I was really expecting to be operating on File instances\n\nOh ig we could do that too. I copied a few ideas from the logger.\n\n> \n> > +\n> > +\t/*\n> > +\t * Metadata needs to go into a separate file because otherwise it'll\n> > +\t * flood the capture script\n> > +\t */\n> > +\tdumpMetadata_ = new std::ofstream(std::string(file) + \".metadata\");\n> > +\tstd::string str = \"frames:\\n\";\n> > +\tdumpMetadata_->write(str.c_str(), str.size());\n> \n> and use the libyaml emitter for this instead of raw writes\n> \n> Have you considered it and decided it was better to use raw writes ?\n\nNo I hadn't. I'll keep it raw for now though until you make the\nYamlEmitter :)\n\n\nThanks,\n\nPaul\n\n> \n> > +\tdumpMetadata_->flush();\n> > }\n> >\n> > PipelineHandler::~PipelineHandler()\n> > {\n> > \tfor (std::shared_ptr media : mediaDevices_)\n> > \t\tmedia->release();\n> > +\n> > +\tif (dumpCaptureScript_)\n> > +\t\tdelete dumpCaptureScript_;\n> > +\n> > +\tif (dumpMetadata_)\n> > +\t\tdelete dumpMetadata_;\n> > }\n> >\n> > /**\n> > @@ -464,6 +487,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n> >\n> > \trequest->_d()->sequence_ = data->requestSequence_++;\n> >\n> > +\tdumpRequest(request, DumpMode::Controls);\n> > +\n> > \tif (request->_d()->cancelled_) {\n> > \t\tcompleteRequest(request);\n> > \t\treturn;\n> > @@ -555,6 +580,8 @@ void PipelineHandler::completeRequest(Request *request)\n> >\n> > \trequest->_d()->complete();\n> >\n> > +\tdumpRequest(request, DumpMode::Metadata);\n> > +\n> > \tCamera::Private *data = camera->_d();\n> >\n> > \twhile (!data->queuedRequests_.empty()) {\n> > @@ -758,6 +785,70 @@ void PipelineHandler::disconnect()\n> > * \\return The CameraManager for this pipeline handler\n> > */\n> >\n> > +void PipelineHandler::dumpConfiguration(const std::set &streams,\n> > +\t\t\t\t\tconst Orientation &orientation)\n> > +{\n> > +\tif (!dumpCaptureScript_)\n> > +\t\treturn;\n> > +\n> > +\tstd::stringstream ss;\n> > +\tss << \"configuration:\" << std::endl;\n> > +\tss << \" orientation: \" << orientation << std::endl;\n> > +\n> > +\t/* TODO Dump Sensor configuration */\n> > +\n> > +\tss << \" streams:\" << std::endl;\n> > +\tfor (const auto &stream : streams) {\n> > +\t\tconst StreamConfiguration &streamConfig = stream->configuration();\n> > +\t\tss << \" - pixelFormat: \" << streamConfig.pixelFormat << std::endl;\n> > +\t\tss << \" size: \" << streamConfig.size << std::endl;\n> > +\t\tss << \" stride: \" << streamConfig.stride << std::endl;\n> > +\t\tss << \" frameSize: \" << streamConfig.frameSize << std::endl;\n> > +\t\tss << \" bufferCount: \" << streamConfig.bufferCount << std::endl;\n> > +\t\tif (streamConfig.colorSpace)\n> > +\t\t\tss << \" colorSpace: \" << streamConfig.colorSpace->toString() << std::endl;\n> > +\t}\n> > +\n> > +\tdumpCaptureScript_->write(ss.str().c_str(), ss.str().size());\n> > +\n> > +\tstd::string str = \"frames:\\n\";\n> > +\tdumpCaptureScript_->write(str.c_str(), str.size());\n> > +\tdumpCaptureScript_->flush();\n> > +}\n> > +\n> > +void PipelineHandler::dumpRequest(Request *request, DumpMode mode)\n> > +{\n> > +\tControlList &controls =\n> > +\t\tmode == DumpMode::Controls ? request->controls()\n> > +\t\t\t\t\t : request->metadata();\n> > +\tstd::ostream *output =\n> > +\t\tmode == DumpMode::Controls ? dumpCaptureScript_\n> > +\t\t\t\t\t : dumpMetadata_;\n> > +\n> > +\tif (!output || controls.empty())\n> > +\t\treturn;\n> > +\n> > +\tstd::stringstream ss;\n> > +\t/* TODO Figure out PFC */\n> > +\tss << \" - \" << request->sequence() << \":\" << std::endl;\n> > +\n> > +\tconst ControlIdMap *idMap = controls.idMap();\n> > +\tfor (const auto &pair : controls) {\n> > +\t\tconst ControlId *ctrlId = idMap->at(pair.first);\n> > +\t\t/* TODO Prettify enums (probably by upgrading ControlValue::toString()) */\n> > +\t\tss << \" \" << ctrlId->name() << \": \" << pair.second.toString() << std::endl;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * TODO Investigate the overhead of flushing this frequently\n> > +\t * Controls aren't going to be queued too frequently so it should be\n> > +\t * fine to dump controls every frame. Metadata on the other hand needs\n> > +\t * to be investigated.\n> > +\t */\n> > +\toutput->write(ss.str().c_str(), ss.str().size());\n> > +\toutput->flush();\n> > +}\n> > +\n> > /**\n> > * \\class PipelineHandlerFactoryBase\n> > * \\brief Base class for pipeline handler factories\n> > --\n> > 2.39.2\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1884BBD80A\n\tfor ;\n\tWed, 2 Oct 2024 10:21:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BCFCC63525;\n\tWed, 2 Oct 2024 12:20:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 22DF863510\n\tfor ;\n\tWed, 2 Oct 2024 12:20:58 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:6ef4:b42e:3cf6:d89b])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CF3EB3EA;\n\tWed, 2 Oct 2024 12:19:23 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XaqGXGzE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727864365;\n\tbh=K+5ASk+N59NXGF1lLljl229na8Qj69RNCxhQojRM6FM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XaqGXGzEZ9Sltt8ND0A+xILdccR73IC0qo9kRqZ8R4rlzmyx7m04fT9FA6E5UOJpr\n\t8ISiJJQj61Ls/v0ONHpw3LkOp7lejyodS+GcXmnT/OXIKB9mPWSPX5MaXHRXJcChqF\n\tteEUlAaYkI7Jdpd+r3HPO8qShmZ0aqXRpYLC7510=","Date":"Wed, 2 Oct 2024 19:20:48 +0900","From":"Paul Elder ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, laurent.pinchart@ideasonboard.com","Subject":"Re: [PATCH 1/2] pipeline: Add support for dumping capture script and\n\tmetadata","Message-ID":"","References":"<20240930170907.1404844-1-paul.elder@ideasonboard.com>\n\t<20240930170907.1404844-2-paul.elder@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31522,"web_url":"https://patchwork.libcamera.org/comment/31522/","msgid":"","date":"2024-10-02T10:53:42","subject":"Re: [PATCH 1/2] pipeline: Add support for dumping capture script and\n\tmetadata","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Wed, Oct 02, 2024 at 11:59:21AM +0200, Jacopo Mondi wrote:\n> Hi Paul,\n> one more question\n> \n> On Tue, Oct 01, 2024 at 02:09:06AM GMT, Paul Elder wrote:\n> > Add support for dumping capture scripts and metadata. The capture\n> > scripts can then be fed into the cam application and a capture can thus\n> > be \"replayed\". Metadata can also be dumped.\n> >\n> > Camera configuration is also dumped to the capture script. The cam\n> > application currently does not support loading configuration from the\n> > capture script, but support for that will be added in a subsequent\n> > patch.\n> >\n> > These can be enabled by a new environment variable.\n> >\n> > Signed-off-by: Paul Elder \n> > ---\n> > include/libcamera/internal/camera.h | 3 +\n> > include/libcamera/internal/pipeline_handler.h | 16 ++++\n> > src/libcamera/camera.cpp | 13 +++\n> > src/libcamera/pipeline_handler.cpp | 93 ++++++++++++++++++-\n> > 4 files changed, 124 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> > index 0add0428bb5d..a42f03d4c755 100644\n> > --- a/include/libcamera/internal/camera.h\n> > +++ b/include/libcamera/internal/camera.h\n> > @@ -19,6 +19,8 @@\n> >\n> > namespace libcamera {\n> >\n> > +enum class Orientation;\n> > +\n> > class CameraControlValidator;\n> > class PipelineHandler;\n> > class Stream;\n> > @@ -65,6 +67,7 @@ private:\n> > \tstd::string id_;\n> > \tstd::set streams_;\n> > \tstd::set activeStreams_;\n> > +\tOrientation orientation_;\n> >\n> > \tbool disconnected_;\n> > \tstd::atomic state_;\n> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > index 0d38080369c5..fb3914185a01 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -9,6 +9,7 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -20,6 +21,8 @@\n> >\n> > namespace libcamera {\n> >\n> > +enum class Orientation;\n> > +\n> > class Camera;\n> > class CameraConfiguration;\n> > class CameraManager;\n> > @@ -68,6 +71,9 @@ public:\n> >\n> > \tCameraManager *cameraManager() const { return manager_; }\n> >\n> > +\tvoid dumpConfiguration(const std::set &streams,\n> > +\t\t\t const Orientation &orientation);\n> > +\n> > protected:\n> > \tvoid registerCamera(std::shared_ptr camera);\n> > \tvoid hotplugMediaDevice(MediaDevice *media);\n> > @@ -81,6 +87,11 @@ protected:\n> > \tCameraManager *manager_;\n> >\n> > private:\n> > +\tenum DumpMode {\n> > +\t\tControls,\n> > +\t\tMetadata,\n> > +\t};\n> > +\n> > \tvoid unlockMediaDevices();\n> >\n> > \tvoid mediaDeviceDisconnected(MediaDevice *media);\n> > @@ -89,6 +100,8 @@ private:\n> > \tvoid doQueueRequest(Request *request);\n> > \tvoid doQueueRequests();\n> >\n> > +\tvoid dumpRequest(Request *request, DumpMode mode);\n> > +\n> > \tstd::vector> mediaDevices_;\n> > \tstd::vector> cameras_;\n> >\n> > @@ -97,6 +110,9 @@ private:\n> > \tconst char *name_;\n> > \tunsigned int useCount_;\n> >\n> > +\tstd::ostream *dumpCaptureScript_;\n> > +\tstd::ostream *dumpMetadata_;\n> > +\n> > \tfriend class PipelineHandlerFactoryBase;\n> > };\n> >\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index a86f552a47bc..1282f99d839b 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -1215,6 +1215,9 @@ int Camera::configure(CameraConfiguration *config)\n> > \t\td->activeStreams_.insert(stream);\n> > \t}\n> >\n> > +\t/* TODO Save sensor configuration for dumping it to capture script */\n> > +\td->orientation_ = config->orientation;\n> > +\n> > \td->setState(Private::CameraConfigured);\n> >\n> > \treturn 0;\n> > @@ -1356,6 +1359,16 @@ int Camera::start(const ControlList *controls)\n> >\n> > \tASSERT(d->requestSequence_ == 0);\n> >\n> > +\t/*\n> > +\t * Invoke method in blocking mode to avoid the risk of writing after\n> > +\t * streaming has started.\n> > +\t * This needs to be here as PipelineHandler::start is a virtual function\n> > +\t * so it is impractical to add the dumping there.\n> > +\t * TODO Pass the sensor configuration, once it is supported\n> > +\t */\n> > +\td->pipe_->invokeMethod(&PipelineHandler::dumpConfiguration,\n> > +\t\t\t ConnectionTypeBlocking, d->activeStreams_, d->orientation_);\n> > +\n> > \tret = d->pipe_->invokeMethod(&PipelineHandler::start,\n> > \t\t\t\t ConnectionTypeBlocking, this, controls);\n> > \tif (ret)\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index e5940469127e..7002b4323bdd 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -8,6 +8,7 @@\n> > #include \"libcamera/internal/pipeline_handler.h\"\n> >\n> > #include \n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -68,14 +69,36 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> > * through the PipelineHandlerFactoryBase::create() function.\n> > */\n> > PipelineHandler::PipelineHandler(CameraManager *manager)\n> > -\t: manager_(manager), useCount_(0)\n> > +\t: manager_(manager), useCount_(0),\n> > +\t dumpCaptureScript_(nullptr), dumpMetadata_(nullptr)\n> > {\n> > +\t/* TODO Print notification that we're dumping capture script */\n> > +\tconst char *file = utils::secure_getenv(\"LIBCAMERA_DUMP_CAPTURE_SCRIPT\");\n> > +\tif (!file)\n> > +\t\treturn;\n> > +\n> > +\tdumpCaptureScript_ = new std::ofstream(file);\n> > +\n> > +\t/*\n> > +\t * Metadata needs to go into a separate file because otherwise it'll\n> > +\t * flood the capture script\n> > +\t */\n> > +\tdumpMetadata_ = new std::ofstream(std::string(file) + \".metadata\");\n> > +\tstd::string str = \"frames:\\n\";\n> > +\tdumpMetadata_->write(str.c_str(), str.size());\n> > +\tdumpMetadata_->flush();\n> \n> Do we automatically dump metadata as well, or should we rather use two\n> distinct environment variables to control dumping of controls and\n> metadata ? Dumping metadata for each frame might be expensive, maybe\n> we should enable it separately ?\n\nGood point yeah maybe we should.\n\n\nPaul\n\n> \n> \n> > }\n> >\n> > PipelineHandler::~PipelineHandler()\n> > {\n> > \tfor (std::shared_ptr media : mediaDevices_)\n> > \t\tmedia->release();\n> > +\n> > +\tif (dumpCaptureScript_)\n> > +\t\tdelete dumpCaptureScript_;\n> > +\n> > +\tif (dumpMetadata_)\n> > +\t\tdelete dumpMetadata_;\n> > }\n> >\n> > /**\n> > @@ -464,6 +487,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n> >\n> > \trequest->_d()->sequence_ = data->requestSequence_++;\n> >\n> > +\tdumpRequest(request, DumpMode::Controls);\n> > +\n> > \tif (request->_d()->cancelled_) {\n> > \t\tcompleteRequest(request);\n> > \t\treturn;\n> > @@ -555,6 +580,8 @@ void PipelineHandler::completeRequest(Request *request)\n> >\n> > \trequest->_d()->complete();\n> >\n> > +\tdumpRequest(request, DumpMode::Metadata);\n> > +\n> > \tCamera::Private *data = camera->_d();\n> >\n> > \twhile (!data->queuedRequests_.empty()) {\n> > @@ -758,6 +785,70 @@ void PipelineHandler::disconnect()\n> > * \\return The CameraManager for this pipeline handler\n> > */\n> >\n> > +void PipelineHandler::dumpConfiguration(const std::set &streams,\n> > +\t\t\t\t\tconst Orientation &orientation)\n> > +{\n> > +\tif (!dumpCaptureScript_)\n> > +\t\treturn;\n> > +\n> > +\tstd::stringstream ss;\n> > +\tss << \"configuration:\" << std::endl;\n> > +\tss << \" orientation: \" << orientation << std::endl;\n> > +\n> > +\t/* TODO Dump Sensor configuration */\n> > +\n> > +\tss << \" streams:\" << std::endl;\n> > +\tfor (const auto &stream : streams) {\n> > +\t\tconst StreamConfiguration &streamConfig = stream->configuration();\n> > +\t\tss << \" - pixelFormat: \" << streamConfig.pixelFormat << std::endl;\n> > +\t\tss << \" size: \" << streamConfig.size << std::endl;\n> > +\t\tss << \" stride: \" << streamConfig.stride << std::endl;\n> > +\t\tss << \" frameSize: \" << streamConfig.frameSize << std::endl;\n> > +\t\tss << \" bufferCount: \" << streamConfig.bufferCount << std::endl;\n> > +\t\tif (streamConfig.colorSpace)\n> > +\t\t\tss << \" colorSpace: \" << streamConfig.colorSpace->toString() << std::endl;\n> > +\t}\n> > +\n> > +\tdumpCaptureScript_->write(ss.str().c_str(), ss.str().size());\n> > +\n> > +\tstd::string str = \"frames:\\n\";\n> > +\tdumpCaptureScript_->write(str.c_str(), str.size());\n> > +\tdumpCaptureScript_->flush();\n> > +}\n> > +\n> > +void PipelineHandler::dumpRequest(Request *request, DumpMode mode)\n> > +{\n> > +\tControlList &controls =\n> > +\t\tmode == DumpMode::Controls ? request->controls()\n> > +\t\t\t\t\t : request->metadata();\n> > +\tstd::ostream *output =\n> > +\t\tmode == DumpMode::Controls ? dumpCaptureScript_\n> > +\t\t\t\t\t : dumpMetadata_;\n> > +\n> > +\tif (!output || controls.empty())\n> > +\t\treturn;\n> > +\n> > +\tstd::stringstream ss;\n> > +\t/* TODO Figure out PFC */\n> > +\tss << \" - \" << request->sequence() << \":\" << std::endl;\n> > +\n> > +\tconst ControlIdMap *idMap = controls.idMap();\n> > +\tfor (const auto &pair : controls) {\n> > +\t\tconst ControlId *ctrlId = idMap->at(pair.first);\n> > +\t\t/* TODO Prettify enums (probably by upgrading ControlValue::toString()) */\n> > +\t\tss << \" \" << ctrlId->name() << \": \" << pair.second.toString() << std::endl;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * TODO Investigate the overhead of flushing this frequently\n> > +\t * Controls aren't going to be queued too frequently so it should be\n> > +\t * fine to dump controls every frame. Metadata on the other hand needs\n> > +\t * to be investigated.\n> > +\t */\n> > +\toutput->write(ss.str().c_str(), ss.str().size());\n> > +\toutput->flush();\n> > +}\n> > +\n> > /**\n> > * \\class PipelineHandlerFactoryBase\n> > * \\brief Base class for pipeline handler factories\n> > --\n> > 2.39.2\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4667BBD80A\n\tfor ;\n\tWed, 2 Oct 2024 10:53:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2EF626351F;\n\tWed, 2 Oct 2024 12:53:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 10F9963510\n\tfor ;\n\tWed, 2 Oct 2024 12:53:49 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:6ef4:b42e:3cf6:d89b])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 58DC53E6;\n\tWed, 2 Oct 2024 12:52:15 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tyzh7boD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727866336;\n\tbh=cBFnbgdnY0cSK6AAcn1EJZQE1DOqnB4rjcOwThj2gQQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tyzh7boDHLZxelZ4kEinVGXTrfV9Mmjv/Das4Wa1NMtDMlSIPkYeQ50OL+KvSST4J\n\tlI1Vqn37E52Izv0FoNCkUNNxZFpzfS8pxfET/LdM28huy65sJMGJjLTIXM9QY8QV17\n\tqr1y7DwEX1cZjQ/mMfI4RzvVw/2MQh0sintpV4p0=","Date":"Wed, 2 Oct 2024 19:53:42 +0900","From":"Paul Elder ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, laurent.pinchart@ideasonboard.com","Subject":"Re: [PATCH 1/2] pipeline: Add support for dumping capture script and\n\tmetadata","Message-ID":"","References":"<20240930170907.1404844-1-paul.elder@ideasonboard.com>\n\t<20240930170907.1404844-2-paul.elder@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31527,"web_url":"https://patchwork.libcamera.org/comment/31527/","msgid":"","date":"2024-10-02T13:28:51","subject":"Re: [PATCH 1/2] pipeline: Add support for dumping capture script and\n\tmetadata","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n2024. szeptember 30., hétfő 19:09 keltezéssel, Paul Elder írta:\n\n> Add support for dumping capture scripts and metadata. The capture\n> scripts can then be fed into the cam application and a capture can thus\n> be \"replayed\". Metadata can also be dumped.\n> \n> Camera configuration is also dumped to the capture script. The cam\n> application currently does not support loading configuration from the\n> capture script, but support for that will be added in a subsequent\n> patch.\n> \n> These can be enabled by a new environment variable.\n> \n> Signed-off-by: Paul Elder \n> ---\n> include/libcamera/internal/camera.h | 3 +\n> include/libcamera/internal/pipeline_handler.h | 16 ++++\n> src/libcamera/camera.cpp | 13 +++\n> src/libcamera/pipeline_handler.cpp | 93 ++++++++++++++++++-\n> 4 files changed, 124 insertions(+), 1 deletion(-)\n> \n> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> index 0add0428bb5d..a42f03d4c755 100644\n> --- a/include/libcamera/internal/camera.h\n> +++ b/include/libcamera/internal/camera.h\n> @@ -19,6 +19,8 @@\n> \n> namespace libcamera {\n> \n> +enum class Orientation;\n> +\n> class CameraControlValidator;\n> class PipelineHandler;\n> class Stream;\n> @@ -65,6 +67,7 @@ private:\n> \tstd::string id_;\n> \tstd::set streams_;\n> \tstd::set activeStreams_;\n> +\tOrientation orientation_;\n> \n> \tbool disconnected_;\n> \tstd::atomic state_;\n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 0d38080369c5..fb3914185a01 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -9,6 +9,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -20,6 +21,8 @@\n> \n> namespace libcamera {\n> \n> +enum class Orientation;\n> +\n> class Camera;\n> class CameraConfiguration;\n> class CameraManager;\n> @@ -68,6 +71,9 @@ public:\n> \n> \tCameraManager *cameraManager() const { return manager_; }\n> \n> +\tvoid dumpConfiguration(const std::set &streams,\n> +\t\t\t const Orientation &orientation);\n> +\n> protected:\n> \tvoid registerCamera(std::shared_ptr camera);\n> \tvoid hotplugMediaDevice(MediaDevice *media);\n> @@ -81,6 +87,11 @@ protected:\n> \tCameraManager *manager_;\n> \n> private:\n> +\tenum DumpMode {\n> +\t\tControls,\n> +\t\tMetadata,\n> +\t};\n> +\n> \tvoid unlockMediaDevices();\n> \n> \tvoid mediaDeviceDisconnected(MediaDevice *media);\n> @@ -89,6 +100,8 @@ private:\n> \tvoid doQueueRequest(Request *request);\n> \tvoid doQueueRequests();\n> \n> +\tvoid dumpRequest(Request *request, DumpMode mode);\n> +\n> \tstd::vector> mediaDevices_;\n> \tstd::vector> cameras_;\n> \n> @@ -97,6 +110,9 @@ private:\n> \tconst char *name_;\n> \tunsigned int useCount_;\n> \n> +\tstd::ostream *dumpCaptureScript_;\n> +\tstd::ostream *dumpMetadata_;\n\nHave you looked at using `std::optional` here instead? Or `std::unique_ptr`?\n\n\n> +\n> \tfriend class PipelineHandlerFactoryBase;\n> };\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index a86f552a47bc..1282f99d839b 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -1215,6 +1215,9 @@ int Camera::configure(CameraConfiguration *config)\n> \t\td->activeStreams_.insert(stream);\n> \t}\n> \n> +\t/* TODO Save sensor configuration for dumping it to capture script */\n> +\td->orientation_ = config->orientation;\n> +\n> \td->setState(Private::CameraConfigured);\n> \n> \treturn 0;\n> @@ -1356,6 +1359,16 @@ int Camera::start(const ControlList *controls)\n> \n> \tASSERT(d->requestSequence_ == 0);\n> \n> +\t/*\n> +\t * Invoke method in blocking mode to avoid the risk of writing after\n> +\t * streaming has started.\n> +\t * This needs to be here as PipelineHandler::start is a virtual function\n> +\t * so it is impractical to add the dumping there.\n> +\t * TODO Pass the sensor configuration, once it is supported\n> +\t */\n> +\td->pipe_->invokeMethod(&PipelineHandler::dumpConfiguration,\n> +\t\t\t ConnectionTypeBlocking, d->activeStreams_, d->orientation_);\n> +\n> \tret = d->pipe_->invokeMethod(&PipelineHandler::start,\n> \t\t\t\t ConnectionTypeBlocking, this, controls);\n> \tif (ret)\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index e5940469127e..7002b4323bdd 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -8,6 +8,7 @@\n> #include \"libcamera/internal/pipeline_handler.h\"\n> \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -68,14 +69,36 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> * through the PipelineHandlerFactoryBase::create() function.\n> */\n> PipelineHandler::PipelineHandler(CameraManager *manager)\n> -\t: manager_(manager), useCount_(0)\n> +\t: manager_(manager), useCount_(0),\n> +\t dumpCaptureScript_(nullptr), dumpMetadata_(nullptr)\n> {\n> +\t/* TODO Print notification that we're dumping capture script */\n> +\tconst char *file = utils::secure_getenv(\"LIBCAMERA_DUMP_CAPTURE_SCRIPT\");\n> +\tif (!file)\n> +\t\treturn;\n> +\n> +\tdumpCaptureScript_ = new std::ofstream(file);\n> +\n> +\t/*\n> +\t * Metadata needs to go into a separate file because otherwise it'll\n> +\t * flood the capture script\n> +\t */\n> +\tdumpMetadata_ = new std::ofstream(std::string(file) + \".metadata\");\n> +\tstd::string str = \"frames:\\n\";\n\nI think `std::string_view` is sufficient for this.\n\n\n> +\tdumpMetadata_->write(str.c_str(), str.size());\n> +\tdumpMetadata_->flush();\n> }\n> \n> PipelineHandler::~PipelineHandler()\n> {\n> \tfor (std::shared_ptr media : mediaDevices_)\n> \t\tmedia->release();\n> +\n> +\tif (dumpCaptureScript_)\n> +\t\tdelete dumpCaptureScript_;\n> +\n> +\tif (dumpMetadata_)\n> +\t\tdelete dumpMetadata_;\n\n`delete` already checks for `nullptr`, so if you want to go the raw ptr way,\nthen there is no need for an extra check. But as I mentioned I think `std::optional`\nor `std::unique_ptr` would probably be preferable.\n\n\n> }\n> \n> /**\n> @@ -464,6 +487,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n> \n> \trequest->_d()->sequence_ = data->requestSequence_++;\n> \n> +\tdumpRequest(request, DumpMode::Controls);\n> +\n> \tif (request->_d()->cancelled_) {\n> \t\tcompleteRequest(request);\n> \t\treturn;\n> @@ -555,6 +580,8 @@ void PipelineHandler::completeRequest(Request *request)\n> \n> \trequest->_d()->complete();\n> \n> +\tdumpRequest(request, DumpMode::Metadata);\n> +\n> \tCamera::Private *data = camera->_d();\n> \n> \twhile (!data->queuedRequests_.empty()) {\n> @@ -758,6 +785,70 @@ void PipelineHandler::disconnect()\n> * \\return The CameraManager for this pipeline handler\n> */\n> \n> +void PipelineHandler::dumpConfiguration(const std::set &streams,\n> +\t\t\t\t\tconst Orientation &orientation)\n> +{\n> +\tif (!dumpCaptureScript_)\n> +\t\treturn;\n> +\n> +\tstd::stringstream ss;\n> +\tss << \"configuration:\" << std::endl;\n> +\tss << \" orientation: \" << orientation << std::endl;\n> +\n> +\t/* TODO Dump Sensor configuration */\n> +\n> +\tss << \" streams:\" << std::endl;\n> +\tfor (const auto &stream : streams) {\n> +\t\tconst StreamConfiguration &streamConfig = stream->configuration();\n> +\t\tss << \" - pixelFormat: \" << streamConfig.pixelFormat << std::endl;\n> +\t\tss << \" size: \" << streamConfig.size << std::endl;\n> +\t\tss << \" stride: \" << streamConfig.stride << std::endl;\n> +\t\tss << \" frameSize: \" << streamConfig.frameSize << std::endl;\n> +\t\tss << \" bufferCount: \" << streamConfig.bufferCount << std::endl;\n> +\t\tif (streamConfig.colorSpace)\n> +\t\t\tss << \" colorSpace: \" << streamConfig.colorSpace->toString() << std::endl;\n> +\t}\n> +\n> +\tdumpCaptureScript_->write(ss.str().c_str(), ss.str().size());\n\nIt's probably best to avoid calling `std::stringstream::str()` twice since it\nreturns a copy of the string twice in this case. I believe you might as well do\n`auto str = std::move(ss).str()`, which should avoid the copy altogether.\n\n\n> +\n> +\tstd::string str = \"frames:\\n\";\n> +\tdumpCaptureScript_->write(str.c_str(), str.size());\n> +\tdumpCaptureScript_->flush();\n> +}\n> +\n> +void PipelineHandler::dumpRequest(Request *request, DumpMode mode)\n> +{\n> +\tControlList &controls =\n> +\t\tmode == DumpMode::Controls ? request->controls()\n> +\t\t\t\t\t : request->metadata();\n> +\tstd::ostream *output =\n> +\t\tmode == DumpMode::Controls ? dumpCaptureScript_\n> +\t\t\t\t\t : dumpMetadata_;\n> +\n> +\tif (!output || controls.empty())\n> +\t\treturn;\n> +\n> +\tstd::stringstream ss;\n> +\t/* TODO Figure out PFC */\n> +\tss << \" - \" << request->sequence() << \":\" << std::endl;\n> +\n> +\tconst ControlIdMap *idMap = controls.idMap();\n> +\tfor (const auto &pair : controls) {\n> +\t\tconst ControlId *ctrlId = idMap->at(pair.first);\n> +\t\t/* TODO Prettify enums (probably by upgrading ControlValue::toString()) */\n> +\t\tss << \" \" << ctrlId->name() << \": \" << pair.second.toString() << std::endl;\n> +\t}\n> +\n> +\t/*\n> +\t * TODO Investigate the overhead of flushing this frequently\n> +\t * Controls aren't going to be queued too frequently so it should be\n> +\t * fine to dump controls every frame. Metadata on the other hand needs\n> +\t * to be investigated.\n> +\t */\n> +\toutput->write(ss.str().c_str(), ss.str().size());\n> +\toutput->flush();\n> +}\n> +\n> /**\n> * \\class PipelineHandlerFactoryBase\n> * \\brief Base class for pipeline handler factories\n> --\n> 2.39.2\n> \n\n\nRegards,\nBarnabás Pőcze","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3A2CABD80A\n\tfor ;\n\tWed, 2 Oct 2024 13:28:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E766B63525;\n\tWed, 2 Oct 2024 15:28:57 +0200 (CEST)","from mail-4316.protonmail.ch (mail-4316.protonmail.ch\n\t[185.70.43.16])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1AD0D63510\n\tfor ;\n\tWed, 2 Oct 2024 15:28:56 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"X7rwXyq2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1727875735; x=1728134935;\n\tbh=gPW34RvlRr1yNy0WZdWhe2/06/i0nlnHtM+ZFQ7sqT4=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector;\n\tb=X7rwXyq2dY5ryfvISP6r2WpGwyclXHKFER6fW323LUBhniSPYJ8OnpmCnChEm8hVm\n\t2LvKcmoEwhQQRYQzg71/PwQAt3ZPiMyYHpIMxIuqK/nHnvs7iS5OS7hvCV3IBuRguP\n\tnVxqGcx59/QTMw5Iq73IUhwV0THhgQpUw2770CmF/8AHE1TrOY2/qmjAzIdXpfipm/\n\tn0KjgSE9AYeWWdkykBJ39X6VWgxJNkhDBknvWtrA5O2dWKiTz3X9J3e8ekrwBayLyb\n\tuJsC7zhDJ85r2puIXGFsRTn6jhlCg785VIez+9ODOrttQImNMVIbGHviUUWRyr43Wm\n\t3p0lk668jOk7A==","Date":"Wed, 02 Oct 2024 13:28:51 +0000","To":"Paul Elder ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org, laurent.pinchart@ideasonboard.com","Subject":"Re: [PATCH 1/2] pipeline: Add support for dumping capture script and\n\tmetadata","Message-ID":"","In-Reply-To":"<20240930170907.1404844-2-paul.elder@ideasonboard.com>","References":"<20240930170907.1404844-1-paul.elder@ideasonboard.com>\n\t<20240930170907.1404844-2-paul.elder@ideasonboard.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"4c0ba48e96f15a089eb57de1e2cfb8d3c17391f7","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31538,"web_url":"https://patchwork.libcamera.org/comment/31538/","msgid":"<20241002234127.GB17537@pendragon.ideasonboard.com>","date":"2024-10-02T23:41:27","subject":"Re: [PATCH 1/2] pipeline: Add support for dumping capture script and\n\tmetadata","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Wed, Oct 02, 2024 at 07:20:48PM +0900, Paul Elder wrote:\n> On Wed, Oct 02, 2024 at 08:28:26AM +0200, Jacopo Mondi wrote:\n> > On Tue, Oct 01, 2024 at 02:09:06AM GMT, Paul Elder wrote:\n> > > Add support for dumping capture scripts and metadata. The capture\n> > > scripts can then be fed into the cam application and a capture can thus\n> > > be \"replayed\". Metadata can also be dumped.\n> > >\n> > > Camera configuration is also dumped to the capture script. The cam\n> > > application currently does not support loading configuration from the\n> > > capture script, but support for that will be added in a subsequent\n> > > patch.\n> > >\n> > > These can be enabled by a new environment variable.\n> > \n> > Which needs to be documented :)\n> \n> I... opened the tab ready to write it then forgot to write it :)\n> \n> > > Signed-off-by: Paul Elder \n> > > ---\n> > > include/libcamera/internal/camera.h | 3 +\n> > > include/libcamera/internal/pipeline_handler.h | 16 ++++\n> > > src/libcamera/camera.cpp | 13 +++\n> > > src/libcamera/pipeline_handler.cpp | 93 ++++++++++++++++++-\n> > > 4 files changed, 124 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> > > index 0add0428bb5d..a42f03d4c755 100644\n> > > --- a/include/libcamera/internal/camera.h\n> > > +++ b/include/libcamera/internal/camera.h\n> > > @@ -19,6 +19,8 @@\n> > >\n> > > namespace libcamera {\n> > >\n> > > +enum class Orientation;\n\nYou should include orientation.h, the compiler will need to to determine\nthe size of the enum, to determine the layout of the Camera class. It's\nincluded indirectly at the moment, which explains why you don't get a\ncompilation error.\n\n> > > +\n> > > class CameraControlValidator;\n> > > class PipelineHandler;\n> > > class Stream;\n> > > @@ -65,6 +67,7 @@ private:\n> > > \tstd::string id_;\n> > > \tstd::set streams_;\n> > > \tstd::set activeStreams_;\n> > > +\tOrientation orientation_;\n> > >\n> > > \tbool disconnected_;\n> > > \tstd::atomic state_;\n> > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > index 0d38080369c5..fb3914185a01 100644\n> > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > @@ -9,6 +9,7 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -20,6 +21,8 @@\n> > >\n> > > namespace libcamera {\n> > >\n> > > +enum class Orientation;\n> > > +\n> > > class Camera;\n> > > class CameraConfiguration;\n> > > class CameraManager;\n> > > @@ -68,6 +71,9 @@ public:\n> > >\n> > > \tCameraManager *cameraManager() const { return manager_; }\n> > >\n> > > +\tvoid dumpConfiguration(const std::set &streams,\n> > > +\t\t\t const Orientation &orientation);\n\nPass it by value, it's an enum.\n\n> > > +\n> > \n> > To define the expected configuration format, I would make 2/2 precede 1/2\n> \n> Indeed 2/2 going first doesn't actually break anything. Sure.\n> \n> > > protected:\n> > > \tvoid registerCamera(std::shared_ptr camera);\n> > > \tvoid hotplugMediaDevice(MediaDevice *media);\n> > > @@ -81,6 +87,11 @@ protected:\n> > > \tCameraManager *manager_;\n> > >\n> > > private:\n> > > +\tenum DumpMode {\n\nMake it an enum class, you already prefix the enumerators with the enum\nname when you use them.\n\n> > > +\t\tControls,\n> > > +\t\tMetadata,\n> > > +\t};\n> > > +\n> > > \tvoid unlockMediaDevices();\n> > >\n> > > \tvoid mediaDeviceDisconnected(MediaDevice *media);\n> > > @@ -89,6 +100,8 @@ private:\n> > > \tvoid doQueueRequest(Request *request);\n> > > \tvoid doQueueRequests();\n> > >\n> > > +\tvoid dumpRequest(Request *request, DumpMode mode);\n> > > +\n> > > \tstd::vector> mediaDevices_;\n> > > \tstd::vector> cameras_;\n> > >\n> > > @@ -97,6 +110,9 @@ private:\n> > > \tconst char *name_;\n> > > \tunsigned int useCount_;\n> > >\n> > > +\tstd::ostream *dumpCaptureScript_;\n> > > +\tstd::ostream *dumpMetadata_;\n> > > +\n> > > \tfriend class PipelineHandlerFactoryBase;\n> > > };\n> > >\n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index a86f552a47bc..1282f99d839b 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -1215,6 +1215,9 @@ int Camera::configure(CameraConfiguration *config)\n> > > \t\td->activeStreams_.insert(stream);\n> > > \t}\n> > >\n> > > +\t/* TODO Save sensor configuration for dumping it to capture script */\n> > > +\td->orientation_ = config->orientation;\n> > > +\n> > > \td->setState(Private::CameraConfigured);\n> > >\n> > > \treturn 0;\n> > > @@ -1356,6 +1359,16 @@ int Camera::start(const ControlList *controls)\n> > >\n> > > \tASSERT(d->requestSequence_ == 0);\n> > >\n> > > +\t/*\n> > > +\t * Invoke method in blocking mode to avoid the risk of writing after\n> > > +\t * streaming has started.\n> > > +\t * This needs to be here as PipelineHandler::start is a virtual function\n> > > +\t * so it is impractical to add the dumping there.\n> > > +\t * TODO Pass the sensor configuration, once it is supported\n> > > +\t */\n> > > +\td->pipe_->invokeMethod(&PipelineHandler::dumpConfiguration,\n> > > +\t\t\t ConnectionTypeBlocking, d->activeStreams_, d->orientation_);\n> > \n> > Can't you pass in the CameraConfiguration which containts the\n> > StreamConfigurations and the the Orientation already ?\n> \n> CameraConfiguration can't be copied because it has virtual functions.\n> And CameraConfiguration is only available in Camera::configure(). So, no.\n> \n> > \n> > > +\n> > > \tret = d->pipe_->invokeMethod(&PipelineHandler::start,\n> > > \t\t\t\t ConnectionTypeBlocking, this, controls);\n> > > \tif (ret)\n> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > index e5940469127e..7002b4323bdd 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -8,6 +8,7 @@\n> > > #include \"libcamera/internal/pipeline_handler.h\"\n> > >\n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > >\n> > > @@ -68,14 +69,36 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> > > * through the PipelineHandlerFactoryBase::create() function.\n> > > */\n> > > PipelineHandler::PipelineHandler(CameraManager *manager)\n> > > -\t: manager_(manager), useCount_(0)\n> > > +\t: manager_(manager), useCount_(0),\n> > > +\t dumpCaptureScript_(nullptr), dumpMetadata_(nullptr)\n> > > {\n> > > +\t/* TODO Print notification that we're dumping capture script */\n\n\\todo\n\nor better, address it.\n\n> > > +\tconst char *file = utils::secure_getenv(\"LIBCAMERA_DUMP_CAPTURE_SCRIPT\");\n> > > +\tif (!file)\n> > > +\t\treturn;\n> > > +\n> > > +\tdumpCaptureScript_ = new std::ofstream(file);\n> > \n> > I was really expecting to be operating on File instances\n> \n> Oh ig we could do that too. I copied a few ideas from the logger.\n> \n> > > +\n> > > +\t/*\n> > > +\t * Metadata needs to go into a separate file because otherwise it'll\n> > > +\t * flood the capture script\n> > > +\t */\n> > > +\tdumpMetadata_ = new std::ofstream(std::string(file) + \".metadata\");\n> > > +\tstd::string str = \"frames:\\n\";\n> > > +\tdumpMetadata_->write(str.c_str(), str.size());\n> > \n> > and use the libyaml emitter for this instead of raw writes\n> > \n> > Have you considered it and decided it was better to use raw writes ?\n> \n> No I hadn't. I'll keep it raw for now though until you make the\n> YamlEmitter :)\n> \n> > > +\tdumpMetadata_->flush();\n> > > }\n> > >\n> > > PipelineHandler::~PipelineHandler()\n> > > {\n> > > \tfor (std::shared_ptr media : mediaDevices_)\n> > > \t\tmedia->release();\n> > > +\n> > > +\tif (dumpCaptureScript_)\n> > > +\t\tdelete dumpCaptureScript_;\n> > > +\n> > > +\tif (dumpMetadata_)\n> > > +\t\tdelete dumpMetadata_;\n> > > }\n> > >\n> > > /**\n> > > @@ -464,6 +487,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n> > >\n> > > \trequest->_d()->sequence_ = data->requestSequence_++;\n> > >\n> > > +\tdumpRequest(request, DumpMode::Controls);\n> > > +\n> > > \tif (request->_d()->cancelled_) {\n> > > \t\tcompleteRequest(request);\n> > > \t\treturn;\n> > > @@ -555,6 +580,8 @@ void PipelineHandler::completeRequest(Request *request)\n> > >\n> > > \trequest->_d()->complete();\n> > >\n> > > +\tdumpRequest(request, DumpMode::Metadata);\n> > > +\n> > > \tCamera::Private *data = camera->_d();\n> > >\n> > > \twhile (!data->queuedRequests_.empty()) {\n> > > @@ -758,6 +785,70 @@ void PipelineHandler::disconnect()\n> > > * \\return The CameraManager for this pipeline handler\n> > > */\n> > >\n> > > +void PipelineHandler::dumpConfiguration(const std::set &streams,\n> > > +\t\t\t\t\tconst Orientation &orientation)\n> > > +{\n> > > +\tif (!dumpCaptureScript_)\n> > > +\t\treturn;\n> > > +\n> > > +\tstd::stringstream ss;\n> > > +\tss << \"configuration:\" << std::endl;\n> > > +\tss << \" orientation: \" << orientation << std::endl;\n> > > +\n> > > +\t/* TODO Dump Sensor configuration */\n\n\\todo\n\nSame below.\n\n> > > +\n> > > +\tss << \" streams:\" << std::endl;\n> > > +\tfor (const auto &stream : streams) {\n> > > +\t\tconst StreamConfiguration &streamConfig = stream->configuration();\n> > > +\t\tss << \" - pixelFormat: \" << streamConfig.pixelFormat << std::endl;\n> > > +\t\tss << \" size: \" << streamConfig.size << std::endl;\n> > > +\t\tss << \" stride: \" << streamConfig.stride << std::endl;\n> > > +\t\tss << \" frameSize: \" << streamConfig.frameSize << std::endl;\n> > > +\t\tss << \" bufferCount: \" << streamConfig.bufferCount << std::endl;\n> > > +\t\tif (streamConfig.colorSpace)\n> > > +\t\t\tss << \" colorSpace: \" << streamConfig.colorSpace->toString() << std::endl;\n> > > +\t}\n> > > +\n> > > +\tdumpCaptureScript_->write(ss.str().c_str(), ss.str().size());\n\nI wonder if you could write\n\n\t*dumpCaptureScript_ << ss.rdbuf();\n\nBut you could use dumpCaptureScript_ directly with the << operator\nabove, no need for an intermediate stringstream.\n\n> > > +\n> > > +\tstd::string str = \"frames:\\n\";\n> > > +\tdumpCaptureScript_->write(str.c_str(), str.size());\n\n\t*dumpCaptureScript_ << \"frames:\\n\";\n\n> > > +\tdumpCaptureScript_->flush();\n> > > +}\n> > > +\n> > > +void PipelineHandler::dumpRequest(Request *request, DumpMode mode)\n> > > +{\n> > > +\tControlList &controls =\n\nconst\n\n> > > +\t\tmode == DumpMode::Controls ? request->controls()\n> > > +\t\t\t\t\t : request->metadata();\n> > > +\tstd::ostream *output =\n> > > +\t\tmode == DumpMode::Controls ? dumpCaptureScript_\n> > > +\t\t\t\t\t : dumpMetadata_;\n> > > +\n> > > +\tif (!output || controls.empty())\n> > > +\t\treturn;\n> > > +\n> > > +\tstd::stringstream ss;\n\nUse output directly, don't create an intermediate stringstream.\n\n> > > +\t/* TODO Figure out PFC */\n> > > +\tss << \" - \" << request->sequence() << \":\" << std::endl;\n> > > +\n> > > +\tconst ControlIdMap *idMap = controls.idMap();\n> > > +\tfor (const auto &pair : controls) {\n> > > +\t\tconst ControlId *ctrlId = idMap->at(pair.first);\n> > > +\t\t/* TODO Prettify enums (probably by upgrading ControlValue::toString()) */\n\nCould you elaborate on this for my information ?\n\n> > > +\t\tss << \" \" << ctrlId->name() << \": \" << pair.second.toString() << std::endl;\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * TODO Investigate the overhead of flushing this frequently\n> > > +\t * Controls aren't going to be queued too frequently so it should be\n\nI'm not too sure about that, I think we'll see some types of\napplications (or frameworks) setting controls in every request. We need\nto be ready for it.\n\n> > > +\t * fine to dump controls every frame. Metadata on the other hand needs\n> > > +\t * to be investigated.\n> > > +\t */\n> > > +\toutput->write(ss.str().c_str(), ss.str().size());\n> > > +\toutput->flush();\n> > > +}\n> > > +\n> > > /**\n> > > * \\class PipelineHandlerFactoryBase\n> > > * \\brief Base class for pipeline handler factories","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 87BECC3257\n\tfor ;\n\tWed, 2 Oct 2024 23:41:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4C68863525;\n\tThu, 3 Oct 2024 01:41:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F25E760553\n\tfor ;\n\tThu, 3 Oct 2024 01:41:30 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E945D55A;\n\tThu, 3 Oct 2024 01:39:57 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"AQESK6Mm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727912398;\n\tbh=IgqAoFHBO19a9FDxn/CtR9sZPVIxIXxiolx/7HJOWpU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AQESK6Mm3vWG0VSSvhUT1U8mhHDldxCQA4J/D6bA9EZSva0oMFNn0RTC3I9DPQ4HX\n\tT+6FyriG9R6Uc/q7UVdE/7cxx5Tk9OXu7rLWqxXh2K9Ir0/WvXtTCXLmck3hFuj72t\n\t3msYv7a9Bdxcxmom8NaeNxno8CCqkTugB6XXCbnw=","Date":"Thu, 3 Oct 2024 02:41:27 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 1/2] pipeline: Add support for dumping capture script and\n\tmetadata","Message-ID":"<20241002234127.GB17537@pendragon.ideasonboard.com>","References":"<20240930170907.1404844-1-paul.elder@ideasonboard.com>\n\t<20240930170907.1404844-2-paul.elder@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31539,"web_url":"https://patchwork.libcamera.org/comment/31539/","msgid":"<20241002234645.GC17537@pendragon.ideasonboard.com>","date":"2024-10-02T23:46:45","subject":"Re: [PATCH 1/2] pipeline: Add support for dumping capture script and\n\tmetadata","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nOn Wed, Oct 02, 2024 at 01:28:51PM +0000, Barnabás Pőcze wrote:\n> 2024. szeptember 30., hétfő 19:09 keltezéssel, Paul Elder írta:\n> \n> > Add support for dumping capture scripts and metadata. The capture\n> > scripts can then be fed into the cam application and a capture can thus\n> > be \"replayed\". Metadata can also be dumped.\n> > \n> > Camera configuration is also dumped to the capture script. The cam\n> > application currently does not support loading configuration from the\n> > capture script, but support for that will be added in a subsequent\n> > patch.\n> > \n> > These can be enabled by a new environment variable.\n> > \n> > Signed-off-by: Paul Elder \n> > ---\n> > include/libcamera/internal/camera.h | 3 +\n> > include/libcamera/internal/pipeline_handler.h | 16 ++++\n> > src/libcamera/camera.cpp | 13 +++\n> > src/libcamera/pipeline_handler.cpp | 93 ++++++++++++++++++-\n> > 4 files changed, 124 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> > index 0add0428bb5d..a42f03d4c755 100644\n> > --- a/include/libcamera/internal/camera.h\n> > +++ b/include/libcamera/internal/camera.h\n> > @@ -19,6 +19,8 @@\n> > \n> > namespace libcamera {\n> > \n> > +enum class Orientation;\n> > +\n> > class CameraControlValidator;\n> > class PipelineHandler;\n> > class Stream;\n> > @@ -65,6 +67,7 @@ private:\n> > \tstd::string id_;\n> > \tstd::set streams_;\n> > \tstd::set activeStreams_;\n> > +\tOrientation orientation_;\n> > \n> > \tbool disconnected_;\n> > \tstd::atomic state_;\n> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > index 0d38080369c5..fb3914185a01 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -9,6 +9,7 @@\n> > \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -20,6 +21,8 @@\n> > \n> > namespace libcamera {\n> > \n> > +enum class Orientation;\n> > +\n> > class Camera;\n> > class CameraConfiguration;\n> > class CameraManager;\n> > @@ -68,6 +71,9 @@ public:\n> > \n> > \tCameraManager *cameraManager() const { return manager_; }\n> > \n> > +\tvoid dumpConfiguration(const std::set &streams,\n> > +\t\t\t const Orientation &orientation);\n> > +\n> > protected:\n> > \tvoid registerCamera(std::shared_ptr camera);\n> > \tvoid hotplugMediaDevice(MediaDevice *media);\n> > @@ -81,6 +87,11 @@ protected:\n> > \tCameraManager *manager_;\n> > \n> > private:\n> > +\tenum DumpMode {\n> > +\t\tControls,\n> > +\t\tMetadata,\n> > +\t};\n> > +\n> > \tvoid unlockMediaDevices();\n> > \n> > \tvoid mediaDeviceDisconnected(MediaDevice *media);\n> > @@ -89,6 +100,8 @@ private:\n> > \tvoid doQueueRequest(Request *request);\n> > \tvoid doQueueRequests();\n> > \n> > +\tvoid dumpRequest(Request *request, DumpMode mode);\n> > +\n> > \tstd::vector> mediaDevices_;\n> > \tstd::vector> cameras_;\n> > \n> > @@ -97,6 +110,9 @@ private:\n> > \tconst char *name_;\n> > \tunsigned int useCount_;\n> > \n> > +\tstd::ostream *dumpCaptureScript_;\n> > +\tstd::ostream *dumpMetadata_;\n> \n> Have you looked at using `std::optional` here instead? Or `std::unique_ptr`?\n\nAvoiding a manual delete is certainly a good idea.\n\n> > +\n> > \tfriend class PipelineHandlerFactoryBase;\n> > };\n> > \n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index a86f552a47bc..1282f99d839b 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -1215,6 +1215,9 @@ int Camera::configure(CameraConfiguration *config)\n> > \t\td->activeStreams_.insert(stream);\n> > \t}\n> > \n> > +\t/* TODO Save sensor configuration for dumping it to capture script */\n> > +\td->orientation_ = config->orientation;\n> > +\n> > \td->setState(Private::CameraConfigured);\n> > \n> > \treturn 0;\n> > @@ -1356,6 +1359,16 @@ int Camera::start(const ControlList *controls)\n> > \n> > \tASSERT(d->requestSequence_ == 0);\n> > \n> > +\t/*\n> > +\t * Invoke method in blocking mode to avoid the risk of writing after\n> > +\t * streaming has started.\n> > +\t * This needs to be here as PipelineHandler::start is a virtual function\n> > +\t * so it is impractical to add the dumping there.\n> > +\t * TODO Pass the sensor configuration, once it is supported\n> > +\t */\n> > +\td->pipe_->invokeMethod(&PipelineHandler::dumpConfiguration,\n> > +\t\t\t ConnectionTypeBlocking, d->activeStreams_, d->orientation_);\n> > +\n> > \tret = d->pipe_->invokeMethod(&PipelineHandler::start,\n> > \t\t\t\t ConnectionTypeBlocking, this, controls);\n> > \tif (ret)\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index e5940469127e..7002b4323bdd 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -8,6 +8,7 @@\n> > #include \"libcamera/internal/pipeline_handler.h\"\n> > \n> > #include \n> > +#include \n> > #include \n> > #include \n> > \n> > @@ -68,14 +69,36 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> > * through the PipelineHandlerFactoryBase::create() function.\n> > */\n> > PipelineHandler::PipelineHandler(CameraManager *manager)\n> > -\t: manager_(manager), useCount_(0)\n> > +\t: manager_(manager), useCount_(0),\n> > +\t dumpCaptureScript_(nullptr), dumpMetadata_(nullptr)\n> > {\n> > +\t/* TODO Print notification that we're dumping capture script */\n> > +\tconst char *file = utils::secure_getenv(\"LIBCAMERA_DUMP_CAPTURE_SCRIPT\");\n> > +\tif (!file)\n> > +\t\treturn;\n> > +\n> > +\tdumpCaptureScript_ = new std::ofstream(file);\n> > +\n> > +\t/*\n> > +\t * Metadata needs to go into a separate file because otherwise it'll\n> > +\t * flood the capture script\n> > +\t */\n> > +\tdumpMetadata_ = new std::ofstream(std::string(file) + \".metadata\");\n> > +\tstd::string str = \"frames:\\n\";\n> \n> I think `std::string_view` is sufficient for this.\n> \n> > +\tdumpMetadata_->write(str.c_str(), str.size());\n> > +\tdumpMetadata_->flush();\n> > }\n> > \n> > PipelineHandler::~PipelineHandler()\n> > {\n> > \tfor (std::shared_ptr media : mediaDevices_)\n> > \t\tmedia->release();\n> > +\n> > +\tif (dumpCaptureScript_)\n> > +\t\tdelete dumpCaptureScript_;\n> > +\n> > +\tif (dumpMetadata_)\n> > +\t\tdelete dumpMetadata_;\n> \n> `delete` already checks for `nullptr`, so if you want to go the raw ptr way,\n> then there is no need for an extra check. But as I mentioned I think `std::optional`\n> or `std::unique_ptr` would probably be preferable.\n> \n> > }\n> > \n> > /**\n> > @@ -464,6 +487,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n> > \n> > \trequest->_d()->sequence_ = data->requestSequence_++;\n> > \n> > +\tdumpRequest(request, DumpMode::Controls);\n> > +\n> > \tif (request->_d()->cancelled_) {\n> > \t\tcompleteRequest(request);\n> > \t\treturn;\n> > @@ -555,6 +580,8 @@ void PipelineHandler::completeRequest(Request *request)\n> > \n> > \trequest->_d()->complete();\n> > \n> > +\tdumpRequest(request, DumpMode::Metadata);\n> > +\n> > \tCamera::Private *data = camera->_d();\n> > \n> > \twhile (!data->queuedRequests_.empty()) {\n> > @@ -758,6 +785,70 @@ void PipelineHandler::disconnect()\n> > * \\return The CameraManager for this pipeline handler\n> > */\n> > \n> > +void PipelineHandler::dumpConfiguration(const std::set &streams,\n> > +\t\t\t\t\tconst Orientation &orientation)\n> > +{\n> > +\tif (!dumpCaptureScript_)\n> > +\t\treturn;\n> > +\n> > +\tstd::stringstream ss;\n> > +\tss << \"configuration:\" << std::endl;\n> > +\tss << \" orientation: \" << orientation << std::endl;\n> > +\n> > +\t/* TODO Dump Sensor configuration */\n> > +\n> > +\tss << \" streams:\" << std::endl;\n> > +\tfor (const auto &stream : streams) {\n> > +\t\tconst StreamConfiguration &streamConfig = stream->configuration();\n> > +\t\tss << \" - pixelFormat: \" << streamConfig.pixelFormat << std::endl;\n> > +\t\tss << \" size: \" << streamConfig.size << std::endl;\n> > +\t\tss << \" stride: \" << streamConfig.stride << std::endl;\n> > +\t\tss << \" frameSize: \" << streamConfig.frameSize << std::endl;\n> > +\t\tss << \" bufferCount: \" << streamConfig.bufferCount << std::endl;\n> > +\t\tif (streamConfig.colorSpace)\n> > +\t\t\tss << \" colorSpace: \" << streamConfig.colorSpace->toString() << std::endl;\n> > +\t}\n> > +\n> > +\tdumpCaptureScript_->write(ss.str().c_str(), ss.str().size());\n> \n> It's probably best to avoid calling `std::stringstream::str()` twice since it\n> returns a copy of the string twice in this case. I believe you might as well do\n> `auto str = std::move(ss).str()`, which should avoid the copy altogether.\n\nEven better, we shouldn't use a stringstream in the first place, we can\noutput to dumpCaptureScript_ directly.\n\n> > +\n> > +\tstd::string str = \"frames:\\n\";\n> > +\tdumpCaptureScript_->write(str.c_str(), str.size());\n> > +\tdumpCaptureScript_->flush();\n> > +}\n> > +\n> > +void PipelineHandler::dumpRequest(Request *request, DumpMode mode)\n> > +{\n> > +\tControlList &controls =\n> > +\t\tmode == DumpMode::Controls ? request->controls()\n> > +\t\t\t\t\t : request->metadata();\n> > +\tstd::ostream *output =\n> > +\t\tmode == DumpMode::Controls ? dumpCaptureScript_\n> > +\t\t\t\t\t : dumpMetadata_;\n> > +\n> > +\tif (!output || controls.empty())\n> > +\t\treturn;\n> > +\n> > +\tstd::stringstream ss;\n> > +\t/* TODO Figure out PFC */\n> > +\tss << \" - \" << request->sequence() << \":\" << std::endl;\n> > +\n> > +\tconst ControlIdMap *idMap = controls.idMap();\n> > +\tfor (const auto &pair : controls) {\n> > +\t\tconst ControlId *ctrlId = idMap->at(pair.first);\n> > +\t\t/* TODO Prettify enums (probably by upgrading ControlValue::toString()) */\n> > +\t\tss << \" \" << ctrlId->name() << \": \" << pair.second.toString() << std::endl;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * TODO Investigate the overhead of flushing this frequently\n> > +\t * Controls aren't going to be queued too frequently so it should be\n> > +\t * fine to dump controls every frame. Metadata on the other hand needs\n> > +\t * to be investigated.\n> > +\t */\n> > +\toutput->write(ss.str().c_str(), ss.str().size());\n> > +\toutput->flush();\n> > +}\n> > +\n> > /**\n> > * \\class PipelineHandlerFactoryBase\n> > * \\brief Base class for pipeline handler factories","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B9E9DBD80A\n\tfor ;\n\tWed, 2 Oct 2024 23:46:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9036163525;\n\tThu, 3 Oct 2024 01:46:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6E7BD60553\n\tfor ;\n\tThu, 3 Oct 2024 01:46:48 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5977F55A;\n\tThu, 3 Oct 2024 01:45:15 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"NwRwiFvc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727912715;\n\tbh=z0m/tG6yuDK9rdlPUInbpU81cXT/rVSE0nYxtOEbDfs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NwRwiFvcMXr5qY0Hp+at3zkqUNA29QwMj4goL3Dmpu9TDIkAEpqXhj2/DAgXb1VEH\n\t4rlGoauY86ObLLTk0zv1/5ozzSby+FOhLmazMWMEpWfSyjGMgW3FhaDmMcbVtZazVx\n\tCrKI3ZKUW+eCIeNe7t+AFhGo3qEqlj6iD/Nj/78Q=","Date":"Thu, 3 Oct 2024 02:46:45 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 1/2] pipeline: Add support for dumping capture script and\n\tmetadata","Message-ID":"<20241002234645.GC17537@pendragon.ideasonboard.com>","References":"<20240930170907.1404844-1-paul.elder@ideasonboard.com>\n\t<20240930170907.1404844-2-paul.elder@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31541,"web_url":"https://patchwork.libcamera.org/comment/31541/","msgid":"","date":"2024-10-03T00:46:38","subject":"Re: [PATCH 1/2] pipeline: Add support for dumping capture script and\n\tmetadata","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Thu, Oct 03, 2024 at 02:41:27AM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Wed, Oct 02, 2024 at 07:20:48PM +0900, Paul Elder wrote:\n> > On Wed, Oct 02, 2024 at 08:28:26AM +0200, Jacopo Mondi wrote:\n> > > On Tue, Oct 01, 2024 at 02:09:06AM GMT, Paul Elder wrote:\n> > > > Add support for dumping capture scripts and metadata. The capture\n> > > > scripts can then be fed into the cam application and a capture can thus\n> > > > be \"replayed\". Metadata can also be dumped.\n> > > >\n> > > > Camera configuration is also dumped to the capture script. The cam\n> > > > application currently does not support loading configuration from the\n> > > > capture script, but support for that will be added in a subsequent\n> > > > patch.\n> > > >\n> > > > These can be enabled by a new environment variable.\n> > > \n> > > Which needs to be documented :)\n> > \n> > I... opened the tab ready to write it then forgot to write it :)\n> > \n> > > > Signed-off-by: Paul Elder \n> > > > ---\n> > > > include/libcamera/internal/camera.h | 3 +\n> > > > include/libcamera/internal/pipeline_handler.h | 16 ++++\n> > > > src/libcamera/camera.cpp | 13 +++\n> > > > src/libcamera/pipeline_handler.cpp | 93 ++++++++++++++++++-\n> > > > 4 files changed, 124 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> > > > index 0add0428bb5d..a42f03d4c755 100644\n> > > > --- a/include/libcamera/internal/camera.h\n> > > > +++ b/include/libcamera/internal/camera.h\n> > > > @@ -19,6 +19,8 @@\n> > > >\n> > > > namespace libcamera {\n> > > >\n> > > > +enum class Orientation;\n> \n> You should include orientation.h, the compiler will need to to determine\n> the size of the enum, to determine the layout of the Camera class. It's\n> included indirectly at the moment, which explains why you don't get a\n> compilation error.\n> \n> > > > +\n> > > > class CameraControlValidator;\n> > > > class PipelineHandler;\n> > > > class Stream;\n> > > > @@ -65,6 +67,7 @@ private:\n> > > > \tstd::string id_;\n> > > > \tstd::set streams_;\n> > > > \tstd::set activeStreams_;\n> > > > +\tOrientation orientation_;\n> > > >\n> > > > \tbool disconnected_;\n> > > > \tstd::atomic state_;\n> > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > > index 0d38080369c5..fb3914185a01 100644\n> > > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > > @@ -9,6 +9,7 @@\n> > > >\n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > > #include \n> > > > @@ -20,6 +21,8 @@\n> > > >\n> > > > namespace libcamera {\n> > > >\n> > > > +enum class Orientation;\n> > > > +\n> > > > class Camera;\n> > > > class CameraConfiguration;\n> > > > class CameraManager;\n> > > > @@ -68,6 +71,9 @@ public:\n> > > >\n> > > > \tCameraManager *cameraManager() const { return manager_; }\n> > > >\n> > > > +\tvoid dumpConfiguration(const std::set &streams,\n> > > > +\t\t\t const Orientation &orientation);\n> \n> Pass it by value, it's an enum.\n> \n> > > > +\n> > > \n> > > To define the expected configuration format, I would make 2/2 precede 1/2\n> > \n> > Indeed 2/2 going first doesn't actually break anything. Sure.\n> > \n> > > > protected:\n> > > > \tvoid registerCamera(std::shared_ptr camera);\n> > > > \tvoid hotplugMediaDevice(MediaDevice *media);\n> > > > @@ -81,6 +87,11 @@ protected:\n> > > > \tCameraManager *manager_;\n> > > >\n> > > > private:\n> > > > +\tenum DumpMode {\n> \n> Make it an enum class, you already prefix the enumerators with the enum\n> name when you use them.\n> \n> > > > +\t\tControls,\n> > > > +\t\tMetadata,\n> > > > +\t};\n> > > > +\n> > > > \tvoid unlockMediaDevices();\n> > > >\n> > > > \tvoid mediaDeviceDisconnected(MediaDevice *media);\n> > > > @@ -89,6 +100,8 @@ private:\n> > > > \tvoid doQueueRequest(Request *request);\n> > > > \tvoid doQueueRequests();\n> > > >\n> > > > +\tvoid dumpRequest(Request *request, DumpMode mode);\n> > > > +\n> > > > \tstd::vector> mediaDevices_;\n> > > > \tstd::vector> cameras_;\n> > > >\n> > > > @@ -97,6 +110,9 @@ private:\n> > > > \tconst char *name_;\n> > > > \tunsigned int useCount_;\n> > > >\n> > > > +\tstd::ostream *dumpCaptureScript_;\n> > > > +\tstd::ostream *dumpMetadata_;\n> > > > +\n> > > > \tfriend class PipelineHandlerFactoryBase;\n> > > > };\n> > > >\n> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > index a86f552a47bc..1282f99d839b 100644\n> > > > --- a/src/libcamera/camera.cpp\n> > > > +++ b/src/libcamera/camera.cpp\n> > > > @@ -1215,6 +1215,9 @@ int Camera::configure(CameraConfiguration *config)\n> > > > \t\td->activeStreams_.insert(stream);\n> > > > \t}\n> > > >\n> > > > +\t/* TODO Save sensor configuration for dumping it to capture script */\n> > > > +\td->orientation_ = config->orientation;\n> > > > +\n> > > > \td->setState(Private::CameraConfigured);\n> > > >\n> > > > \treturn 0;\n> > > > @@ -1356,6 +1359,16 @@ int Camera::start(const ControlList *controls)\n> > > >\n> > > > \tASSERT(d->requestSequence_ == 0);\n> > > >\n> > > > +\t/*\n> > > > +\t * Invoke method in blocking mode to avoid the risk of writing after\n> > > > +\t * streaming has started.\n> > > > +\t * This needs to be here as PipelineHandler::start is a virtual function\n> > > > +\t * so it is impractical to add the dumping there.\n> > > > +\t * TODO Pass the sensor configuration, once it is supported\n> > > > +\t */\n> > > > +\td->pipe_->invokeMethod(&PipelineHandler::dumpConfiguration,\n> > > > +\t\t\t ConnectionTypeBlocking, d->activeStreams_, d->orientation_);\n> > > \n> > > Can't you pass in the CameraConfiguration which containts the\n> > > StreamConfigurations and the the Orientation already ?\n> > \n> > CameraConfiguration can't be copied because it has virtual functions.\n> > And CameraConfiguration is only available in Camera::configure(). So, no.\n> > \n> > > \n> > > > +\n> > > > \tret = d->pipe_->invokeMethod(&PipelineHandler::start,\n> > > > \t\t\t\t ConnectionTypeBlocking, this, controls);\n> > > > \tif (ret)\n> > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > > index e5940469127e..7002b4323bdd 100644\n> > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > @@ -8,6 +8,7 @@\n> > > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > >\n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > >\n> > > > @@ -68,14 +69,36 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> > > > * through the PipelineHandlerFactoryBase::create() function.\n> > > > */\n> > > > PipelineHandler::PipelineHandler(CameraManager *manager)\n> > > > -\t: manager_(manager), useCount_(0)\n> > > > +\t: manager_(manager), useCount_(0),\n> > > > +\t dumpCaptureScript_(nullptr), dumpMetadata_(nullptr)\n> > > > {\n> > > > +\t/* TODO Print notification that we're dumping capture script */\n> \n> \\todo\n> \n> or better, address it.\n> \n> > > > +\tconst char *file = utils::secure_getenv(\"LIBCAMERA_DUMP_CAPTURE_SCRIPT\");\n> > > > +\tif (!file)\n> > > > +\t\treturn;\n> > > > +\n> > > > +\tdumpCaptureScript_ = new std::ofstream(file);\n> > > \n> > > I was really expecting to be operating on File instances\n> > \n> > Oh ig we could do that too. I copied a few ideas from the logger.\n> > \n> > > > +\n> > > > +\t/*\n> > > > +\t * Metadata needs to go into a separate file because otherwise it'll\n> > > > +\t * flood the capture script\n> > > > +\t */\n> > > > +\tdumpMetadata_ = new std::ofstream(std::string(file) + \".metadata\");\n> > > > +\tstd::string str = \"frames:\\n\";\n> > > > +\tdumpMetadata_->write(str.c_str(), str.size());\n> > > \n> > > and use the libyaml emitter for this instead of raw writes\n> > > \n> > > Have you considered it and decided it was better to use raw writes ?\n> > \n> > No I hadn't. I'll keep it raw for now though until you make the\n> > YamlEmitter :)\n> > \n> > > > +\tdumpMetadata_->flush();\n> > > > }\n> > > >\n> > > > PipelineHandler::~PipelineHandler()\n> > > > {\n> > > > \tfor (std::shared_ptr media : mediaDevices_)\n> > > > \t\tmedia->release();\n> > > > +\n> > > > +\tif (dumpCaptureScript_)\n> > > > +\t\tdelete dumpCaptureScript_;\n> > > > +\n> > > > +\tif (dumpMetadata_)\n> > > > +\t\tdelete dumpMetadata_;\n> > > > }\n> > > >\n> > > > /**\n> > > > @@ -464,6 +487,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n> > > >\n> > > > \trequest->_d()->sequence_ = data->requestSequence_++;\n> > > >\n> > > > +\tdumpRequest(request, DumpMode::Controls);\n> > > > +\n> > > > \tif (request->_d()->cancelled_) {\n> > > > \t\tcompleteRequest(request);\n> > > > \t\treturn;\n> > > > @@ -555,6 +580,8 @@ void PipelineHandler::completeRequest(Request *request)\n> > > >\n> > > > \trequest->_d()->complete();\n> > > >\n> > > > +\tdumpRequest(request, DumpMode::Metadata);\n> > > > +\n> > > > \tCamera::Private *data = camera->_d();\n> > > >\n> > > > \twhile (!data->queuedRequests_.empty()) {\n> > > > @@ -758,6 +785,70 @@ void PipelineHandler::disconnect()\n> > > > * \\return The CameraManager for this pipeline handler\n> > > > */\n> > > >\n> > > > +void PipelineHandler::dumpConfiguration(const std::set &streams,\n> > > > +\t\t\t\t\tconst Orientation &orientation)\n> > > > +{\n> > > > +\tif (!dumpCaptureScript_)\n> > > > +\t\treturn;\n> > > > +\n> > > > +\tstd::stringstream ss;\n> > > > +\tss << \"configuration:\" << std::endl;\n> > > > +\tss << \" orientation: \" << orientation << std::endl;\n> > > > +\n> > > > +\t/* TODO Dump Sensor configuration */\n> \n> \\todo\n> \n> Same below.\n> \n> > > > +\n> > > > +\tss << \" streams:\" << std::endl;\n> > > > +\tfor (const auto &stream : streams) {\n> > > > +\t\tconst StreamConfiguration &streamConfig = stream->configuration();\n> > > > +\t\tss << \" - pixelFormat: \" << streamConfig.pixelFormat << std::endl;\n> > > > +\t\tss << \" size: \" << streamConfig.size << std::endl;\n> > > > +\t\tss << \" stride: \" << streamConfig.stride << std::endl;\n> > > > +\t\tss << \" frameSize: \" << streamConfig.frameSize << std::endl;\n> > > > +\t\tss << \" bufferCount: \" << streamConfig.bufferCount << std::endl;\n> > > > +\t\tif (streamConfig.colorSpace)\n> > > > +\t\t\tss << \" colorSpace: \" << streamConfig.colorSpace->toString() << std::endl;\n> > > > +\t}\n> > > > +\n> > > > +\tdumpCaptureScript_->write(ss.str().c_str(), ss.str().size());\n> \n> I wonder if you could write\n> \n> \t*dumpCaptureScript_ << ss.rdbuf();\n> \n> But you could use dumpCaptureScript_ directly with the << operator\n> above, no need for an intermediate stringstream.\n> \n\nack\n\n> > > > +\n> > > > +\tstd::string str = \"frames:\\n\";\n> > > > +\tdumpCaptureScript_->write(str.c_str(), str.size());\n> \n> \t*dumpCaptureScript_ << \"frames:\\n\";\n> \n> > > > +\tdumpCaptureScript_->flush();\n> > > > +}\n> > > > +\n> > > > +void PipelineHandler::dumpRequest(Request *request, DumpMode mode)\n> > > > +{\n> > > > +\tControlList &controls =\n> \n> const\n> \n> > > > +\t\tmode == DumpMode::Controls ? request->controls()\n> > > > +\t\t\t\t\t : request->metadata();\n> > > > +\tstd::ostream *output =\n> > > > +\t\tmode == DumpMode::Controls ? dumpCaptureScript_\n> > > > +\t\t\t\t\t : dumpMetadata_;\n> > > > +\n> > > > +\tif (!output || controls.empty())\n> > > > +\t\treturn;\n> > > > +\n> > > > +\tstd::stringstream ss;\n> \n> Use output directly, don't create an intermediate stringstream.\n> \n> > > > +\t/* TODO Figure out PFC */\n> > > > +\tss << \" - \" << request->sequence() << \":\" << std::endl;\n> > > > +\n> > > > +\tconst ControlIdMap *idMap = controls.idMap();\n> > > > +\tfor (const auto &pair : controls) {\n> > > > +\t\tconst ControlId *ctrlId = idMap->at(pair.first);\n> > > > +\t\t/* TODO Prettify enums (probably by upgrading ControlValue::toString()) */\n> \n> Could you elaborate on this for my information ?\n\nThis will print enums just as numbers, so you'd get \"AeMeteringMode: 0\"\ninstead of \"AeMeteringMode: MeteringCentreWeighted\".\n\n> \n> > > > +\t\tss << \" \" << ctrlId->name() << \": \" << pair.second.toString() << std::endl;\n> > > > +\t}\n> > > > +\n> > > > +\t/*\n> > > > +\t * TODO Investigate the overhead of flushing this frequently\n> > > > +\t * Controls aren't going to be queued too frequently so it should be\n> \n> I'm not too sure about that, I think we'll see some types of\n> applications (or frameworks) setting controls in every request. We need\n> to be ready for it.\n> \n\nGood point. Although I'm not sure how else to determine how often to\nflush. Would it be good enough to add another environment variable to\nset every how many frames to dump? And default to a value > 1? Or count\nhow often it's getting queued and adjust the dump rate dynamically (with\nan environment variable to override to \"dump every frame\" for debugging\npurposes)?\n\nIn either case ig it's time to investigate the actual overhead :/\n\n\nPaul\n\n> > > > +\t * fine to dump controls every frame. Metadata on the other hand needs\n> > > > +\t * to be investigated.\n> > > > +\t */\n> > > > +\toutput->write(ss.str().c_str(), ss.str().size());\n> > > > +\toutput->flush();\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\class PipelineHandlerFactoryBase\n> > > > * \\brief Base class for pipeline handler factories","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5F004C3257\n\tfor ;\n\tThu, 3 Oct 2024 00:46:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 053BB63525;\n\tThu, 3 Oct 2024 02:46:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D7E5862C8F\n\tfor ;\n\tThu, 3 Oct 2024 02:46:46 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:61c1:eb0:c20f:5c1d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7831655;\n\tThu, 3 Oct 2024 02:45:12 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"FCIFJ3Fk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727916313;\n\tbh=o0jAg0YmaprhOjYz38K1oM5jDoyoMOGkqE/u6zBHvQs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FCIFJ3FkWyVIuS1ubsqzymnOxEwMKQ4VoSZbnkgVhqPj1W0wKJ/HlvttrZY8t+JD9\n\tFWy8EIdgvVPfTMr1SU1fbNaODKJA06ppgFUzzp0o2REq5/aqFf92uVODRz8Z+wHmyf\n\tIkFfKh+M2+JZfuK6/J0NF/+3FTsTWpMs1iXZ1QJw=","Date":"Thu, 3 Oct 2024 09:46:38 +0900","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 1/2] pipeline: Add support for dumping capture script and\n\tmetadata","Message-ID":"","References":"<20240930170907.1404844-1-paul.elder@ideasonboard.com>\n\t<20240930170907.1404844-2-paul.elder@ideasonboard.com>\n\t\n\t\n\t<20241002234127.GB17537@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20241002234127.GB17537@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31564,"web_url":"https://patchwork.libcamera.org/comment/31564/","msgid":"<20241003131505.GA5484@pendragon.ideasonboard.com>","date":"2024-10-03T13:15:05","subject":"Re: [PATCH 1/2] pipeline: Add support for dumping capture script and\n\tmetadata","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Oct 03, 2024 at 09:46:38AM +0900, Paul Elder wrote:\n> On Thu, Oct 03, 2024 at 02:41:27AM +0300, Laurent Pinchart wrote:\n> > On Wed, Oct 02, 2024 at 07:20:48PM +0900, Paul Elder wrote:\n> > > On Wed, Oct 02, 2024 at 08:28:26AM +0200, Jacopo Mondi wrote:\n> > > > On Tue, Oct 01, 2024 at 02:09:06AM GMT, Paul Elder wrote:\n> > > > > Add support for dumping capture scripts and metadata. The capture\n> > > > > scripts can then be fed into the cam application and a capture can thus\n> > > > > be \"replayed\". Metadata can also be dumped.\n> > > > >\n> > > > > Camera configuration is also dumped to the capture script. The cam\n> > > > > application currently does not support loading configuration from the\n> > > > > capture script, but support for that will be added in a subsequent\n> > > > > patch.\n> > > > >\n> > > > > These can be enabled by a new environment variable.\n> > > > \n> > > > Which needs to be documented :)\n> > > \n> > > I... opened the tab ready to write it then forgot to write it :)\n> > > \n> > > > > Signed-off-by: Paul Elder \n> > > > > ---\n> > > > > include/libcamera/internal/camera.h | 3 +\n> > > > > include/libcamera/internal/pipeline_handler.h | 16 ++++\n> > > > > src/libcamera/camera.cpp | 13 +++\n> > > > > src/libcamera/pipeline_handler.cpp | 93 ++++++++++++++++++-\n> > > > > 4 files changed, 124 insertions(+), 1 deletion(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> > > > > index 0add0428bb5d..a42f03d4c755 100644\n> > > > > --- a/include/libcamera/internal/camera.h\n> > > > > +++ b/include/libcamera/internal/camera.h\n> > > > > @@ -19,6 +19,8 @@\n> > > > >\n> > > > > namespace libcamera {\n> > > > >\n> > > > > +enum class Orientation;\n> > \n> > You should include orientation.h, the compiler will need to to determine\n> > the size of the enum, to determine the layout of the Camera class. It's\n> > included indirectly at the moment, which explains why you don't get a\n> > compilation error.\n> > \n> > > > > +\n> > > > > class CameraControlValidator;\n> > > > > class PipelineHandler;\n> > > > > class Stream;\n> > > > > @@ -65,6 +67,7 @@ private:\n> > > > > \tstd::string id_;\n> > > > > \tstd::set streams_;\n> > > > > \tstd::set activeStreams_;\n> > > > > +\tOrientation orientation_;\n> > > > >\n> > > > > \tbool disconnected_;\n> > > > > \tstd::atomic state_;\n> > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > > > index 0d38080369c5..fb3914185a01 100644\n> > > > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > > > @@ -9,6 +9,7 @@\n> > > > >\n> > > > > #include \n> > > > > #include \n> > > > > +#include \n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > @@ -20,6 +21,8 @@\n> > > > >\n> > > > > namespace libcamera {\n> > > > >\n> > > > > +enum class Orientation;\n> > > > > +\n> > > > > class Camera;\n> > > > > class CameraConfiguration;\n> > > > > class CameraManager;\n> > > > > @@ -68,6 +71,9 @@ public:\n> > > > >\n> > > > > \tCameraManager *cameraManager() const { return manager_; }\n> > > > >\n> > > > > +\tvoid dumpConfiguration(const std::set &streams,\n> > > > > +\t\t\t const Orientation &orientation);\n> > \n> > Pass it by value, it's an enum.\n> > \n> > > > > +\n> > > > \n> > > > To define the expected configuration format, I would make 2/2 precede 1/2\n> > > \n> > > Indeed 2/2 going first doesn't actually break anything. Sure.\n> > > \n> > > > > protected:\n> > > > > \tvoid registerCamera(std::shared_ptr camera);\n> > > > > \tvoid hotplugMediaDevice(MediaDevice *media);\n> > > > > @@ -81,6 +87,11 @@ protected:\n> > > > > \tCameraManager *manager_;\n> > > > >\n> > > > > private:\n> > > > > +\tenum DumpMode {\n> > \n> > Make it an enum class, you already prefix the enumerators with the enum\n> > name when you use them.\n> > \n> > > > > +\t\tControls,\n> > > > > +\t\tMetadata,\n> > > > > +\t};\n> > > > > +\n> > > > > \tvoid unlockMediaDevices();\n> > > > >\n> > > > > \tvoid mediaDeviceDisconnected(MediaDevice *media);\n> > > > > @@ -89,6 +100,8 @@ private:\n> > > > > \tvoid doQueueRequest(Request *request);\n> > > > > \tvoid doQueueRequests();\n> > > > >\n> > > > > +\tvoid dumpRequest(Request *request, DumpMode mode);\n> > > > > +\n> > > > > \tstd::vector> mediaDevices_;\n> > > > > \tstd::vector> cameras_;\n> > > > >\n> > > > > @@ -97,6 +110,9 @@ private:\n> > > > > \tconst char *name_;\n> > > > > \tunsigned int useCount_;\n> > > > >\n> > > > > +\tstd::ostream *dumpCaptureScript_;\n> > > > > +\tstd::ostream *dumpMetadata_;\n> > > > > +\n> > > > > \tfriend class PipelineHandlerFactoryBase;\n> > > > > };\n> > > > >\n> > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > > index a86f552a47bc..1282f99d839b 100644\n> > > > > --- a/src/libcamera/camera.cpp\n> > > > > +++ b/src/libcamera/camera.cpp\n> > > > > @@ -1215,6 +1215,9 @@ int Camera::configure(CameraConfiguration *config)\n> > > > > \t\td->activeStreams_.insert(stream);\n> > > > > \t}\n> > > > >\n> > > > > +\t/* TODO Save sensor configuration for dumping it to capture script */\n> > > > > +\td->orientation_ = config->orientation;\n> > > > > +\n> > > > > \td->setState(Private::CameraConfigured);\n> > > > >\n> > > > > \treturn 0;\n> > > > > @@ -1356,6 +1359,16 @@ int Camera::start(const ControlList *controls)\n> > > > >\n> > > > > \tASSERT(d->requestSequence_ == 0);\n> > > > >\n> > > > > +\t/*\n> > > > > +\t * Invoke method in blocking mode to avoid the risk of writing after\n> > > > > +\t * streaming has started.\n> > > > > +\t * This needs to be here as PipelineHandler::start is a virtual function\n> > > > > +\t * so it is impractical to add the dumping there.\n> > > > > +\t * TODO Pass the sensor configuration, once it is supported\n> > > > > +\t */\n> > > > > +\td->pipe_->invokeMethod(&PipelineHandler::dumpConfiguration,\n> > > > > +\t\t\t ConnectionTypeBlocking, d->activeStreams_, d->orientation_);\n> > > > \n> > > > Can't you pass in the CameraConfiguration which containts the\n> > > > StreamConfigurations and the the Orientation already ?\n> > > \n> > > CameraConfiguration can't be copied because it has virtual functions.\n> > > And CameraConfiguration is only available in Camera::configure(). So, no.\n> > > \n> > > > \n> > > > > +\n> > > > > \tret = d->pipe_->invokeMethod(&PipelineHandler::start,\n> > > > > \t\t\t\t ConnectionTypeBlocking, this, controls);\n> > > > > \tif (ret)\n> > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > > > index e5940469127e..7002b4323bdd 100644\n> > > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > > @@ -8,6 +8,7 @@\n> > > > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > > >\n> > > > > #include \n> > > > > +#include \n> > > > > #include \n> > > > > #include \n> > > > >\n> > > > > @@ -68,14 +69,36 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> > > > > * through the PipelineHandlerFactoryBase::create() function.\n> > > > > */\n> > > > > PipelineHandler::PipelineHandler(CameraManager *manager)\n> > > > > -\t: manager_(manager), useCount_(0)\n> > > > > +\t: manager_(manager), useCount_(0),\n> > > > > +\t dumpCaptureScript_(nullptr), dumpMetadata_(nullptr)\n> > > > > {\n> > > > > +\t/* TODO Print notification that we're dumping capture script */\n> > \n> > \\todo\n> > \n> > or better, address it.\n> > \n> > > > > +\tconst char *file = utils::secure_getenv(\"LIBCAMERA_DUMP_CAPTURE_SCRIPT\");\n> > > > > +\tif (!file)\n> > > > > +\t\treturn;\n> > > > > +\n> > > > > +\tdumpCaptureScript_ = new std::ofstream(file);\n> > > > \n> > > > I was really expecting to be operating on File instances\n> > > \n> > > Oh ig we could do that too. I copied a few ideas from the logger.\n> > > \n> > > > > +\n> > > > > +\t/*\n> > > > > +\t * Metadata needs to go into a separate file because otherwise it'll\n> > > > > +\t * flood the capture script\n> > > > > +\t */\n> > > > > +\tdumpMetadata_ = new std::ofstream(std::string(file) + \".metadata\");\n> > > > > +\tstd::string str = \"frames:\\n\";\n> > > > > +\tdumpMetadata_->write(str.c_str(), str.size());\n> > > > \n> > > > and use the libyaml emitter for this instead of raw writes\n> > > > \n> > > > Have you considered it and decided it was better to use raw writes ?\n> > > \n> > > No I hadn't. I'll keep it raw for now though until you make the\n> > > YamlEmitter :)\n> > > \n> > > > > +\tdumpMetadata_->flush();\n> > > > > }\n> > > > >\n> > > > > PipelineHandler::~PipelineHandler()\n> > > > > {\n> > > > > \tfor (std::shared_ptr media : mediaDevices_)\n> > > > > \t\tmedia->release();\n> > > > > +\n> > > > > +\tif (dumpCaptureScript_)\n> > > > > +\t\tdelete dumpCaptureScript_;\n> > > > > +\n> > > > > +\tif (dumpMetadata_)\n> > > > > +\t\tdelete dumpMetadata_;\n> > > > > }\n> > > > >\n> > > > > /**\n> > > > > @@ -464,6 +487,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n> > > > >\n> > > > > \trequest->_d()->sequence_ = data->requestSequence_++;\n> > > > >\n> > > > > +\tdumpRequest(request, DumpMode::Controls);\n> > > > > +\n> > > > > \tif (request->_d()->cancelled_) {\n> > > > > \t\tcompleteRequest(request);\n> > > > > \t\treturn;\n> > > > > @@ -555,6 +580,8 @@ void PipelineHandler::completeRequest(Request *request)\n> > > > >\n> > > > > \trequest->_d()->complete();\n> > > > >\n> > > > > +\tdumpRequest(request, DumpMode::Metadata);\n> > > > > +\n> > > > > \tCamera::Private *data = camera->_d();\n> > > > >\n> > > > > \twhile (!data->queuedRequests_.empty()) {\n> > > > > @@ -758,6 +785,70 @@ void PipelineHandler::disconnect()\n> > > > > * \\return The CameraManager for this pipeline handler\n> > > > > */\n> > > > >\n> > > > > +void PipelineHandler::dumpConfiguration(const std::set &streams,\n> > > > > +\t\t\t\t\tconst Orientation &orientation)\n> > > > > +{\n> > > > > +\tif (!dumpCaptureScript_)\n> > > > > +\t\treturn;\n> > > > > +\n> > > > > +\tstd::stringstream ss;\n> > > > > +\tss << \"configuration:\" << std::endl;\n> > > > > +\tss << \" orientation: \" << orientation << std::endl;\n> > > > > +\n> > > > > +\t/* TODO Dump Sensor configuration */\n> > \n> > \\todo\n> > \n> > Same below.\n> > \n> > > > > +\n> > > > > +\tss << \" streams:\" << std::endl;\n> > > > > +\tfor (const auto &stream : streams) {\n> > > > > +\t\tconst StreamConfiguration &streamConfig = stream->configuration();\n> > > > > +\t\tss << \" - pixelFormat: \" << streamConfig.pixelFormat << std::endl;\n> > > > > +\t\tss << \" size: \" << streamConfig.size << std::endl;\n> > > > > +\t\tss << \" stride: \" << streamConfig.stride << std::endl;\n> > > > > +\t\tss << \" frameSize: \" << streamConfig.frameSize << std::endl;\n> > > > > +\t\tss << \" bufferCount: \" << streamConfig.bufferCount << std::endl;\n> > > > > +\t\tif (streamConfig.colorSpace)\n> > > > > +\t\t\tss << \" colorSpace: \" << streamConfig.colorSpace->toString() << std::endl;\n> > > > > +\t}\n> > > > > +\n> > > > > +\tdumpCaptureScript_->write(ss.str().c_str(), ss.str().size());\n> > \n> > I wonder if you could write\n> > \n> > \t*dumpCaptureScript_ << ss.rdbuf();\n> > \n> > But you could use dumpCaptureScript_ directly with the << operator\n> > above, no need for an intermediate stringstream.\n> > \n> \n> ack\n> \n> > > > > +\n> > > > > +\tstd::string str = \"frames:\\n\";\n> > > > > +\tdumpCaptureScript_->write(str.c_str(), str.size());\n> > \n> > \t*dumpCaptureScript_ << \"frames:\\n\";\n> > \n> > > > > +\tdumpCaptureScript_->flush();\n> > > > > +}\n> > > > > +\n> > > > > +void PipelineHandler::dumpRequest(Request *request, DumpMode mode)\n> > > > > +{\n> > > > > +\tControlList &controls =\n> > \n> > const\n> > \n> > > > > +\t\tmode == DumpMode::Controls ? request->controls()\n> > > > > +\t\t\t\t\t : request->metadata();\n> > > > > +\tstd::ostream *output =\n> > > > > +\t\tmode == DumpMode::Controls ? dumpCaptureScript_\n> > > > > +\t\t\t\t\t : dumpMetadata_;\n> > > > > +\n> > > > > +\tif (!output || controls.empty())\n> > > > > +\t\treturn;\n> > > > > +\n> > > > > +\tstd::stringstream ss;\n> > \n> > Use output directly, don't create an intermediate stringstream.\n> > \n> > > > > +\t/* TODO Figure out PFC */\n> > > > > +\tss << \" - \" << request->sequence() << \":\" << std::endl;\n> > > > > +\n> > > > > +\tconst ControlIdMap *idMap = controls.idMap();\n> > > > > +\tfor (const auto &pair : controls) {\n> > > > > +\t\tconst ControlId *ctrlId = idMap->at(pair.first);\n> > > > > +\t\t/* TODO Prettify enums (probably by upgrading ControlValue::toString()) */\n> > \n> > Could you elaborate on this for my information ?\n> \n> This will print enums just as numbers, so you'd get \"AeMeteringMode: 0\"\n> instead of \"AeMeteringMode: MeteringCentreWeighted\".\n\nThe latter would be more readable indeed. At the cost of more CPU cycles\nto output the value, as well as to parse the capture script. We can't\nhave our cake and eat it :(\n\n> > > > > +\t\tss << \" \" << ctrlId->name() << \": \" << pair.second.toString() << std::endl;\n> > > > > +\t}\n> > > > > +\n> > > > > +\t/*\n> > > > > +\t * TODO Investigate the overhead of flushing this frequently\n> > > > > +\t * Controls aren't going to be queued too frequently so it should be\n> > \n> > I'm not too sure about that, I think we'll see some types of\n> > applications (or frameworks) setting controls in every request. We need\n> > to be ready for it.\n> \n> Good point. Although I'm not sure how else to determine how often to\n> flush. Would it be good enough to add another environment variable to\n> set every how many frames to dump? And default to a value > 1? Or count\n> how often it's getting queued and adjust the dump rate dynamically (with\n> an environment variable to override to \"dump every frame\" for debugging\n> purposes)?\n\nLet's not make it overly complicated, you can start with a constant\nflush interval expressed as a number of requests. Store it in a static\nconstexpr, and set it to 1 to start with.\n\n> In either case ig it's time to investigate the actual overhead :/\n\nHaving numbers will certainly help discussions. No need to get the\nnumbers to post v2 though.\n\n> > > > > +\t * fine to dump controls every frame. Metadata on the other hand needs\n> > > > > +\t * to be investigated.\n> > > > > +\t */\n> > > > > +\toutput->write(ss.str().c_str(), ss.str().size());\n> > > > > +\toutput->flush();\n> > > > > +}\n> > > > > +\n> > > > > /**\n> > > > > * \\class PipelineHandlerFactoryBase\n> > > > > * \\brief Base class for pipeline handler factories","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4960DBD80A\n\tfor ;\n\tThu, 3 Oct 2024 13:15:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC94563524;\n\tThu, 3 Oct 2024 15:15:10 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EA90262C92\n\tfor ;\n\tThu, 3 Oct 2024 15:15:08 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4AB50593;\n\tThu, 3 Oct 2024 15:13:35 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Pkc/JOh6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727961215;\n\tbh=fJoyzLL6LpnAwDgG/ivROqHNOkALnRC88GpAJXhODm0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Pkc/JOh66+4n4B9DNroXOfLCnEDCxEJ8dApOn5Y+s20xysI4XJ51ZXFcSbMasfpxp\n\t+jHJIrVygt6QGTWV0c6M+G8syKoWIIJitCbASZI4RtTNe+FN+YgQ2a+ls1jsTLzbIf\n\tQcjLOj0FaxlxL/4KlRATkJuhDsBlt+Q+6w/9is2c=","Date":"Thu, 3 Oct 2024 16:15:05 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 1/2] pipeline: Add support for dumping capture script and\n\tmetadata","Message-ID":"<20241003131505.GA5484@pendragon.ideasonboard.com>","References":"<20240930170907.1404844-1-paul.elder@ideasonboard.com>\n\t<20240930170907.1404844-2-paul.elder@ideasonboard.com>\n\t\n\t\n\t<20241002234127.GB17537@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]