[libcamera-devel] [PATCH v3 09/11] libcamera: pipeline: Add a simple pipeline handler
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Apr 4 01:55:36 CEST 2020
Hi Kieran,
On Tue, Mar 31, 2020 at 12:45:14PM +0100, Kieran Bingham wrote:
> On 20/03/2020 01:48, Laurent Pinchart wrote:
> > From: Martijn Braam <martijn at brixit.nl>
> >
> > This new pipeline handler aims at supporting any simple device without
> > requiring any device-specific code. Simple devices are currently defined
> > as a graph made of one or multiple camera sensors and a single video
> > node, with each sensor connected to the video node through a linear
> > pipeline.
> >
> > The simple pipeline handler will automatically parse the media graph,
> > enumerate sensors, build supported stream configurations, and configure
> > the pipeline, without any device-specific knowledge. It doesn't support
> > configuration of any processing in the pipeline at the moment, but may
> > be extended to support simple processing such as format conversion or
> > scaling in the future.
> >
> > The only device-specific information in the pipeline handler is the list
> > of supported drivers, required for device matching. We may be able to
> > remove this in the future by matching with the simple pipeline handler
> > as a last resort option, after all other pipeline handlers have been
> > tried.
> >
> > Signed-off-by: Martijn Braam <martijn at brixit.nl>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> Some comments and discussion points below, but I think we should work
> towards trying to get this integrated so it can actually be used (which
> is where we'll find the nuances and corner cases as people test it)
:-)
Please see below for a few answers.
> > ---
> > Changes since v2:
> >
> > - Log an error when setupFormats() fail
> > - Propagate getFormat() and setFormat() errors to the caller of
> > setupFormats()
> > - Reorder variable declarations in validate()
> > - Add \todo comment related to the selection of the default format
> > - Use log Error instead of Info if pipeline isn't valid
> > - Rebase on top of V4L2PixelFormat
> >
> > Changes since v1:
> >
> > - Rebase on top of buffer API rework
> > - Expose stream formats
> > - Rework camera data config
> > ---
> > src/libcamera/pipeline/meson.build | 1 +
> > src/libcamera/pipeline/simple/meson.build | 3 +
> > src/libcamera/pipeline/simple/simple.cpp | 710 ++++++++++++++++++++++
> > 3 files changed, 714 insertions(+)
> > create mode 100644 src/libcamera/pipeline/simple/meson.build
> > create mode 100644 src/libcamera/pipeline/simple/simple.cpp
> >
> > diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build
> > index 0d466225a72e..606ba31a0319 100644
> > --- a/src/libcamera/pipeline/meson.build
> > +++ b/src/libcamera/pipeline/meson.build
> > @@ -5,3 +5,4 @@ libcamera_sources += files([
> >
> > subdir('ipu3')
> > subdir('rkisp1')
> > +subdir('simple')
> > diff --git a/src/libcamera/pipeline/simple/meson.build b/src/libcamera/pipeline/simple/meson.build
> > new file mode 100644
> > index 000000000000..4945a3e173cf
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/simple/meson.build
> > @@ -0,0 +1,3 @@
> > +libcamera_sources += files([
> > + 'simple.cpp',
> > +])
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > new file mode 100644
> > index 000000000000..545a99fe31ca
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -0,0 +1,710 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Laurent Pinchart
> > + * Copyright (C) 2019, Martijn Braam
> > + *
> > + * simple.cpp - Pipeline handler for simple pipelines
> > + */
> > +
> > +#include <algorithm>
> > +#include <iterator>
> > +#include <list>
> > +#include <map>
> > +#include <memory>
> > +#include <set>
> > +#include <string>
> > +#include <string.h>
> > +#include <utility>
> > +#include <vector>
> > +
> > +#include <linux/media-bus-format.h>
> > +
> > +#include <libcamera/camera.h>
> > +#include <libcamera/request.h>
> > +#include <libcamera/stream.h>
> > +
> > +#include "camera_sensor.h"
> > +#include "device_enumerator.h"
> > +#include "log.h"
> > +#include "media_device.h"
> > +#include "pipeline_handler.h"
> > +#include "v4l2_subdevice.h"
> > +#include "v4l2_videodevice.h"
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(SimplePipeline)
> > +
> > +class SimplePipelineHandler;
> > +
> > +class SimpleCameraData : public CameraData
> > +{
> > +public:
> > + SimpleCameraData(PipelineHandler *pipe, MediaEntity *sensor,
> > + MediaEntity *video);
> > +
> > + bool isValid() const { return sensor_ != nullptr; }
> > + std::set<Stream *> streams() { return { &stream_ }; }
> > +
> > + int init();
> > + int setupLinks();
> > + int setupFormats(V4L2SubdeviceFormat *format,
> > + V4L2Subdevice::Whence whence);
> > +
> > + struct Entity {
> > + MediaEntity *entity;
> > + MediaLink *link;
> > + };
> > +
> > + struct Configuration {
> > + uint32_t code;
> > + PixelFormat pixelFormat;
> > + Size size;
> > + };
> > +
> > + Stream stream_;
> > + std::unique_ptr<CameraSensor> sensor_;
> > + std::list<Entity> entities_;
> > +
> > + std::vector<Configuration> configs_;
> > + std::map<PixelFormat, Configuration> formats_;
> > +};
> > +
> > +class SimpleCameraConfiguration : public CameraConfiguration
> > +{
> > +public:
> > + SimpleCameraConfiguration(Camera *camera, SimpleCameraData *data);
> > +
> > + Status validate() override;
> > +
> > + const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
> > +
> > +private:
> > + /*
> > + * The SimpleCameraData instance is guaranteed to be valid as long as
> > + * the corresponding Camera instance is valid. In order to borrow a
> > + * reference to the camera data, store a new reference to the camera.
> > + */
> > + std::shared_ptr<Camera> camera_;
> > + const SimpleCameraData *data_;
> > +
> > + V4L2SubdeviceFormat sensorFormat_;
> > +};
> > +
> > +class SimplePipelineHandler : public PipelineHandler
> > +{
> > +public:
> > + SimplePipelineHandler(CameraManager *manager);
> > + ~SimplePipelineHandler();
> > +
> > + CameraConfiguration *generateConfiguration(Camera *camera,
> > + const StreamRoles &roles) override;
> > + int configure(Camera *camera, CameraConfiguration *config) override;
> > +
> > + int exportFrameBuffers(Camera *camera, Stream *stream,
> > + std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> > +
> > + int start(Camera *camera) override;
> > + void stop(Camera *camera) override;
> > +
> > + bool match(DeviceEnumerator *enumerator) override;
> > +
> > + V4L2VideoDevice *video() { return video_; }
> > + V4L2Subdevice *subdev(const MediaEntity *entity);
> > +
> > +protected:
> > + int queueRequestDevice(Camera *camera, Request *request) override;
> > +
> > +private:
> > + SimpleCameraData *cameraData(const Camera *camera)
> > + {
> > + return static_cast<SimpleCameraData *>(
> > + PipelineHandler::cameraData(camera));
> > + }
> > +
> > + int initLinks();
> > +
> > + int createCamera(MediaEntity *sensor);
> > +
> > + void bufferReady(FrameBuffer *buffer);
> > +
> > + MediaDevice *media_;
> > + V4L2VideoDevice *video_;
> > + std::map<const MediaEntity *, V4L2Subdevice> subdevs_;
> > +
> > + Camera *activeCamera_;
> > +};
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Camera Data
> > + */
> > +SimpleCameraData::SimpleCameraData(PipelineHandler *pipe, MediaEntity *sensor,
> > + MediaEntity *video)
> > + : CameraData(pipe)
> > +{
> > + int ret;
> > +
> > + /*
> > + * Walk the pipeline towards the video node and store all entities
> > + * along the way.
> > + */
> > + MediaEntity *source = sensor;
> > +
> > + while (source) {
> > + /* If we have reached the video node, we're done. */
> > + if (source == video)
> > + break;
> > +
> > + /* Use the first output pad that has links. */
> > + MediaPad *sourcePad = nullptr;
> > + for (MediaPad *pad : source->pads()) {
> > + if ((pad->flags() & MEDIA_PAD_FL_SOURCE) &&
> > + !pad->links().empty()) {
> > + sourcePad = pad;
> > + break;
> > + }
> > + }
> > +
> > + if (!sourcePad)
> > + return;
> > +
> > + /* Use the first link that isn't immutable and disabled. */
> > + MediaLink *sourceLink = nullptr;
> > + for (MediaLink *link : sourcePad->links()) {
> > + if ((link->flags() & MEDIA_LNK_FL_ENABLED) ||
> > + !(link->flags() & MEDIA_LNK_FL_IMMUTABLE)) {
> > + sourceLink = link;
> > + break;
> > + }
> > + }
> > +
> > + if (!sourceLink)
> > + return;
> > +
> > + entities_.push_back({ source, sourceLink });
> > +
> > + source = sourceLink->sink()->entity();
> > +
> > + /* Avoid infinite loops. */
> > + auto iter = std::find_if(entities_.begin(), entities_.end(),
> > + [&](const Entity &entity) {
> > + return entity.entity == source;
> > + });
> > + if (iter != entities_.end()) {
> > + LOG(SimplePipeline, Info) << "Loop detected in pipeline";
> > + return;
> > + }
> > + }
> > +
> > + /* We have a valid pipeline, create the camera sensor. */
> > + sensor_ = std::make_unique<CameraSensor>(sensor);
> > + ret = sensor_->init();
> > + if (ret) {
> > + sensor_.reset();
> > + return;
> > + }
> > +}
> > +
> > +int SimpleCameraData::init()
> > +{
> > + SimplePipelineHandler *pipe = static_cast<SimplePipelineHandler *>(pipe_);
> > + V4L2VideoDevice *video = pipe->video();
> > + int ret;
> > +
> > + /*
> > + * Enumerate the possible pipeline configurations. For each media bus
> > + * format supported by the sensor, propagate the formats through the
> > + * pipeline, and enumerate the corresponding possible V4L2 pixel
> > + * formats on the video node.
> > + */
> > + for (unsigned int code : sensor_->mbusCodes()) {
> > + V4L2SubdeviceFormat format{ code, sensor_->resolution() };
> > +
> > + /*
> > + * Setup links first as some subdev drivers take active links
> > + * into account to propaget TRY formats. So is life :-(
>
> /propaget/propagate/
>
> /So is life/Such is life/
>
> > + */
> > + ret = setupLinks();
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = setupFormats(&format, V4L2Subdevice::TryFormat);
> > + if (ret < 0) {
> > + LOG(SimplePipeline, Error)
> > + << "Failed to setup pipeline for media bus code "
> > + << utils::hex(code, 4);
>
> Oh dear, seems we need mbusCode.toString() too! hehe
>
> > + return ret;
>
> Andrey states that this return prevents iterating potentially successful
> mbus-codes, and should instead 'continue'...
I'll reply to that in a separate e-mail/
> > + }
> > +
> > + std::map<V4L2PixelFormat, std::vector<SizeRange>> videoFormats =
> > + video->formats(format.mbus_code);
> > +
> > + LOG(SimplePipeline, Debug)
> > + << "Adding configuration for " << format.size.toString()
> > + << " in pixel formats [ "
> > + << utils::join(videoFormats, ", ",
> > + [](const auto &f) {
> > + return f.first.toString();
> > + })
>
> Aha, I kinda like the utils::join!
>
> > + << " ]";
> > +
> > + /*
> > + * Store the configuration in the formats_ map, mapping
> > + * PixelFormat to configuration. Any previously stored value is
>
> /to configuration/to the current configuration/ ?
I'll write "mapping the PixelFormat to the corresponding configuration".
> > + * overwritten, as the pipeline handler currently doesn't care
> > + * about how a particular PixelFormat is achieved.
>
> As long as there's an output, I agree. If devices have specific
> constraints such as better performance or such with a particular
> mbus-code, then they need to create a platform specific pipeline handler
> to deal with the nuances.
>
> Maybe there might be tuning hints or something later, but certainly not now.
Exactly my thoughts :-)
> > + */
> > + for (const auto &videoFormat : videoFormats) {
> > + PixelFormat pixelFormat = video->toPixelFormat(videoFormat.first);
> > + if (!pixelFormat)
> > + continue;
> > +
> > + Configuration config;
> > + config.code = code;
> > + config.pixelFormat = pixelFormat;
> > + config.size = format.size;
> > +
> > + formats_[pixelFormat] = config;
> > + }
> > + }
> > +
> > + if (formats_.empty()) {
> > + LOG(SimplePipeline, Error) << "No valid configuration found";
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int SimpleCameraData::setupLinks()
> > +{
> > + int ret;
> > +
> > + /*
> > + * Configure all links along the pipeline. Some entities may not allow
> > + * multiple sink links to be enabled together, even on different sink
> > + * pads. We must thus start by disabling all sink links (but the one we
> > + * want to enable) before enabling the pipeline link.
>
> Eeek - this sounds like it could have potential system effects,
> (disabling existing configurations) but I think this is the
> 'Such-is-life' comment I read earlier.
Possibly, but the pipeline handler owns the whole media device, so it
shouldn't be a real issue.
> > + */
> > + for (SimpleCameraData::Entity &e : entities_) {
> > + for (MediaPad *pad : e.link->sink()->entity()->pads()) {
>
> For readability, I would be tempted to pull e.link->sink()->entity() to
> a local var called Entity &remote, or Entity &endpoint, or such,
> essentially to make this line read as:
>
> for (MediaPad *pad : remote->pads()) {
Done with MediaEntity *remote = ...;
> > + for (MediaLink *link : pad->links()) {
> > + if (link == e.link)
> > + continue;
> > +
> > + if ((link->flags() & MEDIA_LNK_FL_ENABLED) &&
> > + !(link->flags() & MEDIA_LNK_FL_IMMUTABLE)) {
> > + ret = link->setEnabled(false);
> > + if (ret < 0)
> > + return ret;
> > + }
> > + }
> > + }
> > +
>
> I'm not going to try it out, but would this be easier / clearer if we
> simply disabled *all* links, then explicitly enable links on our pipeline?
>
> (only a devils-advocate discussion point, not something that's needed).
I considered it after writing this code, and I thought the more complex
approach could be useful in the future, so I wasn't enclined to throw it
away :-)
> > + if (!(e.link->flags() & MEDIA_LNK_FL_ENABLED)) {
> > + ret = e.link->setEnabled(true);
> > + if (ret < 0)
> > + return ret;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int SimpleCameraData::setupFormats(V4L2SubdeviceFormat *format,
> > + V4L2Subdevice::Whence whence)
> > +{
> > + SimplePipelineHandler *pipe = static_cast<SimplePipelineHandler *>(pipe_);
> > + int ret;
> > +
> > + /*
> > + * Configure the format on the sensor output and propagate it through
> > + * the pipeline.
> > + */
> > + ret = sensor_->setFormat(format);
> > + if (ret < 0)
> > + return ret;
> > +
> > + for (const Entity &e : entities_) {
> > + MediaLink *link = e.link;
> > + MediaPad *source = link->source();
> > + MediaPad *sink = link->sink();
> > +
> > + if (source->entity() != sensor_->entity()) {
> > + V4L2Subdevice *subdev = pipe->subdev(source->entity());
> > + ret = subdev->getFormat(source->index(), format, whence);
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > + if (sink->entity()->function() != MEDIA_ENT_F_IO_V4L) {
> > + V4L2Subdevice *subdev = pipe->subdev(sink->entity());
> > + ret = subdev->setFormat(sink->index(), format, whence);
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > + LOG(SimplePipeline, Debug)
> > + << "Link '" << source->entity()->name()
> > + << "':" << source->index()
> > + << " -> '" << sink->entity()->name()
> > + << "':" << sink->index()
> > + << " configured with format " << format->toString();
>
> I wonder if we can get the whole pipeline configuration string to a
> single line... but not required now.
>
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Camera Configuration
> > + */
> > +
> > +SimpleCameraConfiguration::SimpleCameraConfiguration(Camera *camera,
> > + SimpleCameraData *data)
> > + : CameraConfiguration(), camera_(camera->shared_from_this()),
> > + data_(data)
> > +{
> > +}
> > +
> > +CameraConfiguration::Status SimpleCameraConfiguration::validate()
> > +{
> > + Status status = Valid;
> > +
> > + if (config_.empty())
> > + return Invalid;
> > +
> > + /* Cap the number of entries to the available streams. */
> > + if (config_.size() > 1) {
> > + config_.resize(1);
> > + status = Adjusted;
> > + }
> > +
> > + StreamConfiguration &cfg = config_[0];
> > +
> > + /* Adjust the pixel format. */
> > + auto it = data_->formats_.find(cfg.pixelFormat);
> > + if (it == data_->formats_.end())
> > + it = data_->formats_.begin();
> > +
> > + PixelFormat pixelFormat = it->first;
> > + if (cfg.pixelFormat != pixelFormat) {
> > + LOG(SimplePipeline, Debug) << "Adjusting pixel format";
> > + cfg.pixelFormat = pixelFormat;
> > + status = Adjusted;
> > + }
> > +
> > + const SimpleCameraData::Configuration &pipeConfig = it->second;
> > + if (cfg.size != pipeConfig.size) {
> > + LOG(SimplePipeline, Debug)
> > + << "Adjusting size from " << cfg.size.toString()
> > + << " to " << pipeConfig.size.toString();
> > + cfg.size = pipeConfig.size;
> > + status = Adjusted;
> > + }
> > +
> > + cfg.bufferCount = 3;
> > +
> > + return status;
> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Pipeline Handler
> > + */
> > +
> > +SimplePipelineHandler::SimplePipelineHandler(CameraManager *manager)
> > + : PipelineHandler(manager), video_(nullptr)
> > +{
> > +}
> > +
> > +SimplePipelineHandler::~SimplePipelineHandler()
> > +{
> > + delete video_;
> > +}
> > +
> > +CameraConfiguration *SimplePipelineHandler::generateConfiguration(Camera *camera,
> > + const StreamRoles &roles)
> > +{
> > + SimpleCameraData *data = cameraData(camera);
> > + CameraConfiguration *config =
> > + new SimpleCameraConfiguration(camera, data);
> > +
> > + if (roles.empty())
> > + return config;
> > +
> > + /* Create the formats map. */
> > + std::map<PixelFormat, std::vector<SizeRange>> formats;
> > + std::transform(data->formats_.begin(), data->formats_.end(),
> > + std::inserter(formats, formats.end()),
> > + [](const auto &format) -> decltype(formats)::value_type {
> > + const PixelFormat &pixelFormat = format.first;
> > + const Size &size = format.second.size;
> > + return { pixelFormat, { size } };
> > + });
>
> This is quite unfriendly to the reader :-( ... not that I have a better
> alternative ... but eugh
>
> > +
> > + /*
> > + * Create the stream configuration. Take the first entry in the formats
> > + * map as the default, for lack of a better option.
> > + *
> > + * \todo Implement a better way to pick the default format
>
> Is there one? It's a generic configuration for a pipeline handler.
>
> What might be better as a general case, and presumably any logic added
> here would potentially be applicable to most pipeline handlers too.
>
> The biggest Size? The smallest Size? or the nearest size to a list of
> predefined sizes for a given role?
Maybe something like that, or maybe asking kernel drivers to sort
formats according to some priority rules, or something else. I'm not
sure what it should be, but I wanted to record the fact that we should
look at it.
> > + */
> > + StreamConfiguration cfg{ StreamFormats{ formats } };
> > + cfg.pixelFormat = formats.begin()->first;
> > + cfg.size = formats.begin()->second[0].max;
> > +
> > + config->addConfiguration(cfg);
> > +
> > + config->validate();
> > +
> > + return config;
> > +}
> > +
> > +int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
> > +{
> > + SimpleCameraConfiguration *config =
> > + static_cast<SimpleCameraConfiguration *>(c);
> > + SimpleCameraData *data = cameraData(camera);
> > + StreamConfiguration &cfg = config->at(0);
> > + int ret;
> > +
> > + /*
> > + * Configure links on the pipeline and propagate formats from the
> > + * sensor to the video node.
> > + */
> > + ret = data->setupLinks();
> > + if (ret < 0)
> > + return ret;
> > +
> > + const SimpleCameraData::Configuration &pipeConfig =
> > + data->formats_[cfg.pixelFormat];
> > +
> > + V4L2SubdeviceFormat format{ pipeConfig.code, data->sensor_->resolution() };
> > +
> > + ret = data->setupFormats(&format, V4L2Subdevice::ActiveFormat);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Configure the video node. */
> > + V4L2PixelFormat videoFormat = video_->toV4L2PixelFormat(cfg.pixelFormat);
> > +
> > + V4L2DeviceFormat outputFormat = {};
> > + outputFormat.fourcc = videoFormat;
> > + outputFormat.size = cfg.size;
> > +
> > + ret = video_->setFormat(&outputFormat);
> > + if (ret)
> > + return ret;
> > +
> > + if (outputFormat.size != cfg.size || outputFormat.fourcc != videoFormat) {
> > + LOG(SimplePipeline, Error)
> > + << "Unable to configure capture in " << cfg.toString();
> > + return -EINVAL;
> > + }
> > +
> > + cfg.setStream(&data->stream_);
> > +
> > + return 0;
> > +}
> > +
> > +int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,
> > + std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > +{
> > + unsigned int count = stream->configuration().bufferCount;
> > +
> > + return video_->exportBuffers(count, buffers);
> > +}
> > +
> > +int SimplePipelineHandler::start(Camera *camera)
> > +{
> > + SimpleCameraData *data = cameraData(camera);
> > + unsigned int count = data->stream_.configuration().bufferCount;
> > +
> > + int ret = video_->importBuffers(count);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = video_->streamOn();
> > + if (ret < 0) {
> > + video_->releaseBuffers();
>
> I wonder if video_->importBuffers() should be something that now happens
> at video_->streamOn()?
>
> I presume that it is now more of an operation to set up V4L2 with the
> internal buffer structures for passing the information on 'which' buffer
> is in use to V4L2...
>
> Every video stream will have to do this?
>
> Otherwise, if conceptually we don't want to mis-manage buffer handling
> in V4L2VideoDevice::streamOn() we might want to have a V4L2Stream object
> to do so ...
>
> (clearly those are just V4L2VideoDevice API discussion points, and not
> actions on this patch)
Good questions, I hadn't thought about that, and it could indeed make
sense. The operations are separate in V4L2 so one can argue that
V4L2VideoDevice should just be a thin wrapper and separate them, but if
they're always used the same way, it can make sense to refactor that.
Let's keep it in mind. Niklas had proposed a V4L2Stream a while ago I
think, which I didn't like, but that was because of how it tried to
expose things automatically to the Camera object. If it's just an
internal helper, it's an idea we could explore.
> > + return ret;
> > + }
> > +
> > + activeCamera_ = camera;
>
> Will simple pipeline handler support more than one camera?
I'm not sure yet, but I wouldn't rule it out completely. I also think it
could make sense, at some point, to refactor this, and extract code to
an object to model a sensor-to-videodev pipeline. That could be used by
more complex pipeline handlers that have an inline ISP with little
processing coupled with an offline ISP.
> The activeCamera_ feels like a way of supporting more than one camera on
> a pipeline, which could otherwise have been a bool cameraActive to
> represent it's state. In fact I'm not sure it's even needed to do
> that... The only actual check is an assert to ensure that it's set
> before using it in the bufferReady() callbacks, which could likely have
> accessed the camera directly?
>
> But if it's worth keeping this as it is to keep it close to other
> pipeline handlers then so be it.
>
>
> <Edit> Looks like it is used actaully, as the Camera is only created and
> registered with the CameraManager, so we don't really have direct access
> to the Camera within the PipelineHandler, and we have to be given it..
> (even though /we/ created it :-D)
Yes, and I'm thinking it could make sense to store it automatically in
the base CameraData class. That's something we should look at when we'll
refactor pipeline handlers.
> > +
> > + return 0;
> > +}
> > +
> > +void SimplePipelineHandler::stop(Camera *camera)
> > +{
> > + video_->streamOff();
> > + video_->releaseBuffers();
> > + activeCamera_ = nullptr;
> > +}
> > +
> > +int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
> > +{
> > + SimpleCameraData *data = cameraData(camera);
> > + Stream *stream = &data->stream_;
> > +
> > + FrameBuffer *buffer = request->findBuffer(stream);
> > + if (!buffer) {
> > + LOG(SimplePipeline, Error)
> > + << "Attempt to queue request with invalid stream";
> > + return -ENOENT;
> > + }
> > +
> > + return video_->queueBuffer(buffer);
> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Match and Setup
> > + */
> > +
> > +bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> > +{
> > + static const char * const drivers[] = {
> > + "imx7-csi",
> > + "sun6i-csi",
> > + };
> > +
> > + for (const char *driver : drivers) {
> > + DeviceMatch dm(driver);
> > + media_ = acquireMediaDevice(enumerator, dm);
> > + if (media_)
> > + break;
> > + }
> > +
> > + if (!media_)
> > + return false;
> > +
> > + /*
> > + * Locate sensors and video nodes. We only support pipelines with at
> > + * least one sensor and exactly one video captude node.
>
> /captude/capture/
>
> > + */
> > + std::vector<MediaEntity *> sensors;
> > + std::vector<MediaEntity *> videos;
> > +
> > + for (MediaEntity *entity : media_->entities()) {
> > + switch (entity->function()) {
> > + case MEDIA_ENT_F_CAM_SENSOR:
> > + sensors.push_back(entity);
> > + break;
> > +
> > + case MEDIA_ENT_F_IO_V4L:
> > + if (entity->pads().size() == 1 &&
> > + (entity->pads()[0]->flags() & MEDIA_PAD_FL_SINK))
> > + videos.push_back(entity);
> > + break;
> > +
> > + default:
> > + break;
> > + }
> > + }
> > +
> > + if (sensors.empty()) {
> > + LOG(SimplePipeline, Error) << "No sensor found";
> > + return false;
> > + }
> > +
> > + if (videos.size() != 1) {
> > + LOG(SimplePipeline, Error)
> > + << "Pipeline with " << videos.size()
> > + << " video capture nodes is not supported";
> > + return false;
> > + }
> > +
> > + /* Locate and open the capture video node. */
> > + video_ = new V4L2VideoDevice(videos[0]);
> > + if (video_->open() < 0)
> > + return false;
> > +
> > + if (video_->caps().isMultiplanar()) {
> > + LOG(SimplePipeline, Error)
> > + << "V4L2 multiplanar devices are not supported";
> > + return false;
> > + }
> > +
> > + video_->bufferReady.connect(this, &SimplePipelineHandler::bufferReady);
> > +
> > + /*
> > + * Create one camera data instance for each sensor and gather all
> > + * entities in all pipelines.
> > + */
> > + std::vector<std::unique_ptr<SimpleCameraData>> pipelines;
> > + std::set<MediaEntity *> entities;
> > +
> > + pipelines.reserve(sensors.size());
> > +
> > + for (MediaEntity *sensor : sensors) {
> > + std::unique_ptr<SimpleCameraData> data =
> > + std::make_unique<SimpleCameraData>(this, sensor,
> > + videos[0]);
> > + if (!data->isValid()) {
> > + LOG(SimplePipeline, Error)
> > + << "No valid pipeline for sensor '"
> > + << sensor->name() << "', skipping";
> > + continue;
> > + }
> > +
> > + for (SimpleCameraData::Entity &entity : data->entities_)
> > + entities.insert(entity.entity);
> > +
> > + pipelines.push_back(std::move(data));
> > + }
> > +
> > + if (entities.empty())
> > + return false;
> > +
> > + /* Create and open V4L2Subdev instances for all the entities. */
> > + for (MediaEntity *entity : entities) {
>
> /* Create a new map entry with the entity, and a
> * constructed V4L2Subdevice() using the entity for the
> * default constructor. */
>
> or some such? (Otherwise, emplacing the entity twice looks really odd
> and obfuscated)
It's not the default constructor, it's the constructor taking a
MediaEntity. I'll see if I can find a good way to express this without
just documenting the C++ idioms :-)
> > + auto elem = subdevs_.emplace(std::piecewise_construct,
> > + std::forward_as_tuple(entity),
> > + std::forward_as_tuple(entity));
>
>
> Is a piecewise_construct required here?
>
> auto elem = subdevs_.emplace(entity, entity);
>
> should do the trick, (and at least compiles).
It's not required, but it's more efficient in this case (see commit
38dd90307ab2).
> > + V4L2Subdevice *subdev = &elem.first->second;
>
> The first second. Well I really do love pairs :-)
:-) I wish the value type for maps wasn't a std::pair but a
std::map_entry that would inherit from std::pair and expose key and
valud aliases for first and second.
> > + int ret = subdev->open();
> > + if (ret < 0) {
> > + LOG(SimplePipeline, Error)
> > + << "Failed to open " << subdev->deviceNode()
> > + << ": " << strerror(-ret);
> > + return false;
> > + }
> > + }
> > +
> > + /* Initialize each pipeline and register a corresponding camera. */
> > + for (std::unique_ptr<SimpleCameraData> &data : pipelines) {
> > + int ret = data->init();
> > + if (ret < 0)
> > + continue;
> > +
> > + std::shared_ptr<Camera> camera =
> > + Camera::create(this, data->sensor_->entity()->name(),
> > + data->streams());
> > + registerCamera(std::move(camera), std::move(data));
> > + }
> > +
> > + return true;
> > +}
> > +
> > +V4L2Subdevice *SimplePipelineHandler::subdev(const MediaEntity *entity)
> > +{
> > + auto iter = subdevs_.find(entity);
> > + if (iter == subdevs_.end())
> > + return nullptr;
> > +
> > + return &iter->second;
> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Buffer Handling
> > + */
> > +
> > +void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)
> > +{
> > + ASSERT(activeCamera_);
> > + Request *request = buffer->request();
> > + completeBuffer(activeCamera_, request, buffer);
> > + completeRequest(activeCamera_, request);
> > +}
> > +
> > +REGISTER_PIPELINE_HANDLER(SimplePipelineHandler);
> > +
> > +} /* namespace libcamera */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list