[PATCH v3 4/4] libcamera: converter: Replace usage of stream index by Stream pointer
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Jun 24 13:20:07 CEST 2024
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.
--
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