[libcamera-devel] [PATCH v4 1/9] libcamera: pipeline: raspberrypi: Move RPiStream into a separate file

Laurent Pinchart laurent.pinchart at ideasonboard.com
Wed Jul 22 16:17:13 CEST 2020


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 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

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 at 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__ */

-- 
Regards,

Laurent Pinchart


More information about the libcamera-devel mailing list