[RFC v2 4/4] libcamera: pipeline_handler: Use YamlEmitter

[Kieran Bingham]

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?

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.

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



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