[libcamera-devel] [PATCH v3 13/22] libcamera: pipeline: rkisp1: Add format validation for self path
Jacopo Mondi
jacopo at jmondi.org
Mon Sep 28 10:34:49 CEST 2020
Hi Niklas,
On Fri, Sep 25, 2020 at 06:48:25PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your feedback.
>
> On 2020-09-25 17:02:45 +0200, Jacopo Mondi wrote:
> > 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 ?
>
> Well the first role in the array given to generateConfiguration() have
> higher priority then the second. So this laves that the first element in
> the returned configuration shall have higher priority then the second
> right?
>
My point, in this patch and other, is that I don't see where the
priority order constraint comes from. I don't see it in the
Camera::generateConfiguration() or Camera::configure() documentation
and I'm wondering if that's a pipeline-specific behaviour (which, in
general, we should try to introduce, imho).
Thanks
j
> >
> > >
> > > 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
>
> At least in the kernel sources.
>
> >
> > > +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 ?
>
> Yes and no :-) The enabled ones are, but the todos are not.
>
> >
> > > } /* 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
>
> As in prev version I like this much better then to group them in on
> line. But as you say it's matter of taste.
>
> >
> > > + 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 ?
>
> I don't think so, similar to that we don't check stride and frameSize
> right?
>
not sure you know... stride and frameSize are output information,
bufferCount can actually be set by the application, doesn't it ?
> >
> > > + 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 ?
>
> No.
>
> We wish to check for exact match of the requested format. What we wish
> to learn is if the requested configuration can be satisfied on both
> paths without being adjusted.
>
> >
> > > + 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 ?
Asking the same question again, I might have missed where the priority
assignment requirement comes from.
> > 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)
>
> No this logic tries to maximize the possibility for both streams to be
> fully satisfied while still respecting that the first configuration have
> higher priority if not both can be satisfied.
>
> The for-loop bellows assign hardware in a first come first serve fashion
> and once a path have been assigned the assignment is never re-evaluated.
> It first tries to see if the configuration can be an exact match on any
> of the paths and then if it can be adjusted to fit (that is why two
> passes are needed).
>
> Above we check with fitsAllPaths(config_[0]) if the first configuration
> (with has the highest priority) can fit on any path without being
> adjusted. If so then we know it's safe to swap the order and of the
> configurations as to maximize config_[1] to find an exact match on one
> of the pats. We can do this as we already know config_[0] will have en
> exact match on either of them.
>
> If config_[0] can not find an exact match on both paths we do our best
> to make it fit first and config_[1] are stick with what ever resources
> are left.
>
> >
> > > +
> > > + 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 ?
>
> Good point, no it's not needed.
>
Wait, maybe bufferCount or stride has changed, as their modification
doesn't 'Adjust' the stream status.
> >
> > > + 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 :)
>
> I will drop them as they seem to confuse more then help.
>
Thanks, I think it could help if expanded. As a reference on IPU3:
LOG(IPU3, Debug) << "Assigned " << cfg->toString()
<< " to the main output";
> >
> > > + 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.
>
> If it was only sizes that differed between the two yes, but it's not.
> The main path can support RAW formats while the self path can support
> RGB. So sorting by size is unfortunately not possible as we also need to
> consider the formats.
But in that case assigning by format is even easier. RAW goes to main
and RGB goes to self.
>
> >
> > 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 ?
>
> RAW will be a special case on this pipeline as it will limit it to one
> stream. When capturing RAW only the main path can be active.
>
> But same question for RGB formats which are only supported by the self
> path. No this is tested with that in mind as adding BGR888 to the list
> of supported formats in RKISP1_RSZ_SP_FORMATS is all that is needed to
> enable it to be captured. But the frames looks a bit wierd in qcam so I
> will investigate this on top of this series but I do not wish to block
> this already too large series on also enabeling support for BGR888 and
> RGB656.
Agreed on non blocking to wait for RGB.
>
> >
> > > +
> > > + /* 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
>
> --
> Regards,
> Niklas Söderlund
More information about the libcamera-devel
mailing list