[libcamera-devel,v2,10/10] libcamera: pipeline: simple: Integrate converter support

Message ID 20200316214310.27665-11-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • Simple pipeline handler
Related show

Commit Message

Laurent Pinchart March 16, 2020, 9:43 p.m. UTC
Add support for an optional format converter, supported by the
SimpleConverter class. If a converter is available for the pipeline, it
will be used to expose additional pixel formats.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/pipeline/simple/simple.cpp | 192 ++++++++++++++++++++---
 1 file changed, 174 insertions(+), 18 deletions(-)

Comments

Andrey Konovalov March 19, 2020, 10:07 p.m. UTC | #1
Hi Laurent,

On 17.03.2020 00:43, Laurent Pinchart wrote:
> Add support for an optional format converter, supported by the
> SimpleConverter class. If a converter is available for the pipeline, it
> will be used to expose additional pixel formats.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   src/libcamera/pipeline/simple/simple.cpp | 192 ++++++++++++++++++++---
>   1 file changed, 174 insertions(+), 18 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 2126799c54eb..218b28e70eb2 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp

<snip>

> +struct SimplePipelineInfo {
> +	const char *driver;
> +	const char *converter;
> +};

<snip>

> @@ -550,16 +630,21 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
>   
>   bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
>   {
> -	static const char * const drivers[] = {
> -		"imx7-csi",
> -		"sun6i-csi",
> +	static const SimplePipelineInfo infos[] = {
> +		{ "imx7-csi", "pxp" },
> +		{ "sun6i-csi", nullptr },
>   	};
>   
> -	for (const char *driver : drivers) {
> -		DeviceMatch dm(driver);
> +	MediaDevice *converter;
> +
> +	for (const SimplePipelineInfo &info : infos) {
> +		DeviceMatch dm(info.driver);
>   		media_ = acquireMediaDevice(enumerator, dm);
> -		if (media_)
> +		if (media_) {
> +			DeviceMatch converterMatch(info.converter);

info.converter being equal to nullptr here results in:

   what():  basic_string::_M_construct null not valid
Aborted

- as DeviceMatch constructor accepts const std::string.

Maybe use { "sun6i-csi", "" } instead of { "sun6i-csi", nullptr }?


Thanks,
Andrey
Laurent Pinchart March 19, 2020, 11:54 p.m. UTC | #2
Hi Andrey,

On Fri, Mar 20, 2020 at 01:07:06AM +0300, Andrey Konovalov wrote:
> On 17.03.2020 00:43, Laurent Pinchart wrote:
> > Add support for an optional format converter, supported by the
> > SimpleConverter class. If a converter is available for the pipeline, it
> > will be used to expose additional pixel formats.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >   src/libcamera/pipeline/simple/simple.cpp | 192 ++++++++++++++++++++---
> >   1 file changed, 174 insertions(+), 18 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 2126799c54eb..218b28e70eb2 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> 
> <snip>
> 
> > +struct SimplePipelineInfo {
> > +	const char *driver;
> > +	const char *converter;
> > +};
> 
> <snip>
> 
> > @@ -550,16 +630,21 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
> >   
> >   bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> >   {
> > -	static const char * const drivers[] = {
> > -		"imx7-csi",
> > -		"sun6i-csi",
> > +	static const SimplePipelineInfo infos[] = {
> > +		{ "imx7-csi", "pxp" },
> > +		{ "sun6i-csi", nullptr },
> >   	};
> >   
> > -	for (const char *driver : drivers) {
> > -		DeviceMatch dm(driver);
> > +	MediaDevice *converter;
> > +
> > +	for (const SimplePipelineInfo &info : infos) {
> > +		DeviceMatch dm(info.driver);
> >   		media_ = acquireMediaDevice(enumerator, dm);
> > -		if (media_)
> > +		if (media_) {
> > +			DeviceMatch converterMatch(info.converter);
> 
> info.converter being equal to nullptr here results in:
> 
>    what():  basic_string::_M_construct null not valid
> Aborted
> 
> - as DeviceMatch constructor accepts const std::string.
> 
> Maybe use { "sun6i-csi", "" } instead of { "sun6i-csi", nullptr }?

Oops :-/ We could do that, but while testing I've received a warning
from some versions of gcc that the converter variable could be used
uninitialized. I think that's a false positive, but reworking the code
to support nullptr helps fixing that, so I'll go that way.

Patch

diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 2126799c54eb..218b28e70eb2 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -11,6 +11,7 @@ 
 #include <list>
 #include <map>
 #include <memory>
+#include <queue>
 #include <set>
 #include <string>
 #include <string.h>
@@ -31,12 +32,19 @@ 
 #include "v4l2_subdevice.h"
 #include "v4l2_videodevice.h"
 
+#include "converter.h"
+
 namespace libcamera {
 
 LOG_DEFINE_CATEGORY(SimplePipeline)
 
 class SimplePipelineHandler;
 
+struct SimplePipelineInfo {
+	const char *driver;
+	const char *converter;
+};
+
 class SimpleCameraData : public CameraData
 {
 public:
@@ -79,6 +87,8 @@  public:
 
 	const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
 
+	bool needConversion() const { return needConversion_; }
+
 private:
 	/*
 	 * The SimpleCameraData instance is guaranteed to be valid as long as
@@ -89,6 +99,7 @@  private:
 	const SimpleCameraData *data_;
 
 	V4L2SubdeviceFormat sensorFormat_;
+	bool needConversion_;
 };
 
 class SimplePipelineHandler : public PipelineHandler
@@ -111,6 +122,7 @@  public:
 
 	V4L2VideoDevice *video() { return video_; }
 	V4L2Subdevice *subdev(const MediaEntity *entity);
+	SimpleConverter *converter() { return converter_; }
 
 protected:
 	int queueRequestDevice(Camera *camera, Request *request) override;
@@ -127,11 +139,17 @@  private:
 	int createCamera(MediaEntity *sensor);
 
 	void bufferReady(FrameBuffer *buffer);
+	void converterDone(FrameBuffer *input, FrameBuffer *output);
 
 	MediaDevice *media_;
 	V4L2VideoDevice *video_;
 	std::map<const MediaEntity *, V4L2Subdevice> subdevs_;
 
+	SimpleConverter *converter_;
+	bool useConverter_;
+	std::vector<std::unique_ptr<FrameBuffer>> converterBuffers_;
+	std::queue<FrameBuffer *> converterQueue_;
+
 	Camera *activeCamera_;
 };
 
@@ -209,6 +227,7 @@  int SimpleCameraData::init()
 {
 	SimplePipelineHandler *pipe = static_cast<SimplePipelineHandler *>(pipe_);
 	V4L2VideoDevice *video = pipe->video();
+	SimpleConverter *converter = pipe->converter();
 	int ret;
 
 	/*
@@ -258,7 +277,13 @@  int SimpleCameraData::init()
 			config.pixelFormat = pixelFormat;
 			config.size = format.size;
 
-			formats_[pixelFormat] = config;
+			if (!converter) {
+				formats_[pixelFormat] = config;
+				continue;
+			}
+
+			for (PixelFormat format : converter->formats(pixelFormat))
+				formats_[format] = config;
 		}
 	}
 
@@ -385,6 +410,8 @@  CameraConfiguration::Status SimpleCameraConfiguration::validate()
 		status = Adjusted;
 	}
 
+	needConversion_ = cfg.pixelFormat != pipeConfig.pixelFormat;
+
 	if (cfg.size != pipeConfig.size) {
 		LOG(SimplePipeline, Debug)
 			<< "Adjusting size from " << cfg.size.toString()
@@ -403,13 +430,14 @@  CameraConfiguration::Status SimpleCameraConfiguration::validate()
  */
 
 SimplePipelineHandler::SimplePipelineHandler(CameraManager *manager)
-	: PipelineHandler(manager), video_(nullptr)
+	: PipelineHandler(manager), video_(nullptr), converter_(nullptr)
 {
 }
 
 SimplePipelineHandler::~SimplePipelineHandler()
 {
 	delete video_;
+	delete converter_;
 }
 
 CameraConfiguration *SimplePipelineHandler::generateConfiguration(Camera *camera,
@@ -473,22 +501,37 @@  int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
 		return ret;
 
 	/* Configure the video node. */
-	uint32_t fourcc = video_->toV4L2Fourcc(cfg.pixelFormat);
+	uint32_t fourcc = video_->toV4L2Fourcc(pipeConfig.pixelFormat);
 
-	V4L2DeviceFormat outputFormat = {};
-	outputFormat.fourcc = fourcc;
-	outputFormat.size = cfg.size;
+	V4L2DeviceFormat captureFormat = {};
+	captureFormat.fourcc = fourcc;
+	captureFormat.size = cfg.size;
 
-	ret = video_->setFormat(&outputFormat);
+	ret = video_->setFormat(&captureFormat);
 	if (ret)
 		return ret;
 
-	if (outputFormat.size != cfg.size || outputFormat.fourcc != fourcc) {
+	if (captureFormat.fourcc != fourcc || captureFormat.size != cfg.size) {
 		LOG(SimplePipeline, Error)
 			<< "Unable to configure capture in " << cfg.toString();
 		return -EINVAL;
 	}
 
+	/* Configure the converter if required. */
+	useConverter_ = config->needConversion();
+
+	if (useConverter_) {
+		int ret = converter_->configure(pipeConfig.pixelFormat,
+						cfg.pixelFormat, cfg.size);
+		if (ret < 0) {
+			LOG(SimplePipeline, Error)
+				<< "Unable to configure converter";
+			return ret;
+		}
+
+		LOG(SimplePipeline, Debug) << "Using format converter";
+	}
+
 	cfg.setStream(&data->stream_);
 
 	return 0;
@@ -499,24 +542,47 @@  int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,
 {
 	unsigned int count = stream->configuration().bufferCount;
 
-	return video_->exportBuffers(count, buffers);
+	/*
+	 * Export buffers on the converter or capture video node, depending on
+	 * whether the converter is used or not.
+	 */
+	if (useConverter_)
+		return converter_->exportBuffers(count, buffers);
+	else
+		return video_->exportBuffers(count, buffers);
 }
 
 int SimplePipelineHandler::start(Camera *camera)
 {
 	SimpleCameraData *data = cameraData(camera);
 	unsigned int count = data->stream_.configuration().bufferCount;
+	int ret;
 
-	int ret = video_->importBuffers(count);
+	if (useConverter_)
+		ret = video_->allocateBuffers(count, &converterBuffers_);
+	else
+		ret = video_->importBuffers(count);
 	if (ret < 0)
 		return ret;
 
 	ret = video_->streamOn();
 	if (ret < 0) {
-		video_->releaseBuffers();
+		stop(camera);
 		return ret;
 	}
 
+	if (useConverter_) {
+		ret = converter_->start(count);
+		if (ret < 0) {
+			stop(camera);
+			return ret;
+		}
+
+		/* Queue all internal buffers for capture. */
+		for (std::unique_ptr<FrameBuffer> &buffer : converterBuffers_)
+			video_->queueBuffer(buffer.get());
+	}
+
 	activeCamera_ = camera;
 
 	return 0;
@@ -524,8 +590,13 @@  int SimplePipelineHandler::start(Camera *camera)
 
 void SimplePipelineHandler::stop(Camera *camera)
 {
+	if (useConverter_)
+		converter_->stop();
+
 	video_->streamOff();
 	video_->releaseBuffers();
+
+	converterBuffers_.clear();
 	activeCamera_ = nullptr;
 }
 
@@ -541,6 +612,15 @@  int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
 		return -ENOENT;
 	}
 
+	/*
+	 * If conversion is needed, push the buffer to the converter queue, it
+	 * will be handed to the converter in the capture completion handler.
+	 */
+	if (useConverter_) {
+		converterQueue_.push(buffer);
+		return 0;
+	}
+
 	return video_->queueBuffer(buffer);
 }
 
@@ -550,16 +630,21 @@  int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
 
 bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
 {
-	static const char * const drivers[] = {
-		"imx7-csi",
-		"sun6i-csi",
+	static const SimplePipelineInfo infos[] = {
+		{ "imx7-csi", "pxp" },
+		{ "sun6i-csi", nullptr },
 	};
 
-	for (const char *driver : drivers) {
-		DeviceMatch dm(driver);
+	MediaDevice *converter;
+
+	for (const SimplePipelineInfo &info : infos) {
+		DeviceMatch dm(info.driver);
 		media_ = acquireMediaDevice(enumerator, dm);
-		if (media_)
+		if (media_) {
+			DeviceMatch converterMatch(info.converter);
+			converter = acquireMediaDevice(enumerator, converterMatch);
 			break;
+		}
 	}
 
 	if (!media_)
@@ -614,6 +699,19 @@  bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
 
 	video_->bufferReady.connect(this, &SimplePipelineHandler::bufferReady);
 
+	/* Open the converter, if any. */
+	if (converter) {
+		converter_ = new SimpleConverter(converter);
+		if (converter_->open() < 0) {
+			LOG(SimplePipeline, Warning)
+				<< "Failed to open converter, disabling format conversion";
+			delete converter_;
+			converter_ = nullptr;
+		}
+
+		converter_->bufferReady.connect(this, &SimplePipelineHandler::converterDone);
+	}
+
 	/*
 	 * Create one camera data instance for each sensor and gather all
 	 * entities in all pipelines.
@@ -688,12 +786,70 @@  V4L2Subdevice *SimplePipelineHandler::subdev(const MediaEntity *entity)
 
 void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)
 {
-	ASSERT(activeCamera_);
+	/*
+	 * If an error occurred during capture, or if the buffer was cancelled,
+	 * complete the request, even if the converter is in use as there's no
+	 * point converting an erroneous buffer.
+	 */
+	if (buffer->metadata().status != FrameMetadata::FrameSuccess) {
+		if (useConverter_) {
+			/* Requeue the buffer for capture. */
+			video_->queueBuffer(buffer);
+
+			/*
+			 * Get the next user-facing buffer to complete the
+			 * request.
+			 */
+			if (converterQueue_.empty())
+				return;
+
+			buffer = converterQueue_.front();
+			converterQueue_.pop();
+		}
+
+		Request *request = buffer->request();
+		completeBuffer(activeCamera_, request, buffer);
+		completeRequest(activeCamera_, request);
+		return;
+	}
+
+	/*
+	 * Queue the captured and the request buffer to the converter if format
+	 * conversion is needed. If there's no queued request, just requeue the
+	 * captured buffer for capture.
+	 */
+	if (useConverter_) {
+		if (converterQueue_.empty()) {
+			video_->queueBuffer(buffer);
+			return;
+		}
+
+		FrameBuffer *output = converterQueue_.front();
+		converterQueue_.pop();
+
+		converter_->queueBuffers(buffer, output);
+		return;
+	}
+
+	/* Otherwise simply complete the request. */
 	Request *request = buffer->request();
 	completeBuffer(activeCamera_, request, buffer);
 	completeRequest(activeCamera_, request);
 }
 
+void SimplePipelineHandler::converterDone(FrameBuffer *input,
+					  FrameBuffer *output)
+{
+	/* Complete the request. */
+	ASSERT(activeCamera_);
+	Request *request = output->request();
+	completeBuffer(activeCamera_, request, output);
+	completeRequest(activeCamera_, request);
+
+	/* Queue the input buffer back for capture. */
+	video_->queueBuffer(input);
+}
+
 REGISTER_PIPELINE_HANDLER(SimplePipelineHandler);
 
 } /* namespace libcamera */