[libcamera-devel,v3,06/10] libcamera: pipeline: raspberrypi: Rework stream buffer logic for zero-copy

8844 diff mbox series

Stop using v4l2_videodevice::allocateBuffer() for internal buffers and
instead export/import all buffers. This allows the pipeline to return
any stream buffer requested by the application as zero-copy.

Advertise the Unicam Image stream as the RAW capture stream now.

The RPiStream object now maintains a new list of buffers that are
available to queue into a device. This is needed to distinguish between
FrameBuffers allocated for internal use vs externally provided buffers.
When a Request comes in, if a buffer is not provided for an exported
stream, we re-use a buffer from this list.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 228 ++++++++++--------
 .../pipeline/raspberrypi/rpi_stream.cpp       | 122 +++++++---
 .../pipeline/raspberrypi/rpi_stream.h         |  30 ++-
 3 files changed, 226 insertions(+), 154 deletions(-)

Hi Naushir,

Thanks for your work.

On 2020-07-17 09:54:06 +0100, Naushir Patuck wrote:
> Stop using v4l2_videodevice::allocateBuffer() for internal buffers and
> instead export/import all buffers. This allows the pipeline to return
> any stream buffer requested by the application as zero-copy.
> 
> Advertise the Unicam Image stream as the RAW capture stream now.
> 
> The RPiStream object now maintains a new list of buffers that are
> available to queue into a device. This is needed to distinguish between
> FrameBuffers allocated for internal use vs externally provided buffers.
> When a Request comes in, if a buffer is not provided for an exported
> stream, we re-use a buffer from this list.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

