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

Niklas Söderlund niklas.soderlund at ragnatech.se
Sat Jul 18 17:33:44 CEST 2020


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 at 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 at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund


More information about the libcamera-devel mailing list