[PATCH v3 4/4] libcamera: converter: Replace usage of stream index by Stream pointer
Umang Jain
umang.jain at ideasonboard.com
Mon Jun 24 14:44:59 CEST 2024
Hi Kieran
On 24/06/24 4:50 pm, Kieran Bingham wrote:
> Quoting Umang Jain (2024-06-24 08:13:06)
>> 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 "";
> That's possible indeed... Or just keep it as above.
>
> That's sort of where my concern was about using the stream configuration
> as a way to convey which stream it is. But I don't see a better solution
> right now so I'm fine with this. It's a corner case that we'll get
> errors here.
>
>
>> + 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.
> Ok - well I'll leave it to you if you want to post a v4 - it's only the
> commit message to update I think so may not be worth it.
I'll post a v4 with the updated committed message. I have a couple of
calls shuffled in the V4L2M2MConverter::configure() function (trivial
change).
I'll keep this version of logPrefix()
std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const
{
return stream_->configuration().toString();
}
>
> --
> Kieran
>
>
>>>
>>>
>>>
>>>> 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