| Message ID | 20211022143907.3089419-2-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@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> 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
Hi Laurent, Thank you for your feedback. On Mon, 25 Oct 2021 at 14:58, Laurent Pinchart < laurent.pinchart@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@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@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 >
Hi Laurent, On Tue, 26 Oct 2021 at 08:48, Naushir Patuck <naush@raspberrypi.com> wrote: > Hi Laurent, > > Thank you for your feedback. > > On Mon, 25 Oct 2021 at 14:58, Laurent Pinchart < > laurent.pinchart@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@raspberrypi.com> >> > Reviewed-by: David Plowman <david.plowman@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. 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! Naush
Hi Naush, 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@raspberrypi.com> > >> > Reviewed-by: David Plowman <david.plowman@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. 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 ?
Hi Laurent, On Tue, 26 Oct 2021 at 23:46, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > 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@raspberrypi.com> > > >> > Reviewed-by: David Plowman <david.plowman@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 :-) > > 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? Thanks, Naush > -- > Regards, > > Laurent Pinchart >
Hi Naush, 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@raspberrypi.com> > > > >> > Reviewed-by: David Plowman <david.plowman@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.
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@raspberrypi.com> > > > > >> > Reviewed-by: David Plowman <david.plowman@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
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); + + return PixelFormat(); +} + /** * \brief Apply a transform to this BayerFormat * \param[in] t The transform to apply