[PATCH] libcamera: rkisp1: Clamp stream configuration to ISP limit on raw path
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Wed Oct 9 07:52:23 CEST 2024
Hi Umang
On Wed, Oct 09, 2024 at 12:10:37AM GMT, Umang Jain wrote:
> Hi Jacopo,
>
> On 08/10/24 1:18 pm, Jacopo Mondi wrote:
> > Umang,
> > seems like I'm failing to make my point across, sorry about this
> >
> > On Fri, Oct 04, 2024 at 04:45:01PM GMT, Umang Jain wrote:
> > > Hi Jacopo,
> > >
> > > On 04/10/24 3:58 pm, Jacopo Mondi wrote:
> > > > Hi Umang
> > > >
> > > > On Fri, Oct 04, 2024 at 03:17:34PM GMT, Umang Jain wrote:
> > > > > Hi Jacopo
> > > > >
> > > > > On 04/10/24 3:07 pm, Jacopo Mondi wrote:
> > > > > > Hi Umang
> > > > > >
> > > > > > On Fri, Oct 04, 2024 at 10:23:38AM GMT, Umang Jain wrote:
> > > > > > > Commit 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not
> > > > > > > supported by the pipeline") introduced a mechanism to determine maximum
> > > > > > > supported sensor resolution and filter out resolutions that cannot be
> > > > > > > supported by the ISP.
> > > > > > >
> > > > > > > However, it missed to update the raw stream configuration path, where
> > > > > > > it should have clamped the raw stream configuration size to the maximum
> > > > > > > sensor supported resolution.
> > > > > > >
> > > > > > > This patch fixes the above issue and can be confirmed with IMX283
> > > > > > > on i.MX8MP:
> > > > > > >
> > > > > > > From:
> > > > > > > ($) cam -c1 -srole=raw,width=5472,height=3072
> > > > > > > INFO Camera camera.cpp:1197 configuring streams: (0) 5472x3648-SRGGB12
> > > > > > > ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12
> > > > > > > Failed to configure camera
> > > > > > > Failed to start camera session
> > > > > > >
> > > > > > > To:
> > > > > > > ($) cam -c1 -srole=raw,width=5472,height=3072
> > > > > > > INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12
> > > > > > > cam0: Capture until user interrupts by SIGINT
> > > > > > > 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824
> > > > > > > 536.182378 (10.00 fps) cam0-stream0 seq: 000001 bytesused: 25165824
> > > > > > > 536.282375 (10.00 fps) cam0-stream0 seq: 000002 bytesused: 25165824
> > > > > > > ...
> > > > > > >
> > > > > > > Fixes: 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline")
> > > > > > > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > > > > > > ---
> > > > > > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++-
> > > > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > > > > > > index da8d25c3..feb6d89f 100644
> > > > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > > > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > > > > > > @@ -316,9 +316,11 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
> > > > > > > if (isRaw) {
> > > > > > > /*
> > > > > > > * Use the sensor output size closest to the requested stream
> > > > > > > - * size.
> > > > > > > + * size while ensuring the output size doesn't exceed ISP limits.
> > > > > > > */
> > > > > > > uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);
> > > > > > > + cfg->size.boundTo(resolution);
> > > > > > > +
> > > > > > > V4L2SubdeviceFormat sensorFormat =
> > > > > > > sensor->getFormat({ mbusCode }, cfg->size);
> > > > > > CameraSensor::getFormat() returns a sensor resolution large enough to
> > > > > > accomodate the requested size, at least that's how I read the
> > > > > > following part of the documentation
> > > > > >
> > > > > > * \a size indicates the desired size at the output of the sensor. This function
> > > > > > * selects the best media bus code and size supported by the sensor according
> > > > > > * to the following criteria.
> > > > > > *
> > > > > > * - The desired \a size shall fit in the sensor output size to avoid the need
> > > > > > * to up-scale.
> > > > > >
> > > > > > And this part of the code
> > > > > >
> > > > > > if (sz.width < size.width || sz.height < size.height)
> > > > > > continue;
> > > > > >
> > > > > > So, at least in my understanding "sensorFormat" could represent a size
> > > > > > larger than cfg->size. Is this your understanding as well ?
> > > > > "sensorFormat" could represent a size larger than cfg->size
> > > > >
> > > > > and
> > > > >
> > > > > could also represent a size large than max supported resolution (i.e.
> > > > > 'resolution' variable in this case)
> > > > >
> > > > > For e.g. in IMX283 case it would, 5472x3648
> > > > >
> > > > > > In the lines below this function we have
> > > > > >
> > > > > > minResolution = sensorFormat.size;
> > > > > > maxResolution = sensorFormat.size;
> > > > > From the above e.g. of IMX283 minResolution = maxResolution = 5472x3648
> > > > > > cfg->size.boundTo(maxResolution);
> > > > > > cfg->size.expandTo(minResolution);
> > > > > >
> > > > > > So if ((maxResolution = minResolution) > cfg->size) will the calls to
> > > > > > boundTo() followed by expandTo() enlarge cfg->size ?
> > > > > so cfg->size is 5472x3648 here, which can't be supported.
> > > > Sorry, I'm confused. Aren't you confirming me that the resulting
> > > > cfg->size cannot be supported (with this patch I mean) ?
> > > So user passes a relatively high resolution, supported by the sensor (but
> > > not with ISP)
> > >
> > > cam -c1 -srole=raw,width=5472,height=3648
> > >
> > > so cfg-size here is 5472x3648.
> > >
> > > So without this patch, you would right now get:
> > >
> > > ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12
> > > Failed to configure camera
> > > Failed to start camera session
> > >
> > > With this patch patched, you would get:
> > >
> > > INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12
> > > cam0: Capture until user interrupts by SIGINT
> > > 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824
> > > ....
> > >
> > > So What has happened:
> > >
> > > User has requested a raw stream with 5472x3648. It was found that this can't
> > > be supported hence,
> > > the camera configuration is adjusted to a (lower) but maximum supported
> > > resolution and delivers raw
> > > frames with 4096x3072.
> > I understand this fixes your use case but my concern, as explained in
> > he previous email is that
> >
> > cfg->size.boundTo(resolution);
> >
> > V4L2SubdeviceFormat sensorFormat =
> > sensor->getFormat({ mbusCode }, cfg->size);
> >
> > minResolution = sensorFormat.size;
> > maxResolution = sensorFormat.size;
> >
> >
> > cfg->size.boundTo(maxResolution);
> > cfg->size.expandTo(minResolution);
> >
> > as CameraSensor::getFormat() can return a resolution -higher- than the
> > requested one (cfg->size) there is a risk that cfg->size is later
> > expanded to a larger value.
> >
> > Now, if I get this right "resolution" is returned by the newly introduced
> > RkISP1Path::filterSensorResolution() and it's guaranteed to be the
> > "higher sensor's resolution supported by the ISP".
>
> yes, resolution is is the 'highest sensor's resolution supported by the
> ISP' hence, we never allow cfg->size to go beyond that.
> >
> > if that's the case, then CameraSensor::getFormat() will never return
> > a size larger than "cfg->size.boundedTo(resolution)" as "resolution"
> > is known to be a size supported by the sensor itself.
>
> Indeed, that's what's the intent of the patch.
>
> >
> > If my understanding is correct, then please add my tag
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>
> You're understanding is correct.
>
> Do you think this understand needs to be documented in form of comments ?
>
Maybe yes.. Just a small note along the lines of
- * size.
+ * size while ensuring the output size doesn't exceed ISP limits.
+ *
+ * As 'resolution' is the largest sensor resolution
+ * supported by the ISP, CameraSensor::getFormat() will never
+ * return a V4L2SubdeviceFormat with a larger size.
*/
Or whatever you like the most.
Thanks
j
> >
> > > One can argue that we should never adjust camera configuration to a lower
> > > size than cfg->size (i.e. what the user asked).
> > >
> > I don't think that's a problem
> >
> > > But then,
> > >
> > > * \var CameraConfiguration::Adjusted
> > > * The configuration has been adjusted to a valid configuration
> > >
> > > and
> > >
> > > * \retval CameraConfigutation::Adjusted The configuration has been adjusted
> > > * and is now valid. Parameters may have changed for any stream, and stream
> > > * configurations may have been removed. The caller shall check the
> > > * configuration carefully.
> > >
> > >
> > > > > Have you noticed the failure case I mentioned in the commit message?
> > > > While the commmit message suggests this patch makes it valid ?
> > > >
> > > > What am I missing ? :)
> > > >
> > > > > > Wouldn't it be better to use 'resolution' in
> > > > > >
> > > > > > V4L2SubdeviceFormat sensorFormat =
> > > > > > sensor->getFormat({ mbusCode }, cfg->size);
> > > > > >
> > > > > > instead of cfg->size ?
> > > > > No, otherwise it will always pick the highest sensor output size.
> > > > >
> > > > > For e.g. I request -srole=raw,width=1920,height=1080
> > > > >
> > > > > "cfg->size" will pick 2736x1824 for IMX283
> > > > >
> > > > > "resolution" will always pick 4096x3072.
> > > > > > Thanks
> > > > > > j
> > > > > >
> > > > > > > --
> > > > > > > 2.45.2
> > > > > > >
>
More information about the libcamera-devel
mailing list