[libcamera-devel] [PATCH v3 1/1] pipeline: raspberrypi: Fix handling of colour spaces
David Plowman
david.plowman at raspberrypi.com
Tue Jan 31 07:38:32 CET 2023
Hi Kieran, Umag
I didn't spot the question, sorry about that. But yes, more complete
comments always welcome!
Thanks
David
On Mon, 30 Jan 2023 at 11:04, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Hi David,
>
> There's a small question below that is holding this back from being merged.
>
> I'm planning a release tomorrow, so I'd like to get this colourspace fix
> in.
>
> Quoting Umang Jain via libcamera-devel (2023-01-25 06:48:19)
> > Hi David,
> >
> > On 1/20/23 9:52 PM, David Plowman via libcamera-devel wrote:
> > > We implement a custom validateColorSpaces method that forces all
> > > (non-raw) streams to same colour space, whilst distinguishing RGB
> > > streams from YUV ones, as the former must have the YCbCr encoding and
> > > range over-written.
> > >
> > > When we apply the colour space, we always send the full YUV version as
> > > that gets converted correctly to what our hardware drivers expect. It
> > > is also careful to check what comes back as the YCbCr information gets
> > > overwritten again on the way back.
> > >
> > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > > .../pipeline/raspberrypi/raspberrypi.cpp | 115 +++++++++++++++++-
> > > 1 file changed, 114 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 8569df17..f768dced 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -310,6 +310,7 @@ class RPiCameraConfiguration : public CameraConfiguration
> > > public:
> > > RPiCameraConfiguration(const RPiCameraData *data);
> > >
> > > + CameraConfiguration::Status validateColorSpaces(ColorSpaceFlags flags);
> > > Status validate() override;
> > >
> > > /* Cache the combinedTransform_ that will be applied to the sensor */
> > > @@ -317,6 +318,13 @@ public:
> > >
> > > private:
> > > const RPiCameraData *data_;
> > > +
> > > + /*
> > > + * Store the colour spaces that all our streams will have. RGB format streams
> > > + * will be the same but need the YCbCr fields cleared.
> >
> > Would re-pharse it like:
> >
> > /*
> > * Store the colour spaces that all our streams will have. RGB
> > format streams
> > * will have the same colorspace as YUV streams, with YCbCr
> > field cleared and
> > * range set to full.
> > */
>
> Are you happy with the expansion on this comment?
>
>
> --
> Kieran
>
>
>
> >
> > > + */
> > > + std::optional<ColorSpace> yuvColorSpace_;
> > > + std::optional<ColorSpace> rgbColorSpace_;
> > > };
> > >
> > > class PipelineHandlerRPi : public PipelineHandler
> > > @@ -357,6 +365,105 @@ RPiCameraConfiguration::RPiCameraConfiguration(const RPiCameraData *data)
> > > {
> > > }
> > >
> > > +static const std::vector<ColorSpace> validColorSpaces = {
> > > + ColorSpace::Sycc,
> > > + ColorSpace::Smpte170m,
> > > + ColorSpace::Rec709
> > > +};
> > > +
> > > +static std::optional<ColorSpace> findValidColorSpace(const ColorSpace &colourSpace)
> > > +{
> > > + for (auto cs : validColorSpaces) {
> > > + if (colourSpace.primaries == cs.primaries &&
> > > + colourSpace.transferFunction == cs.transferFunction)
> > > + return cs;
> > > + }
> > > +
> > > + return std::nullopt;
> > > +}
> > > +
> > > +static bool isRgb(const PixelFormat &pixFmt)
> > > +{
> > > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
> > > + return info.colourEncoding == PixelFormatInfo::ColourEncodingRGB;
> > > +}
> > > +
> > > +static bool isYuv(const PixelFormat &pixFmt)
> > > +{
> > > + /* The code below would return true for raw mono streams, so weed those out first. */
> > > + if (isRaw(pixFmt))
> > > + return false;
> > > +
> > > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
> > > + return info.colourEncoding == PixelFormatInfo::ColourEncodingYUV;
> > > +}
> > > +
> > > +/*
> > > + * Raspberry Pi drivers expect the following colour spaces:
> > > + * - V4L2_COLORSPACE_RAW for raw streams.
> > > + * - One of V4L2_COLORSPACE_JPEG, V4L2_COLORSPACE_SMPTE170M, V4L2_COLORSPACE_REC709 for
> > > + * non-raw streams. Other fields such as transfer function, YCbCr encoding and
> > > + * quantisation are not used.
> > > + *
> > > + * The libcamera colour spaces that we wish to use corresponding to these are therefore:
> > > + * - ColorSpace::Raw for V4L2_COLORSPACE_RAW
> > > + * - ColorSpace::Sycc for V4L2_COLORSPACE_JPEG
> > > + * - ColorSpace::Smpte170m for V4L2_COLORSPACE_SMPTE170M
> > > + * - ColorSpace::Rec709 for V4L2_COLORSPACE_REC709
> > > + */
> >
> > Thank you for the information. This makes the expectations clearer.
> >
> > With that,
> >
> > Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> >
> >
> > I can fixup the comment while applying the patch (if you are OK with the
> > comment's explanation).
> >
> > > +
> > > +CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_unused]] ColorSpaceFlags flags)
> > > +{
> > > + Status status = Valid;
> > > + yuvColorSpace_.reset();
> > > +
> > > + for (auto cfg : config_) {
> > > + /* First fix up raw streams to have the "raw" colour space. */
> > > + if (isRaw(cfg.pixelFormat)) {
> > > + /* If there was no value here, that doesn't count as "adjusted". */
> > > + if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw) {
> > > + status = Adjusted;
> > > + cfg.colorSpace = ColorSpace::Raw;
> > > + }
> > > + continue;
> > > + }
> > > +
> > > + /* Next we need to find our shared colour space. The first valid one will do. */
> > > + if (cfg.colorSpace && !yuvColorSpace_)
> > > + yuvColorSpace_ = findValidColorSpace(cfg.colorSpace.value());
> > > + }
> > > +
> > > + /* If no colour space was given anywhere, choose sYCC. */
> > > + if (!yuvColorSpace_)
> > > + yuvColorSpace_ = ColorSpace::Sycc;
> > > +
> > > + /* Note the version of this that any RGB streams will have to use. */
> > > + rgbColorSpace_ = yuvColorSpace_;
> > > + rgbColorSpace_->ycbcrEncoding = ColorSpace::YcbcrEncoding::None;
> > > + rgbColorSpace_->range = ColorSpace::Range::Full;
> > > +
> > > + /* Go through the streams again and force everyone to the same colour space. */
> > > + for (auto cfg : config_) {
> > > + if (cfg.colorSpace == ColorSpace::Raw)
> > > + continue;
> > > +
> > > + if (isYuv(cfg.pixelFormat) && cfg.colorSpace != yuvColorSpace_) {
> > > + /* Again, no value means "not adjusted". */
> > > + if (cfg.colorSpace)
> > > + status = Adjusted;
> > > + cfg.colorSpace = yuvColorSpace_;
> > > + }
> > > + if (isRgb(cfg.pixelFormat) && cfg.colorSpace != rgbColorSpace_) {
> > > + /* Be nice, and let the YUV version count as non-adjusted too. */
> > > + if (cfg.colorSpace && cfg.colorSpace != yuvColorSpace_)
> > > + status = Adjusted;
> > > + cfg.colorSpace = rgbColorSpace_;
> > > + }
> > > + }
> > > +
> > > + return status;
> > > +}
> > > +
> > > CameraConfiguration::Status RPiCameraConfiguration::validate()
> > > {
> > > Status status = Valid;
> > > @@ -533,7 +640,8 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> > > V4L2DeviceFormat format;
> > > format.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);
> > > format.size = cfg.size;
> > > - format.colorSpace = cfg.colorSpace;
> > > + /* We want to send the associated YCbCr info through to the driver. */
> > > + format.colorSpace = yuvColorSpace_;
> > >
> > > LOG(RPI, Debug)
> > > << "Try color space " << ColorSpace::toString(cfg.colorSpace);
> > > @@ -542,6 +650,11 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> > > if (ret)
> > > return Invalid;
> > >
> > > + /*
> > > + * But for RGB streams, the YCbCr info gets overwritten on the way back
> > > + * so we must check against what the stream cfg says, not what we actually
> > > + * requested (which carefully included the YCbCr info)!
> > > + */
> > > if (cfg.colorSpace != format.colorSpace) {
> > > status = Adjusted;
> > > LOG(RPI, Debug)
> >
More information about the libcamera-devel
mailing list