[libcamera-devel] [PATCH v2 1/6] libcamera: bayer_format: Add a toPixelFormat() helpers to the BayerFormat class

[Naushir Patuck]

Hi Laurent,

Thank you for your feedback.

On Mon, 25 Oct 2021 at 14:58, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Fri, Oct 22, 2021 at 03:39:02PM +0100, Naushir Patuck wrote:
> > Add a BayerFormat::toPixelFormat() member function to convert a
> BayerFormat to a
> > PixelFormat type. This conversion uses the existing bayerToV4l2
> conversion
> > table.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> >  include/libcamera/internal/bayer_format.h |  3 +++
> >  src/libcamera/bayer_format.cpp            | 13 +++++++++++++
> >  2 files changed, 16 insertions(+)
> >
> > diff --git a/include/libcamera/internal/bayer_format.h
> b/include/libcamera/internal/bayer_format.h
> > index 723382d4168d..247a60cf0e36 100644
> > --- a/include/libcamera/internal/bayer_format.h
> > +++ b/include/libcamera/internal/bayer_format.h
> > @@ -10,6 +10,8 @@
> >  #include <stdint.h>
> >  #include <string>
> >
> > +#include <libcamera/pixel_format.h>
> > +
> >  #include "libcamera/internal/v4l2_pixelformat.h"
> >
> >  namespace libcamera {
> > @@ -50,6 +52,7 @@ public:
> >
> >       V4L2PixelFormat toV4L2PixelFormat() const;
> >       static BayerFormat fromV4L2PixelFormat(V4L2PixelFormat v4l2Format);
> > +     PixelFormat toPixelFormat() const;
> >       BayerFormat transform(Transform t) const;
> >
> >       Order order;
> > diff --git a/src/libcamera/bayer_format.cpp
> b/src/libcamera/bayer_format.cpp
> > index 11355f144f66..c662ba36604c 100644
> > --- a/src/libcamera/bayer_format.cpp
> > +++ b/src/libcamera/bayer_format.cpp
> > @@ -269,6 +269,19 @@ BayerFormat
> BayerFormat::fromV4L2PixelFormat(V4L2PixelFormat v4l2Format)
> >       return BayerFormat();
> >  }
> >
> > +/**
> > + * \brief Convert a BayerFormat into the corresponding PixelFormat
> > + * \return The PixelFormat corresponding to this BayerFormat
> > + */
> > +PixelFormat BayerFormat::toPixelFormat() const
> > +{
> > +     const auto it = bayerToV4l2.find(*this);
> > +     if (it != bayerToV4l2.end())
> > +             return PixelFormat(it->second);
>
> Should we store the PixelFormat in the bayerToV4l2 map to avoid a double
> looking (one here, one in the PixelFormat constructor) ? The map should
> be renamed to bayerToFormat, and be stored as either
>
> std::map<BayerFormat, std::pair<PixelFormat, V4L2PixelFormat>,
> BayerFormatComparator>
>
> or
>
> struct Formats {
>         PixelFormat pixelFormat;
>         V4L2PixelFormat v4l2Format;
> };
>
> std::map<BayerFormat, Formats, BayerFormatComparator>
>

This seems reasonable.  I will update the table with a std::pair for 2-way
conversion.

Regards,
Naush


>
> It could of course be argued that we'll then duplicate data, and that it
> could get out of sync.
>
> I'm concerned about how the BayerFormat class is used to convert a media
> bus code to a pixel format or V4L2 pixel format, but I'll comment on
> that where applicable in the rest of the series.
>
> > +
> > +     return PixelFormat();
> > +}
> > +
> >  /**
> >   * \brief Apply a transform to this BayerFormat
> >   * \param[in] t The transform to apply
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20211026/259ef4a0/attachment.htm>