[libcamera-devel] [PATCH v2 4/4] libcamera: rpi: Handle SensorConfiguration
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Aug 1 11:23:44 CEST 2023
Hi Naush
On Tue, Aug 01, 2023 at 10:02:15AM +0100, Naushir Patuck via libcamera-devel wrote:
> Hi Jacopo,
>
> Thanks for the update. Just one minor comment:
>
> On Mon, 31 Jul 2023 at 12:31, Jacopo Mondi via libcamera-devel
> <libcamera-devel at lists.libcamera.org> wrote:
> >
> > Handle the SensorConfiguration provided by the application in the
> > pipeline validate() and configure() call chains.
> >
> > During validation, first make sure SensorConfiguration is valid, then
> > handle it to compute the sensor format.
> >
> > For the VC4 platform where the RAW stream follows the sensor's
> > configuration adjust the RAW stream configuration to match the sensor
> > configuration.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > ---
> > .../pipeline/rpi/common/pipeline_base.cpp | 62 ++++++++++++++++---
> > .../pipeline/rpi/common/pipeline_base.h | 4 +-
> > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 30 ++++++++-
> > 3 files changed, 82 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index 97acafbbb728..15c7a5c72bdd 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -180,6 +180,11 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> > if (config_.empty())
> > return Invalid;
> >
> > + if (!sensorConfig.valid()) {
> > + LOG(RPI, Error) << "Invalid sensor configuration request";
> > + return Invalid;
> > + }
> > +
> > status = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);
> >
> > /*
> > @@ -207,19 +212,43 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> > std::sort(outStreams.begin(), outStreams.end(),
> > [](auto &l, auto &r) { return l.cfg->size > r.cfg->size; });
> >
> > - /* Compute the sensor configuration. */
> > - unsigned int bitDepth = defaultRawBitDepth;
> > - if (!rawStreams.empty()) {
> > + /* Compute the sensor's format then do any platform specific fixups. */
> > + unsigned int bitDepth;
> > + Size sensorSize;
> > +
> > + if (sensorConfig) {
> > + /* Use the application provided sensor configuration. */
> > + bitDepth = sensorConfig.bitDepth;
> > + sensorSize = sensorConfig.outputSize;
> > + } else if (!rawStreams.empty()) {
> > + /* Use the RAW stream format and size. */
> > BayerFormat bayerFormat = BayerFormat::fromPixelFormat(rawStreams[0].cfg->pixelFormat);
> > bitDepth = bayerFormat.bitDepth;
> > + sensorSize = rawStreams[0].cfg->size;
> > + } else {
> > + bitDepth = defaultRawBitDepth;
> > + sensorSize = outStreams[0].cfg->size;
> > }
> >
> > - sensorFormat_ = data_->findBestFormat(rawStreams.empty() ? outStreams[0].cfg->size
> > - : rawStreams[0].cfg->size,
> > - bitDepth);
> > + sensorFormat_ = data_->findBestFormat(sensorSize, bitDepth);
> > +
> > + /*
> > + * If a sensor configuration has been requested, it should apply
> > + * without modifications.
> > + */
> > + if (sensorConfig) {
> > + BayerFormat bayer = BayerFormat::fromMbusCode(sensorFormat_.mbus_code);
> > +
> > + if (bayer.bitDepth != sensorConfig.bitDepth ||
> > + sensorFormat_.size != sensorConfig.outputSize) {
> > + LOG(RPI, Error) << "Invalid sensor configuration: "
> > + << "bitDepth/size mismatch";
> > + return Invalid;
> > + }
> > + }
> >
> > /* Do any platform specific fixups. */
> > - status = data_->platformValidate(rawStreams, outStreams);
> > + status = data_->platformValidate(this, rawStreams, outStreams);
> > if (status == Invalid)
> > return Invalid;
> >
> > @@ -467,12 +496,25 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
> > std::sort(ispStreams.begin(), ispStreams.end(),
> > [](auto &l, auto &r) { return l.cfg->size > r.cfg->size; });
> >
> > - /* Apply the format on the sensor with any cached transform. */
> > + /*
> > + * Apply the format on the sensor with any cached transform.
> > + *
> > + * If the application has provided a sensor configuration apply it
> > + * instead of just applying a format.
> > + */
> > const RPiCameraConfiguration *rpiConfig =
> > static_cast<const RPiCameraConfiguration *>(config);
> > - V4L2SubdeviceFormat sensorFormat = rpiConfig->sensorFormat_;
> > + V4L2SubdeviceFormat sensorFormat;
> >
> > - ret = data->sensor_->setFormat(&sensorFormat, rpiConfig->combinedTransform_);
> > + if (rpiConfig->sensorConfig) {
> > + ret = data->sensor_->applyConfiguration(rpiConfig->sensorConfig,
> > + rpiConfig->combinedTransform_,
> > + &sensorFormat);
> > + } else {
> > + sensorFormat = rpiConfig->sensorFormat_;
> > + ret = data->sensor_->setFormat(&sensorFormat,
> > + rpiConfig->combinedTransform_);
> > + }
> > if (ret)
> > return ret;
> >
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > index a139c98a5a2b..0a795f4d2689 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > @@ -42,6 +42,7 @@ namespace RPi {
> > /* Map of mbus codes to supported sizes reported by the sensor. */
> > using SensorFormats = std::map<unsigned int, std::vector<Size>>;
> >
> > +class RPiCameraConfiguration;
> > class CameraData : public Camera::Private
> > {
> > public:
> > @@ -72,7 +73,8 @@ public:
> > V4L2VideoDevice *dev;
> > };
> >
> > - virtual CameraConfiguration::Status platformValidate(std::vector<StreamParams> &rawStreams,
> > + virtual CameraConfiguration::Status platformValidate(RPiCameraConfiguration *rpiConfig,
>
> Can the RPiCameraConfiguration *rpiConfig be made const in the declaration?
I'm surprised! platformValidate() could potentially change the
StreamConfiguration part of the CameraConfiguration BUT as you wrap
StreamConfiguration in a custom StreamParam function, we do not access
them through *config, so it can be made const even if conceptually it
is not
for (const auto &[index, cfg] : utils::enumerate(config_)) {
if (isRaw(cfg.pixelFormat))
rawStreams.emplace_back(index, &cfg);
else
outStreams.emplace_back(index, &cfg);
}
...
/* Do any platform specific fixups. */
status = data_->platformValidate(this, rawStreams, outStreams);
StreamParams has the potential to break some correctness guarantees
that 'const' gives you, as it stores StreamConfiguration by pointer,
bypassing the 'const CameraConfiguration'. Can't say I like it very
much, but not a comment for this patch...
>
> Other than that:
> Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
>
>
> > + std::vector<StreamParams> &rawStreams,
> > std::vector<StreamParams> &outStreams) const = 0;
> > virtual int platformConfigure(const V4L2SubdeviceFormat &sensorFormat,
> > std::optional<BayerFormat::Packing> packing,
> > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > index 018cf4881d0e..bf864d4174b2 100644
> > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > @@ -65,7 +65,8 @@ public:
> > {
> > }
> >
> > - CameraConfiguration::Status platformValidate(std::vector<StreamParams> &rawStreams,
> > + CameraConfiguration::Status platformValidate(RPi::RPiCameraConfiguration *rpiConfig,
> > + std::vector<StreamParams> &rawStreams,
> > std::vector<StreamParams> &outStreams) const override;
> >
> > int platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) override;
> > @@ -394,7 +395,8 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer
> > return 0;
> > }
> >
> > -CameraConfiguration::Status Vc4CameraData::platformValidate(std::vector<StreamParams> &rawStreams,
> > +CameraConfiguration::Status Vc4CameraData::platformValidate(RPi::RPiCameraConfiguration *rpiConfig,
> > + std::vector<StreamParams> &rawStreams,
> > std::vector<StreamParams> &outStreams) const
> > {
> > CameraConfiguration::Status status = CameraConfiguration::Status::Valid;
> > @@ -405,9 +407,29 @@ CameraConfiguration::Status Vc4CameraData::platformValidate(std::vector<StreamPa
> > return CameraConfiguration::Status::Invalid;
> > }
> >
> > - if (!rawStreams.empty())
> > + if (!rawStreams.empty()) {
> > rawStreams[0].dev = unicam_[Unicam::Image].dev();
> >
> > + /* Adjust the RAW stream to match the requested sensor config. */
> > + if (rpiConfig->sensorConfig) {
> > + StreamConfiguration *rawStream = rawStreams[0].cfg;
> > + BayerFormat rawBayer = BayerFormat::fromMbusCode(rpiConfig->sensorFormat_.mbus_code);
> > +
> > + /* Handle flips to make sure to match the RAW stream format. */
> > + if (flipsAlterBayerOrder_)
> > + rawBayer = rawBayer.transform(rpiConfig->combinedTransform_);
> > + PixelFormat rawFormat = rawBayer.toPixelFormat();
> > +
> > + if (rawStream->pixelFormat != rawFormat ||
> > + rawStream->size != rpiConfig->sensorConfig.outputSize) {
> > + rawStream->pixelFormat = rawFormat;
> > + rawStream->size = rpiConfig->sensorConfig.outputSize;
> > +
> > + status = CameraConfiguration::Adjusted;
> > + }
> > + }
> > + }
> > +
> > /*
> > * For the two ISP outputs, one stream must be equal or smaller than the
> > * other in all dimensions.
> > @@ -417,6 +439,8 @@ CameraConfiguration::Status Vc4CameraData::platformValidate(std::vector<StreamPa
> > for (unsigned int i = 0; i < outStreams.size(); i++) {
> > Size size;
> >
> > + /* \todo Warn if upscaling: reduces image quality. */
> > +
> > size.width = std::min(outStreams[i].cfg->size.width,
> > outStreams[0].cfg->size.width);
> > size.height = std::min(outStreams[i].cfg->size.height,
> > --
> > 2.40.1
> >
More information about the libcamera-devel
mailing list