[libcamera-devel,v4,1/9] libcamera: pipeline: raspberrypi: Move RPiStream into a separate file
diff mbox series

Message ID 20200720091311.805092-2-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Zero-copy RAW stream work
Related show

Commit Message

Naushir Patuck July 20, 2020, 9:13 a.m. UTC 
Put RPiStream into the RPi namespace and add a new log category (RPISTREAM).
There are no functional changes in this commit.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 .../pipeline/raspberrypi/meson.build          |   1 +
 .../pipeline/raspberrypi/raspberrypi.cpp      | 193 ++----------------
 .../pipeline/raspberrypi/rpi_stream.cpp       | 116 +++++++++++
 .../pipeline/raspberrypi/rpi_stream.h         |  98 +++++++++
 4 files changed, 234 insertions(+), 174 deletions(-)
 create mode 100644 src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
 create mode 100644 src/libcamera/pipeline/raspberrypi/rpi_stream.h

Comments

Kieran Bingham July 21, 2020, 10:45 a.m. UTC | #1 
Hi Naush,

On 20/07/2020 10:13, Naushir Patuck wrote:
> Put RPiStream into the RPi namespace and add a new log category (RPISTREAM).
> There are no functional changes in this commit.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

Great, I love seeing this become more modular, I think that will help
navigating code and maintenance.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  .../pipeline/raspberrypi/meson.build          |   1 +
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 193 ++----------------
>  .../pipeline/raspberrypi/rpi_stream.cpp       | 116 +++++++++++
>  .../pipeline/raspberrypi/rpi_stream.h         |  98 +++++++++
>  4 files changed, 234 insertions(+), 174 deletions(-)
>  create mode 100644 src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
>  create mode 100644 src/libcamera/pipeline/raspberrypi/rpi_stream.h
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build
> index ae0aed3b..7c5b6ff7 100644
> --- a/src/libcamera/pipeline/raspberrypi/meson.build
> +++ b/src/libcamera/pipeline/raspberrypi/meson.build
> @@ -3,5 +3,6 @@
>  libcamera_sources += files([
>      'dma_heaps.cpp',
>      'raspberrypi.cpp',
> +    'rpi_stream.cpp',
>      'staggered_ctrl.cpp',
>  ])
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index bf1c7714..6630ef57 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -19,7 +19,6 @@
>  #include <libcamera/logging.h>
>  #include <libcamera/property_ids.h>
>  #include <libcamera/request.h>
> -#include <libcamera/stream.h>
>  
>  #include <linux/videodev2.h>
>  
> @@ -33,6 +32,7 @@
>  #include "libcamera/internal/v4l2_videodevice.h"
>  
>  #include "dma_heaps.h"
> +#include "rpi_stream.h"
>  #include "staggered_ctrl.h"
>  
>  namespace libcamera {
> @@ -123,165 +123,10 @@ V4L2DeviceFormat findBestMode(V4L2PixFmtMap &formatsMap, const Size &req)
>  	return bestMode;
>  }
>  
> -} /* namespace */
> -
> -/*
> - * Device stream abstraction for either an internal or external stream.
> - * Used for both Unicam and the ISP.
> - */
> -class RPiStream : public Stream
> -{
> -public:
> -	RPiStream()
> -	{
> -	}
> -
> -	RPiStream(const char *name, MediaEntity *dev, bool importOnly = false)
> -		: external_(false), importOnly_(importOnly), name_(name),
> -		  dev_(std::make_unique<V4L2VideoDevice>(dev))
> -	{
> -	}
> -
> -	V4L2VideoDevice *dev() const
> -	{
> -		return dev_.get();
> -	}
> -
> -	void setExternal(bool external)
> -	{
> -		external_ = external;
> -	}
> -
> -	bool 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 isImporter() const
> -	{
> -		return importOnly_;
> -	}
> -
> -	void reset()
> -	{
> -		external_ = false;
> -		internalBuffers_.clear();
> -	}
> -
> -	std::string name() const
> -	{
> -		return name_;
> -	}
> -
> -	void setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> -	{
> -		externalBuffers_ = buffers;
> -	}
> -
> -	const std::vector<std::unique_ptr<FrameBuffer>> *getBuffers() const
> -	{
> -		return external_ ? externalBuffers_ : &internalBuffers_;
> -	}
> -
> -	void releaseBuffers()
> -	{
> -		dev_->releaseBuffers();
> -		if (!external_ && !importOnly_)
> -			internalBuffers_.clear();
> -	}
> -
> -	int importBuffers(unsigned int count)
> -	{
> -		return dev_->importBuffers(count);
> -	}
> -
> -	int allocateBuffers(unsigned int count)
> -	{
> -		return dev_->allocateBuffers(count, &internalBuffers_);
> -	}
> -
> -	int queueBuffers()
> -	{
> -		if (external_)
> -			return 0;
> -
> -		for (auto &b : internalBuffers_) {
> -			int ret = dev_->queueBuffer(b.get());
> -			if (ret) {
> -				LOG(RPI, Error) << "Failed to queue buffers for "
> -						<< name_;
> -				return ret;
> -			}
> -		}
> -
> -		return 0;
> -	}
> -
> -	bool findFrameBuffer(FrameBuffer *buffer) const
> -	{
> -		auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();
> -		auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();
> -
> -		if (importOnly_)
> -			return false;
> -
> -		if (std::find_if(start, end,
> -				 [buffer](std::unique_ptr<FrameBuffer> const &ref) { return ref.get() == buffer; }) != end)
> -			return true;
> -
> -		return false;
> -	}
> -
> -private:
> -	/*
> -	 * Indicates that this stream is active externally, i.e. the buffers
> -	 * are provided by the application.
> -	 */
> -	bool external_;
> -	/* Indicates that this stream only imports buffers, e.g. ISP input. */
> -	bool importOnly_;
> -	/* Stream name identifier. */
> -	std::string name_;
> -	/* The actual device stream. */
> -	std::unique_ptr<V4L2VideoDevice> dev_;
> -	/* Internally allocated framebuffers associated with this device stream. */
> -	std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_;
> -	/* Externally allocated framebuffers associated with this device stream. */
> -	std::vector<std::unique_ptr<FrameBuffer>> *externalBuffers_;
> -};
> -
> -/*
> - * The following class is just a convenient (and typesafe) array of device
> - * streams indexed with an enum class.
> - */
>  enum class Unicam : unsigned int { Image, Embedded };
>  enum class Isp : unsigned int { Input, Output0, Output1, Stats };
>  
> -template<typename E, std::size_t N>
> -class RPiDevice : public std::array<class RPiStream, N>
> -{
> -private:
> -	constexpr auto index(E e) const noexcept
> -	{
> -		return static_cast<std::underlying_type_t<E>>(e);
> -	}
> -public:
> -	RPiStream &operator[](E e)
> -	{
> -		return std::array<class RPiStream, N>::operator[](index(e));
> -	}
> -	const RPiStream &operator[](E e) const
> -	{
> -		return std::array<class RPiStream, N>::operator[](index(e));
> -	}
> -};
> +} /* namespace */
>  
>  class RPiCameraData : public CameraData
>  {
> @@ -305,15 +150,15 @@ public:
>  	void ispOutputDequeue(FrameBuffer *buffer);
>  
>  	void clearIncompleteRequests();
> -	void handleStreamBuffer(FrameBuffer *buffer, const RPiStream *stream);
> +	void handleStreamBuffer(FrameBuffer *buffer, const RPi::RPiStream *stream);
>  	void handleState();
>  
>  	CameraSensor *sensor_;
>  	/* Array of Unicam and ISP device streams and associated buffers/streams. */
> -	RPiDevice<Unicam, 2> unicam_;
> -	RPiDevice<Isp, 4> isp_;
> +	RPi::RPiDevice<Unicam, 2> unicam_;
> +	RPi::RPiDevice<Isp, 4> isp_;
>  	/* The vector below is just for convenience when iterating over all streams. */
> -	std::vector<RPiStream *> streams_;
> +	std::vector<RPi::RPiStream *> streams_;
>  	/* Buffers passed to the IPA. */
>  	std::vector<IPABuffer> ipaBuffers_;
>  
> @@ -762,7 +607,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  int PipelineHandlerRPi::exportFrameBuffers(Camera *camera, Stream *stream,
>  					   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
> -	RPiStream *s = static_cast<RPiStream *>(stream);
> +	RPi::RPiStream *s = static_cast<RPi::RPiStream *>(stream);
>  	unsigned int count = stream->configuration().bufferCount;
>  	int ret = s->dev()->exportBuffers(count, buffers);
>  
> @@ -908,14 +753,14 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
>  	std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this);
>  
>  	/* Locate and open the unicam video streams. */
> -	data->unicam_[Unicam::Embedded] = RPiStream("Unicam Embedded", unicam_->getEntityByName("unicam-embedded"));
> -	data->unicam_[Unicam::Image] = RPiStream("Unicam Image", unicam_->getEntityByName("unicam-image"));
> +	data->unicam_[Unicam::Embedded] = RPi::RPiStream("Unicam Embedded", unicam_->getEntityByName("unicam-embedded"));
> +	data->unicam_[Unicam::Image] = RPi::RPiStream("Unicam Image", unicam_->getEntityByName("unicam-image"));
>  
>  	/* Tag the ISP input stream as an import stream. */
> -	data->isp_[Isp::Input] = RPiStream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true);
> -	data->isp_[Isp::Output0] = RPiStream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1"));
> -	data->isp_[Isp::Output1] = RPiStream("ISP Output1", isp_->getEntityByName("bcm2835-isp0-capture2"));
> -	data->isp_[Isp::Stats] = RPiStream("ISP Stats", isp_->getEntityByName("bcm2835-isp0-capture3"));
> +	data->isp_[Isp::Input] = RPi::RPiStream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true);
> +	data->isp_[Isp::Output0] = RPi::RPiStream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1"));
> +	data->isp_[Isp::Output1] = RPi::RPiStream("ISP Output1", isp_->getEntityByName("bcm2835-isp0-capture2"));
> +	data->isp_[Isp::Stats] = RPi::RPiStream("ISP Stats", isp_->getEntityByName("bcm2835-isp0-capture3"));
>  
>  	/* This is just for convenience so that we can easily iterate over all streams. */
>  	for (auto &stream : data->unicam_)
> @@ -1005,7 +850,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  	 */
>  	unsigned int maxBuffers = 0;
>  	for (const Stream *s : camera->streams())
> -		if (static_cast<const RPiStream *>(s)->isExternal())
> +		if (static_cast<const RPi::RPiStream *>(s)->isExternal())
>  			maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
>  
>  	for (auto const stream : data->streams_) {
> @@ -1255,12 +1100,12 @@ done:
>  
>  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
>  {
> -	const RPiStream *stream = nullptr;
> +	const RPi::RPiStream *stream = nullptr;
>  
>  	if (state_ == State::Stopped)
>  		return;
>  
> -	for (RPiStream const &s : unicam_) {
> +	for (RPi::RPiStream const &s : unicam_) {
>  		if (s.findFrameBuffer(buffer)) {
>  			stream = &s;
>  			break;
> @@ -1316,12 +1161,12 @@ void RPiCameraData::ispInputDequeue(FrameBuffer *buffer)
>  
>  void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
>  {
> -	const RPiStream *stream = nullptr;
> +	const RPi::RPiStream *stream = nullptr;
>  
>  	if (state_ == State::Stopped)
>  		return;
>  
> -	for (RPiStream const &s : isp_) {
> +	for (RPi::RPiStream const &s : isp_) {
>  		if (s.findFrameBuffer(buffer)) {
>  			stream = &s;
>  			break;
> @@ -1402,7 +1247,7 @@ void RPiCameraData::clearIncompleteRequests()
>  	}
>  }
>  
> -void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, const RPiStream *stream)
> +void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, const RPi::RPiStream *stream)
>  {
>  	if (stream->isExternal()) {
>  		if (!dropFrame_) {
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> new file mode 100644
> index 00000000..2edb8b59
> --- /dev/null
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> @@ -0,0 +1,116 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> + *
> + * rpi_stream.cpp - Raspberry Pi device stream abstraction class.
> + */
> +#include "rpi_stream.h"
> +
> +#include "libcamera/internal/log.h"
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(RPISTREAM)
> +
> +namespace RPi {
> +
> +V4L2VideoDevice *RPiStream::dev() const
> +{
> +	return dev_.get();
> +}
> +
> +void RPiStream::setExternal(bool external)
> +{
> +	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_;
> +}
> +
> +void RPiStream::reset()
> +{
> +	external_ = false;
> +	internalBuffers_.clear();
> +}
> +
> +std::string RPiStream::name() const
> +{
> +	return name_;
> +}
> +
> +void RPiStream::setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> +{
> +	externalBuffers_ = buffers;
> +}
> +
> +const std::vector<std::unique_ptr<FrameBuffer>> *RPiStream::getBuffers() const
> +{
> +	return external_ ? externalBuffers_ : &internalBuffers_;
> +}
> +
> +void RPiStream::releaseBuffers()
> +{
> +	dev_->releaseBuffers();
> +	if (!external_ && !importOnly_)
> +		internalBuffers_.clear();
> +}
> +
> +int RPiStream::importBuffers(unsigned int count)
> +{
> +	return dev_->importBuffers(count);
> +}
> +
> +int RPiStream::allocateBuffers(unsigned int count)
> +{
> +	return dev_->allocateBuffers(count, &internalBuffers_);
> +}
> +
> +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_;
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> +{
> +	auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();
> +	auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();
> +
> +	if (importOnly_)
> +		return false;
> +
> +	if (std::find_if(start, end,
> +			 [buffer](std::unique_ptr<FrameBuffer> const &ref) { return ref.get() == buffer; }) != end)
> +		return true;
> +
> +	return false;
> +}
> +
> +} /* namespace RPi */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> new file mode 100644
> index 00000000..3957e342
> --- /dev/null
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> @@ -0,0 +1,98 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> + *
> + * rpi_stream.h - Raspberry Pi device stream abstraction class.
> + */
> +#ifndef __LIBCAMERA_PIPELINE_RPI_STREAM_H__
> +#define __LIBCAMERA_PIPELINE_RPI_STREAM_H__
> +
> +#include <queue>
> +#include <string>
> +#include <vector>
> +
> +#include <libcamera/stream.h>
> +
> +#include "libcamera/internal/v4l2_videodevice.h"
> +
> +namespace libcamera {
> +
> +namespace RPi {
> +
> +/*
> + * Device stream abstraction for either an internal or external stream.
> + * Used for both Unicam and the ISP.
> + */
> +class RPiStream : public Stream
> +{
> +public:
> +	RPiStream()
> +	{
> +	}
> +
> +	RPiStream(const char *name, MediaEntity *dev, bool importOnly = false)
> +		: external_(false), importOnly_(importOnly), name_(name),
> +		  dev_(std::make_unique<V4L2VideoDevice>(dev))
> +	{
> +	}
> +
> +	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 releaseBuffers();
> +	int importBuffers(unsigned int count);
> +	int allocateBuffers(unsigned int count);
> +	int queueBuffers();
> +	bool findFrameBuffer(FrameBuffer *buffer) const;
> +
> +private:
> +	/*
> +	 * Indicates that this stream is active externally, i.e. the buffers
> +	 * are provided by the application.
> +	 */
> +	bool external_;
> +	/* Indicates that this stream only imports buffers, e.g. ISP input. */
> +	bool importOnly_;
> +	/* Stream name identifier. */
> +	std::string name_;
> +	/* The actual device stream. */
> +	std::unique_ptr<V4L2VideoDevice> dev_;
> +	/* Internally allocated framebuffers associated with this device stream. */
> +	std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_;
> +	/* Externally allocated framebuffers associated with this device stream. */
> +	std::vector<std::unique_ptr<FrameBuffer>> *externalBuffers_;
> +};
> +
> +/*
> + * The following class is just a convenient (and typesafe) array of device
> + * streams indexed with an enum class.
> + */
> +template<typename E, std::size_t N>
> +class RPiDevice : public std::array<class RPiStream, N>
> +{
> +private:
> +	constexpr auto index(E e) const noexcept
> +	{
> +		return static_cast<std::underlying_type_t<E>>(e);
> +	}
> +public:
> +	RPiStream &operator[](E e)
> +	{
> +		return std::array<class RPiStream, N>::operator[](index(e));
> +	}
> +	const RPiStream &operator[](E e) const
> +	{
> +		return std::array<class RPiStream, N>::operator[](index(e));
> +	}
> +};
> +
> +} /* namespace RPi */
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_PIPELINE_RPI_STREAM_H__ */
>
Kieran Bingham July 22, 2020, 12:29 p.m. UTC | #2 
Hi Naush,

On 21/07/2020 11:45, Kieran Bingham wrote:
> Hi Naush,
> 
> On 20/07/2020 10:13, Naushir Patuck wrote:
>> Put RPiStream into the RPi namespace and add a new log category (RPISTREAM).
>> There are no functional changes in this commit.
>>
>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
> Great, I love seeing this become more modular, I think that will help
> navigating code and maintenance.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
>> ---
>>  .../pipeline/raspberrypi/meson.build          |   1 +
>>  .../pipeline/raspberrypi/raspberrypi.cpp      | 193 ++----------------
>>  .../pipeline/raspberrypi/rpi_stream.cpp       | 116 +++++++++++
>>  .../pipeline/raspberrypi/rpi_stream.h         |  98 +++++++++
>>  4 files changed, 234 insertions(+), 174 deletions(-)
>>  create mode 100644 src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
>>  create mode 100644 src/libcamera/pipeline/raspberrypi/rpi_stream.h
>>
>> diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build
>> index ae0aed3b..7c5b6ff7 100644
>> --- a/src/libcamera/pipeline/raspberrypi/meson.build
>> +++ b/src/libcamera/pipeline/raspberrypi/meson.build
>> @@ -3,5 +3,6 @@
>>  libcamera_sources += files([
>>      'dma_heaps.cpp',
>>      'raspberrypi.cpp',
>> +    'rpi_stream.cpp',
>>      'staggered_ctrl.cpp',
>>  ])
>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> index bf1c7714..6630ef57 100644
>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> @@ -19,7 +19,6 @@
>>  #include <libcamera/logging.h>
>>  #include <libcamera/property_ids.h>
>>  #include <libcamera/request.h>
>> -#include <libcamera/stream.h>
>>  
>>  #include <linux/videodev2.h>
>>  
>> @@ -33,6 +32,7 @@
>>  #include "libcamera/internal/v4l2_videodevice.h"
>>  
>>  #include "dma_heaps.h"
>> +#include "rpi_stream.h"
>>  #include "staggered_ctrl.h"
>>  
>>  namespace libcamera {
>> @@ -123,165 +123,10 @@ V4L2DeviceFormat findBestMode(V4L2PixFmtMap &formatsMap, const Size &req)
>>  	return bestMode;
>>  }
>>  
>> -} /* namespace */
>> -
>> -/*
>> - * Device stream abstraction for either an internal or external stream.
>> - * Used for both Unicam and the ISP.
>> - */
>> -class RPiStream : public Stream
>> -{
>> -public:
>> -	RPiStream()
>> -	{
>> -	}
>> -
>> -	RPiStream(const char *name, MediaEntity *dev, bool importOnly = false)
>> -		: external_(false), importOnly_(importOnly), name_(name),
>> -		  dev_(std::make_unique<V4L2VideoDevice>(dev))
>> -	{
>> -	}
>> -
>> -	V4L2VideoDevice *dev() const
>> -	{
>> -		return dev_.get();
>> -	}
>> -
>> -	void setExternal(bool external)
>> -	{
>> -		external_ = external;
>> -	}
>> -
>> -	bool 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 isImporter() const
>> -	{
>> -		return importOnly_;
>> -	}
>> -
>> -	void reset()
>> -	{
>> -		external_ = false;
>> -		internalBuffers_.clear();
>> -	}
>> -
>> -	std::string name() const
>> -	{
>> -		return name_;
>> -	}
>> -
>> -	void setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>> -	{
>> -		externalBuffers_ = buffers;
>> -	}
>> -
>> -	const std::vector<std::unique_ptr<FrameBuffer>> *getBuffers() const
>> -	{
>> -		return external_ ? externalBuffers_ : &internalBuffers_;
>> -	}
>> -
>> -	void releaseBuffers()
>> -	{
>> -		dev_->releaseBuffers();
>> -		if (!external_ && !importOnly_)
>> -			internalBuffers_.clear();
>> -	}
>> -
>> -	int importBuffers(unsigned int count)
>> -	{
>> -		return dev_->importBuffers(count);
>> -	}
>> -
>> -	int allocateBuffers(unsigned int count)
>> -	{
>> -		return dev_->allocateBuffers(count, &internalBuffers_);
>> -	}
>> -
>> -	int queueBuffers()
>> -	{
>> -		if (external_)
>> -			return 0;
>> -
>> -		for (auto &b : internalBuffers_) {
>> -			int ret = dev_->queueBuffer(b.get());
>> -			if (ret) {
>> -				LOG(RPI, Error) << "Failed to queue buffers for "
>> -						<< name_;
>> -				return ret;
>> -			}
>> -		}
>> -
>> -		return 0;
>> -	}
>> -
>> -	bool findFrameBuffer(FrameBuffer *buffer) const
>> -	{
>> -		auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();
>> -		auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();
>> -
>> -		if (importOnly_)
>> -			return false;
>> -
>> -		if (std::find_if(start, end,
>> -				 [buffer](std::unique_ptr<FrameBuffer> const &ref) { return ref.get() == buffer; }) != end)
>> -			return true;
>> -
>> -		return false;
>> -	}
>> -
>> -private:
>> -	/*
>> -	 * Indicates that this stream is active externally, i.e. the buffers
>> -	 * are provided by the application.
>> -	 */
>> -	bool external_;
>> -	/* Indicates that this stream only imports buffers, e.g. ISP input. */
>> -	bool importOnly_;
>> -	/* Stream name identifier. */
>> -	std::string name_;
>> -	/* The actual device stream. */
>> -	std::unique_ptr<V4L2VideoDevice> dev_;
>> -	/* Internally allocated framebuffers associated with this device stream. */
>> -	std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_;
>> -	/* Externally allocated framebuffers associated with this device stream. */
>> -	std::vector<std::unique_ptr<FrameBuffer>> *externalBuffers_;
>> -};
>> -
>> -/*
>> - * The following class is just a convenient (and typesafe) array of device
>> - * streams indexed with an enum class.
>> - */
>>  enum class Unicam : unsigned int { Image, Embedded };
>>  enum class Isp : unsigned int { Input, Output0, Output1, Stats };
>>  
>> -template<typename E, std::size_t N>
>> -class RPiDevice : public std::array<class RPiStream, N>
>> -{
>> -private:
>> -	constexpr auto index(E e) const noexcept
>> -	{
>> -		return static_cast<std::underlying_type_t<E>>(e);
>> -	}
>> -public:
>> -	RPiStream &operator[](E e)
>> -	{
>> -		return std::array<class RPiStream, N>::operator[](index(e));
>> -	}
>> -	const RPiStream &operator[](E e) const
>> -	{
>> -		return std::array<class RPiStream, N>::operator[](index(e));
>> -	}
>> -};
>> +} /* namespace */
>>  
>>  class RPiCameraData : public CameraData
>>  {
>> @@ -305,15 +150,15 @@ public:
>>  	void ispOutputDequeue(FrameBuffer *buffer);
>>  
>>  	void clearIncompleteRequests();
>> -	void handleStreamBuffer(FrameBuffer *buffer, const RPiStream *stream);
>> +	void handleStreamBuffer(FrameBuffer *buffer, const RPi::RPiStream *stream);
>>  	void handleState();
>>  
>>  	CameraSensor *sensor_;
>>  	/* Array of Unicam and ISP device streams and associated buffers/streams. */
>> -	RPiDevice<Unicam, 2> unicam_;
>> -	RPiDevice<Isp, 4> isp_;
>> +	RPi::RPiDevice<Unicam, 2> unicam_;
>> +	RPi::RPiDevice<Isp, 4> isp_;
>>  	/* The vector below is just for convenience when iterating over all streams. */
>> -	std::vector<RPiStream *> streams_;
>> +	std::vector<RPi::RPiStream *> streams_;
>>  	/* Buffers passed to the IPA. */
>>  	std::vector<IPABuffer> ipaBuffers_;
>>  
>> @@ -762,7 +607,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>>  int PipelineHandlerRPi::exportFrameBuffers(Camera *camera, Stream *stream,
>>  					   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>>  {
>> -	RPiStream *s = static_cast<RPiStream *>(stream);
>> +	RPi::RPiStream *s = static_cast<RPi::RPiStream *>(stream);
>>  	unsigned int count = stream->configuration().bufferCount;
>>  	int ret = s->dev()->exportBuffers(count, buffers);
>>  
>> @@ -908,14 +753,14 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
>>  	std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this);
>>  
>>  	/* Locate and open the unicam video streams. */
>> -	data->unicam_[Unicam::Embedded] = RPiStream("Unicam Embedded", unicam_->getEntityByName("unicam-embedded"));
>> -	data->unicam_[Unicam::Image] = RPiStream("Unicam Image", unicam_->getEntityByName("unicam-image"));
>> +	data->unicam_[Unicam::Embedded] = RPi::RPiStream("Unicam Embedded", unicam_->getEntityByName("unicam-embedded"));
>> +	data->unicam_[Unicam::Image] = RPi::RPiStream("Unicam Image", unicam_->getEntityByName("unicam-image"));
>>  
>>  	/* Tag the ISP input stream as an import stream. */
>> -	data->isp_[Isp::Input] = RPiStream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true);
>> -	data->isp_[Isp::Output0] = RPiStream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1"));
>> -	data->isp_[Isp::Output1] = RPiStream("ISP Output1", isp_->getEntityByName("bcm2835-isp0-capture2"));
>> -	data->isp_[Isp::Stats] = RPiStream("ISP Stats", isp_->getEntityByName("bcm2835-isp0-capture3"));
>> +	data->isp_[Isp::Input] = RPi::RPiStream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true);
>> +	data->isp_[Isp::Output0] = RPi::RPiStream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1"));
>> +	data->isp_[Isp::Output1] = RPi::RPiStream("ISP Output1", isp_->getEntityByName("bcm2835-isp0-capture2"));
>> +	data->isp_[Isp::Stats] = RPi::RPiStream("ISP Stats", isp_->getEntityByName("bcm2835-isp0-capture3"));
>>  
>>  	/* This is just for convenience so that we can easily iterate over all streams. */
>>  	for (auto &stream : data->unicam_)
>> @@ -1005,7 +850,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>>  	 */
>>  	unsigned int maxBuffers = 0;
>>  	for (const Stream *s : camera->streams())
>> -		if (static_cast<const RPiStream *>(s)->isExternal())
>> +		if (static_cast<const RPi::RPiStream *>(s)->isExternal())
>>  			maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
>>  
>>  	for (auto const stream : data->streams_) {
>> @@ -1255,12 +1100,12 @@ done:
>>  
>>  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
>>  {
>> -	const RPiStream *stream = nullptr;
>> +	const RPi::RPiStream *stream = nullptr;
>>  
>>  	if (state_ == State::Stopped)
>>  		return;
>>  
>> -	for (RPiStream const &s : unicam_) {
>> +	for (RPi::RPiStream const &s : unicam_) {
>>  		if (s.findFrameBuffer(buffer)) {
>>  			stream = &s;
>>  			break;
>> @@ -1316,12 +1161,12 @@ void RPiCameraData::ispInputDequeue(FrameBuffer *buffer)
>>  
>>  void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
>>  {
>> -	const RPiStream *stream = nullptr;
>> +	const RPi::RPiStream *stream = nullptr;
>>  
>>  	if (state_ == State::Stopped)
>>  		return;
>>  
>> -	for (RPiStream const &s : isp_) {
>> +	for (RPi::RPiStream const &s : isp_) {
>>  		if (s.findFrameBuffer(buffer)) {
>>  			stream = &s;
>>  			break;
>> @@ -1402,7 +1247,7 @@ void RPiCameraData::clearIncompleteRequests()
>>  	}
>>  }
>>  
>> -void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, const RPiStream *stream)
>> +void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, const RPi::RPiStream *stream)
>>  {
>>  	if (stream->isExternal()) {
>>  		if (!dropFrame_) {
>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
>> new file mode 100644
>> index 00000000..2edb8b59
>> --- /dev/null
>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
>> @@ -0,0 +1,116 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
>> + *
>> + * rpi_stream.cpp - Raspberry Pi device stream abstraction class.
>> + */
>> +#include "rpi_stream.h"
>> +
>> +#include "libcamera/internal/log.h"
>> +
>> +namespace libcamera {
>> +
>> +LOG_DEFINE_CATEGORY(RPISTREAM)
>> +
>> +namespace RPi {
>> +
>> +V4L2VideoDevice *RPiStream::dev() const
>> +{
>> +	return dev_.get();
>> +}
>> +
>> +void RPiStream::setExternal(bool external)
>> +{
>> +	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_;
>> +}
>> +
>> +void RPiStream::reset()
>> +{
>> +	external_ = false;
>> +	internalBuffers_.clear();
>> +}
>> +
>> +std::string RPiStream::name() const
>> +{
>> +	return name_;
>> +}
>> +
>> +void RPiStream::setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>> +{
>> +	externalBuffers_ = buffers;
>> +}
>> +
>> +const std::vector<std::unique_ptr<FrameBuffer>> *RPiStream::getBuffers() const
>> +{
>> +	return external_ ? externalBuffers_ : &internalBuffers_;
>> +}
>> +
>> +void RPiStream::releaseBuffers()
>> +{
>> +	dev_->releaseBuffers();
>> +	if (!external_ && !importOnly_)
>> +		internalBuffers_.clear();
>> +}
>> +
>> +int RPiStream::importBuffers(unsigned int count)
>> +{
>> +	return dev_->importBuffers(count);
>> +}
>> +
>> +int RPiStream::allocateBuffers(unsigned int count)
>> +{
>> +	return dev_->allocateBuffers(count, &internalBuffers_);
>> +}
>> +
>> +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_;
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
>> +{
>> +	auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();
>> +	auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();
>> +
>> +	if (importOnly_)
>> +		return false;
>> +
>> +	if (std::find_if(start, end,
>> +			 [buffer](std::unique_ptr<FrameBuffer> const &ref) { return ref.get() == buffer; }) != end)
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>> +} /* namespace RPi */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
>> new file mode 100644
>> index 00000000..3957e342
>> --- /dev/null
>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
>> @@ -0,0 +1,98 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
>> + *
>> + * rpi_stream.h - Raspberry Pi device stream abstraction class.
>> + */
>> +#ifndef __LIBCAMERA_PIPELINE_RPI_STREAM_H__
>> +#define __LIBCAMERA_PIPELINE_RPI_STREAM_H__
>> +
>> +#include <queue>
>> +#include <string>
>> +#include <vector>
>> +
>> +#include <libcamera/stream.h>
>> +
>> +#include "libcamera/internal/v4l2_videodevice.h"
>> +
>> +namespace libcamera {
>> +
>> +namespace RPi {
>> +
>> +/*
>> + * Device stream abstraction for either an internal or external stream.
>> + * Used for both Unicam and the ISP.
>> + */
>> +class RPiStream : public Stream
>> +{
>> +public:
>> +	RPiStream()
>> +	{
>> +	}
>> +
>> +	RPiStream(const char *name, MediaEntity *dev, bool importOnly = false)
>> +		: external_(false), importOnly_(importOnly), name_(name),
>> +		  dev_(std::make_unique<V4L2VideoDevice>(dev))
>> +	{
>> +	}
>> +


As mentioned by the time I got to 6/9 (which I looked at last for some
reason), this class became hard to parse as there are so many members so
close together


Although maybe it was because I was looking at a patch diff, but this
seems smaller, I guess the diff adds an extra line for the +/- entries ;-)

But perhaps breaking this up a bit below with:



>> +	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 releaseBuffers();

>> +	int importBuffers(unsigned int count);
>> +	int allocateBuffers(unsigned int count);
>> +	int queueBuffers();
>> +	bool findFrameBuffer(FrameBuffer *buffer) const;

I've punched newlines above (I hope that comes through in the mail) ...
but really all I was wondering is if there is a way to group related
functions into their own blocks.


>> +
>> +private:
>> +	/*
>> +	 * Indicates that this stream is active externally, i.e. the buffers
>> +	 * are provided by the application.
>> +	 */
>> +	bool external_;

>> +	/* Indicates that this stream only imports buffers, e.g. ISP input. */
>> +	bool importOnly_;

>> +	/* Stream name identifier. */
>> +	std::string name_;

>> +	/* The actual device stream. */
>> +	std::unique_ptr<V4L2VideoDevice> dev_;

>> +	/* Internally allocated framebuffers associated with this device stream. */
>> +	std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_;

>> +	/* Externally allocated framebuffers associated with this device stream. */
>> +	std::vector<std::unique_ptr<FrameBuffer>> *externalBuffers_;
>> +};

When there's a comment line above an entry, for me it really helps to
have a newline before it.


Anyway, nothing critical here, just where I think some whitespace would
have helped my weary eyes yesterday ;-)



>> +
>> +/*
>> + * The following class is just a convenient (and typesafe) array of device
>> + * streams indexed with an enum class.
>> + */
>> +template<typename E, std::size_t N>
>> +class RPiDevice : public std::array<class RPiStream, N>
>> +{
>> +private:
>> +	constexpr auto index(E e) const noexcept
>> +	{
>> +		return static_cast<std::underlying_type_t<E>>(e);
>> +	}
>> +public:
>> +	RPiStream &operator[](E e)
>> +	{
>> +		return std::array<class RPiStream, N>::operator[](index(e));
>> +	}
>> +	const RPiStream &operator[](E e) const
>> +	{
>> +		return std::array<class RPiStream, N>::operator[](index(e));
>> +	}
>> +};
>> +
>> +} /* namespace RPi */
>> +
>> +} /* namespace libcamera */
>> +
>> +#endif /* __LIBCAMERA_PIPELINE_RPI_STREAM_H__ */
>>
>
Naushir Patuck July 22, 2020, 12:45 p.m. UTC | #3 
Hi Kieran,

Thank you for the review.  I'll make all the suggested changes and
submit a new version soon.

Regards,
Naush

On Wed, 22 Jul 2020 at 13:29, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Hi Naush,
>
> On 21/07/2020 11:45, Kieran Bingham wrote:
> > Hi Naush,
> >
> > On 20/07/2020 10:13, Naushir Patuck wrote:
> >> Put RPiStream into the RPi namespace and add a new log category (RPISTREAM).
> >> There are no functional changes in this commit.
> >>
> >> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> >> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >
> > Great, I love seeing this become more modular, I think that will help
> > navigating code and maintenance.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> >> ---
> >>  .../pipeline/raspberrypi/meson.build          |   1 +
> >>  .../pipeline/raspberrypi/raspberrypi.cpp      | 193 ++----------------
> >>  .../pipeline/raspberrypi/rpi_stream.cpp       | 116 +++++++++++
> >>  .../pipeline/raspberrypi/rpi_stream.h         |  98 +++++++++
> >>  4 files changed, 234 insertions(+), 174 deletions(-)
> >>  create mode 100644 src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> >>  create mode 100644 src/libcamera/pipeline/raspberrypi/rpi_stream.h
> >>
> >> diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build
> >> index ae0aed3b..7c5b6ff7 100644
> >> --- a/src/libcamera/pipeline/raspberrypi/meson.build
> >> +++ b/src/libcamera/pipeline/raspberrypi/meson.build
> >> @@ -3,5 +3,6 @@
> >>  libcamera_sources += files([
> >>      'dma_heaps.cpp',
> >>      'raspberrypi.cpp',
> >> +    'rpi_stream.cpp',
> >>      'staggered_ctrl.cpp',
> >>  ])
> >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >> index bf1c7714..6630ef57 100644
> >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >> @@ -19,7 +19,6 @@
> >>  #include <libcamera/logging.h>
> >>  #include <libcamera/property_ids.h>
> >>  #include <libcamera/request.h>
> >> -#include <libcamera/stream.h>
> >>
> >>  #include <linux/videodev2.h>
> >>
> >> @@ -33,6 +32,7 @@
> >>  #include "libcamera/internal/v4l2_videodevice.h"
> >>
> >>  #include "dma_heaps.h"
> >> +#include "rpi_stream.h"
> >>  #include "staggered_ctrl.h"
> >>
> >>  namespace libcamera {
> >> @@ -123,165 +123,10 @@ V4L2DeviceFormat findBestMode(V4L2PixFmtMap &formatsMap, const Size &req)
> >>      return bestMode;
> >>  }
> >>
> >> -} /* namespace */
> >> -
> >> -/*
> >> - * Device stream abstraction for either an internal or external stream.
> >> - * Used for both Unicam and the ISP.
> >> - */
> >> -class RPiStream : public Stream
> >> -{
> >> -public:
> >> -    RPiStream()
> >> -    {
> >> -    }
> >> -
> >> -    RPiStream(const char *name, MediaEntity *dev, bool importOnly = false)
> >> -            : external_(false), importOnly_(importOnly), name_(name),
> >> -              dev_(std::make_unique<V4L2VideoDevice>(dev))
> >> -    {
> >> -    }
> >> -
> >> -    V4L2VideoDevice *dev() const
> >> -    {
> >> -            return dev_.get();
> >> -    }
> >> -
> >> -    void setExternal(bool external)
> >> -    {
> >> -            external_ = external;
> >> -    }
> >> -
> >> -    bool 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 isImporter() const
> >> -    {
> >> -            return importOnly_;
> >> -    }
> >> -
> >> -    void reset()
> >> -    {
> >> -            external_ = false;
> >> -            internalBuffers_.clear();
> >> -    }
> >> -
> >> -    std::string name() const
> >> -    {
> >> -            return name_;
> >> -    }
> >> -
> >> -    void setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >> -    {
> >> -            externalBuffers_ = buffers;
> >> -    }
> >> -
> >> -    const std::vector<std::unique_ptr<FrameBuffer>> *getBuffers() const
> >> -    {
> >> -            return external_ ? externalBuffers_ : &internalBuffers_;
> >> -    }
> >> -
> >> -    void releaseBuffers()
> >> -    {
> >> -            dev_->releaseBuffers();
> >> -            if (!external_ && !importOnly_)
> >> -                    internalBuffers_.clear();
> >> -    }
> >> -
> >> -    int importBuffers(unsigned int count)
> >> -    {
> >> -            return dev_->importBuffers(count);
> >> -    }
> >> -
> >> -    int allocateBuffers(unsigned int count)
> >> -    {
> >> -            return dev_->allocateBuffers(count, &internalBuffers_);
> >> -    }
> >> -
> >> -    int queueBuffers()
> >> -    {
> >> -            if (external_)
> >> -                    return 0;
> >> -
> >> -            for (auto &b : internalBuffers_) {
> >> -                    int ret = dev_->queueBuffer(b.get());
> >> -                    if (ret) {
> >> -                            LOG(RPI, Error) << "Failed to queue buffers for "
> >> -                                            << name_;
> >> -                            return ret;
> >> -                    }
> >> -            }
> >> -
> >> -            return 0;
> >> -    }
> >> -
> >> -    bool findFrameBuffer(FrameBuffer *buffer) const
> >> -    {
> >> -            auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();
> >> -            auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();
> >> -
> >> -            if (importOnly_)
> >> -                    return false;
> >> -
> >> -            if (std::find_if(start, end,
> >> -                             [buffer](std::unique_ptr<FrameBuffer> const &ref) { return ref.get() == buffer; }) != end)
> >> -                    return true;
> >> -
> >> -            return false;
> >> -    }
> >> -
> >> -private:
> >> -    /*
> >> -     * Indicates that this stream is active externally, i.e. the buffers
> >> -     * are provided by the application.
> >> -     */
> >> -    bool external_;
> >> -    /* Indicates that this stream only imports buffers, e.g. ISP input. */
> >> -    bool importOnly_;
> >> -    /* Stream name identifier. */
> >> -    std::string name_;
> >> -    /* The actual device stream. */
> >> -    std::unique_ptr<V4L2VideoDevice> dev_;
> >> -    /* Internally allocated framebuffers associated with this device stream. */
> >> -    std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_;
> >> -    /* Externally allocated framebuffers associated with this device stream. */
> >> -    std::vector<std::unique_ptr<FrameBuffer>> *externalBuffers_;
> >> -};
> >> -
> >> -/*
> >> - * The following class is just a convenient (and typesafe) array of device
> >> - * streams indexed with an enum class.
> >> - */
> >>  enum class Unicam : unsigned int { Image, Embedded };
> >>  enum class Isp : unsigned int { Input, Output0, Output1, Stats };
> >>
> >> -template<typename E, std::size_t N>
> >> -class RPiDevice : public std::array<class RPiStream, N>
> >> -{
> >> -private:
> >> -    constexpr auto index(E e) const noexcept
> >> -    {
> >> -            return static_cast<std::underlying_type_t<E>>(e);
> >> -    }
> >> -public:
> >> -    RPiStream &operator[](E e)
> >> -    {
> >> -            return std::array<class RPiStream, N>::operator[](index(e));
> >> -    }
> >> -    const RPiStream &operator[](E e) const
> >> -    {
> >> -            return std::array<class RPiStream, N>::operator[](index(e));
> >> -    }
> >> -};
> >> +} /* namespace */
> >>
> >>  class RPiCameraData : public CameraData
> >>  {
> >> @@ -305,15 +150,15 @@ public:
> >>      void ispOutputDequeue(FrameBuffer *buffer);
> >>
> >>      void clearIncompleteRequests();
> >> -    void handleStreamBuffer(FrameBuffer *buffer, const RPiStream *stream);
> >> +    void handleStreamBuffer(FrameBuffer *buffer, const RPi::RPiStream *stream);
> >>      void handleState();
> >>
> >>      CameraSensor *sensor_;
> >>      /* Array of Unicam and ISP device streams and associated buffers/streams. */
> >> -    RPiDevice<Unicam, 2> unicam_;
> >> -    RPiDevice<Isp, 4> isp_;
> >> +    RPi::RPiDevice<Unicam, 2> unicam_;
> >> +    RPi::RPiDevice<Isp, 4> isp_;
> >>      /* The vector below is just for convenience when iterating over all streams. */
> >> -    std::vector<RPiStream *> streams_;
> >> +    std::vector<RPi::RPiStream *> streams_;
> >>      /* Buffers passed to the IPA. */
> >>      std::vector<IPABuffer> ipaBuffers_;
> >>
> >> @@ -762,7 +607,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >>  int PipelineHandlerRPi::exportFrameBuffers(Camera *camera, Stream *stream,
> >>                                         std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >>  {
> >> -    RPiStream *s = static_cast<RPiStream *>(stream);
> >> +    RPi::RPiStream *s = static_cast<RPi::RPiStream *>(stream);
> >>      unsigned int count = stream->configuration().bufferCount;
> >>      int ret = s->dev()->exportBuffers(count, buffers);
> >>
> >> @@ -908,14 +753,14 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> >>      std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this);
> >>
> >>      /* Locate and open the unicam video streams. */
> >> -    data->unicam_[Unicam::Embedded] = RPiStream("Unicam Embedded", unicam_->getEntityByName("unicam-embedded"));
> >> -    data->unicam_[Unicam::Image] = RPiStream("Unicam Image", unicam_->getEntityByName("unicam-image"));
> >> +    data->unicam_[Unicam::Embedded] = RPi::RPiStream("Unicam Embedded", unicam_->getEntityByName("unicam-embedded"));
> >> +    data->unicam_[Unicam::Image] = RPi::RPiStream("Unicam Image", unicam_->getEntityByName("unicam-image"));
> >>
> >>      /* Tag the ISP input stream as an import stream. */
> >> -    data->isp_[Isp::Input] = RPiStream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true);
> >> -    data->isp_[Isp::Output0] = RPiStream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1"));
> >> -    data->isp_[Isp::Output1] = RPiStream("ISP Output1", isp_->getEntityByName("bcm2835-isp0-capture2"));
> >> -    data->isp_[Isp::Stats] = RPiStream("ISP Stats", isp_->getEntityByName("bcm2835-isp0-capture3"));
> >> +    data->isp_[Isp::Input] = RPi::RPiStream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true);
> >> +    data->isp_[Isp::Output0] = RPi::RPiStream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1"));
> >> +    data->isp_[Isp::Output1] = RPi::RPiStream("ISP Output1", isp_->getEntityByName("bcm2835-isp0-capture2"));
> >> +    data->isp_[Isp::Stats] = RPi::RPiStream("ISP Stats", isp_->getEntityByName("bcm2835-isp0-capture3"));
> >>
> >>      /* This is just for convenience so that we can easily iterate over all streams. */
> >>      for (auto &stream : data->unicam_)
> >> @@ -1005,7 +850,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >>       */
> >>      unsigned int maxBuffers = 0;
> >>      for (const Stream *s : camera->streams())
> >> -            if (static_cast<const RPiStream *>(s)->isExternal())
> >> +            if (static_cast<const RPi::RPiStream *>(s)->isExternal())
> >>                      maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
> >>
> >>      for (auto const stream : data->streams_) {
> >> @@ -1255,12 +1100,12 @@ done:
> >>
> >>  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> >>  {
> >> -    const RPiStream *stream = nullptr;
> >> +    const RPi::RPiStream *stream = nullptr;
> >>
> >>      if (state_ == State::Stopped)
> >>              return;
> >>
> >> -    for (RPiStream const &s : unicam_) {
> >> +    for (RPi::RPiStream const &s : unicam_) {
> >>              if (s.findFrameBuffer(buffer)) {
> >>                      stream = &s;
> >>                      break;
> >> @@ -1316,12 +1161,12 @@ void RPiCameraData::ispInputDequeue(FrameBuffer *buffer)
> >>
> >>  void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> >>  {
> >> -    const RPiStream *stream = nullptr;
> >> +    const RPi::RPiStream *stream = nullptr;
> >>
> >>      if (state_ == State::Stopped)
> >>              return;
> >>
> >> -    for (RPiStream const &s : isp_) {
> >> +    for (RPi::RPiStream const &s : isp_) {
> >>              if (s.findFrameBuffer(buffer)) {
> >>                      stream = &s;
> >>                      break;
> >> @@ -1402,7 +1247,7 @@ void RPiCameraData::clearIncompleteRequests()
> >>      }
> >>  }
> >>
> >> -void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, const RPiStream *stream)
> >> +void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, const RPi::RPiStream *stream)
> >>  {
> >>      if (stream->isExternal()) {
> >>              if (!dropFrame_) {
> >> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> >> new file mode 100644
> >> index 00000000..2edb8b59
> >> --- /dev/null
> >> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> >> @@ -0,0 +1,116 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> >> + *
> >> + * rpi_stream.cpp - Raspberry Pi device stream abstraction class.
> >> + */
> >> +#include "rpi_stream.h"
> >> +
> >> +#include "libcamera/internal/log.h"
> >> +
> >> +namespace libcamera {
> >> +
> >> +LOG_DEFINE_CATEGORY(RPISTREAM)
> >> +
> >> +namespace RPi {
> >> +
> >> +V4L2VideoDevice *RPiStream::dev() const
> >> +{
> >> +    return dev_.get();
> >> +}
> >> +
> >> +void RPiStream::setExternal(bool external)
> >> +{
> >> +    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_;
> >> +}
> >> +
> >> +void RPiStream::reset()
> >> +{
> >> +    external_ = false;
> >> +    internalBuffers_.clear();
> >> +}
> >> +
> >> +std::string RPiStream::name() const
> >> +{
> >> +    return name_;
> >> +}
> >> +
> >> +void RPiStream::setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >> +{
> >> +    externalBuffers_ = buffers;
> >> +}
> >> +
> >> +const std::vector<std::unique_ptr<FrameBuffer>> *RPiStream::getBuffers() const
> >> +{
> >> +    return external_ ? externalBuffers_ : &internalBuffers_;
> >> +}
> >> +
> >> +void RPiStream::releaseBuffers()
> >> +{
> >> +    dev_->releaseBuffers();
> >> +    if (!external_ && !importOnly_)
> >> +            internalBuffers_.clear();
> >> +}
> >> +
> >> +int RPiStream::importBuffers(unsigned int count)
> >> +{
> >> +    return dev_->importBuffers(count);
> >> +}
> >> +
> >> +int RPiStream::allocateBuffers(unsigned int count)
> >> +{
> >> +    return dev_->allocateBuffers(count, &internalBuffers_);
> >> +}
> >> +
> >> +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_;
> >> +                    return ret;
> >> +            }
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> >> +{
> >> +    auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();
> >> +    auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();
> >> +
> >> +    if (importOnly_)
> >> +            return false;
> >> +
> >> +    if (std::find_if(start, end,
> >> +                     [buffer](std::unique_ptr<FrameBuffer> const &ref) { return ref.get() == buffer; }) != end)
> >> +            return true;
> >> +
> >> +    return false;
> >> +}
> >> +
> >> +} /* namespace RPi */
> >> +
> >> +} /* namespace libcamera */
> >> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> >> new file mode 100644
> >> index 00000000..3957e342
> >> --- /dev/null
> >> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> >> @@ -0,0 +1,98 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> >> + *
> >> + * rpi_stream.h - Raspberry Pi device stream abstraction class.
> >> + */
> >> +#ifndef __LIBCAMERA_PIPELINE_RPI_STREAM_H__
> >> +#define __LIBCAMERA_PIPELINE_RPI_STREAM_H__
> >> +
> >> +#include <queue>
> >> +#include <string>
> >> +#include <vector>
> >> +
> >> +#include <libcamera/stream.h>
> >> +
> >> +#include "libcamera/internal/v4l2_videodevice.h"
> >> +
> >> +namespace libcamera {
> >> +
> >> +namespace RPi {
> >> +
> >> +/*
> >> + * Device stream abstraction for either an internal or external stream.
> >> + * Used for both Unicam and the ISP.
> >> + */
> >> +class RPiStream : public Stream
> >> +{
> >> +public:
> >> +    RPiStream()
> >> +    {
> >> +    }
> >> +
> >> +    RPiStream(const char *name, MediaEntity *dev, bool importOnly = false)
> >> +            : external_(false), importOnly_(importOnly), name_(name),
> >> +              dev_(std::make_unique<V4L2VideoDevice>(dev))
> >> +    {
> >> +    }
> >> +
>
>
> As mentioned by the time I got to 6/9 (which I looked at last for some
> reason), this class became hard to parse as there are so many members so
> close together
>
>
> Although maybe it was because I was looking at a patch diff, but this
> seems smaller, I guess the diff adds an extra line for the +/- entries ;-)
>
> But perhaps breaking this up a bit below with:
>
>
>
> >> +    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 releaseBuffers();
>
> >> +    int importBuffers(unsigned int count);
> >> +    int allocateBuffers(unsigned int count);
> >> +    int queueBuffers();
> >> +    bool findFrameBuffer(FrameBuffer *buffer) const;
>
> I've punched newlines above (I hope that comes through in the mail) ...
> but really all I was wondering is if there is a way to group related
> functions into their own blocks.
>
>
> >> +
> >> +private:
> >> +    /*
> >> +     * Indicates that this stream is active externally, i.e. the buffers
> >> +     * are provided by the application.
> >> +     */
> >> +    bool external_;
>
> >> +    /* Indicates that this stream only imports buffers, e.g. ISP input. */
> >> +    bool importOnly_;
>
> >> +    /* Stream name identifier. */
> >> +    std::string name_;
>
> >> +    /* The actual device stream. */
> >> +    std::unique_ptr<V4L2VideoDevice> dev_;
>
> >> +    /* Internally allocated framebuffers associated with this device stream. */
> >> +    std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_;
>
> >> +    /* Externally allocated framebuffers associated with this device stream. */
> >> +    std::vector<std::unique_ptr<FrameBuffer>> *externalBuffers_;
> >> +};
>
> When there's a comment line above an entry, for me it really helps to
> have a newline before it.
>
>
> Anyway, nothing critical here, just where I think some whitespace would
> have helped my weary eyes yesterday ;-)
>
>
>
> >> +
> >> +/*
> >> + * The following class is just a convenient (and typesafe) array of device
> >> + * streams indexed with an enum class.
> >> + */
> >> +template<typename E, std::size_t N>
> >> +class RPiDevice : public std::array<class RPiStream, N>
> >> +{
> >> +private:
> >> +    constexpr auto index(E e) const noexcept
> >> +    {
> >> +            return static_cast<std::underlying_type_t<E>>(e);
> >> +    }
> >> +public:
> >> +    RPiStream &operator[](E e)
> >> +    {
> >> +            return std::array<class RPiStream, N>::operator[](index(e));
> >> +    }
> >> +    const RPiStream &operator[](E e) const
> >> +    {
> >> +            return std::array<class RPiStream, N>::operator[](index(e));
> >> +    }
> >> +};
> >> +
> >> +} /* namespace RPi */
> >> +
> >> +} /* namespace libcamera */
> >> +
> >> +#endif /* __LIBCAMERA_PIPELINE_RPI_STREAM_H__ */
> >>
> >
>
> --
> Regards
> --
> Kieran
Laurent Pinchart July 22, 2020, 2:17 p.m. UTC | #4 
Hi Naush,

Thank you for the patch.

On Wed, Jul 22, 2020 at 01:45:31PM +0100, Naushir Patuck wrote:
> Hi Kieran,
> 
> Thank you for the review.  I'll make all the suggested changes and
> submit a new version soon.
> 
> Regards,
> Naush
> 
> On Wed, 22 Jul 2020 at 13:29, Kieran Bingham wrote:
> > On 21/07/2020 11:45, Kieran Bingham wrote:
> >> On 20/07/2020 10:13, Naushir Patuck wrote:
> >>> Put RPiStream into the RPi namespace and add a new log category (RPISTREAM).
> >>> There are no functional changes in this commit.
> >>>
> >>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> >>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >>
> >> Great, I love seeing this become more modular, I think that will help
> >> navigating code and maintenance.
> >>
> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >>
> >>> ---
> >>>  .../pipeline/raspberrypi/meson.build          |   1 +
> >>>  .../pipeline/raspberrypi/raspberrypi.cpp      | 193 ++----------------
> >>>  .../pipeline/raspberrypi/rpi_stream.cpp       | 116 +++++++++++
> >>>  .../pipeline/raspberrypi/rpi_stream.h         |  98 +++++++++
> >>>  4 files changed, 234 insertions(+), 174 deletions(-)
> >>>  create mode 100644 src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> >>>  create mode 100644 src/libcamera/pipeline/raspberrypi/rpi_stream.h
> >>>
> >>> diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build
> >>> index ae0aed3b..7c5b6ff7 100644
> >>> --- a/src/libcamera/pipeline/raspberrypi/meson.build
> >>> +++ b/src/libcamera/pipeline/raspberrypi/meson.build
> >>> @@ -3,5 +3,6 @@
> >>>  libcamera_sources += files([
> >>>      'dma_heaps.cpp',
> >>>      'raspberrypi.cpp',
> >>> +    'rpi_stream.cpp',
> >>>      'staggered_ctrl.cpp',
> >>>  ])
> >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> index bf1c7714..6630ef57 100644
> >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> @@ -19,7 +19,6 @@
> >>>  #include <libcamera/logging.h>
> >>>  #include <libcamera/property_ids.h>
> >>>  #include <libcamera/request.h>
> >>> -#include <libcamera/stream.h>
> >>>
> >>>  #include <linux/videodev2.h>
> >>>
> >>> @@ -33,6 +32,7 @@
> >>>  #include "libcamera/internal/v4l2_videodevice.h"
> >>>
> >>>  #include "dma_heaps.h"
> >>> +#include "rpi_stream.h"
> >>>  #include "staggered_ctrl.h"
> >>>
> >>>  namespace libcamera {
> >>> @@ -123,165 +123,10 @@ V4L2DeviceFormat findBestMode(V4L2PixFmtMap &formatsMap, const Size &req)
> >>>      return bestMode;
> >>>  }
> >>>
> >>> -} /* namespace */
> >>> -
> >>> -/*
> >>> - * Device stream abstraction for either an internal or external stream.
> >>> - * Used for both Unicam and the ISP.
> >>> - */
> >>> -class RPiStream : public Stream
> >>> -{
> >>> -public:
> >>> -    RPiStream()
> >>> -    {
> >>> -    }
> >>> -
> >>> -    RPiStream(const char *name, MediaEntity *dev, bool importOnly = false)
> >>> -            : external_(false), importOnly_(importOnly), name_(name),
> >>> -              dev_(std::make_unique<V4L2VideoDevice>(dev))
> >>> -    {
> >>> -    }
> >>> -
> >>> -    V4L2VideoDevice *dev() const
> >>> -    {
> >>> -            return dev_.get();
> >>> -    }
> >>> -
> >>> -    void setExternal(bool external)
> >>> -    {
> >>> -            external_ = external;
> >>> -    }
> >>> -
> >>> -    bool 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 isImporter() const
> >>> -    {
> >>> -            return importOnly_;
> >>> -    }
> >>> -
> >>> -    void reset()
> >>> -    {
> >>> -            external_ = false;
> >>> -            internalBuffers_.clear();
> >>> -    }
> >>> -
> >>> -    std::string name() const
> >>> -    {
> >>> -            return name_;
> >>> -    }
> >>> -
> >>> -    void setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >>> -    {
> >>> -            externalBuffers_ = buffers;
> >>> -    }
> >>> -
> >>> -    const std::vector<std::unique_ptr<FrameBuffer>> *getBuffers() const
> >>> -    {
> >>> -            return external_ ? externalBuffers_ : &internalBuffers_;
> >>> -    }
> >>> -
> >>> -    void releaseBuffers()
> >>> -    {
> >>> -            dev_->releaseBuffers();
> >>> -            if (!external_ && !importOnly_)
> >>> -                    internalBuffers_.clear();
> >>> -    }
> >>> -
> >>> -    int importBuffers(unsigned int count)
> >>> -    {
> >>> -            return dev_->importBuffers(count);
> >>> -    }
> >>> -
> >>> -    int allocateBuffers(unsigned int count)
> >>> -    {
> >>> -            return dev_->allocateBuffers(count, &internalBuffers_);
> >>> -    }
> >>> -
> >>> -    int queueBuffers()
> >>> -    {
> >>> -            if (external_)
> >>> -                    return 0;
> >>> -
> >>> -            for (auto &b : internalBuffers_) {
> >>> -                    int ret = dev_->queueBuffer(b.get());
> >>> -                    if (ret) {
> >>> -                            LOG(RPI, Error) << "Failed to queue buffers for "
> >>> -                                            << name_;
> >>> -                            return ret;
> >>> -                    }
> >>> -            }
> >>> -
> >>> -            return 0;
> >>> -    }
> >>> -
> >>> -    bool findFrameBuffer(FrameBuffer *buffer) const
> >>> -    {
> >>> -            auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();
> >>> -            auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();
> >>> -
> >>> -            if (importOnly_)
> >>> -                    return false;
> >>> -
> >>> -            if (std::find_if(start, end,
> >>> -                             [buffer](std::unique_ptr<FrameBuffer> const &ref) { return ref.get() == buffer; }) != end)
> >>> -                    return true;
> >>> -
> >>> -            return false;
> >>> -    }
> >>> -
> >>> -private:
> >>> -    /*
> >>> -     * Indicates that this stream is active externally, i.e. the buffers
> >>> -     * are provided by the application.
> >>> -     */
> >>> -    bool external_;
> >>> -    /* Indicates that this stream only imports buffers, e.g. ISP input. */
> >>> -    bool importOnly_;
> >>> -    /* Stream name identifier. */
> >>> -    std::string name_;
> >>> -    /* The actual device stream. */
> >>> -    std::unique_ptr<V4L2VideoDevice> dev_;
> >>> -    /* Internally allocated framebuffers associated with this device stream. */
> >>> -    std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_;
> >>> -    /* Externally allocated framebuffers associated with this device stream. */
> >>> -    std::vector<std::unique_ptr<FrameBuffer>> *externalBuffers_;
> >>> -};
> >>> -
> >>> -/*
> >>> - * The following class is just a convenient (and typesafe) array of device
> >>> - * streams indexed with an enum class.
> >>> - */
> >>>  enum class Unicam : unsigned int { Image, Embedded };
> >>>  enum class Isp : unsigned int { Input, Output0, Output1, Stats };
> >>>
> >>> -template<typename E, std::size_t N>
> >>> -class RPiDevice : public std::array<class RPiStream, N>
> >>> -{
> >>> -private:
> >>> -    constexpr auto index(E e) const noexcept
> >>> -    {
> >>> -            return static_cast<std::underlying_type_t<E>>(e);
> >>> -    }
> >>> -public:
> >>> -    RPiStream &operator[](E e)
> >>> -    {
> >>> -            return std::array<class RPiStream, N>::operator[](index(e));
> >>> -    }
> >>> -    const RPiStream &operator[](E e) const
> >>> -    {
> >>> -            return std::array<class RPiStream, N>::operator[](index(e));
> >>> -    }
> >>> -};
> >>> +} /* namespace */
> >>>
> >>>  class RPiCameraData : public CameraData
> >>>  {
> >>> @@ -305,15 +150,15 @@ public:
> >>>      void ispOutputDequeue(FrameBuffer *buffer);
> >>>
> >>>      void clearIncompleteRequests();
> >>> -    void handleStreamBuffer(FrameBuffer *buffer, const RPiStream *stream);
> >>> +    void handleStreamBuffer(FrameBuffer *buffer, const RPi::RPiStream *stream);
> >>>      void handleState();
> >>>
> >>>      CameraSensor *sensor_;
> >>>      /* Array of Unicam and ISP device streams and associated buffers/streams. */
> >>> -    RPiDevice<Unicam, 2> unicam_;
> >>> -    RPiDevice<Isp, 4> isp_;
> >>> +    RPi::RPiDevice<Unicam, 2> unicam_;
> >>> +    RPi::RPiDevice<Isp, 4> isp_;
> >>>      /* The vector below is just for convenience when iterating over all streams. */
> >>> -    std::vector<RPiStream *> streams_;
> >>> +    std::vector<RPi::RPiStream *> streams_;
> >>>      /* Buffers passed to the IPA. */
> >>>      std::vector<IPABuffer> ipaBuffers_;
> >>>
> >>> @@ -762,7 +607,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >>>  int PipelineHandlerRPi::exportFrameBuffers(Camera *camera, Stream *stream,
> >>>                                         std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >>>  {
> >>> -    RPiStream *s = static_cast<RPiStream *>(stream);
> >>> +    RPi::RPiStream *s = static_cast<RPi::RPiStream *>(stream);
> >>>      unsigned int count = stream->configuration().bufferCount;
> >>>      int ret = s->dev()->exportBuffers(count, buffers);
> >>>
> >>> @@ -908,14 +753,14 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> >>>      std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this);
> >>>
> >>>      /* Locate and open the unicam video streams. */
> >>> -    data->unicam_[Unicam::Embedded] = RPiStream("Unicam Embedded", unicam_->getEntityByName("unicam-embedded"));
> >>> -    data->unicam_[Unicam::Image] = RPiStream("Unicam Image", unicam_->getEntityByName("unicam-image"));
> >>> +    data->unicam_[Unicam::Embedded] = RPi::RPiStream("Unicam Embedded", unicam_->getEntityByName("unicam-embedded"));
> >>> +    data->unicam_[Unicam::Image] = RPi::RPiStream("Unicam Image", unicam_->getEntityByName("unicam-image"));
> >>>
> >>>      /* Tag the ISP input stream as an import stream. */
> >>> -    data->isp_[Isp::Input] = RPiStream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true);
> >>> -    data->isp_[Isp::Output0] = RPiStream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1"));
> >>> -    data->isp_[Isp::Output1] = RPiStream("ISP Output1", isp_->getEntityByName("bcm2835-isp0-capture2"));
> >>> -    data->isp_[Isp::Stats] = RPiStream("ISP Stats", isp_->getEntityByName("bcm2835-isp0-capture3"));
> >>> +    data->isp_[Isp::Input] = RPi::RPiStream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true);
> >>> +    data->isp_[Isp::Output0] = RPi::RPiStream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1"));
> >>> +    data->isp_[Isp::Output1] = RPi::RPiStream("ISP Output1", isp_->getEntityByName("bcm2835-isp0-capture2"));
> >>> +    data->isp_[Isp::Stats] = RPi::RPiStream("ISP Stats", isp_->getEntityByName("bcm2835-isp0-capture3"));
> >>>
> >>>      /* This is just for convenience so that we can easily iterate over all streams. */
> >>>      for (auto &stream : data->unicam_)
> >>> @@ -1005,7 +850,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >>>       */
> >>>      unsigned int maxBuffers = 0;
> >>>      for (const Stream *s : camera->streams())
> >>> -            if (static_cast<const RPiStream *>(s)->isExternal())
> >>> +            if (static_cast<const RPi::RPiStream *>(s)->isExternal())
> >>>                      maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
> >>>
> >>>      for (auto const stream : data->streams_) {
> >>> @@ -1255,12 +1100,12 @@ done:
> >>>
> >>>  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> >>>  {
> >>> -    const RPiStream *stream = nullptr;
> >>> +    const RPi::RPiStream *stream = nullptr;
> >>>
> >>>      if (state_ == State::Stopped)
> >>>              return;
> >>>
> >>> -    for (RPiStream const &s : unicam_) {
> >>> +    for (RPi::RPiStream const &s : unicam_) {
> >>>              if (s.findFrameBuffer(buffer)) {
> >>>                      stream = &s;
> >>>                      break;
> >>> @@ -1316,12 +1161,12 @@ void RPiCameraData::ispInputDequeue(FrameBuffer *buffer)
> >>>
> >>>  void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> >>>  {
> >>> -    const RPiStream *stream = nullptr;
> >>> +    const RPi::RPiStream *stream = nullptr;
> >>>
> >>>      if (state_ == State::Stopped)
> >>>              return;
> >>>
> >>> -    for (RPiStream const &s : isp_) {
> >>> +    for (RPi::RPiStream const &s : isp_) {
> >>>              if (s.findFrameBuffer(buffer)) {
> >>>                      stream = &s;
> >>>                      break;
> >>> @@ -1402,7 +1247,7 @@ void RPiCameraData::clearIncompleteRequests()
> >>>      }
> >>>  }
> >>>
> >>> -void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, const RPiStream *stream)
> >>> +void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, const RPi::RPiStream *stream)
> >>>  {
> >>>      if (stream->isExternal()) {
> >>>              if (!dropFrame_) {
> >>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> >>> new file mode 100644
> >>> index 00000000..2edb8b59
> >>> --- /dev/null
> >>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> >>> @@ -0,0 +1,116 @@
> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>> +/*
> >>> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> >>> + *
> >>> + * rpi_stream.cpp - Raspberry Pi device stream abstraction class.
> >>> + */
> >>> +#include "rpi_stream.h"
> >>> +
> >>> +#include "libcamera/internal/log.h"
> >>> +
> >>> +namespace libcamera {
> >>> +
> >>> +LOG_DEFINE_CATEGORY(RPISTREAM)
> >>> +
> >>> +namespace RPi {
> >>> +
> >>> +V4L2VideoDevice *RPiStream::dev() const
> >>> +{
> >>> +    return dev_.get();
> >>> +}
> >>> +
> >>> +void RPiStream::setExternal(bool external)
> >>> +{
> >>> +    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_;
> >>> +}
> >>> +
> >>> +void RPiStream::reset()
> >>> +{
> >>> +    external_ = false;
> >>> +    internalBuffers_.clear();
> >>> +}
> >>> +
> >>> +std::string RPiStream::name() const
> >>> +{
> >>> +    return name_;
> >>> +}
> >>> +
> >>> +void RPiStream::setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >>> +{
> >>> +    externalBuffers_ = buffers;
> >>> +}
> >>> +
> >>> +const std::vector<std::unique_ptr<FrameBuffer>> *RPiStream::getBuffers() const
> >>> +{
> >>> +    return external_ ? externalBuffers_ : &internalBuffers_;
> >>> +}
> >>> +
> >>> +void RPiStream::releaseBuffers()
> >>> +{
> >>> +    dev_->releaseBuffers();
> >>> +    if (!external_ && !importOnly_)
> >>> +            internalBuffers_.clear();
> >>> +}
> >>> +
> >>> +int RPiStream::importBuffers(unsigned int count)
> >>> +{
> >>> +    return dev_->importBuffers(count);
> >>> +}
> >>> +
> >>> +int RPiStream::allocateBuffers(unsigned int count)
> >>> +{
> >>> +    return dev_->allocateBuffers(count, &internalBuffers_);
> >>> +}
> >>> +
> >>> +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_;
> >>> +                    return ret;
> >>> +            }
> >>> +    }
> >>> +
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> >>> +{
> >>> +    auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();
> >>> +    auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();
> >>> +
> >>> +    if (importOnly_)
> >>> +            return false;
> >>> +
> >>> +    if (std::find_if(start, end,
> >>> +                     [buffer](std::unique_ptr<FrameBuffer> const &ref) { return ref.get() == buffer; }) != end)
> >>> +            return true;
> >>> +
> >>> +    return false;
> >>> +}
> >>> +
> >>> +} /* namespace RPi */
> >>> +
> >>> +} /* namespace libcamera */
> >>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> >>> new file mode 100644
> >>> index 00000000..3957e342
> >>> --- /dev/null
> >>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> >>> @@ -0,0 +1,98 @@
> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>> +/*
> >>> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> >>> + *
> >>> + * rpi_stream.h - Raspberry Pi device stream abstraction class.
> >>> + */
> >>> +#ifndef __LIBCAMERA_PIPELINE_RPI_STREAM_H__
> >>> +#define __LIBCAMERA_PIPELINE_RPI_STREAM_H__
> >>> +
> >>> +#include <queue>
> >>> +#include <string>
> >>> +#include <vector>
> >>> +
> >>> +#include <libcamera/stream.h>
> >>> +
> >>> +#include "libcamera/internal/v4l2_videodevice.h"
> >>> +
> >>> +namespace libcamera {
> >>> +
> >>> +namespace RPi {
> >>> +
> >>> +/*
> >>> + * Device stream abstraction for either an internal or external stream.
> >>> + * Used for both Unicam and the ISP.
> >>> + */
> >>> +class RPiStream : public Stream

Drive-by comment, as this class is in the RPi namespace, you could name
is Stream. Same for the RPiDevice class below. Up to you (and of course
not for this patch).

> >>> +{
> >>> +public:
> >>> +    RPiStream()
> >>> +    {
> >>> +    }
> >>> +
> >>> +    RPiStream(const char *name, MediaEntity *dev, bool importOnly = false)
> >>> +            : external_(false), importOnly_(importOnly), name_(name),
> >>> +              dev_(std::make_unique<V4L2VideoDevice>(dev))
> >>> +    {
> >>> +    }
> >>> +
> >
> > As mentioned by the time I got to 6/9 (which I looked at last for some
> > reason), this class became hard to parse as there are so many members so
> > close together
> >
> >
> > Although maybe it was because I was looking at a patch diff, but this
> > seems smaller, I guess the diff adds an extra line for the +/- entries ;-)
> >
> > But perhaps breaking this up a bit below with:
> >
> >>> +    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 releaseBuffers();
> >
> >>> +    int importBuffers(unsigned int count);
> >>> +    int allocateBuffers(unsigned int count);
> >>> +    int queueBuffers();
> >>> +    bool findFrameBuffer(FrameBuffer *buffer) const;
> >
> > I've punched newlines above (I hope that comes through in the mail) ...
> > but really all I was wondering is if there is a way to group related
> > functions into their own blocks.

For what it's worth, I also like separating groups of related members
with blank lines.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> >>> +
> >>> +private:
> >>> +    /*
> >>> +     * Indicates that this stream is active externally, i.e. the buffers
> >>> +     * are provided by the application.
> >>> +     */
> >>> +    bool external_;
> >
> >>> +    /* Indicates that this stream only imports buffers, e.g. ISP input. */
> >>> +    bool importOnly_;
> >
> >>> +    /* Stream name identifier. */
> >>> +    std::string name_;
> >
> >>> +    /* The actual device stream. */
> >>> +    std::unique_ptr<V4L2VideoDevice> dev_;
> >
> >>> +    /* Internally allocated framebuffers associated with this device stream. */
> >>> +    std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_;
> >
> >>> +    /* Externally allocated framebuffers associated with this device stream. */
> >>> +    std::vector<std::unique_ptr<FrameBuffer>> *externalBuffers_;
> >>> +};
> >
> > When there's a comment line above an entry, for me it really helps to
> > have a newline before it.
> >
> > Anyway, nothing critical here, just where I think some whitespace would
> > have helped my weary eyes yesterday ;-)
> >
> >>> +
> >>> +/*
> >>> + * The following class is just a convenient (and typesafe) array of device
> >>> + * streams indexed with an enum class.
> >>> + */
> >>> +template<typename E, std::size_t N>
> >>> +class RPiDevice : public std::array<class RPiStream, N>
> >>> +{
> >>> +private:
> >>> +    constexpr auto index(E e) const noexcept
> >>> +    {
> >>> +            return static_cast<std::underlying_type_t<E>>(e);
> >>> +    }
> >>> +public:
> >>> +    RPiStream &operator[](E e)
> >>> +    {
> >>> +            return std::array<class RPiStream, N>::operator[](index(e));
> >>> +    }
> >>> +    const RPiStream &operator[](E e) const
> >>> +    {
> >>> +            return std::array<class RPiStream, N>::operator[](index(e));
> >>> +    }
> >>> +};
> >>> +
> >>> +} /* namespace RPi */
> >>> +
> >>> +} /* namespace libcamera */
> >>> +
> >>> +#endif /* __LIBCAMERA_PIPELINE_RPI_STREAM_H__ */

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build
index ae0aed3b..7c5b6ff7 100644
--- a/src/libcamera/pipeline/raspberrypi/meson.build
+++ b/src/libcamera/pipeline/raspberrypi/meson.build
@@ -3,5 +3,6 @@ 
 libcamera_sources += files([
     'dma_heaps.cpp',
     'raspberrypi.cpp',
+    'rpi_stream.cpp',
     'staggered_ctrl.cpp',
 ])
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index bf1c7714..6630ef57 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -19,7 +19,6 @@ 
 #include <libcamera/logging.h>
 #include <libcamera/property_ids.h>
 #include <libcamera/request.h>
-#include <libcamera/stream.h>
 
 #include <linux/videodev2.h>
 
@@ -33,6 +32,7 @@ 
 #include "libcamera/internal/v4l2_videodevice.h"
 
 #include "dma_heaps.h"
+#include "rpi_stream.h"
 #include "staggered_ctrl.h"
 
 namespace libcamera {
@@ -123,165 +123,10 @@  V4L2DeviceFormat findBestMode(V4L2PixFmtMap &formatsMap, const Size &req)
 	return bestMode;
 }
 
-} /* namespace */
-
-/*
- * Device stream abstraction for either an internal or external stream.
- * Used for both Unicam and the ISP.
- */
-class RPiStream : public Stream
-{
-public:
-	RPiStream()
-	{
-	}
-
-	RPiStream(const char *name, MediaEntity *dev, bool importOnly = false)
-		: external_(false), importOnly_(importOnly), name_(name),
-		  dev_(std::make_unique<V4L2VideoDevice>(dev))
-	{
-	}
-
-	V4L2VideoDevice *dev() const
-	{
-		return dev_.get();
-	}
-
-	void setExternal(bool external)
-	{
-		external_ = external;
-	}
-
-	bool 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 isImporter() const
-	{
-		return importOnly_;
-	}
-
-	void reset()
-	{
-		external_ = false;
-		internalBuffers_.clear();
-	}
-
-	std::string name() const
-	{
-		return name_;
-	}
-
-	void setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
-	{
-		externalBuffers_ = buffers;
-	}
-
-	const std::vector<std::unique_ptr<FrameBuffer>> *getBuffers() const
-	{
-		return external_ ? externalBuffers_ : &internalBuffers_;
-	}
-
-	void releaseBuffers()
-	{
-		dev_->releaseBuffers();
-		if (!external_ && !importOnly_)
-			internalBuffers_.clear();
-	}
-
-	int importBuffers(unsigned int count)
-	{
-		return dev_->importBuffers(count);
-	}
-
-	int allocateBuffers(unsigned int count)
-	{
-		return dev_->allocateBuffers(count, &internalBuffers_);
-	}
-
-	int queueBuffers()
-	{
-		if (external_)
-			return 0;
-
-		for (auto &b : internalBuffers_) {
-			int ret = dev_->queueBuffer(b.get());
-			if (ret) {
-				LOG(RPI, Error) << "Failed to queue buffers for "
-						<< name_;
-				return ret;
-			}
-		}
-
-		return 0;
-	}
-
-	bool findFrameBuffer(FrameBuffer *buffer) const
-	{
-		auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();
-		auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();
-
-		if (importOnly_)
-			return false;
-
-		if (std::find_if(start, end,
-				 [buffer](std::unique_ptr<FrameBuffer> const &ref) { return ref.get() == buffer; }) != end)
-			return true;
-
-		return false;
-	}
-
-private:
-	/*
-	 * Indicates that this stream is active externally, i.e. the buffers
-	 * are provided by the application.
-	 */
-	bool external_;
-	/* Indicates that this stream only imports buffers, e.g. ISP input. */
-	bool importOnly_;
-	/* Stream name identifier. */
-	std::string name_;
-	/* The actual device stream. */
-	std::unique_ptr<V4L2VideoDevice> dev_;
-	/* Internally allocated framebuffers associated with this device stream. */
-	std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_;
-	/* Externally allocated framebuffers associated with this device stream. */
-	std::vector<std::unique_ptr<FrameBuffer>> *externalBuffers_;
-};
-
-/*
- * The following class is just a convenient (and typesafe) array of device
- * streams indexed with an enum class.
- */
 enum class Unicam : unsigned int { Image, Embedded };
 enum class Isp : unsigned int { Input, Output0, Output1, Stats };
 
-template<typename E, std::size_t N>
-class RPiDevice : public std::array<class RPiStream, N>
-{
-private:
-	constexpr auto index(E e) const noexcept
-	{
-		return static_cast<std::underlying_type_t<E>>(e);
-	}
-public:
-	RPiStream &operator[](E e)
-	{
-		return std::array<class RPiStream, N>::operator[](index(e));
-	}
-	const RPiStream &operator[](E e) const
-	{
-		return std::array<class RPiStream, N>::operator[](index(e));
-	}
-};
+} /* namespace */
 
 class RPiCameraData : public CameraData
 {
@@ -305,15 +150,15 @@  public:
 	void ispOutputDequeue(FrameBuffer *buffer);
 
 	void clearIncompleteRequests();
-	void handleStreamBuffer(FrameBuffer *buffer, const RPiStream *stream);
+	void handleStreamBuffer(FrameBuffer *buffer, const RPi::RPiStream *stream);
 	void handleState();
 
 	CameraSensor *sensor_;
 	/* Array of Unicam and ISP device streams and associated buffers/streams. */
-	RPiDevice<Unicam, 2> unicam_;
-	RPiDevice<Isp, 4> isp_;
+	RPi::RPiDevice<Unicam, 2> unicam_;
+	RPi::RPiDevice<Isp, 4> isp_;
 	/* The vector below is just for convenience when iterating over all streams. */
-	std::vector<RPiStream *> streams_;
+	std::vector<RPi::RPiStream *> streams_;
 	/* Buffers passed to the IPA. */
 	std::vector<IPABuffer> ipaBuffers_;
 
@@ -762,7 +607,7 @@  int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
 int PipelineHandlerRPi::exportFrameBuffers(Camera *camera, Stream *stream,
 					   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
 {
-	RPiStream *s = static_cast<RPiStream *>(stream);
+	RPi::RPiStream *s = static_cast<RPi::RPiStream *>(stream);
 	unsigned int count = stream->configuration().bufferCount;
 	int ret = s->dev()->exportBuffers(count, buffers);
 
@@ -908,14 +753,14 @@  bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
 	std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this);
 
 	/* Locate and open the unicam video streams. */
-	data->unicam_[Unicam::Embedded] = RPiStream("Unicam Embedded", unicam_->getEntityByName("unicam-embedded"));
-	data->unicam_[Unicam::Image] = RPiStream("Unicam Image", unicam_->getEntityByName("unicam-image"));
+	data->unicam_[Unicam::Embedded] = RPi::RPiStream("Unicam Embedded", unicam_->getEntityByName("unicam-embedded"));
+	data->unicam_[Unicam::Image] = RPi::RPiStream("Unicam Image", unicam_->getEntityByName("unicam-image"));
 
 	/* Tag the ISP input stream as an import stream. */
-	data->isp_[Isp::Input] = RPiStream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true);
-	data->isp_[Isp::Output0] = RPiStream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1"));
-	data->isp_[Isp::Output1] = RPiStream("ISP Output1", isp_->getEntityByName("bcm2835-isp0-capture2"));
-	data->isp_[Isp::Stats] = RPiStream("ISP Stats", isp_->getEntityByName("bcm2835-isp0-capture3"));
+	data->isp_[Isp::Input] = RPi::RPiStream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true);
+	data->isp_[Isp::Output0] = RPi::RPiStream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1"));
+	data->isp_[Isp::Output1] = RPi::RPiStream("ISP Output1", isp_->getEntityByName("bcm2835-isp0-capture2"));
+	data->isp_[Isp::Stats] = RPi::RPiStream("ISP Stats", isp_->getEntityByName("bcm2835-isp0-capture3"));
 
 	/* This is just for convenience so that we can easily iterate over all streams. */
 	for (auto &stream : data->unicam_)
@@ -1005,7 +850,7 @@  int PipelineHandlerRPi::prepareBuffers(Camera *camera)
 	 */
 	unsigned int maxBuffers = 0;
 	for (const Stream *s : camera->streams())
-		if (static_cast<const RPiStream *>(s)->isExternal())
+		if (static_cast<const RPi::RPiStream *>(s)->isExternal())
 			maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
 
 	for (auto const stream : data->streams_) {
@@ -1255,12 +1100,12 @@  done:
 
 void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
 {
-	const RPiStream *stream = nullptr;
+	const RPi::RPiStream *stream = nullptr;
 
 	if (state_ == State::Stopped)
 		return;
 
-	for (RPiStream const &s : unicam_) {
+	for (RPi::RPiStream const &s : unicam_) {
 		if (s.findFrameBuffer(buffer)) {
 			stream = &s;
 			break;
@@ -1316,12 +1161,12 @@  void RPiCameraData::ispInputDequeue(FrameBuffer *buffer)
 
 void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
 {
-	const RPiStream *stream = nullptr;
+	const RPi::RPiStream *stream = nullptr;
 
 	if (state_ == State::Stopped)
 		return;
 
-	for (RPiStream const &s : isp_) {
+	for (RPi::RPiStream const &s : isp_) {
 		if (s.findFrameBuffer(buffer)) {
 			stream = &s;
 			break;
@@ -1402,7 +1247,7 @@  void RPiCameraData::clearIncompleteRequests()
 	}
 }
 
-void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, const RPiStream *stream)
+void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, const RPi::RPiStream *stream)
 {
 	if (stream->isExternal()) {
 		if (!dropFrame_) {
diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
new file mode 100644
index 00000000..2edb8b59
--- /dev/null
+++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
@@ -0,0 +1,116 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
+ *
+ * rpi_stream.cpp - Raspberry Pi device stream abstraction class.
+ */
+#include "rpi_stream.h"
+
+#include "libcamera/internal/log.h"
+
+namespace libcamera {
+
+LOG_DEFINE_CATEGORY(RPISTREAM)
+
+namespace RPi {
+
+V4L2VideoDevice *RPiStream::dev() const
+{
+	return dev_.get();
+}
+
+void RPiStream::setExternal(bool external)
+{
+	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_;
+}
+
+void RPiStream::reset()
+{
+	external_ = false;
+	internalBuffers_.clear();
+}
+
+std::string RPiStream::name() const
+{
+	return name_;
+}
+
+void RPiStream::setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
+{
+	externalBuffers_ = buffers;
+}
+
+const std::vector<std::unique_ptr<FrameBuffer>> *RPiStream::getBuffers() const
+{
+	return external_ ? externalBuffers_ : &internalBuffers_;
+}
+
+void RPiStream::releaseBuffers()
+{
+	dev_->releaseBuffers();
+	if (!external_ && !importOnly_)
+		internalBuffers_.clear();
+}
+
+int RPiStream::importBuffers(unsigned int count)
+{
+	return dev_->importBuffers(count);
+}
+
+int RPiStream::allocateBuffers(unsigned int count)
+{
+	return dev_->allocateBuffers(count, &internalBuffers_);
+}
+
+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_;
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
+{
+	auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();
+	auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();
+
+	if (importOnly_)
+		return false;
+
+	if (std::find_if(start, end,
+			 [buffer](std::unique_ptr<FrameBuffer> const &ref) { return ref.get() == buffer; }) != end)
+		return true;
+
+	return false;
+}
+
+} /* namespace RPi */
+
+} /* namespace libcamera */
diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
new file mode 100644
index 00000000..3957e342
--- /dev/null
+++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
@@ -0,0 +1,98 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
+ *
+ * rpi_stream.h - Raspberry Pi device stream abstraction class.
+ */
+#ifndef __LIBCAMERA_PIPELINE_RPI_STREAM_H__
+#define __LIBCAMERA_PIPELINE_RPI_STREAM_H__
+
+#include <queue>
+#include <string>
+#include <vector>
+
+#include <libcamera/stream.h>
+
+#include "libcamera/internal/v4l2_videodevice.h"
+
+namespace libcamera {
+
+namespace RPi {
+
+/*
+ * Device stream abstraction for either an internal or external stream.
+ * Used for both Unicam and the ISP.
+ */
+class RPiStream : public Stream
+{
+public:
+	RPiStream()
+	{
+	}
+
+	RPiStream(const char *name, MediaEntity *dev, bool importOnly = false)
+		: external_(false), importOnly_(importOnly), name_(name),
+		  dev_(std::make_unique<V4L2VideoDevice>(dev))
+	{
+	}
+
+	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 releaseBuffers();
+	int importBuffers(unsigned int count);
+	int allocateBuffers(unsigned int count);
+	int queueBuffers();
+	bool findFrameBuffer(FrameBuffer *buffer) const;
+
+private:
+	/*
+	 * Indicates that this stream is active externally, i.e. the buffers
+	 * are provided by the application.
+	 */
+	bool external_;
+	/* Indicates that this stream only imports buffers, e.g. ISP input. */
+	bool importOnly_;
+	/* Stream name identifier. */
+	std::string name_;
+	/* The actual device stream. */
+	std::unique_ptr<V4L2VideoDevice> dev_;
+	/* Internally allocated framebuffers associated with this device stream. */
+	std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_;
+	/* Externally allocated framebuffers associated with this device stream. */
+	std::vector<std::unique_ptr<FrameBuffer>> *externalBuffers_;
+};
+
+/*
+ * The following class is just a convenient (and typesafe) array of device
+ * streams indexed with an enum class.
+ */
+template<typename E, std::size_t N>
+class RPiDevice : public std::array<class RPiStream, N>
+{
+private:
+	constexpr auto index(E e) const noexcept
+	{
+		return static_cast<std::underlying_type_t<E>>(e);
+	}
+public:
+	RPiStream &operator[](E e)
+	{
+		return std::array<class RPiStream, N>::operator[](index(e));
+	}
+	const RPiStream &operator[](E e) const
+	{
+		return std::array<class RPiStream, N>::operator[](index(e));
+	}
+};
+
+} /* namespace RPi */
+
+} /* namespace libcamera */
+
+#endif /* __LIBCAMERA_PIPELINE_RPI_STREAM_H__ */