[PATCH v6 07/18] libcamera: software_isp: Add Debayer base class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Mar 27 20:29:10 CET 2024
Hi Hans,
On Wed, Mar 27, 2024 at 04:29:05PM +0100, Hans de Goede wrote:
> On 3/27/24 3:00 PM, Laurent Pinchart wrote:
> > Hi Milan and Hans,
> >
> > Thank you for the patch.
>
> Same as with the previous review: Thank you for the review.
Sorry for taking so long to review the whole series, I should have done
so earlier.
> Milan is going to prepare a v7 of this series, so I'm going to <snip>
> all the trivial remarks and only comment on the remaining remarks.
>
> > On Tue, Mar 19, 2024 at 01:35:54PM +0100, Milan Zamazal wrote:
> >> From: Hans de Goede <hdegoede at redhat.com>
> >>
> >> Add a base class for debayer implementations. This is intended to be
> >> suitable for both GPU (or otherwise) accelerated debayer implementations
> >> as well as CPU based debayering.
> >>
> >> Doxygen documentation by Dennis Bonke.
> >>
> >> Tested-by: Bryan O'Donoghue <bryan.odonoghue at linaro.org> # sc8280xp Lenovo x13s
> >> Tested-by: Pavel Machek <pavel at ucw.cz>
> >> Reviewed-by: Pavel Machek <pavel at ucw.cz>
> >> Reviewed-by: Milan Zamazal <mzamazal at redhat.com>
> >> Co-developed-by: Dennis Bonke <admin at dennisbonke.com>
> >> Signed-off-by: Dennis Bonke <admin at dennisbonke.com>
> >> Co-developed-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> >> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> >> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> >> ---
> >> .../internal/software_isp/debayer_params.h | 48 ++++++++
> >> .../internal/software_isp/meson.build | 1 +
> >> src/libcamera/software_isp/debayer.cpp | 29 +++++
> >> src/libcamera/software_isp/debayer.h | 104 ++++++++++++++++++
> >> src/libcamera/software_isp/meson.build | 1 +
> >> 5 files changed, 183 insertions(+)
> >> create mode 100644 include/libcamera/internal/software_isp/debayer_params.h
> >> create mode 100644 src/libcamera/software_isp/debayer.cpp
> >> create mode 100644 src/libcamera/software_isp/debayer.h
> >>
> >> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h
> >> new file mode 100644
> >> index 00000000..98965fa1
> >> --- /dev/null
> >> +++ b/include/libcamera/internal/software_isp/debayer_params.h
> >> @@ -0,0 +1,48 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2023, Red Hat Inc.
> >> + *
> >> + * Authors:
> >> + * Hans de Goede <hdegoede at redhat.com>
> >> + *
> >> + * debayer_params.h - DebayerParams header
> >> + */
> >> +
> >> +#pragma once
> >> +
> >> +namespace libcamera {
> >> +
> >> +/**
> >> + * \brief Struct to hold the debayer parameters.
> >> + */
> >
> > Documentation in .cpp files.
> >
> >> +struct DebayerParams {
> >> + /**
> >> + * \brief const value for 1.0 gain
> >> + */
> >> + static constexpr unsigned int kGain10 = 256;
> >
> > Forcing the caller to deal with the internal representation of gains
> > isn't nice, especially given that it precludes implementing gains of
> > different precisions in different backend. Wouldn't it be better to pass
> > the values as floating point numbers, and convert them to the internal
> > representation in the implementation of process() before using them ?
>
> That is a good idea. Is it ok if we put this at the top of
> the SoftwareISP-TODO doc as low hanging fruit and then tackle
> this soon after the current version is merged ?
I'm OK with that. Let's get this merged, and then start tackling some of
the technical debt before adding more advanced features.
> The current version has seen a lot of testing so I would prefer
> to not make any non trivial changes for the initial merge.
>
> But this definitely is something which we can tackle soon
> after merging.
>
> <snip>
>
> >> + virtual int configure(const StreamConfiguration &inputCfg,
> >> + const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) = 0;
> >> +
> >> + /**
> >> + * \brief Get the width and height at which the bayer pattern repeats.
> >> + * \param[in] inputFormat The input format.
> >> + *
> >> + * Valid sizes are: 2x2, 4x2 or 4x4.
> >> + *
> >> + * \return pattern size or an empty size for unsupported inputFormats.
> >> + */
> >> + virtual Size patternSize(PixelFormat inputFormat) = 0;
> >
> > Unless I'm mistaken, this is used internally in the DebayerCpu class
> > only. I think you can drop the function from the interface, and make it
> > private.
>
> Ack.
>
> Note that the whole base + CPU class split is still somewhat in flux,
> unlike for the swstats code where I've squashed the 2 into 1 class this
> has been left split for now in anticipation of the GPU accelerated debayering
> Bryan is working on.
>
> I think that a lot of your overal API design questions will come more into
> focus once we have an initial MVP / POC from Bryan. We might even end up
> just squashing the 2 classes into 1 then, but Bryan has said that he sees
> potential for code sharing in the base class.
I was thinking the same, we'll have a better view of the design once
Bryan will have a second user of the base class.
> >> +
> >> + /**
> >> + * \brief Get the supported output formats.
> >> + * \param[in] inputFormat The input format.
> >> + *
> >> + * \return all supported output formats or an empty vector if there are none.
> >> + */
> >> + virtual std::vector<PixelFormat> formats(PixelFormat inputFormat) = 0;
> >> +
> >> + /**
> >> + * \brief Get the stride and the frame size.
> >> + * \param[in] outputFormat The output format.
> >> + * \param[in] size The output size.
> >> + *
> >> + * \return a tuple of the stride and the frame size, or a tuple with 0,0 if there is no valid output config.
> >
> > Line wrap.
> >
> >> + */
> >> + virtual std::tuple<unsigned int, unsigned int>
> >> + strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0;
> >> +
> >> + /**
> >> + * \brief Process the bayer data into the requested format.
> >> + * \param[in] input The input buffer.
> >> + * \param[in] output The output buffer.
> >> + * \param[in] params The parameters to be used in debayering.
> >> + *
> >> + * \note DebayerParams is passed by value deliberately so that a copy is passed
> >> + * when this is run in another thread by invokeMethod().
> >
> > Possibly something to address later, by storing ISP parameters in
> > per-frame buffers like we do for hardware ISPs.
>
> Right, I guess this is another item to add to the SoftwareISP-TODO doc.
I think this will be part of the refactoring needed to have per-frame
stats and params.
> >> + */
> >> + virtual void process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0;
> >> +
> >> + /**
> >> + * \brief Get the supported output sizes for the given input format and size.
> >> + * \param[in] inputFormat The input format.
> >> + * \param[in] inputSize The input size.
> >> + *
> >> + * \return The valid size ranges or an empty range if there are none.
> >> + */
> >> + virtual SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) = 0;
> >> +
> >> + /**
> >> + * \brief Signals when the input buffer is ready.
> >> + */
> >> + Signal<FrameBuffer *> inputBufferReady;
> >> +
> >> + /**
> >> + * \brief Signals when the output buffer is ready.
> >> + */
> >> + Signal<FrameBuffer *> outputBufferReady;
> >
> > Do you envision that we would have backends that could emit those
> > signals at different times ? If not you could combine them into a single
> > signal, that would be more efficient.
>
> This is modeled after the converter class. I think it would be good
> to keep these. If we get a more advanced pipeline with e.g. using the
> VPU for denoising (I recently learned at least Intel VPU s can do this)
> then I can see the pipeline using an intermediate buffer and us releasing
> the input buffer after debayering while the output buffer will not
> be ready until the VPU is done.
Let's keep this as-is for now, we can always change it later.
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list