[PATCH v3 4/4] libcamera: converter: Replace usage of stream index by Stream pointer
Umang Jain
umang.jain at ideasonboard.com
Mon Jun 24 09:13:06 CEST 2024
Hi Kieran
On 12/06/24 6:10 pm, Kieran Bingham wrote:
> Quoting Umang Jain (2024-06-12 06:34:56)
>> 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
> That's actually what I'm worried about - the entire stream configuration
> sounds like too much, but without a demonstration/printout - I don't know.
>
> The log-prefix is the part of a log message that indicates what the log
> message is from. It's supposed to be 'short'.
>
> But if 'it's just "1920x1080-MJPEG" or such - then maybe it's fine.
> That's why I asked "What does it print?"
This is what it exactly prints:
[20:34:50.175903750] [1267] INFO Converter converter_v4l2_m2m.cpp:155
2736x1824-NV12: output()->queueBuffer 0xffff84023c50
Is one of the example. This looks fine to me.
However, the validility of the call to stream_->configuration() is
actually depending on the configure() phase. It is documentation as :
/**
* \var Stream::configuration_
* \brief The stream configuration
*
* The configuration for the stream is set by any successful call to
* Camera::configure() that includes the stream, and remains valid
until the
* next call to Camera::configure() regardless of if it includes the
stream.
*/
So if you try to put a LOG(Converter,...) before the configure(), the
print log prefix comes out to be: "0x0-<INVALID>"
For example:
[20:25:02.595393125] [1251] ERROR Converter converter_v4l2_m2m.cpp:49
0x0-<INVALID>: m2m open return : 0
This is not ideal. Maybe we need to
std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const
{
- return stream_->configuration().toString();
+ if (stream_->configuration().size <= Size(0, 0))
+ return "";
+ else
+ return stream_->configuration().toString();
}
What are your thoughts ?
>
> Could you capture a before and after of the logs here and compare them
> please? (Ideally pasting a print of one line before and after here in
> this thread). Highlighting what the change does to the logprefix would
> be something I'd include in the commit message.
>
> """
> 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, reporting "1920x1080-MJPEG" in place of "stream0".
> """
>
> (Of course I made up the example, so please ... update with what it
> actually produces!)
Yes, this seems correct.
>
>
>
>
>> 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).
> Aha - ok that's fine then.
>
>>>
>>>> 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 ?
>>
> Yup - this is otherwise fine with me - worst case would be updating the
> commit message while applying perhaps.
>
> --
> Kieran
>
>
>>> --
>>> 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