[PATCH v2 4/4] libcamera: converter: Replace usage of stream index by Stream pointer
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed May 29 13:09:06 CEST 2024
Quoting Kieran Bingham (2024-05-29 11:00:15)
> Quoting Umang Jain (2024-05-29 08:02:48)
> > 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_.
>
> Unfortunately - this patch breaks the SoftISP on Lenovo-X13s presently.
>
> I haven't looked into why yet, just that it breaks the camera.
>
The CI also picked up that the Doxygen names need cleanup in here:
- https://gitlab.freedesktop.org/camera/libcamera/-/jobs/59287644
FAILED: Documentation/api-html
/usr/bin/doxygen Documentation/Doxyfile
/builds/camera/libcamera/src/libcamera/converter.cpp:113: error: argument 'output' of command @param is not found in the argument list of libcamera::Converter::exportBuffers(const Stream *stream, unsigned int count, std::vector< std::unique_ptr< FrameBuffer > > *buffers)=0 (warning treated as error, aborting now)
--
Regards
Kieran
> > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > ---
> > include/libcamera/internal/converter.h | 5 ++-
> > .../internal/converter/converter_v4l2_m2m.h | 11 ++---
> > .../internal/software_isp/software_isp.h | 5 ++-
> > .../converter/converter_v4l2_m2m.cpp | 40 ++++++++++---------
> > src/libcamera/pipeline/simple/simple.cpp | 14 +++----
> > src/libcamera/software_isp/software_isp.cpp | 13 +++---
> > 6 files changed, 46 insertions(+), 42 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..58fd19db 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 *, 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 7e9fae6a..09cc0f6f 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/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> > index 6309a0c0..a48b2a87 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,10 +333,13 @@ 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];
> > + streams_.emplace(cfg.stream(),
> > + V4L2M2MStream(this, cfg.stream()));
> > +
> > + V4L2M2MStream &stream = streams_.at(cfg.stream());
> >
> > if (!stream.isValid()) {
> > LOG(Converter, Error)
> > @@ -345,7 +348,7 @@ int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,
> > break;
> > }
> >
> > - ret = stream.configure(inputCfg, outputCfgs[i]);
> > + ret = stream.configure(inputCfg, cfg);
> > if (ret < 0)
> > break;
> > }
> > @@ -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 db3575c3..01ad91a7 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -278,7 +278,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_;
> > @@ -837,7 +837,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;
> > @@ -1304,10 +1304,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);
> > }
> > @@ -1399,7 +1397,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()) {
> > /*
> > @@ -1408,7 +1406,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 ac10d82d..7cadd585 100644
> > --- a/src/libcamera/software_isp/software_isp.cpp
> > +++ b/src/libcamera/software_isp/software_isp.cpp
> > @@ -227,13 +227,13 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg,
> > * \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++) {
> > @@ -265,7 +265,7 @@ int SoftwareIsp::exportBuffers(unsigned int output, unsigned int count,
> > * \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
> > @@ -274,14 +274,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