[libcamera-devel] [PATCH 2/3] pipeline: rkisp1: Fix validation when sensor max is larger than ISP input
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Jul 23 10:51:44 CEST 2024
Quoting Umang Jain (2024-07-23 08:30:56)
> Hello all,
>
> ressurcting this old patch as it has went abandoned...
>
> On 17/01/24 9:27 pm, Laurent Pinchart via libcamera-devel wrote:
> > On Wed, Jan 17, 2024 at 11:43:43AM +0000, Kieran Bingham via libcamera-devel wrote:
> >> Quoting Paul Elder via libcamera-devel (2024-01-16 09:17:53)
> >>> If the maximum sensor output size is larger than the maximum ISP input
> >>> size, the maximum sensor size could be selected and the pipeline would
> >>> fail with an EPIPE. Fix this by clamping the sensor size to the ISP
> >>> input size at validation time.
> >> I think we should do more here (in a separate patch) to filter out
> >> sensor modes that are larger than the ISP max as well.
> >>
> >> At the moment it seems too easy for the rkisp1 pipeline handler to
> >> select an input configuration that it can't actually configure.
> >>
> >> But that's aside from this patch itself.
> >>
> >>> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> >>> ---
> >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 ++++++++++--
> >>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 26 +++++++++----------
> >>> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 4 ++-
> >>> 3 files changed, 30 insertions(+), 17 deletions(-)
> >>>
> >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>> index 586b46d64..fb67ba8f4 100644
> >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>> @@ -1202,11 +1202,24 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
> >>> if (param_->open() < 0)
> >>> return false;
> >>>
> >>> + /*
> >>> + * Retrieve ISP maximum input size for config validation in the path
> > s/Retrieve/Retrieve the/
> >
> >>> + * classes
> > s/$/./
> >
> >>> + */
> >>> + Size ispMaxInSize = { 0, 0 };
> > That's the default, so you can just write
> >
> > Size ispMaxInSize;
> >
> > I also find ispMaxInputSize more readable.
> >
> >>> + V4L2Subdevice::Formats ispFormats = isp_->formats(0);
> > const if you don't need to modify it.
> >
> >>> + for (const auto &[mbus, sizes] : ispFormats) {
> >>> + for (const auto &size : sizes) {
> >>> + if (ispMaxInSize < size.max)
> >>> + ispMaxInSize = size.max;
> >>> + }
> >>> + }
> > Do the maximum input size depend on the media bus format ? And to we
> > have multiple sizes in the sizes vector ? We could take some shortcuts,
> > knowing how the ISP driver is implemented, taking the first entry in
> > both ispFormats and sizes.
> >
> >>> +
> >>> /* Locate and open the ISP main and self paths. */
> >>> - if (!mainPath_.init(media_))
> >>> + if (!mainPath_.init(media_, ispMaxInSize))
> >>> return false;
> >>>
> >>> - if (hasSelfPath_ && !selfPath_.init(media_))
> >>> + if (hasSelfPath_ && !selfPath_.init(media_, ispMaxInSize))
> >>> return false;
> >>>
> >>> mainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);
> >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> >>> index b62b645ca..eaab7e857 100644
> >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> >>> @@ -62,7 +62,7 @@ RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
> >>> {
> >>> }
> >>>
> >>> -bool RkISP1Path::init(MediaDevice *media)
> >>> +bool RkISP1Path::init(MediaDevice *media, const Size &ispMaxInSize)
> > ispMaxInputSize here too if you rename it above. Same below.
> >
> >> It feels a bit odd to me passing ispMaxInSize as the only parameter to
> >> initialize. But I guess it save init from re-determining it for each
> >> path, and maybe it's the only variable for now - so I think we're ok
> >> here.
> >>
> >> I wonder if we should iterate and 'try' configurations to only expose
> >> those which can be supported. But we can tackle that on top I think.
> > Agreed.
> >
> >> So for now I am ok with this:
> >>
> >> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > with the above comments taken into account.
> >
> >>> {
> >>> std::string resizer = std::string("rkisp1_resizer_") + name_ + "path";
> >>> std::string video = std::string("rkisp1_") + name_ + "path";
> >>> @@ -76,6 +76,7 @@ bool RkISP1Path::init(MediaDevice *media)
> >>> return false;
> >>>
> >>> populateFormats();
> >>> + ispMaxInSize_ = ispMaxInSize;
> >>>
> >>> link_ = media->link("rkisp1_isp", 2, resizer, 0);
> >>> if (!link_)
> >>> @@ -172,7 +173,9 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,
> >>> /* Add all the RAW sizes the sensor can produce for this code. */
> >>> for (const auto &rawSize : sensor->sizes(mbusCode)) {
> >>> if (rawSize.width > maxResolution_.width ||
> >>> - rawSize.height > maxResolution_.height)
> >>> + rawSize.height > maxResolution_.height ||
> >>> + rawSize.width > ispMaxInSize_.width ||
> >>> + rawSize.height > ispMaxInSize_.height)
> >>> continue;
> >>>
> >>> streamFormats[format].push_back({ rawSize, rawSize });
> >>> @@ -275,8 +278,9 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
> >>> if (!found)
> >>> cfg->pixelFormat = isRaw ? rawFormat : formats::NV12;
> >>>
> >>> - Size minResolution;
> >>> - Size maxResolution;
> >>> + Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> >>> + .boundedTo(resolution);
> >>> + Size minResolution = minResolution_.expandedToAspectRatio(resolution);
>
> I think one point of discussion went un-noticed here, is whether this is
> the correct behaviour. If the sensor input size is larger than that of
> ISP, the maxResolution_ of the ISP is bounded to aspect ratio of the
> sensor highest resolution.
>
> I hit his recently - running IMX283 on i.MX8MP. I noticed the pipeline
> was getting a maximum supported resolution of 4096x2944 (ISP input max
> resolution is 4096x3072). And the resolution of IMX283 was 5472x3648
>
> So aspect ratio of IMX283 5472x3648 (1.5) brings the aspect ratio of ISP
> max input 4096x3072(1.33) to 4096x2944 (1.55)
>
> So the question here really is whether to preserve the original aspect
> ratio of the sensor's maximum output (4096x2944) and treat as that
> maxResolution - or the maxResolution should remain the same as supported
> by the ISP (4096x3072) in all cases.
Indeed, we now know that a sensor might expose cropped outputs that are
not the same aspect ratio as full sensor dimensions. We of course would
want to preserve /that/ aspect ratio too though - so I think it means we
really need to do better at how we expose what stream configurations are
possible entirely.
There could be 16:9, or 4:3 - or any modes with any other aspect ratio
provided by the sensor. Preserving only the full sensor aspect ratio
does not make sense.
--
Kieran
>
> >>>
> >>> if (isRaw) {
> >>> /*
> >>> @@ -287,16 +291,10 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
> >>> V4L2SubdeviceFormat sensorFormat =
> >>> sensor->getFormat({ mbusCode }, cfg->size);
> >>>
> >>> - minResolution = sensorFormat.size;
> >>> - maxResolution = sensorFormat.size;
> >>> - } else {
> >>> - /*
> >>> - * Adjust the size based on the sensor resolution and absolute
> >>> - * limits of the ISP.
> >>> - */
> >>> - minResolution = minResolution_.expandedToAspectRatio(resolution);
> >>> - maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> >>> - .boundedTo(resolution);
> >>> + if (!sensorFormat.size.isNull()) {
> >>> + minResolution = sensorFormat.size.boundedTo(ispMaxInSize_);
> >>> + maxResolution = sensorFormat.size.boundedTo(ispMaxInSize_);
> >>> + }
> >>> }
> >>>
> >>> cfg->size.boundTo(maxResolution);
> >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> >>> index cd77957ee..c96bd5557 100644
> >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> >>> @@ -35,7 +35,7 @@ public:
> >>> RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
> >>> const Size &minResolution, const Size &maxResolution);
> >>>
> >>> - bool init(MediaDevice *media);
> >>> + bool init(MediaDevice *media, const Size &ispMaxInSize);
> >>>
> >>> int setEnabled(bool enable) { return link_->setEnabled(enable); }
> >>> bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }
> >>> @@ -77,6 +77,8 @@ private:
> >>> std::unique_ptr<V4L2Subdevice> resizer_;
> >>> std::unique_ptr<V4L2VideoDevice> video_;
> >>> MediaLink *link_;
> >>> +
> >>> + Size ispMaxInSize_;
> >>> };
> >>>
> >>> class RkISP1MainPath : public RkISP1Path
>
More information about the libcamera-devel
mailing list