| Message ID | 20230731113115.5915-5-jacopo.mondi@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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@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? Other than that: Reviewed-by: Naushir Patuck <naush@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 >
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@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@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@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 > >
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@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@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@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 > > >
Hi Jacopo, On Tue, 1 Aug 2023 at 10:23, Jacopo Mondi <jacopo.mondi@ideasonboard.com> 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@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@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... I see. Agree that the StreamParams sort of duplicates the new rpiConfig param, but with the added benefit of being sorted on size. This is something we can look at changing on top of this change. In the meantime, I agree we probably don't want to use const for rpiConfig for this reason. Regards, Naush > > > > > > Other than that: > > Reviewed-by: Naushir Patuck <naush@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 > > >
Quoting Naushir Patuck via libcamera-devel (2023-08-01 10:02:15) > 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@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@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? > > Other than that: > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> I've got nothing additional to add so: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.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 > >
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, + 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,
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@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(-)