[RFC v2 4/4] libcamera: pipeline_handler: Use YamlEmitter
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Mon Oct 21 11:33:51 CEST 2024
Hi Kieran
On Sat, Oct 19, 2024 at 11:35:05PM +0100, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2024-10-17 13:52:19)
> > Replace raw output usage with YamlEmitter.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > ---
> > include/libcamera/internal/pipeline_handler.h | 11 +-
> > src/libcamera/pipeline_handler.cpp | 102 +++++++++---------
> > 2 files changed, 61 insertions(+), 52 deletions(-)
> >
> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > index fb3914185a01..a10d7b1fab8c 100644
> > --- a/include/libcamera/internal/pipeline_handler.h
> > +++ b/include/libcamera/internal/pipeline_handler.h
> > @@ -19,6 +19,8 @@
> > #include <libcamera/controls.h>
> > #include <libcamera/stream.h>
> >
> > +#include "libcamera/internal/yaml_emitter.h"
> > +
> > namespace libcamera {
> >
> > enum class Orientation;
> > @@ -110,8 +112,13 @@ private:
> > const char *name_;
> > unsigned int useCount_;
> >
> > - std::ostream *dumpCaptureScript_;
> > - std::ostream *dumpMetadata_;
> > + std::unique_ptr<YamlRoot> controlsEmitter_;
> > + std::unique_ptr<YamlDict> controlsDict_;
> > + std::unique_ptr<YamlList> controlsList_;
> > +
> > + std::unique_ptr<YamlRoot> metadataEmitter_;
> > + std::unique_ptr<YamlDict> metadataDict_;
> > + std::unique_ptr<YamlList> metadataList_;
>
> Will 'all' users of a YamlEmitter instance need each of a root/dict/list
> - or is this just specific to this usage?
It depends what you have to do.
If a root/dict/list has to be kept open for the whole duration of the
library, like in this case as it needs to dump frames/metadata for the
whole library lifetime, then yes, you have to keep them in scope.
I suspect that a root node has anyway to be kept around in most cases,
as deleting it terminates the yaml stream and closes the output file.
Deleting an object (that has been initialized by something valid) emit
the yaml end sequences associated to the obejct type, so if you delete
an instance of a dict/list earlier you wont be able to add stuff to
it. If you delete a Root earlier the yaml stream is closed as well as
the output file.
>
> I don't think this series has taken on comments from Pauls' series,
> where I saw a lot of duplication between different 'output streams'
> which should be factored out.
>
Sorry, not what this series is about. I think once we reach a decent
status on YamlEmitter the frame/metadata dumping should be
reimplemented on top. At least this was my understanding, but I would
like to know what's Paul preference as well..
> I can't tell if they should be factored out to a 'YamlEmitter' class
> each - or if ther eshould be some extra use-case specific class specific
> to the metadata here ...
According to Laurent's comment of not adding anything specific to
libcamera's types to the YamlEmitter interface for the moment being, I
don't think we will subclass it for specific use cases (to be honest I
can't think of what would be made metadata-specific in a derived class)
>
>
>
> >
> > friend class PipelineHandlerFactoryBase;
> > };
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 7002b4323bdd..533d0f8fc132 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -69,36 +69,31 @@ LOG_DEFINE_CATEGORY(Pipeline)
> > * through the PipelineHandlerFactoryBase::create() function.
> > */
> > PipelineHandler::PipelineHandler(CameraManager *manager)
> > - : manager_(manager), useCount_(0),
> > - dumpCaptureScript_(nullptr), dumpMetadata_(nullptr)
> > + : manager_(manager), useCount_(0)
> > {
> > /* TODO Print notification that we're dumping capture script */
> > const char *file = utils::secure_getenv("LIBCAMERA_DUMP_CAPTURE_SCRIPT");
> > if (!file)
> > return;
> >
> > - dumpCaptureScript_ = new std::ofstream(file);
> > + std::string filePath(file);
> > + controlsEmitter_ = YamlEmitter::root(filePath);
> > + controlsDict_ = controlsEmitter_->dict();
> >
> > /*
> > * Metadata needs to go into a separate file because otherwise it'll
> > * flood the capture script
> > */
> > - dumpMetadata_ = new std::ofstream(std::string(file) + ".metadata");
> > - std::string str = "frames:\n";
> > - dumpMetadata_->write(str.c_str(), str.size());
> > - dumpMetadata_->flush();
> > + std::string metadataFilePath = filePath + ".metadata";
> > + metadataEmitter_ = YamlEmitter::root(metadataFilePath);
> > + metadataDict_ = metadataEmitter_->dict();
> > + metadataList_ = metadataDict_->list("frames");
> > }
> >
> > PipelineHandler::~PipelineHandler()
> > {
> > for (std::shared_ptr<MediaDevice> media : mediaDevices_)
> > media->release();
> > -
> > - if (dumpCaptureScript_)
> > - delete dumpCaptureScript_;
> > -
> > - if (dumpMetadata_)
> > - delete dumpMetadata_;
> > }
> >
> > /**
> > @@ -788,65 +783,72 @@ void PipelineHandler::disconnect()
> > void PipelineHandler::dumpConfiguration(const std::set<const Stream *> &streams,
> > const Orientation &orientation)
> > {
> > - if (!dumpCaptureScript_)
> > + if (!controlsEmitter_)
> > return;
> >
> > - std::stringstream ss;
> > - ss << "configuration:" << std::endl;
> > - ss << " orientation: " << orientation << std::endl;
> > + auto configurationDict = controlsDict_->dict("configuration");
> > +
> > + std::stringstream o;
> > + o << orientation;
> > + (*configurationDict)["orientation"] = o.str();
> >
> > /* TODO Dump Sensor configuration */
> >
> > - ss << " streams:" << std::endl;
> > + auto streamsList = configurationDict->list("streams");
> > +
> > for (const auto &stream : streams) {
> > const StreamConfiguration &streamConfig = stream->configuration();
> > - ss << " - pixelFormat: " << streamConfig.pixelFormat << std::endl;
> > - ss << " size: " << streamConfig.size << std::endl;
> > - ss << " stride: " << streamConfig.stride << std::endl;
> > - ss << " frameSize: " << streamConfig.frameSize << std::endl;
> > - ss << " bufferCount: " << streamConfig.bufferCount << std::endl;
> > - if (streamConfig.colorSpace)
> > - ss << " colorSpace: " << streamConfig.colorSpace->toString() << std::endl;
> > - }
> > + auto yamlStream = streamsList->dict();
> >
> > - dumpCaptureScript_->write(ss.str().c_str(), ss.str().size());
> > + (*yamlStream)["pixelformat"] = streamConfig.pixelFormat.toString();
> > + (*yamlStream)["size"] = streamConfig.size.toString();
> > + (*yamlStream)["stride"] = std::to_string(streamConfig.stride);
> > + (*yamlStream)["frameSize"] = std::to_string(streamConfig.frameSize);
> > + (*yamlStream)["bufferCount"] = std::to_string(streamConfig.bufferCount);
> >
> > - std::string str = "frames:\n";
> > - dumpCaptureScript_->write(str.c_str(), str.size());
> > - dumpCaptureScript_->flush();
> > + if (streamConfig.colorSpace)
> > + (*yamlStream)["colorSpace"] =
> > + streamConfig.colorSpace->toString();
> > + }
> > }
> >
> > void PipelineHandler::dumpRequest(Request *request, DumpMode mode)
> > {
> > - ControlList &controls =
> > - mode == DumpMode::Controls ? request->controls()
> > - : request->metadata();
> > - std::ostream *output =
> > - mode == DumpMode::Controls ? dumpCaptureScript_
> > - : dumpMetadata_;
> > -
> > - if (!output || controls.empty())
> > + if (!controlsEmitter_)
> > return;
> >
> > - std::stringstream ss;
> > + ControlList &controls = mode == DumpMode::Controls ? request->controls()
> > + : request->metadata();
> > + if (controls.empty())
> > + return;
> > +
> > + std::unique_ptr<YamlDict> yamlFrame;
> > + if (mode == DumpMode::Controls) {
> > + if (!controlsEmitter_)
> > + return;
> > +
> > + if (!controlsList_)
> > + controlsList_ = controlsDict_->list("frames");
> > +
> > + yamlFrame = controlsList_->dict();
> > + } else {
> > + if (!metadataEmitter_)
> > + return;
> > +
> > + yamlFrame = metadataList_->dict();
> > + }
> > +
> > + auto yamlCtrls = yamlFrame->dict(std::to_string(request->sequence()));
> > +
> > /* TODO Figure out PFC */
> > - ss << " - " << request->sequence() << ":" << std::endl;
> >
> > const ControlIdMap *idMap = controls.idMap();
> > for (const auto &pair : controls) {
> > const ControlId *ctrlId = idMap->at(pair.first);
> > +
> > /* TODO Prettify enums (probably by upgrading ControlValue::toString()) */
> > - ss << " " << ctrlId->name() << ": " << pair.second.toString() << std::endl;
> > + (*yamlCtrls)[ctrlId->name()] = pair.second.toString();
> > }
> > -
> > - /*
> > - * TODO Investigate the overhead of flushing this frequently
> > - * Controls aren't going to be queued too frequently so it should be
> > - * fine to dump controls every frame. Metadata on the other hand needs
> > - * to be investigated.
> > - */
> > - output->write(ss.str().c_str(), ss.str().size());
> > - output->flush();
> > }
> >
> > /**
> > --
> > 2.47.0
> >
More information about the libcamera-devel
mailing list