[libcamera-devel] [PATCH 12/13] libcamera: pipeline: rkisp1: Add format validation for self path
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Sep 15 02:15:21 CEST 2020
Hello,
On Mon, Sep 14, 2020 at 01:48:20PM +0200, Niklas Söderlund wrote:
> On 2020-08-20 12:12:25 +0200, Jacopo Mondi wrote:
> > On Thu, Aug 13, 2020 at 02:52:45AM +0200, Niklas Söderlund wrote:
> > > Extend the format validation to work with both man and self path. The
> > > heuristics honors that the first stream in the configuration have the
> > > highest priority while still examining both streams for a best match.
> > >
> > > This change extends the formats supported by the Cameras backed by this
> > > 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>
> > > ---
> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 220 ++++++++++++++++++-----
> > > 1 file changed, 171 insertions(+), 49 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 3761b7ef7a9386e3..f106b105a47bb4f7 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>
> > > @@ -50,6 +51,20 @@ static const std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{
> > > /* \todo Add support for 8-bit greyscale to DRM formats */
> > > };
> > >
> > > +static const Size RKISP1_RSZ_SP_SRC_MIN = { 32, 16 };
> > > +static const Size RKISP1_RSZ_SP_SRC_MAX = { 1920, 1920 };
> > > +static const std::array<PixelFormat, 9> RKISP1_RSZ_SP_FORMATS{
> >
> > same comments as in previous patches.
> >
> > Considering how you have to transport the template in the validatePath
> > function, I would use vectors here for the formats enumeration.
>
> I wrestled with this while developing it I wanted to use vector but
> thought it would be frowned upon ;-) I will switch to a vector in v2,
> this will however prevent it from being a constexpr.
std::array will be more efficient though. You can simplify
validatePath() by passing a Span<PixelFormat>, so it won't need to be a
template function.
> > > + formats::YUYV,
> > > + formats::YVYU,
> > > + formats::VYUY,
> > > + formats::NV16,
> > > + formats::NV61,
> > > + formats::NV21,
> > > + formats::NV12,
> > > + formats::BGR888,
> > > + formats::RGB565,
> > > +};
> > > +
> > > class PipelineHandlerRkISP1;
> > > class RkISP1ActionQueueBuffers;
> > > class RkISP1CameraData;
> > > @@ -179,13 +194,23 @@ public:
> > > private:
> > > static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> > >
> > > + bool fitAnyPath(const StreamConfiguration &cfg);
> > > +
> > > + template<std::size_t N>
> > > + CameraConfiguration::Status validatePath(StreamConfiguration *cfg,
> > > + const std::array<PixelFormat, N> &formats,
> >
> > A vector would be much nicer here.
> >
> > > + 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_;
> > >
> > > V4L2SubdeviceFormat sensorFormat_;
> > > };
> > > @@ -495,6 +520,76 @@ 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;
> > > +}
> > > +
> > > +template<std::size_t N>
> > > +CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(
> > > + StreamConfiguration *cfg, const std::array<PixelFormat, N> &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);
> > > + 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;
> >
> > Do we need to go to the video device here ? We know the format/sizes
> > are supported already...
>
> I do this to retrieve the true size as there might be aliment issues and
> such which we can retrieve from the kernel instead of mirroring it in
> the pipeline.
>
> >
> > > +
> > > + 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_);
> > > +}
> > > +
> > > CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> > > {
> > > const CameraSensor *sensor = data_->sensor_;
> > > @@ -504,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
> > > + * priority but if both main path and self path can satisfy it evaluate
> > > + * second stream first.
> > > + */
> > > + 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());
> >
> > Nice, but -why- ? What are the criteria you use to decide which
> > configu gets to which stream ? In example, I see the self path being
> > limited in sizes compared to the main, shouldn't this be taken into
> > account ? Same for the supported formats. How to you establish the
> > here reported 'order' ?
>
> It is in the fitAnyPath() check is it not? The design is based on that
> the first stream i the configuration requested have the highest priority
> to be satisfied. If both main and self path can satisfy the highest
> priority we reverse the priority to allow for the second stream to have
> the highest chance to also meets it requested configuration (or get as
> close as possible to it).
>
> > > +
> > > + 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";
> > > + 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) {
> >
> > Can't you get the status from here instead of repeating the check for
> > each stream 2 times ? You would only need to return Invalid if you
> > detect it, otherwise assign the value of validate*Path() to status.
> >
> > The only other reason I see here to keep 4 if switches is to printout
> > the result, but that could indeed be done depending on the returned
> > status.
>
> No I can't, this needs to be a two step process. One pass to try and get
> an exact match one one where adjustments are allowed. If we merge the
> two the first stream will always be mapped to the main path (either as a
> direct match or adjusted) and the second stream to the self path.
>
> By doing it as a two step pass we allow for an exact match on the self
> path where it would have been accepted as an adjusted match on the main
> path.
>
> > > + selfPathAvailable = false;
> > > + cfg = tryCfg;
> > > + cfg.setStream(&data_->selfPathStream_);
> > > + status = Adjusted;
> > > + LOG(RkISP1, Debug) << "Adjust match self";
> > > + continue;
> > > + }
> > > + }
> > > +
> > > + /* 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,
> > > @@ -532,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
More information about the libcamera-devel
mailing list