I really like this patch. I'm no expert on the raspberrypi pipeline so I 
think it needs a second review of someone who knows it better then me.  
But save a small nit bellow I see nothing that is odd, so with the nit 
fixed,

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 228 ++++++++++--------
>  .../pipeline/raspberrypi/rpi_stream.cpp       | 122 +++++++---
>  .../pipeline/raspberrypi/rpi_stream.h         |  30 ++-
>  3 files changed, 226 insertions(+), 154 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index ca8434a3..f63bf497 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -199,7 +199,8 @@ private:
>  	void checkRequestCompleted();
>  	void tryRunPipeline();
>  	void tryFlushQueues();
> -	FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp, V4L2VideoDevice *dev);
> +	FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,
> +				 RPi::RPiStream *stream);
>  
>  	unsigned int ispOutputCount_;
>  };
> @@ -520,8 +521,15 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  		StreamConfiguration &cfg = config->at(i);
>  
>  		if (isRaw(cfg.pixelFormat)) {
> -			cfg.setStream(&data->isp_[Isp::Input]);
> -			data->isp_[Isp::Input].setExternal(true);
> +			cfg.setStream(&data->unicam_[Unicam::Image]);
> +			/*
> +			 * We must set both Unicam streams as external, even
> +			 * though the application may only request RAW frames.
> +			 * This is because we match timestamps on both streams
> +			 * to synchronise buffers.
> +			 */
> +			data->unicam_[Unicam::Image].setExternal(true);
> +			data->unicam_[Unicam::Embedded].setExternal(true);
>  			continue;
>  		}
>  
> @@ -624,7 +632,7 @@ int PipelineHandlerRPi::exportFrameBuffers(Camera *camera, Stream *stream,
>  	unsigned int count = stream->configuration().bufferCount;
>  	int ret = s->dev()->exportBuffers(count, buffers);
>  
> -	s->setExternalBuffers(buffers);
> +	s->setExportedBuffers(buffers);
>  
>  	return ret;
>  }
> @@ -724,14 +732,23 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
>  	if (data->state_ == RPiCameraData::State::Stopped)
>  		return -EINVAL;
>  
> -	/* Ensure all external streams have associated buffers! */
> -	for (auto &stream : data->isp_) {
> -		if (!stream.isExternal())
> -			continue;
> +	LOG(RPI, Debug) << "queueRequestDevice: New request.";
>  
> -		if (!request->findBuffer(&stream)) {
> -			LOG(RPI, Error) << "Attempt to queue request with invalid stream.";
> -			return -ENOENT;
> +	/* Push all buffers supplied in the Request to the respective streams. */
> +	for (auto stream : data->streams_) {
> +		if (stream->isExternal()) {
> +			FrameBuffer *buffer = request->findBuffer(stream);
> +			/*
> +			 * A nullptr in buffer means that we should queue an internally
> +			 * allocated buffer.
> +			 *
> +			 * The below queueBuffer() call will do nothing if there are not
> +			 * enough internal buffers allocated, but this will be handled by
> +			 * queuing the request for buffers in the RPiStream object.
> +			 */
> +			int ret = stream->queueBuffer(buffer);
> +			if (ret)
> +				return ret;
>  		}
>  	}
>  
> @@ -820,12 +837,10 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
>  	/* Initialize the camera properties. */
>  	data->properties_ = data->sensor_->properties();
>  
> -	/*
> -	 * List the available output streams.
> -	 * Currently cannot do Unicam streams!
> -	 */
> +	/*List the available streams an application may request. */
>  	std::set<Stream *> streams;
> -	streams.insert(&data->isp_[Isp::Input]);
> +	streams.insert(&data->unicam_[Unicam::Image]);
> +	streams.insert(&data->unicam_[Unicam::Embedded]);
>  	streams.insert(&data->isp_[Isp::Output0]);
>  	streams.insert(&data->isp_[Isp::Output1]);
>  	streams.insert(&data->isp_[Isp::Stats]);
> @@ -843,9 +858,28 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
>  	int ret;
>  
>  	for (auto const stream : data->streams_) {
> -		ret = stream->queueBuffers();
> -		if (ret < 0)
> -			return ret;
> +		if (!stream->isExternal()) {
> +			ret = stream->queueAllBuffers();
> +			if (ret < 0)
> +				return ret;
> +		} else {
> +			/*
> +			 * For external streams, we must queue up a set of internal
> +			 * buffers to handle the number of drop frames requested by
> +			 * the IPA. This is done by passing nullptr in queueBuffer().
> +			 *
> +			 * The below queueBuffer() call will do nothing if there
> +			 * are not enough internal buffers allocated, but this will
> +			 * be handled by queuing the request for buffers in the
> +			 * RPiStream object.
> +			 */
> +			unsigned int i;
> +			for (i = 0; i < data->dropFrameCount_; i++) {
> +				int ret = stream->queueBuffer(nullptr);
> +				if (ret)
> +					return ret;
> +			}
> +		}
>  	}
>  
>  	return 0;
> @@ -859,7 +893,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  	/*
>  	 * Decide how many internal buffers to allocate. For now, simply look
>  	 * at how many external buffers will be provided. Will need to improve
> -	 * this logic.
> +	 * this logic. However, we really must have all stream allocate the same
> +	 * number of buffers to simplify error handling in queueRequestDevice().
>  	 */
>  	unsigned int maxBuffers = 0;
>  	for (const Stream *s : camera->streams())
> @@ -867,33 +902,9 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  			maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
>  
>  	for (auto const stream : data->streams_) {
> -		if (stream->isExternal() || stream->isImporter()) {
> -			/*
> -			 * If a stream is marked as external reserve memory to
> -			 * prepare to import as many buffers are requested in
> -			 * the stream configuration.
> -			 *
> -			 * If a stream is an internal stream with importer
> -			 * role, reserve as many buffers as possible.
> -			 */
> -			unsigned int count = stream->isExternal()
> -						     ? stream->configuration().bufferCount
> -						     : maxBuffers;
> -			ret = stream->importBuffers(count);
> -			if (ret < 0)
> -				return ret;
> -		} else {
> -			/*
> -			 * If the stream is an internal exporter allocate and
> -			 * export as many buffers as possible to its internal
> -			 * pool.
> -			 */
> -			ret = stream->allocateBuffers(maxBuffers);
> -			if (ret < 0) {
> -				freeBuffers(camera);
> -				return ret;
> -			}
> -		}
> +		ret = stream->prepareBuffers(maxBuffers);
> +		if (ret < 0)
> +			return ret;
>  	}
>  
>  	/*
> @@ -901,7 +912,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  	 * the IPA and RPI_IPA_EVENT_SIGNAL_ISP_PREPARE event.
>  	 */
>  	count = 0;
> -	for (auto const &b : *data->unicam_[Unicam::Image].getBuffers()) {
> +	for (auto const &b : data->unicam_[Unicam::Image].getBuffers()) {
>  		b->setCookie(count++);
>  	}
>  
> @@ -910,14 +921,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  	 * the IPA.
>  	 */
>  	count = 0;
> -	for (auto const &b : *data->isp_[Isp::Stats].getBuffers()) {
> +	for (auto const &b : data->isp_[Isp::Stats].getBuffers()) {
>  		b->setCookie(count++);
>  		data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b->cookie(),
>  					      .planes = b->planes() });
>  	}
>  
>  	count = 0;
> -	for (auto const &b : *data->unicam_[Unicam::Embedded].getBuffers()) {
> +	for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {
>  		b->setCookie(count++);
>  		data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b->cookie(),
>  					      .planes = b->planes() });
> @@ -1089,7 +1100,7 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
>  	switch (action.operation) {
>  	case RPI_IPA_ACTION_STATS_METADATA_COMPLETE: {
>  		unsigned int bufferId = action.data[0];
> -		FrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get();
> +		FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
>  
>  		handleStreamBuffer(buffer, &isp_[Isp::Stats]);
>  		/* Fill the Request metadata buffer with what the IPA has provided */
> @@ -1100,19 +1111,19 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
>  
>  	case RPI_IPA_ACTION_EMBEDDED_COMPLETE: {
>  		unsigned int bufferId = action.data[0];
> -		FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers()->at(bufferId).get();
> +		FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);
>  		handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
>  		break;
>  	}
>  
>  	case RPI_IPA_ACTION_RUN_ISP: {
>  		unsigned int bufferId = action.data[0];
> -		FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers()->at(bufferId).get();
> +		FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
>  
>  		LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << buffer->cookie()
>  				<< ", timestamp: " << buffer->metadata().timestamp;
>  
> -		isp_[Isp::Input].dev()->queueBuffer(buffer);
> +		isp_[Isp::Input].queueBuffer(buffer);
>  		ispOutputCount_ = 0;
>  		break;
>  	}
> @@ -1213,22 +1224,27 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
>  			<< ", buffer id " << buffer->cookie()
>  			<< ", timestamp: " << buffer->metadata().timestamp;
>  
> -	handleStreamBuffer(buffer, stream);
> -
>  	/*
> -	 * Increment the number of ISP outputs generated.
> -	 * This is needed to track dropped frames.
> +	 * ISP statistics buffer must not be re-queued or sent back to the
> +	 * application until after the IPA signals so.
>  	 */
> -	ispOutputCount_++;
> -
> -	/* If this is a stats output, hand it to the IPA now. */
>  	if (stream == &isp_[Isp::Stats]) {
>  		IPAOperationData op;
>  		op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;
>  		op.data = { RPiIpaMask::STATS | buffer->cookie() };
>  		ipa_->processEvent(op);
> +	} else {
> +		/* Any other ISP output can be handed back to the application now. */
> +		handleStreamBuffer(buffer, stream);
>  	}
>  
> +	/*
> +	 * Increment the number of ISP outputs generated.
> +	 * This is needed to track dropped frames.
> +	 */
> +	ispOutputCount_++;
> +
> +
>  	handleState();
>  }
>  
> @@ -1241,8 +1257,12 @@ void RPiCameraData::clearIncompleteRequests()
>  	 */
>  	for (auto const request : requestQueue_) {
>  		for (auto const stream : streams_) {
> -			if (stream->isExternal())
> -				stream->dev()->queueBuffer(request->findBuffer(stream));
> +			if (!stream->isExternal())
> +				continue;
> +
> +			FrameBuffer *buffer = request->findBuffer(stream);
> +			if (buffer)
> +				stream->queueBuffer(buffer);
>  		}
>  	}
>  
> @@ -1271,7 +1291,7 @@ void RPiCameraData::clearIncompleteRequests()
>  			 * Has the buffer already been handed back to the
>  			 * request? If not, do so now.
>  			 */
> -			if (buffer->request())
> +			if (buffer && buffer->request())
>  				pipe_->completeBuffer(camera_, request, buffer);
>  		}
>  
> @@ -1283,30 +1303,24 @@ void RPiCameraData::clearIncompleteRequests()
>  void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)
>  {
>  	if (stream->isExternal()) {
> -		if (!dropFrameCount_) {
> -			Request *request = buffer->request();
> +		Request *request = requestQueue_.front();
> +
> +		if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {
> +			/*
> +			 * Tag the buffer as completed, returning it to the
> +			 * application.
> +			 */
>  			pipe_->completeBuffer(camera_, request, buffer);
> +		} else {
> +			/*
> +			 * This buffer was not part of the Request, so we can
> +			 * recycle it.
> +			 */
> +			stream->returnBuffer(buffer);
>  		}
>  	} else {
> -		/* Special handling for RAW buffer Requests.
> -		 *
> -		 * The ISP input stream is alway an import stream, but if the
> -		 * current Request has been made for a buffer on the stream,
> -		 * simply memcpy to the Request buffer and requeue back to the
> -		 * device.
> -		 */
> -		if (stream == &unicam_[Unicam::Image] && !dropFrameCount_) {
> -			const Stream *rawStream = static_cast<const Stream *>(&isp_[Isp::Input]);
> -			Request *request = requestQueue_.front();
> -			FrameBuffer *raw = request->findBuffer(const_cast<Stream *>(rawStream));
> -			if (raw) {
> -				raw->copyFrom(buffer);
> -				pipe_->completeBuffer(camera_, request, raw);
> -			}
> -		}
> -
> -		/* Simply requeue the buffer. */
> -		stream->dev()->queueBuffer(buffer);
> +		/* Simply re-queue the buffer to the requested stream. */
> +		stream->queueBuffer(buffer);
>  	}
>  }
>  
> @@ -1390,7 +1404,7 @@ void RPiCameraData::tryRunPipeline()
>  	 * current bayer buffer will be removed and re-queued to the driver.
>  	 */
>  	embeddedBuffer = updateQueue(embeddedQueue_, bayerBuffer->metadata().timestamp,
> -				     unicam_[Unicam::Embedded].dev());
> +				     &unicam_[Unicam::Embedded]);
>  
>  	if (!embeddedBuffer) {
>  		LOG(RPI, Debug) << "Could not find matching embedded buffer";
> @@ -1409,7 +1423,7 @@ void RPiCameraData::tryRunPipeline()
>  
>  		embeddedBuffer = embeddedQueue_.front();
>  		bayerBuffer = updateQueue(bayerQueue_, embeddedBuffer->metadata().timestamp,
> -					  unicam_[Unicam::Image].dev());
> +					  &unicam_[Unicam::Image]);
>  
>  		if (!bayerBuffer) {
>  			LOG(RPI, Debug) << "Could not find matching bayer buffer - ending.";
> @@ -1417,11 +1431,7 @@ void RPiCameraData::tryRunPipeline()
>  		}
>  	}
>  
> -	/*
> -	 * Take the first request from the queue and action the IPA.
> -	 * Unicam buffers for the request have already been queued as they come
> -	 * in.
> -	 */
> +	/* Take the first request from the queue and action the IPA. */
>  	Request *request = requestQueue_.front();
>  
>  	/*
> @@ -1433,12 +1443,6 @@ void RPiCameraData::tryRunPipeline()
>  	op.controls = { request->controls() };
>  	ipa_->processEvent(op);
>  
> -	/* Queue up any ISP buffers passed into the request. */
> -	for (auto &stream : isp_) {
> -		if (stream.isExternal())
> -			stream.dev()->queueBuffer(request->findBuffer(&stream));
> -	}
> -
>  	/* Ready to use the buffers, pop them off the queue. */
>  	bayerQueue_.pop();
>  	embeddedQueue_.pop();
> @@ -1468,32 +1472,42 @@ void RPiCameraData::tryFlushQueues()
>  	 * and give a chance for the hardware to return to lock-step. We do have
>  	 * to drop all interim frames.
>  	 */
> -	if (unicam_[Unicam::Image].getBuffers()->size() == bayerQueue_.size() &&
> -	    unicam_[Unicam::Embedded].getBuffers()->size() == embeddedQueue_.size()) {
> +	if (unicam_[Unicam::Image].getBuffers().size() == bayerQueue_.size() &&
> +	    unicam_[Unicam::Embedded].getBuffers().size() == embeddedQueue_.size()) {
> +		/* This cannot happen when Unicam streams are external. */
> +		assert(!unicam_[Unicam::Image].isExternal());
> +
>  		LOG(RPI, Warning) << "Flushing all buffer queues!";
>  
>  		while (!bayerQueue_.empty()) {
> -			unicam_[Unicam::Image].dev()->queueBuffer(bayerQueue_.front());
> +			unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());
>  			bayerQueue_.pop();
>  		}
>  
>  		while (!embeddedQueue_.empty()) {
> -			unicam_[Unicam::Embedded].dev()->queueBuffer(embeddedQueue_.front());
> +			unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());
>  			embeddedQueue_.pop();
>  		}
>  	}
>  }
>  
>  FrameBuffer *RPiCameraData::updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,
> -					V4L2VideoDevice *dev)
> +					RPi::RPiStream *stream)
>  {
> +	/*
> +	 * If the unicam streams are external (both have to the same), then we
> +	 * can only return out the top buffer in the queue, and assume they have
> +	 * been synced by queuing at the same time. We cannot drop these frames,
> +	 * as they may have been provided externally.
> +	 */
>  	while (!q.empty()) {
>  		FrameBuffer *b = q.front();
> -		if (b->metadata().timestamp < timestamp) {
> +		if (!stream->isExternal() && b->metadata().timestamp < timestamp) {
>  			q.pop();
> -			dev->queueBuffer(b);
> -			LOG(RPI, Warning) << "Dropping input frame!";
> -		} else if (b->metadata().timestamp == timestamp) {
> +			stream->queueBuffer(b);
> +			LOG(RPI, Warning) << "Dropping unmatched input frame in stream "
> +					  << stream->name();
> +		} else if (stream->isExternal() || b->metadata().timestamp == timestamp) {
>  			/* The calling function will pop the item from the queue. */
>  			return b;
>  		} else {
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> index 57e5cf72..53a335e3 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> @@ -21,30 +21,20 @@ V4L2VideoDevice *RPiStream::dev() const
>  
>  void RPiStream::setExternal(bool external)
>  {
> +	/* Import streams cannot be external. */
> +	assert(!external || !importOnly_);
>  	external_ = external;
>  }
>  
>  bool RPiStream::isExternal() const
>  {
> -	/*
> -	 * Import streams cannot be external.
> -	 *
> -	 * RAW capture is a special case where we simply copy the RAW
> -	 * buffer out of the request. All other buffer handling happens
> -	 * as if the stream is internal.
> -	 */
> -	return external_ && !importOnly_;
> -}
> -
> -bool RPiStream::isImporter() const
> -{
> -	return importOnly_;
> +	return external_;
>  }
>  
>  void RPiStream::reset()
>  {
>  	external_ = false;
> -	internalBuffers_.clear();
> +	clearBuffers();
>  }
>  
>  std::string RPiStream::name() const
> @@ -52,65 +42,123 @@ std::string RPiStream::name() const
>  	return name_;
>  }
>  
> -void RPiStream::setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> +void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
> -	externalBuffers_ = buffers;
> +	std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_),
> +		       [](std::unique_ptr<FrameBuffer> &b) { return b.get(); });
>  }
>  
> -const std::vector<std::unique_ptr<FrameBuffer>> *RPiStream::getBuffers() const
> +const std::vector<FrameBuffer *> &RPiStream::getBuffers() const
>  {
> -	return external_ ? externalBuffers_ : &internalBuffers_;
> +	return bufferList_;
>  }
>  
>  void RPiStream::releaseBuffers()
>  {
>  	dev_->releaseBuffers();
> -	if (!external_ && !importOnly_)
> -		internalBuffers_.clear();
> +	clearBuffers();
>  }
>  
> -int RPiStream::importBuffers(unsigned int count)
> +int RPiStream::prepareBuffers(unsigned int count)
>  {
> +	int ret;
> +
> +	if (!importOnly_) {
> +		if (count) {
> +			/* Export some frame buffers for internal use. */
> +			ret = dev_->exportBuffers(count, &internalBuffers_);
> +			if (ret < 0)
> +				return ret;
> +
> +			/* Add these exported buffers to the internal/external buffer list. */
> +			setExportedBuffers(&internalBuffers_);
> +
> +			/* Add these buffers to the queue of internal usable buffers. */
> +			for (auto const &buffer : internalBuffers_)
> +				availableBuffers_.push(buffer.get());
> +		}
> +
> +		/* We must import all internal/external exported buffers. */
> +		count = bufferList_.size();
> +	}
> +
>  	return dev_->importBuffers(count);
>  }
>  
> -int RPiStream::allocateBuffers(unsigned int count)
> +int RPiStream::queueAllBuffers()
>  {
> -	return dev_->allocateBuffers(count, &internalBuffers_);
> -}
> +	int ret;
>  
> -int RPiStream::queueBuffers()
> -{
>  	if (external_)
>  		return 0;
>  
> -	for (auto &b : internalBuffers_) {
> -		int ret = dev_->queueBuffer(b.get());
> -		if (ret) {
> -			LOG(RPISTREAM, Error) << "Failed to queue buffers for "
> -					      << name_;
> +	while (!availableBuffers_.empty()) {
> +		ret = queueBuffer(availableBuffers_.front());
> +		if (ret < 0)
>  			return ret;
> -		}
> +
> +		availableBuffers_.pop();
>  	}
>  
>  	return 0;
>  }
>  
> -bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> +int RPiStream::queueBuffer(FrameBuffer *buffer)
> +{
> +	/*
> +	 * A nullptr buffer implies an external stream, but no external
> +	 * buffer has been supplied. So, pick one from the availableBuffers_
> +	 * queue.
> +	 */
> +	if (!buffer) {
> +		if (availableBuffers_.empty()) {
> +			LOG(RPISTREAM, Warning) << "No buffers available for "
> +						<< name_;
> +			return -EINVAL;
> +		}
> +
> +		buffer = availableBuffers_.front();
> +		availableBuffers_.pop();
> +	}
> +
> +	LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie()
> +			      << " for " << name_;
> +
> +	int ret = dev_->queueBuffer(buffer);
> +	if (ret) {
> +		LOG(RPISTREAM, Error) << "Failed to queue buffer for "
> +				      << name_;
> +	}
> +
> +	return ret;
> +}
> +
> +void RPiStream::returnBuffer(FrameBuffer *buffer)
>  {
> -	auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();
> -	auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();
> +	/* This can only be called for external streams. */
> +	assert(external_);
> +
> +	availableBuffers_.push(buffer);
> +}
>  
> +bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> +{
>  	if (importOnly_)
>  		return false;
>  
> -	if (std::find_if(start, end,
> -			 [buffer](std::unique_ptr<FrameBuffer> const &ref) { return ref.get() == buffer; }) != end)
> +	if (std::find(bufferList_.begin(), bufferList_.end(), buffer) != bufferList_.end())
>  		return true;

Cant this be made simpler by ?

    if (bufferList_.find(buffer) != bufferList_.end())

>  
>  	return false;
>  }
>  
> +void RPiStream::clearBuffers()
> +{
> +	availableBuffers_ = std::queue<FrameBuffer *>{};
> +	internalBuffers_.clear();
> +	bufferList_.clear();
> +}
> +
>  } /* namespace RPi */
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> index 40fff81d..019e236d 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> @@ -38,21 +38,22 @@ public:
>  	V4L2VideoDevice *dev() const;
>  	void setExternal(bool external);
>  	bool isExternal() const;
> -	bool isImporter() const;
>  	void reset();
>  	std::string name() const;
> -	void setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> -	const std::vector<std::unique_ptr<FrameBuffer>> *getBuffers() const;
> +	void setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> +	const std::vector<FrameBuffer *> &getBuffers() const;
>  	void releaseBuffers();
> -	int importBuffers(unsigned int count);
> -	int allocateBuffers(unsigned int count);
> -	int queueBuffers();
> +	int prepareBuffers(unsigned int count);
> +	int queueAllBuffers();
> +	int queueBuffer(FrameBuffer *buffer);
> +	void returnBuffer(FrameBuffer *buffer);
>  	bool findFrameBuffer(FrameBuffer *buffer) const;
>  
>  private:
> +	void clearBuffers();
>  	/*
>  	 * Indicates that this stream is active externally, i.e. the buffers
> -	 * are provided by the application.
> +	 * might be provided by (and returned to) the application.
>  	 */
>  	bool external_;
>  	/* Indicates that this stream only imports buffers, e.g. ISP input. */
> @@ -61,10 +62,19 @@ private:
>  	std::string name_;
>  	/* The actual device stream. */
>  	std::unique_ptr<V4L2VideoDevice> dev_;
> -	/* Internally allocated framebuffers associated with this device stream. */
> +	/* All framebuffers associated with this device stream. */
> +	std::vector<FrameBuffer *> bufferList_;
> +	/*
> +	 * List of frame buffer that we can use if none have been provided by
> +	 * the application for external streams. This is populated by the
> +	 * buffers exported internally.
> +	 */
> +	std::queue<FrameBuffer *> availableBuffers_;
> +	/*
> +	 * This is a list of buffers exported internally. Need to keep this around
> +	 * as the stream needs to maintain ownership of these buffers.
> +	 */
>  	std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_;
> -	/* Externally allocated framebuffers associated with this device stream. */
> -	std::vector<std::unique_ptr<FrameBuffer>> *externalBuffers_;
>  };
>  
>  /*
> -- 
> 2.25.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,

Thanks for the review.

On Sat, 18 Jul 2020 at 16:33, Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
>
> Hi Naushir,
>
> Thanks for your work.
>
> On 2020-07-17 09:54:06 +0100, Naushir Patuck wrote:
> > Stop using v4l2_videodevice::allocateBuffer() for internal buffers and
> > instead export/import all buffers. This allows the pipeline to return
> > any stream buffer requested by the application as zero-copy.
> >
> > Advertise the Unicam Image stream as the RAW capture stream now.
> >
> > The RPiStream object now maintains a new list of buffers that are
> > available to queue into a device. This is needed to distinguish between
> > FrameBuffers allocated for internal use vs externally provided buffers.
> > When a Request comes in, if a buffer is not provided for an exported
> > stream, we re-use a buffer from this list.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
>
> I really like this patch. I'm no expert on the raspberrypi pipeline so I
> think it needs a second review of someone who knows it better then me.
> But save a small nit bellow I see nothing that is odd, so with the nit
> fixed,
>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 228 ++++++++++--------
> >  .../pipeline/raspberrypi/rpi_stream.cpp       | 122 +++++++---
> >  .../pipeline/raspberrypi/rpi_stream.h         |  30 ++-
> >  3 files changed, 226 insertions(+), 154 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index ca8434a3..f63bf497 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -199,7 +199,8 @@ private:
> >       void checkRequestCompleted();
> >       void tryRunPipeline();
> >       void tryFlushQueues();
> > -     FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp, V4L2VideoDevice *dev);
> > +     FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,
> > +                              RPi::RPiStream *stream);
> >
> >       unsigned int ispOutputCount_;
> >  };
> > @@ -520,8 +521,15 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >               StreamConfiguration &cfg = config->at(i);
> >
> >               if (isRaw(cfg.pixelFormat)) {
> > -                     cfg.setStream(&data->isp_[Isp::Input]);
> > -                     data->isp_[Isp::Input].setExternal(true);
> > +                     cfg.setStream(&data->unicam_[Unicam::Image]);
> > +                     /*
> > +                      * We must set both Unicam streams as external, even
> > +                      * though the application may only request RAW frames.
> > +                      * This is because we match timestamps on both streams
> > +                      * to synchronise buffers.
> > +                      */
> > +                     data->unicam_[Unicam::Image].setExternal(true);
> > +                     data->unicam_[Unicam::Embedded].setExternal(true);
> >                       continue;
> >               }
> >
> > @@ -624,7 +632,7 @@ int PipelineHandlerRPi::exportFrameBuffers(Camera *camera, Stream *stream,
> >       unsigned int count = stream->configuration().bufferCount;
> >       int ret = s->dev()->exportBuffers(count, buffers);
> >
> > -     s->setExternalBuffers(buffers);
> > +     s->setExportedBuffers(buffers);
> >
> >       return ret;
> >  }
> > @@ -724,14 +732,23 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
> >       if (data->state_ == RPiCameraData::State::Stopped)
> >               return -EINVAL;
> >
> > -     /* Ensure all external streams have associated buffers! */
> > -     for (auto &stream : data->isp_) {
> > -             if (!stream.isExternal())
> > -                     continue;
> > +     LOG(RPI, Debug) << "queueRequestDevice: New request.";
> >
> > -             if (!request->findBuffer(&stream)) {
> > -                     LOG(RPI, Error) << "Attempt to queue request with invalid stream.";
> > -                     return -ENOENT;
> > +     /* Push all buffers supplied in the Request to the respective streams. */
> > +     for (auto stream : data->streams_) {
> > +             if (stream->isExternal()) {
> > +                     FrameBuffer *buffer = request->findBuffer(stream);
> > +                     /*
> > +                      * A nullptr in buffer means that we should queue an internally
> > +                      * allocated buffer.
> > +                      *
> > +                      * The below queueBuffer() call will do nothing if there are not
> > +                      * enough internal buffers allocated, but this will be handled by
> > +                      * queuing the request for buffers in the RPiStream object.
> > +                      */
> > +                     int ret = stream->queueBuffer(buffer);
> > +                     if (ret)
> > +                             return ret;
> >               }
> >       }
> >
> > @@ -820,12 +837,10 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> >       /* Initialize the camera properties. */
> >       data->properties_ = data->sensor_->properties();
> >
> > -     /*
> > -      * List the available output streams.
> > -      * Currently cannot do Unicam streams!
> > -      */
> > +     /*List the available streams an application may request. */
> >       std::set<Stream *> streams;
> > -     streams.insert(&data->isp_[Isp::Input]);
> > +     streams.insert(&data->unicam_[Unicam::Image]);
> > +     streams.insert(&data->unicam_[Unicam::Embedded]);
> >       streams.insert(&data->isp_[Isp::Output0]);
> >       streams.insert(&data->isp_[Isp::Output1]);
> >       streams.insert(&data->isp_[Isp::Stats]);
> > @@ -843,9 +858,28 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
> >       int ret;
> >
> >       for (auto const stream : data->streams_) {
> > -             ret = stream->queueBuffers();
> > -             if (ret < 0)
> > -                     return ret;
> > +             if (!stream->isExternal()) {
> > +                     ret = stream->queueAllBuffers();
> > +                     if (ret < 0)
> > +                             return ret;
> > +             } else {
> > +                     /*
> > +                      * For external streams, we must queue up a set of internal
> > +                      * buffers to handle the number of drop frames requested by
> > +                      * the IPA. This is done by passing nullptr in queueBuffer().
> > +                      *
> > +                      * The below queueBuffer() call will do nothing if there
> > +                      * are not enough internal buffers allocated, but this will
> > +                      * be handled by queuing the request for buffers in the
> > +                      * RPiStream object.
> > +                      */
> > +                     unsigned int i;
> > +                     for (i = 0; i < data->dropFrameCount_; i++) {
> > +                             int ret = stream->queueBuffer(nullptr);
> > +                             if (ret)
> > +                                     return ret;
> > +                     }
> > +             }
> >       }
> >
> >       return 0;
> > @@ -859,7 +893,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >       /*
> >        * Decide how many internal buffers to allocate. For now, simply look
> >        * at how many external buffers will be provided. Will need to improve
> > -      * this logic.
> > +      * this logic. However, we really must have all stream allocate the same
> > +      * number of buffers to simplify error handling in queueRequestDevice().
> >        */
> >       unsigned int maxBuffers = 0;
> >       for (const Stream *s : camera->streams())
> > @@ -867,33 +902,9 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >                       maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
> >
> >       for (auto const stream : data->streams_) {
> > -             if (stream->isExternal() || stream->isImporter()) {
> > -                     /*
> > -                      * If a stream is marked as external reserve memory to
> > -                      * prepare to import as many buffers are requested in
> > -                      * the stream configuration.
> > -                      *
> > -                      * If a stream is an internal stream with importer
> > -                      * role, reserve as many buffers as possible.
> > -                      */
> > -                     unsigned int count = stream->isExternal()
> > -                                                  ? stream->configuration().bufferCount
> > -                                                  : maxBuffers;
> > -                     ret = stream->importBuffers(count);
> > -                     if (ret < 0)
> > -                             return ret;
> > -             } else {
> > -                     /*
> > -                      * If the stream is an internal exporter allocate and
> > -                      * export as many buffers as possible to its internal
> > -                      * pool.
> > -                      */
> > -                     ret = stream->allocateBuffers(maxBuffers);
> > -                     if (ret < 0) {
> > -                             freeBuffers(camera);
> > -                             return ret;
> > -                     }
> > -             }
> > +             ret = stream->prepareBuffers(maxBuffers);
> > +             if (ret < 0)
> > +                     return ret;
> >       }
> >
> >       /*
> > @@ -901,7 +912,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >        * the IPA and RPI_IPA_EVENT_SIGNAL_ISP_PREPARE event.
> >        */
> >       count = 0;
> > -     for (auto const &b : *data->unicam_[Unicam::Image].getBuffers()) {
> > +     for (auto const &b : data->unicam_[Unicam::Image].getBuffers()) {
> >               b->setCookie(count++);
> >       }
> >
> > @@ -910,14 +921,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >        * the IPA.
> >        */
> >       count = 0;
> > -     for (auto const &b : *data->isp_[Isp::Stats].getBuffers()) {
> > +     for (auto const &b : data->isp_[Isp::Stats].getBuffers()) {
> >               b->setCookie(count++);
> >               data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b->cookie(),
> >                                             .planes = b->planes() });
> >       }
> >
> >       count = 0;
> > -     for (auto const &b : *data->unicam_[Unicam::Embedded].getBuffers()) {
> > +     for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {
> >               b->setCookie(count++);
> >               data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b->cookie(),
> >                                             .planes = b->planes() });
> > @@ -1089,7 +1100,7 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> >       switch (action.operation) {
> >       case RPI_IPA_ACTION_STATS_METADATA_COMPLETE: {
> >               unsigned int bufferId = action.data[0];
> > -             FrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get();
> > +             FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
> >
> >               handleStreamBuffer(buffer, &isp_[Isp::Stats]);
> >               /* Fill the Request metadata buffer with what the IPA has provided */
> > @@ -1100,19 +1111,19 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> >
> >       case RPI_IPA_ACTION_EMBEDDED_COMPLETE: {
> >               unsigned int bufferId = action.data[0];
> > -             FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers()->at(bufferId).get();
> > +             FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);
> >               handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> >               break;
> >       }
> >
> >       case RPI_IPA_ACTION_RUN_ISP: {
> >               unsigned int bufferId = action.data[0];
> > -             FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers()->at(bufferId).get();
> > +             FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
> >
> >               LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << buffer->cookie()
> >                               << ", timestamp: " << buffer->metadata().timestamp;
> >
> > -             isp_[Isp::Input].dev()->queueBuffer(buffer);
> > +             isp_[Isp::Input].queueBuffer(buffer);
> >               ispOutputCount_ = 0;
> >               break;
> >       }
> > @@ -1213,22 +1224,27 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> >                       << ", buffer id " << buffer->cookie()
> >                       << ", timestamp: " << buffer->metadata().timestamp;
> >
> > -     handleStreamBuffer(buffer, stream);
> > -
> >       /*
> > -      * Increment the number of ISP outputs generated.
> > -      * This is needed to track dropped frames.
> > +      * ISP statistics buffer must not be re-queued or sent back to the
> > +      * application until after the IPA signals so.
> >        */
> > -     ispOutputCount_++;
> > -
> > -     /* If this is a stats output, hand it to the IPA now. */
> >       if (stream == &isp_[Isp::Stats]) {
> >               IPAOperationData op;
> >               op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;
> >               op.data = { RPiIpaMask::STATS | buffer->cookie() };
> >               ipa_->processEvent(op);
> > +     } else {
> > +             /* Any other ISP output can be handed back to the application now. */
> > +             handleStreamBuffer(buffer, stream);
> >       }
> >
> > +     /*
> > +      * Increment the number of ISP outputs generated.
> > +      * This is needed to track dropped frames.
> > +      */
> > +     ispOutputCount_++;
> > +
> > +
> >       handleState();
> >  }
> >
> > @@ -1241,8 +1257,12 @@ void RPiCameraData::clearIncompleteRequests()
> >        */
> >       for (auto const request : requestQueue_) {
> >               for (auto const stream : streams_) {
> > -                     if (stream->isExternal())
> > -                             stream->dev()->queueBuffer(request->findBuffer(stream));
> > +                     if (!stream->isExternal())
> > +                             continue;
> > +
> > +                     FrameBuffer *buffer = request->findBuffer(stream);
> > +                     if (buffer)
> > +                             stream->queueBuffer(buffer);
> >               }
> >       }
> >
> > @@ -1271,7 +1291,7 @@ void RPiCameraData::clearIncompleteRequests()
> >                        * Has the buffer already been handed back to the
> >                        * request? If not, do so now.
> >                        */
> > -                     if (buffer->request())
> > +                     if (buffer && buffer->request())
> >                               pipe_->completeBuffer(camera_, request, buffer);
> >               }
> >
> > @@ -1283,30 +1303,24 @@ void RPiCameraData::clearIncompleteRequests()
> >  void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)
> >  {
> >       if (stream->isExternal()) {
> > -             if (!dropFrameCount_) {
> > -                     Request *request = buffer->request();
> > +             Request *request = requestQueue_.front();
> > +
> > +             if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {
> > +                     /*
> > +                      * Tag the buffer as completed, returning it to the
> > +                      * application.
> > +                      */
> >                       pipe_->completeBuffer(camera_, request, buffer);
> > +             } else {
> > +                     /*
> > +                      * This buffer was not part of the Request, so we can
> > +                      * recycle it.
> > +                      */
> > +                     stream->returnBuffer(buffer);
> >               }
> >       } else {
> > -             /* Special handling for RAW buffer Requests.
> > -              *
> > -              * The ISP input stream is alway an import stream, but if the
> > -              * current Request has been made for a buffer on the stream,
> > -              * simply memcpy to the Request buffer and requeue back to the
> > -              * device.
> > -              */
> > -             if (stream == &unicam_[Unicam::Image] && !dropFrameCount_) {
> > -                     const Stream *rawStream = static_cast<const Stream *>(&isp_[Isp::Input]);
> > -                     Request *request = requestQueue_.front();
> > -                     FrameBuffer *raw = request->findBuffer(const_cast<Stream *>(rawStream));
> > -                     if (raw) {
> > -                             raw->copyFrom(buffer);
> > -                             pipe_->completeBuffer(camera_, request, raw);
> > -                     }
> > -             }
> > -
> > -             /* Simply requeue the buffer. */
> > -             stream->dev()->queueBuffer(buffer);
> > +             /* Simply re-queue the buffer to the requested stream. */
> > +             stream->queueBuffer(buffer);
> >       }
> >  }
> >
> > @@ -1390,7 +1404,7 @@ void RPiCameraData::tryRunPipeline()
> >        * current bayer buffer will be removed and re-queued to the driver.
> >        */
> >       embeddedBuffer = updateQueue(embeddedQueue_, bayerBuffer->metadata().timestamp,
> > -                                  unicam_[Unicam::Embedded].dev());
> > +                                  &unicam_[Unicam::Embedded]);
> >
> >       if (!embeddedBuffer) {
> >               LOG(RPI, Debug) << "Could not find matching embedded buffer";
> > @@ -1409,7 +1423,7 @@ void RPiCameraData::tryRunPipeline()
> >
> >               embeddedBuffer = embeddedQueue_.front();
> >               bayerBuffer = updateQueue(bayerQueue_, embeddedBuffer->metadata().timestamp,
> > -                                       unicam_[Unicam::Image].dev());
> > +                                       &unicam_[Unicam::Image]);
> >
> >               if (!bayerBuffer) {
> >                       LOG(RPI, Debug) << "Could not find matching bayer buffer - ending.";
> > @@ -1417,11 +1431,7 @@ void RPiCameraData::tryRunPipeline()
> >               }
> >       }
> >
> > -     /*
> > -      * Take the first request from the queue and action the IPA.
> > -      * Unicam buffers for the request have already been queued as they come
> > -      * in.
> > -      */
> > +     /* Take the first request from the queue and action the IPA. */
> >       Request *request = requestQueue_.front();
> >
> >       /*
> > @@ -1433,12 +1443,6 @@ void RPiCameraData::tryRunPipeline()
> >       op.controls = { request->controls() };
> >       ipa_->processEvent(op);
> >
> > -     /* Queue up any ISP buffers passed into the request. */
> > -     for (auto &stream : isp_) {
> > -             if (stream.isExternal())
> > -                     stream.dev()->queueBuffer(request->findBuffer(&stream));
> > -     }
> > -
> >       /* Ready to use the buffers, pop them off the queue. */
> >       bayerQueue_.pop();
> >       embeddedQueue_.pop();
> > @@ -1468,32 +1472,42 @@ void RPiCameraData::tryFlushQueues()
> >        * and give a chance for the hardware to return to lock-step. We do have
> >        * to drop all interim frames.
> >        */
> > -     if (unicam_[Unicam::Image].getBuffers()->size() == bayerQueue_.size() &&
> > -         unicam_[Unicam::Embedded].getBuffers()->size() == embeddedQueue_.size()) {
> > +     if (unicam_[Unicam::Image].getBuffers().size() == bayerQueue_.size() &&
> > +         unicam_[Unicam::Embedded].getBuffers().size() == embeddedQueue_.size()) {
> > +             /* This cannot happen when Unicam streams are external. */
> > +             assert(!unicam_[Unicam::Image].isExternal());
> > +
> >               LOG(RPI, Warning) << "Flushing all buffer queues!";
> >
> >               while (!bayerQueue_.empty()) {
> > -                     unicam_[Unicam::Image].dev()->queueBuffer(bayerQueue_.front());
> > +                     unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());
> >                       bayerQueue_.pop();
> >               }
> >
> >               while (!embeddedQueue_.empty()) {
> > -                     unicam_[Unicam::Embedded].dev()->queueBuffer(embeddedQueue_.front());
> > +                     unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());
> >                       embeddedQueue_.pop();
> >               }
> >       }
> >  }
> >
> >  FrameBuffer *RPiCameraData::updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,
> > -                                     V4L2VideoDevice *dev)
> > +                                     RPi::RPiStream *stream)
> >  {
> > +     /*
> > +      * If the unicam streams are external (both have to the same), then we
> > +      * can only return out the top buffer in the queue, and assume they have
> > +      * been synced by queuing at the same time. We cannot drop these frames,
> > +      * as they may have been provided externally.
> > +      */
> >       while (!q.empty()) {
> >               FrameBuffer *b = q.front();
> > -             if (b->metadata().timestamp < timestamp) {
> > +             if (!stream->isExternal() && b->metadata().timestamp < timestamp) {
> >                       q.pop();
> > -                     dev->queueBuffer(b);
> > -                     LOG(RPI, Warning) << "Dropping input frame!";
> > -             } else if (b->metadata().timestamp == timestamp) {
> > +                     stream->queueBuffer(b);
> > +                     LOG(RPI, Warning) << "Dropping unmatched input frame in stream "
> > +                                       << stream->name();
> > +             } else if (stream->isExternal() || b->metadata().timestamp == timestamp) {
> >                       /* The calling function will pop the item from the queue. */
> >                       return b;
> >               } else {
> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > index 57e5cf72..53a335e3 100644
> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > @@ -21,30 +21,20 @@ V4L2VideoDevice *RPiStream::dev() const
> >
> >  void RPiStream::setExternal(bool external)
> >  {
> > +     /* Import streams cannot be external. */
> > +     assert(!external || !importOnly_);
> >       external_ = external;
> >  }
> >
> >  bool RPiStream::isExternal() const
> >  {
> > -     /*
> > -      * Import streams cannot be external.
> > -      *
> > -      * RAW capture is a special case where we simply copy the RAW
> > -      * buffer out of the request. All other buffer handling happens
> > -      * as if the stream is internal.
> > -      */
> > -     return external_ && !importOnly_;
> > -}
> > -
> > -bool RPiStream::isImporter() const
> > -{
> > -     return importOnly_;
> > +     return external_;
> >  }
> >
> >  void RPiStream::reset()
> >  {
> >       external_ = false;
> > -     internalBuffers_.clear();
> > +     clearBuffers();
> >  }
> >
> >  std::string RPiStream::name() const
> > @@ -52,65 +42,123 @@ std::string RPiStream::name() const
> >       return name_;
> >  }
> >
> > -void RPiStream::setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > +void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >  {
> > -     externalBuffers_ = buffers;
> > +     std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_),
> > +                    [](std::unique_ptr<FrameBuffer> &b) { return b.get(); });
> >  }
> >
> > -const std::vector<std::unique_ptr<FrameBuffer>> *RPiStream::getBuffers() const
> > +const std::vector<FrameBuffer *> &RPiStream::getBuffers() const
> >  {
> > -     return external_ ? externalBuffers_ : &internalBuffers_;
> > +     return bufferList_;
> >  }
> >
> >  void RPiStream::releaseBuffers()
> >  {
> >       dev_->releaseBuffers();
> > -     if (!external_ && !importOnly_)
> > -             internalBuffers_.clear();
> > +     clearBuffers();
> >  }
> >
> > -int RPiStream::importBuffers(unsigned int count)
> > +int RPiStream::prepareBuffers(unsigned int count)
> >  {
> > +     int ret;
> > +
> > +     if (!importOnly_) {
> > +             if (count) {
> > +                     /* Export some frame buffers for internal use. */
> > +                     ret = dev_->exportBuffers(count, &internalBuffers_);
> > +                     if (ret < 0)
> > +                             return ret;
> > +
> > +                     /* Add these exported buffers to the internal/external buffer list. */
> > +                     setExportedBuffers(&internalBuffers_);
> > +
> > +                     /* Add these buffers to the queue of internal usable buffers. */
> > +                     for (auto const &buffer : internalBuffers_)
> > +                             availableBuffers_.push(buffer.get());
> > +             }
> > +
> > +             /* We must import all internal/external exported buffers. */
> > +             count = bufferList_.size();
> > +     }
> > +
> >       return dev_->importBuffers(count);
> >  }
> >
> > -int RPiStream::allocateBuffers(unsigned int count)
> > +int RPiStream::queueAllBuffers()
> >  {
> > -     return dev_->allocateBuffers(count, &internalBuffers_);
> > -}
> > +     int ret;
> >
> > -int RPiStream::queueBuffers()
> > -{
> >       if (external_)
> >               return 0;
> >
> > -     for (auto &b : internalBuffers_) {
> > -             int ret = dev_->queueBuffer(b.get());
> > -             if (ret) {
> > -                     LOG(RPISTREAM, Error) << "Failed to queue buffers for "
> > -                                           << name_;
> > +     while (!availableBuffers_.empty()) {
> > +             ret = queueBuffer(availableBuffers_.front());
> > +             if (ret < 0)
> >                       return ret;
> > -             }
> > +
> > +             availableBuffers_.pop();
> >       }
> >
> >       return 0;
> >  }
> >
> > -bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> > +int RPiStream::queueBuffer(FrameBuffer *buffer)
> > +{
> > +     /*
> > +      * A nullptr buffer implies an external stream, but no external
> > +      * buffer has been supplied. So, pick one from the availableBuffers_
> > +      * queue.
> > +      */
> > +     if (!buffer) {
> > +             if (availableBuffers_.empty()) {
> > +                     LOG(RPISTREAM, Warning) << "No buffers available for "
> > +                                             << name_;
> > +                     return -EINVAL;
> > +             }
> > +
> > +             buffer = availableBuffers_.front();
> > +             availableBuffers_.pop();
> > +     }
> > +
> > +     LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie()
> > +                           << " for " << name_;
> > +
> > +     int ret = dev_->queueBuffer(buffer);
> > +     if (ret) {
> > +             LOG(RPISTREAM, Error) << "Failed to queue buffer for "
> > +                                   << name_;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +void RPiStream::returnBuffer(FrameBuffer *buffer)
> >  {
> > -     auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();
> > -     auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();
> > +     /* This can only be called for external streams. */
> > +     assert(external_);
> > +
> > +     availableBuffers_.push(buffer);
> > +}
> >
> > +bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> > +{
> >       if (importOnly_)
> >               return false;
> >
> > -     if (std::find_if(start, end,
> > -                      [buffer](std::unique_ptr<FrameBuffer> const &ref) { return ref.get() == buffer; }) != end)
> > +     if (std::find(bufferList_.begin(), bufferList_.end(), buffer) != bufferList_.end())
> >               return true;
>
> Cant this be made simpler by ?
>
>     if (bufferList_.find(buffer) != bufferList_.end())
>

bufferList_ is a std::vector, so in this case I must use std::find here.

> >
> >       return false;
> >  }
> >
> > +void RPiStream::clearBuffers()
> > +{
> > +     availableBuffers_ = std::queue<FrameBuffer *>{};
> > +     internalBuffers_.clear();
> > +     bufferList_.clear();
> > +}
> > +
> >  } /* namespace RPi */
> >
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > index 40fff81d..019e236d 100644
> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > @@ -38,21 +38,22 @@ public:
> >       V4L2VideoDevice *dev() const;
> >       void setExternal(bool external);
> >       bool isExternal() const;
> > -     bool isImporter() const;
> >       void reset();
> >       std::string name() const;
> > -     void setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> > -     const std::vector<std::unique_ptr<FrameBuffer>> *getBuffers() const;
> > +     void setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> > +     const std::vector<FrameBuffer *> &getBuffers() const;
> >       void releaseBuffers();
> > -     int importBuffers(unsigned int count);
> > -     int allocateBuffers(unsigned int count);
> > -     int queueBuffers();
> > +     int prepareBuffers(unsigned int count);
> > +     int queueAllBuffers();
> > +     int queueBuffer(FrameBuffer *buffer);
> > +     void returnBuffer(FrameBuffer *buffer);
> >       bool findFrameBuffer(FrameBuffer *buffer) const;
> >
> >  private:
> > +     void clearBuffers();
> >       /*
> >        * Indicates that this stream is active externally, i.e. the buffers
> > -      * are provided by the application.
> > +      * might be provided by (and returned to) the application.
> >        */
> >       bool external_;
> >       /* Indicates that this stream only imports buffers, e.g. ISP input. */
> > @@ -61,10 +62,19 @@ private:
> >       std::string name_;
> >       /* The actual device stream. */
> >       std::unique_ptr<V4L2VideoDevice> dev_;
> > -     /* Internally allocated framebuffers associated with this device stream. */
> > +     /* All framebuffers associated with this device stream. */
> > +     std::vector<FrameBuffer *> bufferList_;
> > +     /*
> > +      * List of frame buffer that we can use if none have been provided by
> > +      * the application for external streams. This is populated by the
> > +      * buffers exported internally.
> > +      */
> > +     std::queue<FrameBuffer *> availableBuffers_;
> > +     /*
> > +      * This is a list of buffers exported internally. Need to keep this around
> > +      * as the stream needs to maintain ownership of these buffers.
> > +      */
> >       std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_;
> > -     /* Externally allocated framebuffers associated with this device stream. */
> > -     std::vector<std::unique_ptr<FrameBuffer>> *externalBuffers_;
> >  };
> >
> >  /*
> > --
> > 2.25.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund

Regards,
Naush