[libcamera-devel] [PATCH v2 1/4] libcamera: Add the fromV4L2PixelFormat function
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jan 11 01:53:11 CET 2021
Hi Sebastian,
Thank you for the patch.
On Fri, Jan 08, 2021 at 12:29:30PM +0100, Jacopo Mondi wrote:
> On Fri, Jan 01, 2021 at 10:50:25AM +0100, Sebastian Fricke wrote:
> > Hey everyone,
> >
> > I have noticed that I forgot to remove the declaration from the header
> > file. This will be fixed in V3.
>
> Yep, you will also have to rebase, but that should be quite painless
>
> Also, this patch breaks the RPi pipeline handler here.
> Which shuld probably be ported to the new API in this path too
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index f121328ee9a9..ede153651ce9 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -358,7 +358,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> */
> V4L2PixelFormat fourcc = sensorFormat.fourcc;
> if (data_->flipsAlterBayerOrder_) {
> - BayerFormat bayer(fourcc);
> + BayerFormat bayer = BayerFormat::fromV4L2PixelFormat(fourcc);
> bayer.order = data_->nativeBayerOrder_;
> bayer = bayer.transform(combined);
> fourcc = bayer.toV4L2PixelFormat();
> @@ -1007,7 +1007,7 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> BayerFormat bayerFormat;
> for (const auto &iter : dev->formats()) {
> V4L2PixelFormat v4l2Format = iter.first;
> - bayerFormat = BayerFormat(v4l2Format);
> + bayerFormat = BayerFormat::fromV4L2PixelFormat(v4l2Format);
> if (bayerFormat.isValid())
> break;
> }
>
>
> > On 31.12.2020 16:53, Sebastian Fricke wrote:
> > > Add a static member function to get the corresponding Bayer-format
>
> BayerFormat
>
> > > from a given V4L2PixelFormat.
> > > Replace an existing method to instantiate an object from a matching
> > > V4l2PixelFormat, to not duplicate the code.
>
> method to instantiate an object = constructor :)
>
> > > The motivation behind this patch is to align the overall structure
> > > of the BayerFormat class with other parts of the code base, such as
> > > the V4L2PixelFormat class.
> > >
> > > Remove the v4l2ToBayer mapping table and use the bayerToV4l2 mapping
> > > table by searching for a mapped element to get the corresponding key.
> > > The downside of this approach is a slightly worse time complexity, but
> > > the upside is a smaller codebase and lower memory consumption. As the
> > > function is probably not used very frequently, I tend to favor the
> > > mentioned upsides.
These are two separate changes, which would be best split in two
patches. No big deal for this patch, just something to keep in mind for
later.
>
> Agreed!
>
> > >
> > > Signed-off-by: Sebastian Fricke <sebastian.fricke.linux at gmail.com>
> > > ---
> > > include/libcamera/internal/bayer_format.h | 1 +
> > > src/libcamera/bayer_format.cpp | 63 +++++++----------------
> > > 2 files changed, 19 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h
> > > index 4280b76b..8efe1382 100644
> > > --- a/include/libcamera/internal/bayer_format.h
> > > +++ b/include/libcamera/internal/bayer_format.h
> > > @@ -48,6 +48,7 @@ public:
> > > std::string toString() const;
> > >
> > > V4L2PixelFormat toV4L2PixelFormat() const;
> > > + static BayerFormat fromV4L2PixelFormat(V4L2PixelFormat v4l2Format);
>
> static const BayerFormat &
> const V4L2PixelFormat &v4l2Format
V4L2PixelFormat is just a u32, we can pass it by value instead of
reference. Returning a reference is good.
> > > BayerFormat transform(Transform t) const;
> > >
> > > Order order;
> > > diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp
> > > index c42792ff..26065b66 100644
> > > --- a/src/libcamera/bayer_format.cpp
> > > +++ b/src/libcamera/bayer_format.cpp
> > > @@ -7,6 +7,7 @@
> > >
> > > #include "libcamera/internal/bayer_format.h"
> > >
> > > +#include <algorithm>
> > > #include <map>
> > >
> > > #include <libcamera/transform.h>
> > > @@ -57,37 +58,6 @@ namespace libcamera {
> > >
> > > namespace {
> > >
> > > -const std::map<V4L2PixelFormat, BayerFormat> v4l2ToBayer{
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8), { BayerFormat::BGGR, 8, BayerFormat::None } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SGBRG8), { BayerFormat::GBRG, 8, BayerFormat::None } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SGRBG8), { BayerFormat::GRBG, 8, BayerFormat::None } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SRGGB8), { BayerFormat::RGGB, 8, BayerFormat::None } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10), { BayerFormat::BGGR, 10, BayerFormat::None } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10), { BayerFormat::GBRG, 10, BayerFormat::None } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10), { BayerFormat::GRBG, 10, BayerFormat::None } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10), { BayerFormat::RGGB, 10, BayerFormat::None } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10P), { BayerFormat::BGGR, 10, BayerFormat::CSI2Packed } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10P), { BayerFormat::GBRG, 10, BayerFormat::CSI2Packed } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10P), { BayerFormat::GRBG, 10, BayerFormat::CSI2Packed } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10P), { BayerFormat::RGGB, 10, BayerFormat::CSI2Packed } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10), { BayerFormat::BGGR, 10, BayerFormat::IPU3Packed } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10), { BayerFormat::GBRG, 10, BayerFormat::IPU3Packed } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10), { BayerFormat::GRBG, 10, BayerFormat::IPU3Packed } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10), { BayerFormat::RGGB, 10, BayerFormat::IPU3Packed } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12), { BayerFormat::BGGR, 12, BayerFormat::None } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12), { BayerFormat::GBRG, 12, BayerFormat::None } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12), { BayerFormat::GRBG, 12, BayerFormat::None } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12), { BayerFormat::RGGB, 12, BayerFormat::None } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12P), { BayerFormat::BGGR, 12, BayerFormat::CSI2Packed } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12P), { BayerFormat::GBRG, 12, BayerFormat::CSI2Packed } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12P), { BayerFormat::GRBG, 12, BayerFormat::CSI2Packed } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12P), { BayerFormat::RGGB, 12, BayerFormat::CSI2Packed } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SBGGR16), { BayerFormat::BGGR, 16, BayerFormat::None } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SGBRG16), { BayerFormat::GBRG, 16, BayerFormat::None } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SGRBG16), { BayerFormat::GRBG, 16, BayerFormat::None } },
> > > - { V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16), { BayerFormat::RGGB, 16, BayerFormat::None } },
> > > -};
> > > -
> > > /* Define a slightly arbitrary ordering so that we can use a std::map. */
> > > struct BayerFormatComparator {
> > > constexpr bool operator()(const BayerFormat &lhs, const BayerFormat &rhs) const
> > > @@ -155,20 +125,6 @@ const std::map<BayerFormat, V4L2PixelFormat, BayerFormatComparator> bayerToV4l2{
> > > * \param[in] p The type of packing applied to the pixel values
> > > */
> > >
> > > -/**
> > > - * \brief Construct a BayerFormat from a V4L2PixelFormat
> > > - * \param[in] v4l2Format The raw format to convert into a BayerFormat
> > > - */
> > > -BayerFormat::BayerFormat(V4L2PixelFormat v4l2Format)
> > > - : order(BGGR), packing(None)
> > > -{
> > > - const auto it = v4l2ToBayer.find(v4l2Format);
> > > - if (it == v4l2ToBayer.end())
> > > - bitDepth = 0;
> > > - else
> > > - *this = it->second;
> > > -}
> > > -
> > > /**
> > > * \fn BayerFormat::isValid()
> > > * \brief Return whether a BayerFormat is valid
> > > @@ -217,6 +173,23 @@ V4L2PixelFormat BayerFormat::toV4L2PixelFormat() const
> > > return V4L2PixelFormat();
> > > }
> > >
> > > +/**
> > > + * \brief Convert \a v4l2Format into the corresponding BayerFormat
s/into/to/
> > > + * \param[in] v4l2Format The raw format to convert into a BayerFormat
> > > + * \return The BayerFormat corresponding to \a v4l2Format
> > > + */
> > > +BayerFormat BayerFormat::fromV4L2PixelFormat(V4L2PixelFormat v4l2Format)
> > > +{
> > > + auto it = std::find_if(
> > > + bayerToV4l2.begin(),
> > > + bayerToV4l2.end(),
> > > + [v4l2Format](const auto &i) { return i.second == v4l2Format; });
The indentation is strange.
auto it = std::find_if(bayerToV4l2.begin(), bayerToV4l2.end(),
[v4l2Format](const auto &i) {
return i.second == v4l2Format;
});
> &v4l2Format
You can even write [&](const auto &i).
> Minors apart, I like the direction this patch takes
With these small issues fixed,
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > + if (it != bayerToV4l2.end())
> > > + return it->first;
> > > +
> > > + return BayerFormat();
> > > +}
> > > +
> > > /**
> > > * \brief Apply a transform to this BayerFormat
> > > * \param[in] t The transform to apply
> > > --
> > > 2.25.1
> > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list