[{"id":4053,"web_url":"https://patchwork.libcamera.org/comment/4053/","msgid":"<20200317141812.tyrusaekmqxw3ac2@uno.localdomain>","date":"2020-03-17T14:18:12","subject":"Re: [libcamera-devel] [PATCH v2 08/10] libcamera: pipeline: Add a\n\tsimple pipeline handler","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Mar 16, 2020 at 11:43:08PM +0200, Laurent Pinchart wrote:\n> From: Martijn Braam \n>\n> This new pipeline handler aims at supporting any simple device without\n> requiring any device-specific code. Simple devices are currently defined\n> as a graph made of one or multiple camera sensors and a single video\n> node, with each sensor connected to the video node through a linear\n> pipeline.\n>\n> The simple pipeline handler will automatically parse the media graph,\n> enumerate sensors, build supported stream configurations, and configure\n> the pipeline, without any device-specific knowledge. It doesn't support\n> configuration of any processing in the pipeline at the moment, but may\n> be extended to support simple processing such as format conversion or\n> scaling in the future.\n>\n> The only device-specific information in the pipeline handler is the list\n> of supported drivers, required for device matching. We may be able to\n> remove this in the future by matching with the simple pipeline handler\n> as a last resort option, after all other pipeline handlers have been\n> tried.\n>\n> Signed-off-by: Martijn Braam \n> Signed-off-by: Laurent Pinchart \n> ---\n> Changes since v1:\n>\n> - Rebase on top of buffer API rework\n> - Expose stream formats\n> - Rework camera data config\n> ---\n> src/libcamera/pipeline/meson.build | 1 +\n> src/libcamera/pipeline/simple/meson.build | 3 +\n> src/libcamera/pipeline/simple/simple.cpp | 699 ++++++++++++++++++++++\n> 3 files changed, 703 insertions(+)\n> create mode 100644 src/libcamera/pipeline/simple/meson.build\n> create mode 100644 src/libcamera/pipeline/simple/simple.cpp\n>\n> diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build\n> index 0d466225a72e..606ba31a0319 100644\n> --- a/src/libcamera/pipeline/meson.build\n> +++ b/src/libcamera/pipeline/meson.build\n> @@ -5,3 +5,4 @@ libcamera_sources += files([\n>\n> subdir('ipu3')\n> subdir('rkisp1')\n> +subdir('simple')\n> diff --git a/src/libcamera/pipeline/simple/meson.build b/src/libcamera/pipeline/simple/meson.build\n> new file mode 100644\n> index 000000000000..4945a3e173cf\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/simple/meson.build\n> @@ -0,0 +1,3 @@\n> +libcamera_sources += files([\n> + 'simple.cpp',\n\nIs this worth its own subdirectory or can it live at top level like\nuvc and vimc ?\n\n> +])\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> new file mode 100644\n> index 000000000000..2126799c54eb\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -0,0 +1,699 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Laurent Pinchart\n> + * Copyright (C) 2019, Martijn Braam\n> + *\n> + * simple.cpp - Pipeline handler for simple pipelines\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"camera_sensor.h\"\n> +#include \"device_enumerator.h\"\n> +#include \"log.h\"\n> +#include \"media_device.h\"\n> +#include \"pipeline_handler.h\"\n> +#include \"v4l2_subdevice.h\"\n> +#include \"v4l2_videodevice.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(SimplePipeline)\n> +\n> +class SimplePipelineHandler;\n> +\n> +class SimpleCameraData : public CameraData\n> +{\n> +public:\n> +\tSimpleCameraData(PipelineHandler *pipe, MediaEntity *sensor,\n> +\t\t\t MediaEntity *video);\n> +\n> +\tbool isValid() const { return sensor_ != nullptr; }\n> +\tstd::set streams() { return { &stream_ }; }\n> +\n> +\tint init();\n> +\tint setupLinks();\n> +\tint setupFormats(V4L2SubdeviceFormat *format,\n> +\t\t\t V4L2Subdevice::Whence whence);\n> +\n> +\tstruct Entity {\n> +\t\tMediaEntity *entity;\n> +\t\tMediaLink *link;\n> +\t};\n> +\n> +\tstruct Configuration {\n> +\t\tuint32_t code;\n> +\t\tPixelFormat pixelFormat;\n> +\t\tSize size;\n> +\t};\n> +\n> +\tStream stream_;\n> +\tstd::unique_ptr sensor_;\n> +\tstd::list entities_;\n> +\n> +\tstd::vector configs_;\n> +\tstd::map formats_;\n> +};\n> +\n> +class SimpleCameraConfiguration : public CameraConfiguration\n> +{\n> +public:\n> +\tSimpleCameraConfiguration(Camera *camera, SimpleCameraData *data);\n> +\n> +\tStatus validate() override;\n> +\n> +\tconst V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }\n> +\n> +private:\n> +\t/*\n> +\t * The SimpleCameraData instance is guaranteed to be valid as long as\n> +\t * the corresponding Camera instance is valid. In order to borrow a\n> +\t * reference to the camera data, store a new reference to the camera.\n> +\t */\n> +\tstd::shared_ptr camera_;\n> +\tconst SimpleCameraData *data_;\n> +\n> +\tV4L2SubdeviceFormat sensorFormat_;\n> +};\n> +\n> +class SimplePipelineHandler : public PipelineHandler\n> +{\n> +public:\n> +\tSimplePipelineHandler(CameraManager *manager);\n> +\t~SimplePipelineHandler();\n> +\n> +\tCameraConfiguration *generateConfiguration(Camera *camera,\n> +\t\t\t\t\t\t const StreamRoles &roles) override;\n> +\tint configure(Camera *camera, CameraConfiguration *config) override;\n> +\n> +\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t std::vector> *buffers) override;\n> +\n> +\tint start(Camera *camera) override;\n> +\tvoid stop(Camera *camera) override;\n> +\n> +\tbool match(DeviceEnumerator *enumerator) override;\n> +\n> +\tV4L2VideoDevice *video() { return video_; }\n> +\tV4L2Subdevice *subdev(const MediaEntity *entity);\n> +\n> +protected:\n> +\tint queueRequestDevice(Camera *camera, Request *request) override;\n> +\n> +private:\n> +\tSimpleCameraData *cameraData(const Camera *camera)\n> +\t{\n> +\t\treturn static_cast(\n> +\t\t\tPipelineHandler::cameraData(camera));\n> +\t}\n> +\n> +\tint initLinks();\n> +\n> +\tint createCamera(MediaEntity *sensor);\n> +\n> +\tvoid bufferReady(FrameBuffer *buffer);\n> +\n> +\tMediaDevice *media_;\n> +\tV4L2VideoDevice *video_;\n> +\tstd::map subdevs_;\n> +\n> +\tCamera *activeCamera_;\n> +};\n> +\n> +/* -----------------------------------------------------------------------------\n> + * Camera Data\n> + */\n> +SimpleCameraData::SimpleCameraData(PipelineHandler *pipe, MediaEntity *sensor,\n> +\t\t\t\t MediaEntity *video)\n> +\t: CameraData(pipe)\n> +{\n> +\tint ret;\n> +\n> +\t/*\n> +\t * Walk the pipeline towards the video node and store all entities\n> +\t * along the way.\n> +\t */\n> +\tMediaEntity *source = sensor;\n> +\n> +\twhile (source) {\n> +\t\t/* If we have reached the video node, we're done. */\n> +\t\tif (source == video)\n> +\t\t\tbreak;\n> +\n> +\t\t/* Use the first output pad that has links. */\n> +\t\tMediaPad *sourcePad = nullptr;\n> +\t\tfor (MediaPad *pad : source->pads()) {\n> +\t\t\tif ((pad->flags() & MEDIA_PAD_FL_SOURCE) &&\n> +\t\t\t !pad->links().empty()) {\n> +\t\t\t\tsourcePad = pad;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (!sourcePad)\n> +\t\t\treturn;\n> +\n> +\t\t/* Use the first link that isn't immutable and disabled. */\n> +\t\tMediaLink *sourceLink = nullptr;\n> +\t\tfor (MediaLink *link : sourcePad->links()) {\n> +\t\t\tif ((link->flags() & MEDIA_LNK_FL_ENABLED) ||\n> +\t\t\t !(link->flags() & MEDIA_LNK_FL_IMMUTABLE)) {\n> +\t\t\t\tsourceLink = link;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (!sourceLink)\n> +\t\t\treturn;\n> +\n> +\t\tentities_.push_back({ source, sourceLink });\n> +\n> +\t\tsource = sourceLink->sink()->entity();\n> +\n> +\t\t/* Avoid infinite loops. */\n\nIs there a case for circular loops in media pipelines ?\n\n> +\t\tauto iter = std::find_if(entities_.begin(), entities_.end(),\n> +\t\t\t\t\t [&](const Entity &entity) {\n> +\t\t\t\t\t\t return entity.entity == source;\n> +\t\t\t\t\t });\n> +\t\tif (iter != entities_.end()) {\n> +\t\t\tLOG(SimplePipeline, Info) << \"Loop detected in pipeline\";\n> +\t\t\treturn;\n> +\t\t}\n> +\t}\n> +\n> +\t/* We have a valid pipeline, create the camera sensor. */\n> +\tsensor_ = std::make_unique(sensor);\n> +\tret = sensor_->init();\n> +\tif (ret) {\n> +\t\tsensor_.reset();\n> +\t\treturn;\n> +\t}\n> +}\n> +\n> +int SimpleCameraData::init()\n> +{\n> +\tSimplePipelineHandler *pipe = static_cast(pipe_);\n> +\tV4L2VideoDevice *video = pipe->video();\n> +\tint ret;\n> +\n> +\t/*\n> +\t * Enumerate the possible pipeline configurations. For each media bus\n> +\t * format supported by the sensor, propagate the formats through the\n> +\t * pipeline, and enumerate the corresponding possible V4L2 pixel\n> +\t * formats on the video node.\n> +\t */\n> +\tfor (unsigned int code : sensor_->mbusCodes()) {\n> +\t\tV4L2SubdeviceFormat format{ code, sensor_->resolution() };\n\nThis (and in SimplePipeline::configure()) you seems to assume the max\nsensor resolution is available for all mbus codes. I know the below\nsetupLinks() applies the format to the sensor, so size will be\nadjusted, but I wonder if would not be worth associating in\nConfiguration the max sensor size associated with an mbus code to\navoid adjustments.\n\nAlso, speaking of it, isn't \"Configuration\" a too generic name ? I had\nto look it up to see if it's a construct of this pipeline handler or a\ngeneral libcamera construct.\n\n> +\n> +\t\t/*\n> +\t\t * Setup links first as some subdev drivers take active links\n> +\t\t * into account to propaget TRY formats. So is life :-(\n> +\t\t */\n> +\t\tret = setupLinks();\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n> +\n> +\t\tret = setupFormats(&format, V4L2Subdevice::TryFormat);\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n\nSO for each supported mbus_code we go through a full pipeline\nconfiguration. setupFormat() only fails when applying the format to\nthe sensor, so I wonder if a full pipeline config is required. Or\nshould you fail when applying formats in setupFormats to intermediate\nentities as well ?\n\n> +\n> +\t\tstd::vector formats =\n> +\t\t\tvideo->formats(format.mbus_code).formats();\n> +\n> +\t\tLOG(SimplePipeline, Debug)\n> +\t\t\t<< \"Adding configuration for \" << format.size.toString()\n> +\t\t\t<< \" in pixel formats [ \"\n> +\t\t\t<< utils::join(formats, \", \",\n> +\t\t\t\t [](unsigned int f) { return std::to_string(f); })\n> +\t\t\t<< \" ]\";\n> +\n> +\t\t/*\n> +\t\t * Store the configuration in the formats_ map, mapping\n> +\t\t * PixelFormat to configuration. Any previously stored value is\n> +\t\t * overwritten, as the pipeline handler currently doesn't care\n> +\t\t * about how a particular PixelFormat is achieved.\n> +\t\t */\n> +\t\tfor (unsigned int v4l2Format : formats) {\n> +\t\t\tPixelFormat pixelFormat = video->toPixelFormat(v4l2Format);\n> +\t\t\tif (!pixelFormat)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tConfiguration config;\n> +\t\t\tconfig.code = code;\n> +\t\t\tconfig.pixelFormat = pixelFormat;\n> +\t\t\tconfig.size = format.size;\n> +\n> +\t\t\tformats_[pixelFormat] = config;\n> +\t\t}\n> +\t}\n> +\n> +\tif (formats_.empty()) {\n> +\t\tLOG(SimplePipeline, Error) << \"No valid configuration found\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int SimpleCameraData::setupLinks()\n> +{\n> +\tint ret;\n> +\n> +\t/*\n> +\t * Configure all links along the pipeline. Some entities may not allow\n> +\t * multiple sink links to be enabled together, even on different sink\n> +\t * pads. We must thus start by disabling all sink links (but the one we\n> +\t * want to enable) before enabling the pipeline link.\n> +\t */\n> +\tfor (SimpleCameraData::Entity &e : entities_) {\n> +\t\tfor (MediaPad *pad : e.link->sink()->entity()->pads()) {\n> +\t\t\tfor (MediaLink *link : pad->links()) {\n> +\t\t\t\tif (link == e.link)\n> +\t\t\t\t\tcontinue;\n> +\n> +\t\t\t\tif ((link->flags() & MEDIA_LNK_FL_ENABLED) &&\n> +\t\t\t\t !(link->flags() & MEDIA_LNK_FL_IMMUTABLE)) {\n> +\t\t\t\t\tret = link->setEnabled(false);\n> +\t\t\t\t\tif (ret < 0)\n> +\t\t\t\t\t\treturn ret;\n> +\t\t\t\t}\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (!(e.link->flags() & MEDIA_LNK_FL_ENABLED)) {\n> +\t\t\tret = e.link->setEnabled(true);\n> +\t\t\tif (ret < 0)\n> +\t\t\t\treturn ret;\n> +\t\t}\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int SimpleCameraData::setupFormats(V4L2SubdeviceFormat *format,\n> +\t\t\t\t V4L2Subdevice::Whence whence)\n\nNot aligned to ( or is it my editor ?\n\n> +{\n> +\tSimplePipelineHandler *pipe = static_cast(pipe_);\n> +\tint ret;\n> +\n> +\t/*\n> +\t * Configure the format on the sensor output and propagate it through\n> +\t * the pipeline.\nnn> +\t */\n> +\tret = sensor_->setFormat(format);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tfor (const Entity &e : entities_) {\n> +\t\tMediaLink *link = e.link;\n> +\t\tMediaPad *source = link->source();\n> +\t\tMediaPad *sink = link->sink();\n> +\n> +\t\tif (source->entity() != sensor_->entity()) {\n> +\t\t\tV4L2Subdevice *subdev = pipe->subdev(source->entity());\n> +\t\t\tsubdev->getFormat(source->index(), format, whence);\n> +\t\t}\n> +\n> +\t\tif (sink->entity()->function() != MEDIA_ENT_F_IO_V4L) {\n> +\t\t\tV4L2Subdevice *subdev = pipe->subdev(sink->entity());\n> +\t\t\tsubdev->setFormat(sink->index(), format, whence);\n\nAs reported above in SimpleCameraData::init(), should you check for return\nvalue here ?\n\n> +\t\t}\n> +\n\nBrilliant, if not a bit terse to parse..\nMaybe handling the sensor and the video node outside of the loop would\nhelp making this a bit more clear, but it works and is compact, so\ntake this comment as I'm thinking out loud.\n\n> +\t\tLOG(SimplePipeline, Debug)\n> +\t\t\t<< \"Link '\" << source->entity()->name()\n> +\t\t\t<< \"':\" << source->index()\n> +\t\t\t<< \" -> '\" << sink->entity()->name()\n> +\t\t\t<< \"':\" << sink->index()\n> +\t\t\t<< \" configured with format \" << format->toString();\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +/* -----------------------------------------------------------------------------\n> + * Camera Configuration\n> + */\n> +\n> +SimpleCameraConfiguration::SimpleCameraConfiguration(Camera *camera,\n> +\t\t\t\t\t\t SimpleCameraData *data)\n> +\t: CameraConfiguration(), camera_(camera->shared_from_this()),\n> +\t data_(data)\n> +{\n> +}\n> +\n> +CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> +{\n> +\tStatus status = Valid;\n> +\n> +\tif (config_.empty())\n> +\t\treturn Invalid;\n> +\n> +\t/* Cap the number of entries to the available streams. */\n> +\tif (config_.size() > 1) {\n> +\t\tconfig_.resize(1);\n> +\t\tstatus = Adjusted;\n> +\t}\n> +\n> +\tStreamConfiguration &cfg = config_[0];\n> +\n> +\t/* Adjust the pixel format. */\n> +\tauto it = data_->formats_.find(cfg.pixelFormat);\n> +\tif (it == data_->formats_.end())\n> +\t\tit = data_->formats_.begin();\n> +\n> +\tPixelFormat pixelFormat = it->first;\n> +\tconst SimpleCameraData::Configuration &pipeConfig = it->second;\n\nnit: move this below\n\n> +\n> +\tif (cfg.pixelFormat != pixelFormat) {\n> +\t\tLOG(SimplePipeline, Debug) << \"Adjusting pixel format\";\n> +\t\tcfg.pixelFormat = pixelFormat;\n> +\t\tstatus = Adjusted;\n> +\t}\n> +\n> +\tif (cfg.size != pipeConfig.size) {\n> +\t\tLOG(SimplePipeline, Debug)\n> +\t\t\t<< \"Adjusting size from \" << cfg.size.toString()\n> +\t\t\t<< \" to \" << pipeConfig.size.toString();\n> +\t\tcfg.size = pipeConfig.size;\n> +\t\tstatus = Adjusted;\n> +\t}\n> +\n> +\tcfg.bufferCount = 3;\n\nShould't this come from the video device ?\n\n> +\n> +\treturn status;\n> +}\n> +\n> +/* -----------------------------------------------------------------------------\n> + * Pipeline Handler\n> + */\n> +\n> +SimplePipelineHandler::SimplePipelineHandler(CameraManager *manager)\n> +\t: PipelineHandler(manager), video_(nullptr)\n> +{\n> +}\n> +\n> +SimplePipelineHandler::~SimplePipelineHandler()\n> +{\n> +\tdelete video_;\n> +}\n> +\n> +CameraConfiguration *SimplePipelineHandler::generateConfiguration(Camera *camera,\n> +\t\t\t\t\t\t\t\t const StreamRoles &roles)\n> +{\n> +\tSimpleCameraData *data = cameraData(camera);\n> +\tCameraConfiguration *config =\n> +\t\tnew SimpleCameraConfiguration(camera, data);\n> +\n> +\tif (roles.empty())\n> +\t\treturn config;\n\nall roles are supported then ? There are not many ways around it if\nnot selecting the the stream with the max size for still capture and\ndefaulting to something smaller to viewfinder, but these are arbitrary\nchoices I guess\n\n> +\n> +\t/* Create the formats map. */\n> +\tstd::map> formats;\n> +\tstd::transform(data->formats_.begin(), data->formats_.end(),\n> +\t\t std::inserter(formats, formats.end()),\n> +\t\t [](const auto &format) -> decltype(formats)::value_type {\n> +\t\t\t const PixelFormat &pixelFormat = format.first;\n> +\t\t\t const Size &size = format.second.size;\n> +\t\t\t return { pixelFormat, { size } };\n> +\t\t });\n> +\n> +\t/*\n> +\t * Create the stream configuration. Take the first entry in the formats\n> +\t * map as the default, for lack of a better option.\n> +\t */\n\nWhich are not sorted, am I wrong ?\n\n> +\tStreamConfiguration cfg{ StreamFormats{ formats } };\n> +\tcfg.pixelFormat = formats.begin()->first;\n> +\tcfg.size = formats.begin()->second[0].max;\n> +\n> +\tconfig->addConfiguration(cfg);\n> +\n> +\tconfig->validate();\n> +\n> +\treturn config;\n> +}\n> +\n> +int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> +{\n> +\tSimpleCameraConfiguration *config =\n> +\t\tstatic_cast(c);\n> +\tSimpleCameraData *data = cameraData(camera);\n> +\tStreamConfiguration &cfg = config->at(0);\n> +\tint ret;\n> +\n> +\t/*\n> +\t * Configure links on the pipeline and propagate formats from the\n> +\t * sensor to the video node.\n> +\t */\n> +\tret = data->setupLinks();\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tconst SimpleCameraData::Configuration &pipeConfig =\n> +\t\tdata->formats_[cfg.pixelFormat];\n> +\n> +\tV4L2SubdeviceFormat format{ pipeConfig.code, data->sensor_->resolution() };\n> +\n> +\tret = data->setupFormats(&format, V4L2Subdevice::ActiveFormat);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\t/* Configure the video node. */\n> +\tuint32_t fourcc = video_->toV4L2Fourcc(cfg.pixelFormat);\n> +\n> +\tV4L2DeviceFormat outputFormat = {};\n> +\toutputFormat.fourcc = fourcc;\n> +\toutputFormat.size = cfg.size;\n> +\n> +\tret = video_->setFormat(&outputFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tif (outputFormat.size != cfg.size || outputFormat.fourcc != fourcc) {\n> +\t\tLOG(SimplePipeline, Error)\n> +\t\t\t<< \"Unable to configure capture in \" << cfg.toString();\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tcfg.setStream(&data->stream_);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t\t std::vector> *buffers)\n> +{\n> +\tunsigned int count = stream->configuration().bufferCount;\n> +\n> +\treturn video_->exportBuffers(count, buffers);\n> +}\n> +\n> +int SimplePipelineHandler::start(Camera *camera)\n> +{\n> +\tSimpleCameraData *data = cameraData(camera);\n> +\tunsigned int count = data->stream_.configuration().bufferCount;\n> +\n> +\tint ret = video_->importBuffers(count);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tret = video_->streamOn();\n> +\tif (ret < 0) {\n> +\t\tvideo_->releaseBuffers();\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tactiveCamera_ = camera;\n> +\n> +\treturn 0;\n> +}\n> +\n> +void SimplePipelineHandler::stop(Camera *camera)\n> +{\n> +\tvideo_->streamOff();\n> +\tvideo_->releaseBuffers();\n> +\tactiveCamera_ = nullptr;\n> +}\n> +\n> +int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n> +{\n> +\tSimpleCameraData *data = cameraData(camera);\n> +\tStream *stream = &data->stream_;\n\nAs in bufferReady we complete the request as soon as the buffer is\ncomplete, should we make sure the request refers to a single stream ?\n\n> +\n> +\tFrameBuffer *buffer = request->findBuffer(stream);\n> +\tif (!buffer) {\n> +\t\tLOG(SimplePipeline, Error)\n> +\t\t\t<< \"Attempt to queue request with invalid stream\";\n> +\t\treturn -ENOENT;\n> +\t}\n> +\n> +\treturn video_->queueBuffer(buffer);\n> +}\n> +\n> +/* -----------------------------------------------------------------------------\n> + * Match and Setup\n> + */\n> +\n> +bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n> +{\n> +\tstatic const char * const drivers[] = {\n> +\t\t\"imx7-csi\",\n> +\t\t\"sun6i-csi\",\n> +\t};\n> +\n> +\tfor (const char *driver : drivers) {\n> +\t\tDeviceMatch dm(driver);\n> +\t\tmedia_ = acquireMediaDevice(enumerator, dm);\n> +\t\tif (media_)\n> +\t\t\tbreak;\n> +\t}\n> +\n> +\tif (!media_)\n> +\t\treturn false;\n> +\n> +\t/*\n> +\t * Locate sensors and video nodes. We only support pipelines with at\n> +\t * least one sensor and exactly one video captude node.\n> +\t */\n> +\tstd::vector sensors;\n> +\tstd::vector videos;\n> +\n> +\tfor (MediaEntity *entity : media_->entities()) {\n> +\t\tswitch (entity->function()) {\n> +\t\tcase MEDIA_ENT_F_CAM_SENSOR:\n> +\t\t\tsensors.push_back(entity);\n> +\t\t\tbreak;\n> +\n> +\t\tcase MEDIA_ENT_F_IO_V4L:\n> +\t\t\tif (entity->pads().size() == 1 &&\n\nIsn't this an arbitrary restinction ? Can't a video node a more than\none, maybe not connected, pad ?\n\n> +\t\t\t (entity->pads()[0]->flags() & MEDIA_PAD_FL_SINK))\n> +\t\t\t\tvideos.push_back(entity);\n> +\t\t\tbreak;\n> +\n> +\t\tdefault:\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tif (sensors.empty()) {\n> +\t\tLOG(SimplePipeline, Info) << \"No sensor found\";\n> +\t\treturn false;\n> +\t}\n> +\n> +\tif (videos.size() != 1) {\n\nWhy a vector ? Cant you populate a single pointer and fail if it is\nalready != nullptr when walking device nodes ?\n\n> +\t\tLOG(SimplePipeline, Info)\n\ns/Info/Error here and above\n\n> +\t\t\t<< \"Pipeline with \" << videos.size()\n> +\t\t\t<< \" video capture nodes is not supported\";\n> +\t\treturn false;\n> +\t}\n> +\n> +\t/* Locate and open the capture video node. */\n> +\tvideo_ = new V4L2VideoDevice(videos[0]);\n\nIf you remove the vector you can use video_ directly\n\n> +\tif (video_->open() < 0)\n> +\t\treturn false;\n> +\n> +\tif (video_->caps().isMultiplanar()) {\n\nWhy not mplane API support ?\n\n> +\t\tLOG(SimplePipeline, Info)\n\nError here as well ?\n\n> +\t\t\t<< \"V4L2 multiplanar devices are not supported\";\n> +\t\treturn false;\n> +\t}\n> +\n> +\tvideo_->bufferReady.connect(this, &SimplePipelineHandler::bufferReady);\n> +\n> +\t/*\n> +\t * Create one camera data instance for each sensor and gather all\n> +\t * entities in all pipelines.\n> +\t */\n\nI might have missed the case for more sensor connected to the same\nCSI-2 receiver..\n\n> +\tstd::vector> pipelines;\n> +\tstd::set entities;\n> +\n> +\tpipelines.reserve(sensors.size());\n> +\n> +\tfor (MediaEntity *sensor : sensors) {\n> +\t\tstd::unique_ptr data =\n> +\t\t\tstd::make_unique(this, sensor,\n> +\t\t\t\t\t\t\t videos[0]);\n> +\t\tif (!data->isValid()) {\n> +\t\t\tLOG(SimplePipeline, Info)\n\nseems like using Info in place of Error is intentional\n\nQuite some media entities, links, pads dancing in this pipeline :)\nGood job!\n\nThanks\n j\n\n> +\t\t\t\t<< \"No valid pipeline for sensor '\"\n> +\t\t\t\t<< sensor->name() << \"', skipping\";\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tfor (SimpleCameraData::Entity &entity : data->entities_)\n> +\t\t\tentities.insert(entity.entity);\n> +\n> +\t\tpipelines.push_back(std::move(data));\n> +\t}\n> +\n> +\tif (entities.empty())\n> +\t\treturn false;\n> +\n> +\t/* Create and open V4L2Subdev instances for all the entities. */\n> +\tfor (MediaEntity *entity : entities) {\n> +\t\tauto elem = subdevs_.emplace(std::piecewise_construct,\n> +\t\t\t\t\t std::forward_as_tuple(entity),\n> +\t\t\t\t\t std::forward_as_tuple(entity));\n> +\t\tV4L2Subdevice *subdev = &elem.first->second;\n> +\t\tint ret = subdev->open();\n> +\t\tif (ret < 0) {\n> +\t\t\tLOG(SimplePipeline, Error)\n> +\t\t\t\t<< \"Failed to open \" << subdev->deviceNode()\n> +\t\t\t\t<< \": \" << strerror(-ret);\n> +\t\t\treturn false;\n> +\t\t}\n> +\t}\n> +\n> +\t/* Initialize each pipeline and register a corresponding camera. */\n> +\tfor (std::unique_ptr &data : pipelines) {\n> +\t\tint ret = data->init();\n> +\t\tif (ret < 0)\n> +\t\t\tcontinue;\n> +\n> +\t\tstd::shared_ptr camera =\n> +\t\t\tCamera::create(this, data->sensor_->entity()->name(),\n> +\t\t\t\t data->streams());\n> +\t\tregisterCamera(std::move(camera), std::move(data));\n> +\t}\n> +\n> +\treturn true;\n> +}\n> +\n> +V4L2Subdevice *SimplePipelineHandler::subdev(const MediaEntity *entity)\n> +{\n> +\tauto iter = subdevs_.find(entity);\n> +\tif (iter == subdevs_.end())\n> +\t\treturn nullptr;\n> +\n> +\treturn &iter->second;\n> +}\n> +\n> +/* -----------------------------------------------------------------------------\n> + * Buffer Handling\n> + */\n> +\n> +void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n> +{\n> +\tASSERT(activeCamera_);\n> +\tRequest *request = buffer->request();\n> +\tcompleteBuffer(activeCamera_, request, buffer);\n> +\tcompleteRequest(activeCamera_, request);\n> +}\n> +\n> +REGISTER_PIPELINE_HANDLER(SimplePipelineHandler);\n> +\n> +} /* namespace libcamera */\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AF8CD62923\n\tfor ;\n\tTue, 17 Mar 2020 15:15:18 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 4395540006;\n\tTue, 17 Mar 2020 14:15:16 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 17 Mar 2020 15:18:12 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Martijn Braam , \n\tMickael GUENE ,\n\tBenjamin GAIGNARD ","Message-ID":"<20200317141812.tyrusaekmqxw3ac2@uno.localdomain>","References":"<20200316214310.27665-1-laurent.pinchart@ideasonboard.com>\n\t<20200316214310.27665-9-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200316214310.27665-9-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 08/10] libcamera: pipeline: Add a\n\tsimple pipeline handler","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","X-List-Received-Date":"Tue, 17 Mar 2020 14:15:18 -0000"}},{"id":4064,"web_url":"https://patchwork.libcamera.org/comment/4064/","msgid":"<20200317205603.GH2527@pendragon.ideasonboard.com>","date":"2020-03-17T20:56:03","subject":"Re: [libcamera-devel] [PATCH v2 08/10] libcamera: pipeline: Add a\n\tsimple pipeline handler","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Mar 17, 2020 at 03:18:12PM +0100, Jacopo Mondi wrote:\n> On Mon, Mar 16, 2020 at 11:43:08PM +0200, Laurent Pinchart wrote:\n> > From: Martijn Braam \n> >\n> > This new pipeline handler aims at supporting any simple device without\n> > requiring any device-specific code. Simple devices are currently defined\n> > as a graph made of one or multiple camera sensors and a single video\n> > node, with each sensor connected to the video node through a linear\n> > pipeline.\n> >\n> > The simple pipeline handler will automatically parse the media graph,\n> > enumerate sensors, build supported stream configurations, and configure\n> > the pipeline, without any device-specific knowledge. It doesn't support\n> > configuration of any processing in the pipeline at the moment, but may\n> > be extended to support simple processing such as format conversion or\n> > scaling in the future.\n> >\n> > The only device-specific information in the pipeline handler is the list\n> > of supported drivers, required for device matching. We may be able to\n> > remove this in the future by matching with the simple pipeline handler\n> > as a last resort option, after all other pipeline handlers have been\n> > tried.\n> >\n> > Signed-off-by: Martijn Braam \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > Changes since v1:\n> >\n> > - Rebase on top of buffer API rework\n> > - Expose stream formats\n> > - Rework camera data config\n> > ---\n> > src/libcamera/pipeline/meson.build | 1 +\n> > src/libcamera/pipeline/simple/meson.build | 3 +\n> > src/libcamera/pipeline/simple/simple.cpp | 699 ++++++++++++++++++++++\n> > 3 files changed, 703 insertions(+)\n> > create mode 100644 src/libcamera/pipeline/simple/meson.build\n> > create mode 100644 src/libcamera/pipeline/simple/simple.cpp\n> >\n> > diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build\n> > index 0d466225a72e..606ba31a0319 100644\n> > --- a/src/libcamera/pipeline/meson.build\n> > +++ b/src/libcamera/pipeline/meson.build\n> > @@ -5,3 +5,4 @@ libcamera_sources += files([\n> >\n> > subdir('ipu3')\n> > subdir('rkisp1')\n> > +subdir('simple')\n> > diff --git a/src/libcamera/pipeline/simple/meson.build b/src/libcamera/pipeline/simple/meson.build\n> > new file mode 100644\n> > index 000000000000..4945a3e173cf\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/simple/meson.build\n> > @@ -0,0 +1,3 @@\n> > +libcamera_sources += files([\n> > + 'simple.cpp',\n> \n> Is this worth its own subdirectory or can it live at top level like\n> uvc and vimc ?\n\nSee patch 09/10 :-)\n\nI think UVC and vimc should move to subdirectories anyway, one\nsubdirectory per pipeline handler is cleaner in my opinion. I'll send a\npatch to propose that.\n\n> > +])\n> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > new file mode 100644\n> > index 000000000000..2126799c54eb\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -0,0 +1,699 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Laurent Pinchart\n> > + * Copyright (C) 2019, Martijn Braam\n> > + *\n> > + * simple.cpp - Pipeline handler for simple pipelines\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"camera_sensor.h\"\n> > +#include \"device_enumerator.h\"\n> > +#include \"log.h\"\n> > +#include \"media_device.h\"\n> > +#include \"pipeline_handler.h\"\n> > +#include \"v4l2_subdevice.h\"\n> > +#include \"v4l2_videodevice.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(SimplePipeline)\n> > +\n> > +class SimplePipelineHandler;\n> > +\n> > +class SimpleCameraData : public CameraData\n> > +{\n> > +public:\n> > +\tSimpleCameraData(PipelineHandler *pipe, MediaEntity *sensor,\n> > +\t\t\t MediaEntity *video);\n> > +\n> > +\tbool isValid() const { return sensor_ != nullptr; }\n> > +\tstd::set streams() { return { &stream_ }; }\n> > +\n> > +\tint init();\n> > +\tint setupLinks();\n> > +\tint setupFormats(V4L2SubdeviceFormat *format,\n> > +\t\t\t V4L2Subdevice::Whence whence);\n> > +\n> > +\tstruct Entity {\n> > +\t\tMediaEntity *entity;\n> > +\t\tMediaLink *link;\n> > +\t};\n> > +\n> > +\tstruct Configuration {\n> > +\t\tuint32_t code;\n> > +\t\tPixelFormat pixelFormat;\n> > +\t\tSize size;\n> > +\t};\n> > +\n> > +\tStream stream_;\n> > +\tstd::unique_ptr sensor_;\n> > +\tstd::list entities_;\n> > +\n> > +\tstd::vector configs_;\n> > +\tstd::map formats_;\n> > +};\n> > +\n> > +class SimpleCameraConfiguration : public CameraConfiguration\n> > +{\n> > +public:\n> > +\tSimpleCameraConfiguration(Camera *camera, SimpleCameraData *data);\n> > +\n> > +\tStatus validate() override;\n> > +\n> > +\tconst V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }\n> > +\n> > +private:\n> > +\t/*\n> > +\t * The SimpleCameraData instance is guaranteed to be valid as long as\n> > +\t * the corresponding Camera instance is valid. In order to borrow a\n> > +\t * reference to the camera data, store a new reference to the camera.\n> > +\t */\n> > +\tstd::shared_ptr camera_;\n> > +\tconst SimpleCameraData *data_;\n> > +\n> > +\tV4L2SubdeviceFormat sensorFormat_;\n> > +};\n> > +\n> > +class SimplePipelineHandler : public PipelineHandler\n> > +{\n> > +public:\n> > +\tSimplePipelineHandler(CameraManager *manager);\n> > +\t~SimplePipelineHandler();\n> > +\n> > +\tCameraConfiguration *generateConfiguration(Camera *camera,\n> > +\t\t\t\t\t\t const StreamRoles &roles) override;\n> > +\tint configure(Camera *camera, CameraConfiguration *config) override;\n> > +\n> > +\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> > +\t\t\t std::vector> *buffers) override;\n> > +\n> > +\tint start(Camera *camera) override;\n> > +\tvoid stop(Camera *camera) override;\n> > +\n> > +\tbool match(DeviceEnumerator *enumerator) override;\n> > +\n> > +\tV4L2VideoDevice *video() { return video_; }\n> > +\tV4L2Subdevice *subdev(const MediaEntity *entity);\n> > +\n> > +protected:\n> > +\tint queueRequestDevice(Camera *camera, Request *request) override;\n> > +\n> > +private:\n> > +\tSimpleCameraData *cameraData(const Camera *camera)\n> > +\t{\n> > +\t\treturn static_cast(\n> > +\t\t\tPipelineHandler::cameraData(camera));\n> > +\t}\n> > +\n> > +\tint initLinks();\n> > +\n> > +\tint createCamera(MediaEntity *sensor);\n> > +\n> > +\tvoid bufferReady(FrameBuffer *buffer);\n> > +\n> > +\tMediaDevice *media_;\n> > +\tV4L2VideoDevice *video_;\n> > +\tstd::map subdevs_;\n> > +\n> > +\tCamera *activeCamera_;\n> > +};\n> > +\n> > +/* -----------------------------------------------------------------------------\n> > + * Camera Data\n> > + */\n> > +SimpleCameraData::SimpleCameraData(PipelineHandler *pipe, MediaEntity *sensor,\n> > +\t\t\t\t MediaEntity *video)\n> > +\t: CameraData(pipe)\n> > +{\n> > +\tint ret;\n> > +\n> > +\t/*\n> > +\t * Walk the pipeline towards the video node and store all entities\n> > +\t * along the way.\n> > +\t */\n> > +\tMediaEntity *source = sensor;\n> > +\n> > +\twhile (source) {\n> > +\t\t/* If we have reached the video node, we're done. */\n> > +\t\tif (source == video)\n> > +\t\t\tbreak;\n> > +\n> > +\t\t/* Use the first output pad that has links. */\n> > +\t\tMediaPad *sourcePad = nullptr;\n> > +\t\tfor (MediaPad *pad : source->pads()) {\n> > +\t\t\tif ((pad->flags() & MEDIA_PAD_FL_SOURCE) &&\n> > +\t\t\t !pad->links().empty()) {\n> > +\t\t\t\tsourcePad = pad;\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\tif (!sourcePad)\n> > +\t\t\treturn;\n> > +\n> > +\t\t/* Use the first link that isn't immutable and disabled. */\n> > +\t\tMediaLink *sourceLink = nullptr;\n> > +\t\tfor (MediaLink *link : sourcePad->links()) {\n> > +\t\t\tif ((link->flags() & MEDIA_LNK_FL_ENABLED) ||\n> > +\t\t\t !(link->flags() & MEDIA_LNK_FL_IMMUTABLE)) {\n> > +\t\t\t\tsourceLink = link;\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\tif (!sourceLink)\n> > +\t\t\treturn;\n> > +\n> > +\t\tentities_.push_back({ source, sourceLink });\n> > +\n> > +\t\tsource = sourceLink->sink()->entity();\n> > +\n> > +\t\t/* Avoid infinite loops. */\n> \n> Is there a case for circular loops in media pipelines ?\n\nNot that I know of, but as we don't know what driver we'll deal with, I\nthought it would be better to print an error message than looping until\nthe stack overflows. Do you think that's overkill ?\n\n> > +\t\tauto iter = std::find_if(entities_.begin(), entities_.end(),\n> > +\t\t\t\t\t [&](const Entity &entity) {\n> > +\t\t\t\t\t\t return entity.entity == source;\n> > +\t\t\t\t\t });\n> > +\t\tif (iter != entities_.end()) {\n> > +\t\t\tLOG(SimplePipeline, Info) << \"Loop detected in pipeline\";\n> > +\t\t\treturn;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\t/* We have a valid pipeline, create the camera sensor. */\n> > +\tsensor_ = std::make_unique(sensor);\n> > +\tret = sensor_->init();\n> > +\tif (ret) {\n> > +\t\tsensor_.reset();\n> > +\t\treturn;\n> > +\t}\n> > +}\n> > +\n> > +int SimpleCameraData::init()\n> > +{\n> > +\tSimplePipelineHandler *pipe = static_cast(pipe_);\n> > +\tV4L2VideoDevice *video = pipe->video();\n> > +\tint ret;\n> > +\n> > +\t/*\n> > +\t * Enumerate the possible pipeline configurations. For each media bus\n> > +\t * format supported by the sensor, propagate the formats through the\n> > +\t * pipeline, and enumerate the corresponding possible V4L2 pixel\n> > +\t * formats on the video node.\n> > +\t */\n> > +\tfor (unsigned int code : sensor_->mbusCodes()) {\n> > +\t\tV4L2SubdeviceFormat format{ code, sensor_->resolution() };\n> \n> This (and in SimplePipeline::configure()) you seems to assume the max\n> sensor resolution is available for all mbus codes. I know the below\n> setupLinks() applies the format to the sensor, so size will be\n> adjusted, but I wonder if would not be worth associating in\n> Configuration the max sensor size associated with an mbus code to\n> avoid adjustments.\n\nIt's the usual case. I know some sensors could implement this\ndifferently, but I think that's room for future improvements. The simple\npipeline handler is nowhere close to be 100% complete.\n\n> Also, speaking of it, isn't \"Configuration\" a too generic name ? I had\n> to look it up to see if it's a construct of this pipeline handler or a\n> general libcamera construct.\n\nI'm not too fond of it either, so if you have a better alternative, I\ncan change it. I didn't want to go for SimpleConfiguration though, as\nthe name is too long, and sounds awkward as the Configuration struct is\nalready a member of SimpleCameraData.\n\n> > +\n> > +\t\t/*\n> > +\t\t * Setup links first as some subdev drivers take active links\n> > +\t\t * into account to propaget TRY formats. So is life :-(\n> > +\t\t */\n> > +\t\tret = setupLinks();\n> > +\t\tif (ret < 0)\n> > +\t\t\treturn ret;\n> > +\n> > +\t\tret = setupFormats(&format, V4L2Subdevice::TryFormat);\n> > +\t\tif (ret < 0)\n> > +\t\t\treturn ret;\n> \n> SO for each supported mbus_code we go through a full pipeline\n> configuration. setupFormat() only fails when applying the format to\n> the sensor, so I wonder if a full pipeline config is required. Or\n> should you fail when applying formats in setupFormats to intermediate\n> entities as well ?\n\nIt's not about failing, it's about propagating the format through the\npipeline. I'll fix setupFormats() to propagate errors though.\n\n> > +\n> > +\t\tstd::vector formats =\n> > +\t\t\tvideo->formats(format.mbus_code).formats();\n> > +\n> > +\t\tLOG(SimplePipeline, Debug)\n> > +\t\t\t<< \"Adding configuration for \" << format.size.toString()\n> > +\t\t\t<< \" in pixel formats [ \"\n> > +\t\t\t<< utils::join(formats, \", \",\n> > +\t\t\t\t [](unsigned int f) { return std::to_string(f); })\n> > +\t\t\t<< \" ]\";\n> > +\n> > +\t\t/*\n> > +\t\t * Store the configuration in the formats_ map, mapping\n> > +\t\t * PixelFormat to configuration. Any previously stored value is\n> > +\t\t * overwritten, as the pipeline handler currently doesn't care\n> > +\t\t * about how a particular PixelFormat is achieved.\n> > +\t\t */\n> > +\t\tfor (unsigned int v4l2Format : formats) {\n> > +\t\t\tPixelFormat pixelFormat = video->toPixelFormat(v4l2Format);\n> > +\t\t\tif (!pixelFormat)\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\tConfiguration config;\n> > +\t\t\tconfig.code = code;\n> > +\t\t\tconfig.pixelFormat = pixelFormat;\n> > +\t\t\tconfig.size = format.size;\n> > +\n> > +\t\t\tformats_[pixelFormat] = config;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tif (formats_.empty()) {\n> > +\t\tLOG(SimplePipeline, Error) << \"No valid configuration found\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int SimpleCameraData::setupLinks()\n> > +{\n> > +\tint ret;\n> > +\n> > +\t/*\n> > +\t * Configure all links along the pipeline. Some entities may not allow\n> > +\t * multiple sink links to be enabled together, even on different sink\n> > +\t * pads. We must thus start by disabling all sink links (but the one we\n> > +\t * want to enable) before enabling the pipeline link.\n> > +\t */\n> > +\tfor (SimpleCameraData::Entity &e : entities_) {\n> > +\t\tfor (MediaPad *pad : e.link->sink()->entity()->pads()) {\n> > +\t\t\tfor (MediaLink *link : pad->links()) {\n> > +\t\t\t\tif (link == e.link)\n> > +\t\t\t\t\tcontinue;\n> > +\n> > +\t\t\t\tif ((link->flags() & MEDIA_LNK_FL_ENABLED) &&\n> > +\t\t\t\t !(link->flags() & MEDIA_LNK_FL_IMMUTABLE)) {\n> > +\t\t\t\t\tret = link->setEnabled(false);\n> > +\t\t\t\t\tif (ret < 0)\n> > +\t\t\t\t\t\treturn ret;\n> > +\t\t\t\t}\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\tif (!(e.link->flags() & MEDIA_LNK_FL_ENABLED)) {\n> > +\t\t\tret = e.link->setEnabled(true);\n> > +\t\t\tif (ret < 0)\n> > +\t\t\t\treturn ret;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int SimpleCameraData::setupFormats(V4L2SubdeviceFormat *format,\n> > +\t\t\t\t V4L2Subdevice::Whence whence)\n> \n> Not aligned to ( or is it my editor ?\n\nIt seems to be your editor.\n\n> > +{\n> > +\tSimplePipelineHandler *pipe = static_cast(pipe_);\n> > +\tint ret;\n> > +\n> > +\t/*\n> > +\t * Configure the format on the sensor output and propagate it through\n> > +\t * the pipeline.\n> > +\t */\n> > +\tret = sensor_->setFormat(format);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tfor (const Entity &e : entities_) {\n> > +\t\tMediaLink *link = e.link;\n> > +\t\tMediaPad *source = link->source();\n> > +\t\tMediaPad *sink = link->sink();\n> > +\n> > +\t\tif (source->entity() != sensor_->entity()) {\n> > +\t\t\tV4L2Subdevice *subdev = pipe->subdev(source->entity());\n> > +\t\t\tsubdev->getFormat(source->index(), format, whence);\n> > +\t\t}\n> > +\n> > +\t\tif (sink->entity()->function() != MEDIA_ENT_F_IO_V4L) {\n> > +\t\t\tV4L2Subdevice *subdev = pipe->subdev(sink->entity());\n> > +\t\t\tsubdev->setFormat(sink->index(), format, whence);\n> \n> As reported above in SimpleCameraData::init(), should you check for return\n> value here ?\n\nYes, I'll change that.\n\n> > +\t\t}\n> > +\n> \n> Brilliant, if not a bit terse to parse..\n> Maybe handling the sensor and the video node outside of the loop would\n> help making this a bit more clear, but it works and is compact, so\n> take this comment as I'm thinking out loud.\n\nBut they are outside of the loop already :-) The checks are meant to\nignore them in the loop. Each iteration needs to get the format on the\nsource and set it on the sink, for each link, with two exceptions:\n\n- For the first link (where the source is the sensor), we have already\n just called setFormat() on the sensor, so there's no need to call\n getFormat(), that would be a waste of CPU time.\n\n- For the last link, we don't want to set the format on the sink, as\n video nodes are not subdevs, they don't have pad ops.\n\n> > +\t\tLOG(SimplePipeline, Debug)\n> > +\t\t\t<< \"Link '\" << source->entity()->name()\n> > +\t\t\t<< \"':\" << source->index()\n> > +\t\t\t<< \" -> '\" << sink->entity()->name()\n> > +\t\t\t<< \"':\" << sink->index()\n> > +\t\t\t<< \" configured with format \" << format->toString();\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/* -----------------------------------------------------------------------------\n> > + * Camera Configuration\n> > + */\n> > +\n> > +SimpleCameraConfiguration::SimpleCameraConfiguration(Camera *camera,\n> > +\t\t\t\t\t\t SimpleCameraData *data)\n> > +\t: CameraConfiguration(), camera_(camera->shared_from_this()),\n> > +\t data_(data)\n> > +{\n> > +}\n> > +\n> > +CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> > +{\n> > +\tStatus status = Valid;\n> > +\n> > +\tif (config_.empty())\n> > +\t\treturn Invalid;\n> > +\n> > +\t/* Cap the number of entries to the available streams. */\n> > +\tif (config_.size() > 1) {\n> > +\t\tconfig_.resize(1);\n> > +\t\tstatus = Adjusted;\n> > +\t}\n> > +\n> > +\tStreamConfiguration &cfg = config_[0];\n> > +\n> > +\t/* Adjust the pixel format. */\n> > +\tauto it = data_->formats_.find(cfg.pixelFormat);\n> > +\tif (it == data_->formats_.end())\n> > +\t\tit = data_->formats_.begin();\n> > +\n> > +\tPixelFormat pixelFormat = it->first;\n> > +\tconst SimpleCameraData::Configuration &pipeConfig = it->second;\n> \n> nit: move this below\n\nDone.\n\n> > +\n> > +\tif (cfg.pixelFormat != pixelFormat) {\n> > +\t\tLOG(SimplePipeline, Debug) << \"Adjusting pixel format\";\n> > +\t\tcfg.pixelFormat = pixelFormat;\n> > +\t\tstatus = Adjusted;\n> > +\t}\n> > +\n> > +\tif (cfg.size != pipeConfig.size) {\n> > +\t\tLOG(SimplePipeline, Debug)\n> > +\t\t\t<< \"Adjusting size from \" << cfg.size.toString()\n> > +\t\t\t<< \" to \" << pipeConfig.size.toString();\n> > +\t\tcfg.size = pipeConfig.size;\n> > +\t\tstatus = Adjusted;\n> > +\t}\n> > +\n> > +\tcfg.bufferCount = 3;\n> \n> Should't this come from the video device ?\n\nOnce V4L2 will give us the information, sure :-)\n\n> > +\n> > +\treturn status;\n> > +}\n> > +\n> > +/* -----------------------------------------------------------------------------\n> > + * Pipeline Handler\n> > + */\n> > +\n> > +SimplePipelineHandler::SimplePipelineHandler(CameraManager *manager)\n> > +\t: PipelineHandler(manager), video_(nullptr)\n> > +{\n> > +}\n> > +\n> > +SimplePipelineHandler::~SimplePipelineHandler()\n> > +{\n> > +\tdelete video_;\n> > +}\n> > +\n> > +CameraConfiguration *SimplePipelineHandler::generateConfiguration(Camera *camera,\n> > +\t\t\t\t\t\t\t\t const StreamRoles &roles)\n> > +{\n> > +\tSimpleCameraData *data = cameraData(camera);\n> > +\tCameraConfiguration *config =\n> > +\t\tnew SimpleCameraConfiguration(camera, data);\n> > +\n> > +\tif (roles.empty())\n> > +\t\treturn config;\n> \n> all roles are supported then ? There are not many ways around it if\n> not selecting the the stream with the max size for still capture and\n> defaulting to something smaller to viewfinder, but these are arbitrary\n> choices I guess\n\nThere's a single stream, without scaling, so the role is a bit\nirrelevant I think. Maybe we'll implement something smarter in the\nfuture, maybe not.\n\n> > +\n> > +\t/* Create the formats map. */\n> > +\tstd::map> formats;\n> > +\tstd::transform(data->formats_.begin(), data->formats_.end(),\n> > +\t\t std::inserter(formats, formats.end()),\n> > +\t\t [](const auto &format) -> decltype(formats)::value_type {\n> > +\t\t\t const PixelFormat &pixelFormat = format.first;\n> > +\t\t\t const Size &size = format.second.size;\n> > +\t\t\t return { pixelFormat, { size } };\n> > +\t\t });\n> > +\n> > +\t/*\n> > +\t * Create the stream configuration. Take the first entry in the formats\n> > +\t * map as the default, for lack of a better option.\n> > +\t */\n> \n> Which are not sorted, am I wrong ?\n\nThey're sorted by PixelFormat (std::map is ordered). This is purely\narbitrary, and I think we should improve that in the future. I'll add a\n\\todo.\n\n> > +\tStreamConfiguration cfg{ StreamFormats{ formats } };\n> > +\tcfg.pixelFormat = formats.begin()->first;\n> > +\tcfg.size = formats.begin()->second[0].max;\n> > +\n> > +\tconfig->addConfiguration(cfg);\n> > +\n> > +\tconfig->validate();\n> > +\n> > +\treturn config;\n> > +}\n> > +\n> > +int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> > +{\n> > +\tSimpleCameraConfiguration *config =\n> > +\t\tstatic_cast(c);\n> > +\tSimpleCameraData *data = cameraData(camera);\n> > +\tStreamConfiguration &cfg = config->at(0);\n> > +\tint ret;\n> > +\n> > +\t/*\n> > +\t * Configure links on the pipeline and propagate formats from the\n> > +\t * sensor to the video node.\n> > +\t */\n> > +\tret = data->setupLinks();\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tconst SimpleCameraData::Configuration &pipeConfig =\n> > +\t\tdata->formats_[cfg.pixelFormat];\n> > +\n> > +\tV4L2SubdeviceFormat format{ pipeConfig.code, data->sensor_->resolution() };\n> > +\n> > +\tret = data->setupFormats(&format, V4L2Subdevice::ActiveFormat);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\t/* Configure the video node. */\n> > +\tuint32_t fourcc = video_->toV4L2Fourcc(cfg.pixelFormat);\n> > +\n> > +\tV4L2DeviceFormat outputFormat = {};\n> > +\toutputFormat.fourcc = fourcc;\n> > +\toutputFormat.size = cfg.size;\n> > +\n> > +\tret = video_->setFormat(&outputFormat);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tif (outputFormat.size != cfg.size || outputFormat.fourcc != fourcc) {\n> > +\t\tLOG(SimplePipeline, Error)\n> > +\t\t\t<< \"Unable to configure capture in \" << cfg.toString();\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tcfg.setStream(&data->stream_);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n> > +\t\t\t\t\t std::vector> *buffers)\n> > +{\n> > +\tunsigned int count = stream->configuration().bufferCount;\n> > +\n> > +\treturn video_->exportBuffers(count, buffers);\n> > +}\n> > +\n> > +int SimplePipelineHandler::start(Camera *camera)\n> > +{\n> > +\tSimpleCameraData *data = cameraData(camera);\n> > +\tunsigned int count = data->stream_.configuration().bufferCount;\n> > +\n> > +\tint ret = video_->importBuffers(count);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tret = video_->streamOn();\n> > +\tif (ret < 0) {\n> > +\t\tvideo_->releaseBuffers();\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tactiveCamera_ = camera;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +void SimplePipelineHandler::stop(Camera *camera)\n> > +{\n> > +\tvideo_->streamOff();\n> > +\tvideo_->releaseBuffers();\n> > +\tactiveCamera_ = nullptr;\n> > +}\n> > +\n> > +int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n> > +{\n> > +\tSimpleCameraData *data = cameraData(camera);\n> > +\tStream *stream = &data->stream_;\n> \n> As in bufferReady we complete the request as soon as the buffer is\n> complete, should we make sure the request refers to a single stream ?\n\nAs we create a single stream, that's enforced by the Camera class and\nthe Request class. Camera::queueRequest() verifies that all buffers in\nthe request correspond to active streams for the camera, and the request\nclass stores buffers in a map indexed by Stream pointer, so we can't\nhave two buffers for the same stream.\n\n> > +\n> > +\tFrameBuffer *buffer = request->findBuffer(stream);\n> > +\tif (!buffer) {\n> > +\t\tLOG(SimplePipeline, Error)\n> > +\t\t\t<< \"Attempt to queue request with invalid stream\";\n> > +\t\treturn -ENOENT;\n> > +\t}\n> > +\n> > +\treturn video_->queueBuffer(buffer);\n> > +}\n> > +\n> > +/* -----------------------------------------------------------------------------\n> > + * Match and Setup\n> > + */\n> > +\n> > +bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n> > +{\n> > +\tstatic const char * const drivers[] = {\n> > +\t\t\"imx7-csi\",\n> > +\t\t\"sun6i-csi\",\n> > +\t};\n> > +\n> > +\tfor (const char *driver : drivers) {\n> > +\t\tDeviceMatch dm(driver);\n> > +\t\tmedia_ = acquireMediaDevice(enumerator, dm);\n> > +\t\tif (media_)\n> > +\t\t\tbreak;\n> > +\t}\n> > +\n> > +\tif (!media_)\n> > +\t\treturn false;\n> > +\n> > +\t/*\n> > +\t * Locate sensors and video nodes. We only support pipelines with at\n> > +\t * least one sensor and exactly one video captude node.\n> > +\t */\n> > +\tstd::vector sensors;\n> > +\tstd::vector videos;\n> > +\n> > +\tfor (MediaEntity *entity : media_->entities()) {\n> > +\t\tswitch (entity->function()) {\n> > +\t\tcase MEDIA_ENT_F_CAM_SENSOR:\n> > +\t\t\tsensors.push_back(entity);\n> > +\t\t\tbreak;\n> > +\n> > +\t\tcase MEDIA_ENT_F_IO_V4L:\n> > +\t\t\tif (entity->pads().size() == 1 &&\n> \n> Isn't this an arbitrary restinction ? Can't a video node a more than\n> one, maybe not connected, pad ?\n\nNot currently, no. And I want the rest of this code to be able to assume\nthat the video node has a single pad, hence the check here. We will lift\nthat restriction later if needed.\n\n> > +\t\t\t (entity->pads()[0]->flags() & MEDIA_PAD_FL_SINK))\n> > +\t\t\t\tvideos.push_back(entity);\n> > +\t\t\tbreak;\n> > +\n> > +\t\tdefault:\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tif (sensors.empty()) {\n> > +\t\tLOG(SimplePipeline, Info) << \"No sensor found\";\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\tif (videos.size() != 1) {\n> \n> Why a vector ? Cant you populate a single pointer and fail if it is\n> already != nullptr when walking device nodes ?\n\nI could, but I found it clearer this way, the check really says \"is\nthere more than one video node ?\", and goes at the end. Efficiency isn't\nan issue, for all valid pipelines there will be a single video node, and\nthis isn't a hot path. Note that I will need to add at least a null\ncheck after the loop to catch the size() == 0 case, even if I move the\nsize() > 1 case within the loop. I can change it if you think checking\nin the loop is better.\n\n> > +\t\tLOG(SimplePipeline, Info)\n> \n> s/Info/Error here and above\n\nI used Info here as it may not be an error, it could just be that the\nhardware isn't compatible with the simple pipeline handler. But maybe\nthe driver name shouldn't be listed in that case. I'll change to Error.\n\n> > +\t\t\t<< \"Pipeline with \" << videos.size()\n> > +\t\t\t<< \" video capture nodes is not supported\";\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\t/* Locate and open the capture video node. */\n> > +\tvideo_ = new V4L2VideoDevice(videos[0]);\n> \n> If you remove the vector you can use video_ directly\n> \n> > +\tif (video_->open() < 0)\n> > +\t\treturn false;\n> > +\n> > +\tif (video_->caps().isMultiplanar()) {\n> \n> Why not mplane API support ?\n\nIt's just not supported yet. I'm sure there are lots of features that\nwill be added to this pipeline handler over time, this being one of\nthem. I wonder if it will become our most complex pipeline handler,\ndespite its name :-)\n\n> > +\t\tLOG(SimplePipeline, Info)\n> \n> Error here as well ?\n> \n> > +\t\t\t<< \"V4L2 multiplanar devices are not supported\";\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\tvideo_->bufferReady.connect(this, &SimplePipelineHandler::bufferReady);\n> > +\n> > +\t/*\n> > +\t * Create one camera data instance for each sensor and gather all\n> > +\t * entities in all pipelines.\n> > +\t */\n> \n> I might have missed the case for more sensor connected to the same\n> CSI-2 receiver..\n\nYes, there are platform with two sensors connected to the same CSI-2\nreceiver (or sometimes to two CSI-2 receivers that are then connected to\na video mux).\n\n> > +\tstd::vector> pipelines;\n> > +\tstd::set entities;\n> > +\n> > +\tpipelines.reserve(sensors.size());\n> > +\n> > +\tfor (MediaEntity *sensor : sensors) {\n> > +\t\tstd::unique_ptr data =\n> > +\t\t\tstd::make_unique(this, sensor,\n> > +\t\t\t\t\t\t\t videos[0]);\n> > +\t\tif (!data->isValid()) {\n> > +\t\t\tLOG(SimplePipeline, Info)\n> \n> seems like using Info in place of Error is intentional\n>\n> Quite some media entities, links, pads dancing in this pipeline :)\n> Good job!\n\nThanks :-) And thank you for the detailed review !\n\n> > +\t\t\t\t<< \"No valid pipeline for sensor '\"\n> > +\t\t\t\t<< sensor->name() << \"', skipping\";\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > +\n> > +\t\tfor (SimpleCameraData::Entity &entity : data->entities_)\n> > +\t\t\tentities.insert(entity.entity);\n> > +\n> > +\t\tpipelines.push_back(std::move(data));\n> > +\t}\n> > +\n> > +\tif (entities.empty())\n> > +\t\treturn false;\n> > +\n> > +\t/* Create and open V4L2Subdev instances for all the entities. */\n> > +\tfor (MediaEntity *entity : entities) {\n> > +\t\tauto elem = subdevs_.emplace(std::piecewise_construct,\n> > +\t\t\t\t\t std::forward_as_tuple(entity),\n> > +\t\t\t\t\t std::forward_as_tuple(entity));\n> > +\t\tV4L2Subdevice *subdev = &elem.first->second;\n> > +\t\tint ret = subdev->open();\n> > +\t\tif (ret < 0) {\n> > +\t\t\tLOG(SimplePipeline, Error)\n> > +\t\t\t\t<< \"Failed to open \" << subdev->deviceNode()\n> > +\t\t\t\t<< \": \" << strerror(-ret);\n> > +\t\t\treturn false;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\t/* Initialize each pipeline and register a corresponding camera. */\n> > +\tfor (std::unique_ptr &data : pipelines) {\n> > +\t\tint ret = data->init();\n> > +\t\tif (ret < 0)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tstd::shared_ptr camera =\n> > +\t\t\tCamera::create(this, data->sensor_->entity()->name(),\n> > +\t\t\t\t data->streams());\n> > +\t\tregisterCamera(std::move(camera), std::move(data));\n> > +\t}\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > +V4L2Subdevice *SimplePipelineHandler::subdev(const MediaEntity *entity)\n> > +{\n> > +\tauto iter = subdevs_.find(entity);\n> > +\tif (iter == subdevs_.end())\n> > +\t\treturn nullptr;\n> > +\n> > +\treturn &iter->second;\n> > +}\n> > +\n> > +/* -----------------------------------------------------------------------------\n> > + * Buffer Handling\n> > + */\n> > +\n> > +void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n> > +{\n> > +\tASSERT(activeCamera_);\n> > +\tRequest *request = buffer->request();\n> > +\tcompleteBuffer(activeCamera_, request, buffer);\n> > +\tcompleteRequest(activeCamera_, request);\n> > +}\n> > +\n> > +REGISTER_PIPELINE_HANDLER(SimplePipelineHandler);\n> > +\n> > +} /* namespace libcamera */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2AD7062923\n\tfor ;\n\tTue, 17 Mar 2020 21:56:09 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 748D4F9;\n\tTue, 17 Mar 2020 21:56:08 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584478568;\n\tbh=V8+ILcKh72HgOMfFeCW6csD+DT5Si5C/b52KeKs8E+8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bHdRWO6Wx1t7eRP95B4MKD45NedBWX07KDZf+d1TRv/wIKD20UzdjS4B0Pyxk41ta\n\tGCo2HfCjTM5JgiL24QUb4T+gCzH288oBjd1HSr9T4FbWLmSyadkGuqXI1ah7I40uFX\n\tnalKW45AzvuwF+ErVPuZvAk4k4qv0hxNxUCT/lbg=","Date":"Tue, 17 Mar 2020 22:56:03 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, Martijn Braam , \n\tMickael GUENE ,\n\tBenjamin GAIGNARD ","Message-ID":"<20200317205603.GH2527@pendragon.ideasonboard.com>","References":"<20200316214310.27665-1-laurent.pinchart@ideasonboard.com>\n\t<20200316214310.27665-9-laurent.pinchart@ideasonboard.com>\n\t<20200317141812.tyrusaekmqxw3ac2@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200317141812.tyrusaekmqxw3ac2@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 08/10] libcamera: pipeline: Add a\n\tsimple pipeline handler","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","X-List-Received-Date":"Tue, 17 Mar 2020 20:56:09 -0000"}}]