[PATCH v3 4/4] libcamera: converter: Replace usage of stream index by Stream pointer
Umang Jain
umang.jain at ideasonboard.com
Wed Jun 12 07:34:56 CEST 2024
Hi Kieran
On 31/05/24 5:03 pm, Kieran Bingham wrote:
> Quoting Umang Jain (2024-05-31 06:25:05)
>> 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_.
>>
>> 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 ++-
>> src/libcamera/converter.cpp | 6 +--
>> .../converter/converter_v4l2_m2m.cpp | 40 ++++++++++---------
>> src/libcamera/pipeline/simple/simple.cpp | 14 +++----
>> src/libcamera/software_isp/software_isp.cpp | 17 ++++----
>> 7 files changed, 51 insertions(+), 47 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.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
>> * 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..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();
> I'm still weary about this. What does it print ?
>
> Though the stream config might be useful to convey which convertor is
> being used if there are multiple streams - but I think it should really
> be more tied to what the /convertor/ is rather than the stream. Maybe we
> should drop the logPrefix or maybe do something better here.
This should print the entire stream configuration of the Converter logs,
no ? Earlier it would print "stream0" or "stream1" which is less
expressive IMO
I don't have a opinion on this right now, but there might be a better
option.
>
> But I don't mind if this is done on top or later though.
Seconded. If all agrees...
>
>
>> }
>>
>> 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());
> Why is this line removed?
This is removed because streams_ is not a std::vector anymore (see the
change above in the patch). streams_ is now a std::map, for which
reserve() is non-existent (rather not makes sense).
>
>
>> 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..fe0946d7 100644
>> --- a/src/libcamera/software_isp/software_isp.cpp
>> +++ b/src/libcamera/software_isp/software_isp.cpp
>> @@ -221,19 +221,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
>> * \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++) {
>> @@ -260,12 +260,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
>> * 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
>> @@ -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 */
> I still think this "can't" happen, or should have been prevented during
> configure()/validate() but it's existing code so it's fine to just
> update - but I think this error check could be dropped sometime.
This pre-existing code in soft ISP was copy/pasted and adjusted
accordingly to fit the needs. Proper validation should be done as you
suggested (probably orthogonal to this series)
>
>
> I think the comments above are relatively minor (the dropped reserve
> seems like something that could be kept? But it's not so harmful if
> there's a legitimate reason to drop it...)
commented above.
>
> So
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> and lets see if anyone else has anything more to add.
I'll wait for more feedback and reviews, as currently I don't see a need
for v4 ?
>
> --
> Kieran
>
>> 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