[libcamera-devel] [PATCH v4 1/9] libcamera: pipeline: raspberrypi: Move RPiStream into a separate file
Naushir Patuck
naush at raspberrypi.com
Wed Jul 22 14:45:31 CEST 2020
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 at 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 at raspberrypi.com>
> >> Reviewed-by: Niklas Söderlund <niklas.soderlund at 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 at 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
More information about the libcamera-devel
mailing list