[PATCH] libcamera: rkisp1: Clamp stream configuration to ISP limit on raw path
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Oct 8 09:48:30 CEST 2024
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".
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.
If my understanding is correct, then please add my tag
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>
> 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