[libcamera-devel] [PATCH 4/6] libcamera: camera_sensor: Register ColorFilterArrangement

[Jacopo Mondi]

Hi Niklas,

On Sun, Dec 27, 2020 at 12:12:46PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2020-12-23 18:47:07 +0100, Jacopo Mondi wrote:
> > Inspect the list of media bus codes supported by the camera sensor
> > in order to deduce the color filter arrangement and register the
> > ColorFilterArrangement draft property.
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/camera_sensor.cpp | 38 +++++++++++++++++++++++++++++----
> >  1 file changed, 34 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 1628ba9c892b..bdf4415fda0e 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -17,6 +17,7 @@
> >
> >  #include <libcamera/property_ids.h>
> >
> > +#include "libcamera/internal/bayer_format.h"
> >  #include "libcamera/internal/formats.h"
> >  #include "libcamera/internal/sysfs.h"
> >  #include "libcamera/internal/utils.h"
> > @@ -178,10 +179,6 @@ int CameraSensor::init()
> >  	if (ret < 0)
> >  		return ret;
> >
> > -	ret = initProperties();
> > -	if (ret)
> > -		return ret;
> > -
> >  	/* Enumerate, sort and cache media bus codes and sizes. */
> >  	formats_ = subdev_->formats(pad_);
> >  	if (formats_.empty()) {
> > @@ -210,6 +207,10 @@ int CameraSensor::init()
> >  	 */
> >  	resolution_ = sizes_.back();
> >
> > +	ret = initProperties();
> > +	if (ret)
> > +		return ret;
> > +
> >  	return 0;
> >  }
> >
> > @@ -307,6 +308,35 @@ int CameraSensor::initProperties()
> >  		properties_.set(properties::PixelArrayActiveAreas, { crop });
> >  	}
> >
> > +	/* Color filter array pattern, register only for RAW sensors. */
> > +	for (const auto &format : formats_) {
> > +		unsigned int mbusCode = format.first;
> > +		BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);
> > +		if (!bayerFormat.isValid())
> > +			continue;
> > +
> > +		int32_t cfa = -1;
> > +		switch (bayerFormat.order) {
> > +		case BayerFormat::BGGR:
> > +			cfa = properties::draft::BGGR;
> > +			break;
> > +		case BayerFormat::GBRG:
> > +			cfa = properties::draft::GBRG;
> > +			break;
> > +		case BayerFormat::GRBG:
> > +			cfa = properties::draft::GRBG;
> > +			break;
> > +		case BayerFormat::RGGB:
> > +			cfa = properties::draft::RGGB;
> > +			break;
> > +		default:
> > +			continue;
>
> nit: Would it make sens to have a fatal error here? It should not happen
> but if we ever add a new Bayer filter arrangement it could easily be
> missed to also add support for it here.

It shouldn't happen, but we would indeed lose it if it does, you're right.

Actually, if we add a new bayer ordering (as in a new
BayerFormat::Order entry) and I drop the default, we can rely on the
compiler's -Wswitch flag that complains if we don't handle all
possible values in an enumeration inside a switch. I could drop the
default case, and remove the 'cfa' variable intialization, which was
not required in first place.

Thanks
  j

>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>
> > +		}
> > +
> > +		properties_.set(properties::draft::ColorFilterArrangement, cfa);
> > +		break;
> > +	}
> > +
> >  	return 0;
> >  }
> >
> > --
> > 2.29.2
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund