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

[Laurent Pinchart]

On Wed, Oct 27, 2021 at 11:41:02AM +0300, Laurent Pinchart wrote:
> On Wed, Oct 27, 2021 at 08:26:19AM +0100, Naushir Patuck wrote:
> > On Tue, 26 Oct 2021 at 23:46, Laurent Pinchart wrote:
> > > On Tue, Oct 26, 2021 at 11:48:24AM +0100, Naushir Patuck wrote:
> > > > On Tue, 26 Oct 2021 at 08:48, Naushir Patuck wrote:
> > > > > On Mon, 25 Oct 2021 at 14:58, Laurent Pinchart wrote:
> > > > >> 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.
> > > >
> > > > Unfortunately this has hit an unexpected complication. DRM formats, and subsequently
> > > > PixelFormat types do not define Mono Bayer formats. The corresponding mbus codes
> > > > are MEDIA_BUS_FMT_Y8_1X8 and MEDIA_BUS_FMT_Y10_1X10.
> > >
> > > DRM and V4L2 define greyscale formats (only 8-bit for DRM with
> > > DRM_FORMAT_R8, 10- and 12-bit formats could be easily added, and
> > > V4L2_PIX_FMT_GREY, V4L2_PIX_FMT_Y10 and V4L2_PIX_FMT_Y12 for V4L2).
> > > They're not Bayer formats indeed, because Bayer filters are only for
> > > colour formats.
> > 
> > I did sport the DRM_FORMAT_R8, but the comment of "/* 8 bpp Red */" make me
> > think it was not entirely correct to use that for mono sensors.  However, I
> > am happy to do so now that you have suggested it :-)
> 
> The naming is a bit unfortunate, it comes from OpenGL I think.
> 
> > > On a side note, I think it was a historical mistake in V4L2 to add Bayer
> > > formats, we should have defined RAW8/10/12/14/16 formats with the CFA
> > > pattern being carried out of band.
> > >
> > > > Would it be acceptable if the conversion regards these mono BayerFormat types as invalid
> > > > PixelFormat types with a todo to add appropriate support in the future? Mono sensor support
> > > > is still usable as we go from mbus code -> V4L2 4CC correctly, but the application can never
> > > > request a mono output - which it can't do today anyway!
> > >
> > > For 8-bit I think you can simply use formats::R8. For 10- and 12-bit, would
> > > https://git.libcamera.org/libcamera/pinchartl/libcamera.git/log/?h=simple/imx8
> > > help ?
> > 
> > Yes it would! I am happy to cherry-pick commits 26f0ce03 and 940d4df4 that add R10 and R12 formats
> > to this series if you like?
> 
> Fine with me. I need to send the first patch to the dri-devel mailing
> list, I'll try to do that today.

https://lore.kernel.org/dri-devel/20211027233140.12268-1-laurent.pinchart@ideasonboard.com/T/#u

-- 
Regards,

Laurent Pinchart