[libcamera-devel] [PATCH v2 12/13] libcamera: pipeline: rkisp1: Add format validation for self path
Niklas Söderlund
niklas.soderlund at ragnatech.se
Tue Sep 22 16:48:18 CEST 2020
Hi Laurent,
Thanks for your feedback.
On 2020-09-15 04:32:17 +0300, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Mon, Sep 14, 2020 at 04:21:48PM +0200, Niklas Söderlund wrote:
> > Extend the format validation to work with both man and self path. The
>
> s/man/main/
> s/path/paths/
>
> > heuristics honors that the first stream in the configuration have the
>
> s/have/has/
>
> > highest priority while still examining both streams for a best match.
> >
> > This change extends the formats supported by the Cameras backed by this
>
> s/Cameras/cameras/
>
> > pipeline to also include RGB888 and RGB656, that is only available on
> > the self path.
> >
> > 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 v1
> > - Store formats in std::vector instead of std::array to avoid template
> > usage for validate function.
>
> As commented (too late) on v1, you don't have to do this, you can keep
> an array, and pass a Span<PixelFormat> to validate().
>
> > ---
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 222 +++++++++++++++++------
> > 1 file changed, 171 insertions(+), 51 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index bc961f8e78f2c979..851ff68f138b98dd 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>
> > @@ -40,7 +41,7 @@ LOG_DEFINE_CATEGORY(RkISP1)
> > namespace {
> > constexpr Size RKISP1_RSZ_MP_SRC_MIN { 32, 16 };
> > constexpr Size RKISP1_RSZ_MP_SRC_MAX { 4416, 3312 };
> > - constexpr std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{
> > + const std::vector<PixelFormat> RKISP1_RSZ_MP_FORMATS{
> > formats::YUYV,
> > formats::YVYU,
> > formats::VYUY,
> > @@ -50,7 +51,21 @@ namespace {
> > 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 };
> > + const std::vector<PixelFormat> RKISP1_RSZ_SP_FORMATS{
> > + formats::YUYV,
> > + formats::YVYU,
> > + formats::VYUY,
> > + formats::NV16,
> > + formats::NV61,
> > + formats::NV21,
> > + formats::NV12,
> > + formats::BGR888,
> > + formats::RGB565,
> > + };
> > +};
> >
> > class PipelineHandlerRkISP1;
> > class RkISP1ActionQueueBuffers;
> > @@ -181,13 +196,22 @@ public:
> > private:
> > static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> >
> > + bool fitAnyPath(const StreamConfiguration &cfg);
> > +
> > + CameraConfiguration::Status validatePath(StreamConfiguration *cfg,
> > + const std::vector<PixelFormat> &formats,
> > + const Size &min, const Size &max,
> > + V4L2VideoDevice *video);
> > + CameraConfiguration::Status validateMainPath(StreamConfiguration *cfg);
> > + CameraConfiguration::Status validateSelfPath(StreamConfiguration *cfg);
> > +
> > /*
> > * The RkISP1CameraData instance is guaranteed to be valid as long as the
> > * corresponding Camera instance is valid. In order to borrow a
> > * reference to the camera data, store a new reference to the camera.
> > */
> > std::shared_ptr<Camera> camera_;
> > - const RkISP1CameraData *data_;
> > + RkISP1CameraData *data_;
>
> This isn't nice :-( I'd like to find a wait to keep it const. Can be
> done in a separate patch.
I really tried to keep this const but all I can think of is the fetch
the Stream* from camera_ instead of data_ in the rather odd hack,
Stream *mainStream = nullptr;
Stream *selfStream = nullptr;
for (Stream *stream : camera_->streams()) {
if (stream == &data_->mainPathStream_)
mainStream = stream;
else if (stream == &data_->selfPathStream_)
selfStream = stream;
}
ASSERT(mainStream && selfStream);
But this seems like casting a const variable to a non-const with extra
steps. Looking at the IPU3 pipeline handler this is exactly what is
done, from IPU3CameraConfiguration::validate():
cfg->setStream(const_cast<Stream *>(&data_->outStream_));
I think this highlights an issue with our overall design and something
that should be corrected. As this transient more pipeline handlers I
will record this as a todo and use the IPU3 pattern here to make it easy
to find a solution which solves the root problem.
>
> >
> > V4L2SubdeviceFormat sensorFormat_;
> > };
> > @@ -497,6 +521,75 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
> > data_ = data;
> > }
> >
> > +bool RkISP1CameraConfiguration::fitAnyPath(const StreamConfiguration &cfg)
> > +{
> > + if (std::find(RKISP1_RSZ_MP_FORMATS.begin(),
> > + RKISP1_RSZ_MP_FORMATS.end(), cfg.pixelFormat) ==
> > + RKISP1_RSZ_MP_FORMATS.end())
> > + return false;
> > +
> > + if (cfg.size < RKISP1_RSZ_MP_SRC_MIN || cfg.size > RKISP1_RSZ_MP_SRC_MAX)
> > + return false;
> > +
> > + if (std::find(RKISP1_RSZ_SP_FORMATS.begin(),
> > + RKISP1_RSZ_SP_FORMATS.end(), cfg.pixelFormat) ==
> > + RKISP1_RSZ_SP_FORMATS.end())
> > + return false;
> > +
> > + if (cfg.size < RKISP1_RSZ_SP_SRC_MIN || cfg.size > RKISP1_RSZ_SP_SRC_MAX)
> > + return false;
> > +
> > + return true;
>
> The implementation looks like a fitsAllPaths(), not any path.
>
> > +}
> > +
> > +CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(
> > + StreamConfiguration *cfg, const std::vector<PixelFormat> &formats,
> > + const Size &min, const Size &max, V4L2VideoDevice *video)
>
> formats, min and max are also candidates to be stored in the RkISP1Path
> class.
>
> > +{
> > + 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);
> > + 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) {
> > + 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_);
> > +}
>
> And those two functions could then disappear. Are you really sure you
> want to address all that on top ?
That is my current plan. But to extinguish any doubt of that work I will
do it on-top now and submit it as part of v3.
>
> > +
> > CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> > {
> > const CameraSensor *sensor = data_->sensor_;
> > @@ -506,22 +599,86 @@ 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 them streams. The first stream have the highest
>
> "them streams" ?
> s/have/has/
>
> > + * priority but if both main path and self path can satisfy it evaluate
> > + * second stream first.
>
> Why ?
>
> > + */
> > + std::vector<unsigned int> order(config_.size());
> > + std::iota(order.begin(), order.end(), 0);
> > + if (config_.size() == 2 && fitAnyPath(config_[0]))
> > + std::reverse(order.begin(), order.end());
>
> A bit complicated for a vector of two elements :-)
>
> > +
> > + 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(&data_->mainPathStream_);
> > + LOG(RkISP1, Debug) << "Exact match main";
>
> The message is a bit cryptic for someone reading the log without knowing
> the code.
Sure, but is not the Debug level intended for people debugging the code?
I would be fine dropping the LOG() statements all together. I found them
useful when testing this series and chased to leave them in.
>
> > + continue;
> > + }
> > + }
> > +
> > + if (selfPathAvailable) {
> > + StreamConfiguration tryCfg = cfg;
> > + if (validateSelfPath(&tryCfg) == Valid) {
> > + selfPathAvailable = false;
> > + cfg = tryCfg;
> > + cfg.setStream(&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(&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(&data_->selfPathStream_);
> > + status = Adjusted;
> > + LOG(RkISP1, Debug) << "Adjust match self";
> > + continue;
> > + }
> > + }
>
> There's lots of code duplication, adding a RkISP1Path class would help.
>
> > +
> > + /* 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,
> > @@ -534,47 +691,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;
> > }
> >
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list