| Message ID | 20201223174709.45457-5-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
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@ideasonboard.com> > Signed-off-by: Jacopo Mondi <jacopo@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. Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > + } > + > + properties_.set(properties::draft::ColorFilterArrangement, cfa); > + break; > + } > + > return 0; > } > > -- > 2.29.2 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@ideasonboard.com> > > Signed-off-by: Jacopo Mondi <jacopo@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@ragnatech.se> > > > + } > > + > > + properties_.set(properties::draft::ColorFilterArrangement, cfa); > > + break; > > + } > > + > > return 0; > > } > > > > -- > > 2.29.2 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
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; + } + + properties_.set(properties::draft::ColorFilterArrangement, cfa); + break; + } + return 0; }