[libcamera-devel] [PATCH v2 4/4] libcamera: rpi: Handle SensorConfiguration
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Aug 1 11:25:32 CEST 2023
On Tue, Aug 01, 2023 at 11:23:44AM +0200, Jacopo Mondi wrote:
> 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
s/function/structure/ ofc
> 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