[PATCH v4 4/4] libcamera: converter: Replace usage of stream index by Stream pointer
Paul Elder
paul.elder at ideasonboard.com
Tue Jul 2 13:52:10 CEST 2024
On Mon, Jun 24, 2024 at 07:18:59PM +0530, Umang Jain wrote:
> The converter interface uses the unsigned int output stream index to map
> to the output frame buffers. This is cumbersome to implement new
> converters because one has to keep around additional book keeping
> to track the streams with their correct indexes.
>
> The v4l2_converter_m2m and simple pipeline handler are adapted to
> use the new interface. This work roped in software ISP as well,
> which also seems to use indexes (although it doesn't implement converter
> interface) because of a common conversionQueue_ queue used for
> converter_ and swIsp_.
>
> The logPrefix is no longer able to generate an index from a stream, and
> is updated to be more expressive by reporting the stream configuration
> instead, for example, reporting "1920x1080-MJPEG" in place of
> "stream0".
>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> include/libcamera/internal/converter.h | 5 ++-
> .../internal/converter/converter_v4l2_m2m.h | 11 ++---
> .../internal/software_isp/software_isp.h | 5 ++-
> src/libcamera/converter.cpp | 6 +--
> .../converter/converter_v4l2_m2m.cpp | 42 ++++++++++---------
> src/libcamera/pipeline/simple/simple.cpp | 14 +++----
> src/libcamera/software_isp/software_isp.cpp | 17 ++++----
> 7 files changed, 52 insertions(+), 48 deletions(-)
>
> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> index 5d74db6b..b51563d7 100644
> --- a/include/libcamera/internal/converter.h
> +++ b/include/libcamera/internal/converter.h
> @@ -26,6 +26,7 @@ namespace libcamera {
> class FrameBuffer;
> class MediaDevice;
> class PixelFormat;
> +class Stream;
> struct StreamConfiguration;
>
> class Converter
> @@ -46,14 +47,14 @@ public:
>
> virtual int configure(const StreamConfiguration &inputCfg,
> const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) = 0;
> - virtual int exportBuffers(unsigned int output, unsigned int count,
> + virtual int exportBuffers(const Stream *stream, unsigned int count,
> std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
>
> virtual int start() = 0;
> virtual void stop() = 0;
>
> virtual int queueBuffers(FrameBuffer *input,
> - const std::map<unsigned int, FrameBuffer *> &outputs) = 0;
> + const std::map<const Stream *, FrameBuffer *> &outputs) = 0;
>
> Signal<FrameBuffer *> inputBufferReady;
> Signal<FrameBuffer *> outputBufferReady;
> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> index 0da62290..b9e59899 100644
> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> @@ -28,6 +28,7 @@ class FrameBuffer;
> class MediaDevice;
> class Size;
> class SizeRange;
> +class Stream;
> struct StreamConfiguration;
> class V4L2M2MDevice;
>
> @@ -47,20 +48,20 @@ public:
>
> int configure(const StreamConfiguration &inputCfg,
> const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfg);
> - int exportBuffers(unsigned int output, unsigned int count,
> + int exportBuffers(const Stream *stream, unsigned int count,
> std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>
> int start();
> void stop();
>
> int queueBuffers(FrameBuffer *input,
> - const std::map<unsigned int, FrameBuffer *> &outputs);
> + const std::map<const Stream *, FrameBuffer *> &outputs);
>
> private:
> class V4L2M2MStream : protected Loggable
> {
> public:
> - V4L2M2MStream(V4L2M2MConverter *converter, unsigned int index);
> + V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream);
>
> bool isValid() const { return m2m_ != nullptr; }
>
> @@ -82,7 +83,7 @@ private:
> void outputBufferReady(FrameBuffer *buffer);
>
> V4L2M2MConverter *converter_;
> - unsigned int index_;
> + const Stream *stream_;
> std::unique_ptr<V4L2M2MDevice> m2m_;
>
> unsigned int inputBufferCount_;
> @@ -91,7 +92,7 @@ private:
>
> std::unique_ptr<V4L2M2MDevice> m2m_;
>
> - std::vector<V4L2M2MStream> streams_;
> + std::map<const Stream *, std::unique_ptr<V4L2M2MStream>> streams_;
> std::map<FrameBuffer *, unsigned int> queue_;
> };
>
> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
> index c5338c05..f8e00003 100644
> --- a/include/libcamera/internal/software_isp/software_isp.h
> +++ b/include/libcamera/internal/software_isp/software_isp.h
> @@ -37,6 +37,7 @@ namespace libcamera {
> class DebayerCpu;
> class FrameBuffer;
> class PixelFormat;
> +class Stream;
> struct StreamConfiguration;
>
> LOG_DECLARE_CATEGORY(SoftwareIsp)
> @@ -62,7 +63,7 @@ public:
> const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,
> const ControlInfoMap &sensorControls);
>
> - int exportBuffers(unsigned int output, unsigned int count,
> + int exportBuffers(const Stream *stream, unsigned int count,
> std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>
> void processStats(const ControlList &sensorControls);
> @@ -71,7 +72,7 @@ public:
> void stop();
>
> int queueBuffers(FrameBuffer *input,
> - const std::map<unsigned int, FrameBuffer *> &outputs);
> + const std::map<const Stream *, FrameBuffer *> &outputs);
>
> void process(FrameBuffer *input, FrameBuffer *output);
>
> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> index d3d38c1b..c5e38937 100644
> --- a/src/libcamera/converter.cpp
> +++ b/src/libcamera/converter.cpp
> @@ -111,12 +111,12 @@ Converter::~Converter()
> /**
> * \fn Converter::exportBuffers()
> * \brief Export buffers from the converter device
> - * \param[in] output Output stream index exporting the buffers
> + * \param[in] stream Output stream pointer exporting the buffers
> * \param[in] count Number of buffers to allocate
> * \param[out] buffers Vector to store allocated buffers
> *
> * This function operates similarly to V4L2VideoDevice::exportBuffers() on the
> - * output stream indicated by the \a output index.
> + * output stream indicated by the \a output.
> *
> * \return The number of allocated buffers on success or a negative error code
> * otherwise
> @@ -137,7 +137,7 @@ Converter::~Converter()
> * \fn Converter::queueBuffers()
> * \brief Queue buffers to converter device
> * \param[in] input The frame buffer to apply the conversion
> - * \param[out] outputs The container holding the output stream indexes and
> + * \param[out] outputs The container holding the output stream pointer and
s/pointer/pointers/
> * their respective frame buffer outputs.
> *
> * This function queues the \a input frame buffer on the output streams of the
> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> index 6309a0c0..2e77872e 100644
> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> @@ -35,8 +35,8 @@ LOG_DECLARE_CATEGORY(Converter)
> * V4L2M2MConverter::V4L2M2MStream
> */
>
> -V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, unsigned int index)
> - : converter_(converter), index_(index)
> +V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream)
> + : converter_(converter), stream_(stream)
> {
> m2m_ = std::make_unique<V4L2M2MDevice>(converter->deviceNode());
>
> @@ -157,7 +157,7 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe
>
> std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const
> {
> - return "stream" + std::to_string(index_);
> + return stream_->configuration().toString();
> }
>
> void V4L2M2MConverter::V4L2M2MStream::outputBufferReady(FrameBuffer *buffer)
> @@ -333,21 +333,24 @@ int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,
> int ret = 0;
>
> streams_.clear();
> - streams_.reserve(outputCfgs.size());
>
> for (unsigned int i = 0; i < outputCfgs.size(); ++i) {
> - V4L2M2MStream &stream = streams_.emplace_back(this, i);
> + const StreamConfiguration &cfg = outputCfgs[i];
> + std::unique_ptr<V4L2M2MStream> stream =
> + std::make_unique<V4L2M2MStream>(this, cfg.stream());
>
> - if (!stream.isValid()) {
> + if (!stream->isValid()) {
> LOG(Converter, Error)
> << "Failed to create stream " << i;
> ret = -EINVAL;
> break;
> }
>
> - ret = stream.configure(inputCfg, outputCfgs[i]);
> + ret = stream->configure(inputCfg, cfg);
> if (ret < 0)
> break;
> +
> + streams_.emplace(cfg.stream(), std::move(stream));
> }
>
> if (ret < 0) {
> @@ -361,13 +364,14 @@ int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,
> /**
> * \copydoc libcamera::Converter::exportBuffers
> */
> -int V4L2M2MConverter::exportBuffers(unsigned int output, unsigned int count,
> +int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,
> std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> {
> - if (output >= streams_.size())
> + auto iter = streams_.find(stream);
> + if (iter == streams_.end())
> return -EINVAL;
>
> - return streams_[output].exportBuffers(count, buffers);
> + return iter->second->exportBuffers(count, buffers);
> }
>
> /**
> @@ -377,8 +381,8 @@ int V4L2M2MConverter::start()
> {
> int ret;
>
> - for (V4L2M2MStream &stream : streams_) {
> - ret = stream.start();
> + for (auto &iter : streams_) {
> + ret = iter.second->start();
> if (ret < 0) {
> stop();
> return ret;
> @@ -393,15 +397,15 @@ int V4L2M2MConverter::start()
> */
> void V4L2M2MConverter::stop()
> {
> - for (V4L2M2MStream &stream : utils::reverse(streams_))
> - stream.stop();
> + for (auto &iter : streams_)
> + iter.second->stop();
> }
>
> /**
> * \copydoc libcamera::Converter::queueBuffers
> */
> int V4L2M2MConverter::queueBuffers(FrameBuffer *input,
> - const std::map<unsigned int, FrameBuffer *> &outputs)
> + const std::map<const Stream *, FrameBuffer *> &outputs)
> {
> std::set<FrameBuffer *> outputBufs;
> int ret;
> @@ -414,11 +418,9 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,
> if (outputs.empty())
> return -EINVAL;
>
> - for (auto [index, buffer] : outputs) {
> + for (auto [stream, buffer] : outputs) {
> if (!buffer)
> return -EINVAL;
> - if (index >= streams_.size())
> - return -EINVAL;
>
> outputBufs.insert(buffer);
> }
> @@ -427,8 +429,8 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,
> return -EINVAL;
>
> /* Queue the input and output buffers to all the streams. */
> - for (auto [index, buffer] : outputs) {
> - ret = streams_[index].queueBuffers(input, buffer);
> + for (auto [stream, buffer] : outputs) {
> + ret = streams_.at(stream)->queueBuffers(input, buffer);
> if (ret < 0)
> return ret;
> }
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index eb36578e..5eb1dd21 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -277,7 +277,7 @@ public:
> std::map<PixelFormat, std::vector<const Configuration *>> formats_;
>
> std::vector<std::unique_ptr<FrameBuffer>> conversionBuffers_;
> - std::queue<std::map<unsigned int, FrameBuffer *>> conversionQueue_;
> + std::queue<std::map<const Stream *, FrameBuffer *>> conversionQueue_;
> bool useConversion_;
>
> std::unique_ptr<Converter> converter_;
> @@ -836,7 +836,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)
> Request *request = buffer->request();
>
> if (useConversion_ && !conversionQueue_.empty()) {
> - const std::map<unsigned int, FrameBuffer *> &outputs =
> + const std::map<const Stream *, FrameBuffer *> &outputs =
> conversionQueue_.front();
> if (!outputs.empty()) {
> FrameBuffer *outputBuffer = outputs.begin()->second;
> @@ -1303,10 +1303,8 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,
> */
> if (data->useConversion_)
> return data->converter_
> - ? data->converter_->exportBuffers(data->streamIndex(stream),
> - count, buffers)
> - : data->swIsp_->exportBuffers(data->streamIndex(stream),
> - count, buffers);
> + ? data->converter_->exportBuffers(stream, count, buffers)
> + : data->swIsp_->exportBuffers(stream, count, buffers);
> else
> return data->video_->exportBuffers(count, buffers);
> }
> @@ -1398,7 +1396,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
> SimpleCameraData *data = cameraData(camera);
> int ret;
>
> - std::map<unsigned int, FrameBuffer *> buffers;
> + std::map<const Stream *, FrameBuffer *> buffers;
>
> for (auto &[stream, buffer] : request->buffers()) {
> /*
> @@ -1407,7 +1405,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
> * completion handler.
> */
> if (data->useConversion_) {
> - buffers.emplace(data->streamIndex(stream), buffer);
> + buffers.emplace(stream, buffer);
> } else {
> ret = data->video_->queueBuffer(buffer);
> if (ret < 0)
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index 3fb7ec8c..f7a518c0 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -241,19 +241,19 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg,
>
> /**
> * \brief Export the buffers from the Software ISP
> - * \param[in] output Output stream index exporting the buffers
> + * \param[in] stream Output stream exporting the buffers
s/ / /
> * \param[in] count Number of buffers to allocate
> * \param[out] buffers Vector to store the allocated buffers
> * \return The number of allocated buffers on success or a negative error code
> * otherwise
> */
> -int SoftwareIsp::exportBuffers(unsigned int output, unsigned int count,
> +int SoftwareIsp::exportBuffers(const Stream *stream, unsigned int count,
> std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> {
> ASSERT(debayer_ != nullptr);
>
> /* single output for now */
> - if (output >= 1)
> + if (stream == nullptr)
> return -EINVAL;
>
> for (unsigned int i = 0; i < count; i++) {
> @@ -280,12 +280,12 @@ int SoftwareIsp::exportBuffers(unsigned int output, unsigned int count,
> /**
> * \brief Queue buffers to Software ISP
> * \param[in] input The input framebuffer
> - * \param[in] outputs The container holding the output stream indexes and
> + * \param[in] outputs The container holding the output stream pointer and
s/pointer/pointers/
Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> * their respective frame buffer outputs
> * \return 0 on success, a negative errno on failure
> */
> int SoftwareIsp::queueBuffers(FrameBuffer *input,
> - const std::map<unsigned int, FrameBuffer *> &outputs)
> + const std::map<const Stream *, FrameBuffer *> &outputs)
> {
> /*
> * Validate the outputs as a sanity check: at least one output is
> @@ -294,14 +294,15 @@ int SoftwareIsp::queueBuffers(FrameBuffer *input,
> if (outputs.empty())
> return -EINVAL;
>
> - for (auto [index, buffer] : outputs) {
> + for (auto [stream, buffer] : outputs) {
> if (!buffer)
> return -EINVAL;
> - if (index >= 1) /* only single stream atm */
> + if (outputs.size() != 1) /* only single stream atm */
> return -EINVAL;
> }
>
> - process(input, outputs.at(0));
> + for (auto iter = outputs.begin(); iter != outputs.end(); iter++)
> + process(input, iter->second);
>
> return 0;
> }
> --
> 2.44.0
>
More information about the libcamera-devel
mailing list