[libcamera-devel] [PATCH 5/6] libcamera: pipeline: rkisp1: Implement color space support

[Florian Sylvestre]

Hi Laurent,

Reviewed-by: Florian Sylvestre <fsylvestre at baylibre.com>

On Thu, 25 Aug 2022 at 23:17, Paul Elder via libcamera-devel
<libcamera-devel at lists.libcamera.org> wrote:
>
> Hi Laurent,
>
> On Thu, Aug 25, 2022 at 11:17:11PM +0300, Laurent Pinchart wrote:
> > Hi Paul,
> >
> > On Thu, Aug 25, 2022 at 03:08:13PM -0500, paul.elder at ideasonboard.com wrote:
> > > On Tue, Aug 23, 2022 at 08:43:13PM +0300, Laurent Pinchart via libcamera-devel wrote:
> > >
> > > I think some changelog would be nice to have :)
> >
> > Oops :-) I'll add
> >
> > Implement color space support in the rkisp1 pipeline handler, in the
> > configuration generation, configuration validation and camera
> > configuration. As all the processing related to the color space is
> > performed in the part of the pipeline shared by all streams, a single
> > color space must cover all stream configurations. This is enforced
> > manually when generating the configuration, and with the
> > validateColorSpaces() helper when validating it.
> >
> > Only the Y'CbCr encoding and quantization range are currently taken into
> > account, and they are programmed through the V4L2 color space API. The
> > primary colors chromaticities and the transfer function need to be
> > configured in the ISP parameters buffer, and thus conveyed to the IPA,
> > but the rkisp1 driver does not currently support tone mapping, so this
> > will be done later.
>
> Looks good.
>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
>
> >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > ---
> > > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 38 ++++++++++++++++++++----
> > > >  1 file changed, 33 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > index 03bbe6b467ae..25fbf9f1a0a9 100644
> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > @@ -18,6 +18,7 @@
> > > >  #include <libcamera/base/utils.h>
> > > >
> > > >  #include <libcamera/camera.h>
> > > > +#include <libcamera/color_space.h>
> > > >  #include <libcamera/control_ids.h>
> > > >  #include <libcamera/formats.h>
> > > >  #include <libcamera/framebuffer.h>
> > > > @@ -416,11 +417,13 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> > > >  {
> > > >   const CameraSensor *sensor = data_->sensor_.get();
> > > >   unsigned int pathCount = data_->selfPath_ ? 2 : 1;
> > > > - Status status = Valid;
> > > > + Status status;
> > > >
> > > >   if (config_.empty())
> > > >           return Invalid;
> > > >
> > > > + status = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);
> > > > +
> > > >   if (transform != Transform::Identity) {
> > > >           transform = Transform::Identity;
> > > >           status = Adjusted;
> > > > @@ -547,21 +550,44 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
> > > >   if (roles.empty())
> > > >           return config;
> > > >
> > > > + /*
> > > > +  * As the ISP can't output different color spaces for the main and self
> > > > +  * path, pick a sensible default color space based on the role of the
> > > > +  * first stream and use it for all streams.
> > > > +  */
> > > > + std::optional<ColorSpace> colorSpace;
> > > > +
> > > >   bool mainPathAvailable = true;
> > > >   bool selfPathAvailable = data->selfPath_;
> > > > +
> > > >   for (const StreamRole role : roles) {
> > > >           bool useMainPath;
> > > >
> > > >           switch (role) {
> > > > -         case StreamRole::StillCapture: {
> > > > +         case StreamRole::StillCapture:
> > > >                   useMainPath = mainPathAvailable;
> > > > +                 /* JPEG encoders typically expect sYCC. */
> > > > +                 if (!colorSpace)
> > > > +                         colorSpace = ColorSpace::Sycc;
> > > >                   break;
> > > > -         }
> > > > +
> > > >           case StreamRole::Viewfinder:
> > > > -         case StreamRole::VideoRecording: {
> > > >                   useMainPath = !selfPathAvailable;
> > > > +                 /*
> > > > +                  * sYCC is the YCbCr encoding of sRGB, which is commonly
> > > > +                  * used by displays.
> > > > +                  */
> > > > +                 if (!colorSpace)
> > > > +                         colorSpace = ColorSpace::Sycc;
> > > >                   break;
> > > > -         }
> > > > +
> > > > +         case StreamRole::VideoRecording:
> > > > +                 useMainPath = !selfPathAvailable;
> > > > +                 /* Rec. 709 is a good default for HD video recording. */
> > > > +                 if (!colorSpace)
> > > > +                         colorSpace = ColorSpace::Rec709;
> > > > +                 break;
> > > > +
> > > >           default:
> > > >                   LOG(RkISP1, Warning)
> > > >                           << "Requested stream role not supported: " << role;
> > > > @@ -580,6 +606,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
> > > >                   selfPathAvailable = false;
> > > >           }
> > > >
> > > > +         cfg.colorSpace = colorSpace;
> > > >           config->addConfiguration(cfg);
> > > >   }
> > > >
> > > > @@ -642,6 +669,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> > > >   if (ret < 0)
> > > >           return ret;
> > > >
> > > > + format.colorSpace = config->at(0).colorSpace;
> > > >   ret = isp_->setFormat(2, &format);
> > > >   if (ret < 0)
> > > >           return ret;
> >
> > --
> > Regards,
> >
> > Laurent Pinchart



-- 
Florian Sylvestre