[libcamera-devel] [PATCH v3 09/11] libcamera: pipeline: Add a simple pipeline handler

Andrey Konovalov andrey.konovalov at linaro.org
Fri Apr 3 15:13:35 CEST 2020


Hi,

On 31.03.2020 14:45, Kieran Bingham wrote:
> Hi Laurent (/Martijn...)
> 
> 
> 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)
> 
> --
> Kieran
> 
> 
> 
>> ---
>> 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

<snip>

>> +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've taken a deeper look at the ov5640 + db410c logs I've received, and the above
return *could* be a potential problem, but definitely not in this (ov5640 + db410c)
case.

We only saw SimpleCameraData::setupFormats() returning error due to failing
sensor_->setFormat(format) call.
This happened if before the cam is started (and the stream cfg isn't set to a
particular width*height) the sensor is configured for the frame rate greater than 15 fps.
In particular, in the ov5640's driver probe function the initial mode is set to
"YUV422 UYVY VGA at 30fps".
ov5640_enum_frame_size() doesn't consider the current fps at all, so the maximum
frame size reported is 2592x1944. But it only works at 15fps max, and an attempt
to set the sensor format to a one with the maximum possible frame size of 2592x1944
results in -EINVAL returned by SUBDEV_S_FMT.
(So in our ov5640 + db410c case replacing the "return" with "continue" does let the pipeline
handler to try all the other media bus codes if the first one fails, but they all fail the
same way as the first one does.)

If this issue with the frame rate is worked around in some way, the enumeration
of the possible pipeline configurations completes kind of OK: a good configuration
is selected in the end. But during the enumeration all the configurations, the
broken ones included are considered as the working ones:

The links are set up this way ("entity name":pad_number]):

"ov5640 4-0076":0 -> "msm_csiphy0":0,"msm_csiphy0":1 -> "msm_csid0":0,"msm_csid0":1 ->
	-> "msm_ispif0":0,"msm_ispif0":1 -> "msm_vfe0_rdi0":0


SimpleCameraData::setupFormats() creates a broken pipeline when it tries the sensor format
not supported by the rest of the entities. This results in e.g. the following:

"ov5640 4-0076":0[MEDIA_BUS_FMT_RGB565_2X8_BE] -> [MEDIA_BUS_FMT_UYVY8_2X8]"msm_csiphy0":0,
    "msm_csiphy0":1[MEDIA_BUS_FMT_UYVY8_2X8] -> [MEDIA_BUS_FMT_UYVY8_2X8]"msm_csid0":0, etc

I.e. an attempt to set fmt on "msm_csiphy0":0 to MEDIA_BUS_FMT_RGB565_2X8_BE results in the
format set to MEDIA_BUS_FMT_UYVY8_2X8 and no error is returned here (and down the pipeline
as MEDIA_BUS_FMT_UYVY8_2X8 is set OK for all the other pads). So we get the first link misconfigured,
but the result is considered as a good pipeline as no error was returned while the format
from the sensor output was propagated from the sensor output down to "msm_vfe0_rdi0":0.
And the broken configuration is stored in the formats_ map (in the end, it works in our
ov5640+db410c case as the broken configurations are rewritten with the valid ones later).

Who/what is to ensure that for each of the links, the source and the sink pads of the same link
are configured to the same format?

Thanks,
Andrey

>> +		}
>> +
>> +		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/ ?
> 
>> +		 * 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.
> 
> 
>> +		 */
>> +		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.
> 
> 
>> +	 */
>> +	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()) {
> 
>> +			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).
> 
>> +		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?
> 
>> +	 */
>> +	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)
> 
> 
>> +		return ret;
>> +	}
>> +
>> +	activeCamera_ = camera;
> 
> Will simple pipeline handler support more than one camera?
> 
> 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)
> 
> 
>> +
>> +	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)
> 
>> +		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).
> 
> 
>> +		V4L2Subdevice *subdev = &elem.first->second;
> 
> The first second. Well I really do love pairs :-)
> 
>> +		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 */
>>
> 


More information about the libcamera-devel mailing list