[libcamera-devel] [PATCH v3 13/22] libcamera: pipeline: rkisp1: Add format validation for self path
Jacopo Mondi
jacopo at jmondi.org
Fri Sep 25 17:02:45 CEST 2020
Hi Niklas,
On Fri, Sep 25, 2020 at 03:41:58AM +0200, Niklas Söderlund wrote:
> Extend the format validation to work with both main and self paths. The
> heuristics honors that the first stream in the configuration has the
> highest priority while still examining both streams for a best match.
I've heard you mentioning the priority of streams being defined by
their order multiple time, but this seems to be specific to this
pipeline handler, or have I missed it ?
>
> It is not possible to capture from the self path as the self stream is
> not yet exposed to applications.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> * Changes since v2
> - Fix spelling in commit message.
> - Use Span<> instead of turning arrays to vectors.
> - Keep data_ const and cast 'const Streams*' to non-const using
> const_cast<Stream *>() to match the IPU3 pipeline.
> - Rename fitAnyPath() to fitsAllPaths().
> - Expand documentation for why second stream is evaluated first if the
> fist stream can use either stream.
> - Drop support for RGB888 and RGB656 for selfpath which was present in
> v2 as the driver delivers odd data when the frames are observed.
>
> * Changes since v1
> - Store formats in std::vector instead of std::array to avoid template
> usage for validate function.
> ---
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 210 +++++++++++++++++------
> 1 file changed, 162 insertions(+), 48 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index cd3049485746edd6..bd53183a034efaff 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -9,6 +9,7 @@
> #include <array>
> #include <iomanip>
> #include <memory>
> +#include <numeric>
> #include <queue>
>
> #include <linux/media-bus-format.h>
> @@ -19,6 +20,7 @@
> #include <libcamera/formats.h>
> #include <libcamera/ipa/rkisp1.h>
> #include <libcamera/request.h>
> +#include <libcamera/span.h>
> #include <libcamera/stream.h>
>
> #include "libcamera/internal/camera_sensor.h"
> @@ -50,6 +52,19 @@ constexpr std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{
> formats::NV12,
> /* \todo Add support for 8-bit greyscale to DRM formats */
> };
> +
> +constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };
> +constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 };
1920x1920 ? Maybe it's just the way it actually is
> +constexpr std::array<PixelFormat, 7> RKISP1_RSZ_SP_FORMATS{
> + formats::YUYV,
> + formats::YVYU,
> + formats::VYUY,
> + formats::NV16,
> + formats::NV61,
> + formats::NV21,
> + formats::NV12,
> + /* \todo Add support for BGR888 and RGB565 */
> +};
Aren't these equal to the main path ones ?
> } /* namespace */
>
> class PipelineHandlerRkISP1;
> @@ -181,6 +196,14 @@ public:
> private:
> static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
>
> + CameraConfiguration::Status validatePath(StreamConfiguration *cfg,
> + const Span<const PixelFormat> &formats,
> + const Size &min, const Size &max,
> + V4L2VideoDevice *video);
> + CameraConfiguration::Status validateMainPath(StreamConfiguration *cfg);
> + CameraConfiguration::Status validateSelfPath(StreamConfiguration *cfg);
> + bool fitsAllPaths(const StreamConfiguration &cfg);
> +
> /*
> * The RkISP1CameraData instance is guaranteed to be valid as long as the
> * corresponding Camera instance is valid. In order to borrow a
> @@ -492,6 +515,69 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
> data_ = data;
> }
>
> +CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(
> + StreamConfiguration *cfg, const Span<const PixelFormat> &formats,
> + const Size &min, const Size &max, V4L2VideoDevice *video)
> +{
> + const StreamConfiguration reqCfg = *cfg;
> + Status status = Valid;
> +
> + if (std::find(formats.begin(), formats.end(), cfg->pixelFormat) ==
> + formats.end())
> + cfg->pixelFormat = formats::NV12;
> +
> + cfg->size.boundTo(max);
> + cfg->size.expandTo(min);
This could be a one liner, but it's a matter of tastes
> + cfg->bufferCount = RKISP1_BUFFER_COUNT;
> +
> + V4L2DeviceFormat format = {};
> + format.fourcc = video->toV4L2PixelFormat(cfg->pixelFormat);
> + format.size = cfg->size;
> +
> + int ret = video->tryFormat(&format);
> + if (ret)
> + return Invalid;
> +
> + cfg->stride = format.planes[0].bpl;
> + cfg->frameSize = format.planes[0].size;
> +
> + if (cfg->pixelFormat != reqCfg.pixelFormat || cfg->size != reqCfg.size) {
Should you check for bufferCount too ?
> + LOG(RkISP1, Debug)
> + << "Adjusting format from " << reqCfg.toString()
> + << " to " << cfg->toString();
> + status = Adjusted;
> + }
> +
> + return status;
> +}
> +
> +CameraConfiguration::Status RkISP1CameraConfiguration::validateMainPath(StreamConfiguration *cfg)
> +{
> + return validatePath(cfg, RKISP1_RSZ_MP_FORMATS, RKISP1_RSZ_MP_SRC_MIN,
> + RKISP1_RSZ_MP_SRC_MAX, data_->mainPathVideo_);
> +}
> +
> +CameraConfiguration::Status RkISP1CameraConfiguration::validateSelfPath(StreamConfiguration *cfg)
> +{
> + return validatePath(cfg, RKISP1_RSZ_SP_FORMATS, RKISP1_RSZ_SP_SRC_MIN,
> + RKISP1_RSZ_SP_SRC_MAX, data_->selfPathVideo_);
> +}
> +
> +bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)
> +{
> + StreamConfiguration config;
> +
> + config = cfg;
> + if (validateMainPath(&config) != Valid)
Shouldn't we accept Adjusted too if we want to check if the stream
'fits' the path ?
> + return false;
> +
> + config = cfg;
> + if (validateSelfPath(&config) != Valid)
> + return false;
> +
> + return true;
> +}
> +
> CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> {
> const CameraSensor *sensor = data_->sensor_;
> @@ -501,22 +587,87 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> return Invalid;
>
> /* Cap the number of entries to the available streams. */
> - if (config_.size() > 1) {
> - config_.resize(1);
> + if (config_.size() > 2) {
> + config_.resize(2);
> status = Adjusted;
> }
>
> - StreamConfiguration &cfg = config_[0];
> -
> - /* Adjust the pixel format. */
> - if (std::find(RKISP1_RSZ_MP_FORMATS.begin(), RKISP1_RSZ_MP_FORMATS.end(),
> - cfg.pixelFormat) == RKISP1_RSZ_MP_FORMATS.end()) {
> - LOG(RkISP1, Debug) << "Adjusting format to NV12";
> - cfg.pixelFormat = formats::NV12,
> - status = Adjusted;
> + /*
> + * If there are more then one stream in the configuration figure out the
> + * order to evaluate the streams. The first stream has the highest
> + * priority but if both main path and self path can satisfy it evaluate
> + * second stream first as the first stream is guaranteed to work with
> + * whichever path is not used by the second one.
> + */
> + std::vector<unsigned int> order(config_.size());
> + std::iota(order.begin(), order.end(), 0);
> + if (config_.size() == 2 && fitsAllPaths(config_[0]))
> + std::reverse(order.begin(), order.end());
Let alone the implementation which is nice, is really the order of the
configurations that defines to which output they should be assigned ?
I'm not familiar with the platform, but in example, I would expect the
larger to go to the main path (as at this time they support the same
formats)
> +
> + bool mainPathAvailable = true;
> + bool selfPathAvailable = true;
> + for (unsigned int index : order) {
> + StreamConfiguration &cfg = config_[index];
> +
> + /* Try to match stream without adjusting configuration. */
> + if (mainPathAvailable) {
> + StreamConfiguration tryCfg = cfg;
> + if (validateMainPath(&tryCfg) == Valid) {
> + mainPathAvailable = false;
> + cfg = tryCfg;
> + cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_));
> + LOG(RkISP1, Debug) << "Exact match main";
> + continue;
> + }
> + }
> +
> + if (selfPathAvailable) {
> + StreamConfiguration tryCfg = cfg;
> + if (validateSelfPath(&tryCfg) == Valid) {
> + selfPathAvailable = false;
> + cfg = tryCfg;
Do you need to re-assign it if it's valid ?
> + cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_));
> + LOG(RkISP1, Debug) << "Exact match self";
> + continue;
> + }
> + }
> +
> + /* Try to match stream allowing adjusting configuration. */
> + if (mainPathAvailable) {
> + StreamConfiguration tryCfg = cfg;
> + if (validateMainPath(&tryCfg) == Adjusted) {
> + mainPathAvailable = false;
> + cfg = tryCfg;
> + cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_));
> + status = Adjusted;
> + LOG(RkISP1, Debug) << "Adjust match main";
> + continue;
> + }
> + }
> +
> + if (selfPathAvailable) {
> + StreamConfiguration tryCfg = cfg;
> + if (validateSelfPath(&tryCfg) == Adjusted) {
> + selfPathAvailable = false;
> + cfg = tryCfg;
> + cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_));
> + status = Adjusted;
> + LOG(RkISP1, Debug) << "Adjust match self";
If I were to read this without reading the code, I would be puzzled.
same for other messages :)
> + continue;
> + }
> + }
I dug out the comment from the review of the first version where I
asked "why" and the answer was that it's done not to adjust a stream
where it is not needed, so you initially only wants "valid" then try
accept "adjusted" if "valid" cannot be obtained.
So, what are the resons for "adjusting" ? Or either the pixel format
is wrong, and you would need to adjust on both nodes, or if the sizes
are larger the ones supported by the selfpath, in that case you
fallback to the main path and even if in that case they're too large,
adjust the stream. I think you could work this out easily if you sort
streams by size, but as I didn't get where "first as higher priority"
constraints come from, I might be mistaken.
If size sorted you try with main path first, if it's not taken and you
accept Valid || Adjusted. The second stream goes to the selfpath, and
you accept Valid || Adjusted there too without duplicating the check.
But if the priority order is justified or even part of the libcamera
API documentation, I think what you have here is fine.
Also, I don't know how RAW will work with this pipeline, but will this
double pass scale well in that case or there shouldn't be particular
issues ?
> +
> + /* All paths rejected configuraiton. */
> + LOG(RkISP1, Debug) << "Camera configuration not supported "
> + << cfg.toString();
> + return Invalid;
> }
>
> /* Select the sensor format. */
> + Size maxSize;
> + for (const StreamConfiguration &cfg : config_)
> + maxSize = std::max(maxSize, cfg.size);
> +
> sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,
> MEDIA_BUS_FMT_SGBRG12_1X12,
> MEDIA_BUS_FMT_SGRBG12_1X12,
> @@ -529,47 +680,10 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> MEDIA_BUS_FMT_SGBRG8_1X8,
> MEDIA_BUS_FMT_SGRBG8_1X8,
> MEDIA_BUS_FMT_SRGGB8_1X8 },
> - cfg.size);
> + maxSize);
> if (sensorFormat_.size.isNull())
> sensorFormat_.size = sensor->resolution();
>
> - /*
> - * Provide a suitable default that matches the sensor aspect
> - * ratio and clamp the size to the hardware bounds.
> - *
> - * \todo: Check the hardware alignment constraints.
> - */
> - const Size size = cfg.size;
> -
> - if (cfg.size.isNull()) {
> - cfg.size.width = 1280;
> - cfg.size.height = 1280 * sensorFormat_.size.height
> - / sensorFormat_.size.width;
> - }
> -
> - cfg.size.boundTo(RKISP1_RSZ_MP_SRC_MAX);
> - cfg.size.expandTo(RKISP1_RSZ_MP_SRC_MIN);
> -
> - if (cfg.size != size) {
> - LOG(RkISP1, Debug)
> - << "Adjusting size from " << size.toString()
> - << " to " << cfg.size.toString();
> - status = Adjusted;
> - }
> -
> - cfg.bufferCount = RKISP1_BUFFER_COUNT;
> -
> - V4L2DeviceFormat format = {};
> - format.fourcc = data_->mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat);
> - format.size = cfg.size;
> -
> - int ret = data_->mainPathVideo_->tryFormat(&format);
> - if (ret)
> - return Invalid;
> -
> - cfg.stride = format.planes[0].bpl;
> - cfg.frameSize = format.planes[0].size;
> -
> return status;
> }
>
> --
> 2.28.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list