[libcamera-devel] [PATCH v1] pipeline: raspberrypi: Return all ISP resolutions from generateConfiguration()

[David Plowman]

Hi Kieran

Thanks for the comments.

On Fri, 7 Jan 2022 at 15:34, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Quoting David Plowman (2022-01-06 16:16:47)
> > Hi Naush
> >
> > Thanks for this patch!
> >
> > On Thu, 6 Jan 2022 at 11:32, Naushir Patuck <naush at raspberrypi.com> wrote:
> > >
> > > The libcamerasrc gstreamer component does seem to not allow stream resolutions
> > > that are not advertised by PipelineHandler::generateConfiguration(). This has
> > > been raised in a bug report [1].
> > >
> > > Fix this behavior by advertising a SizeRange from the minimum ISP resolution, up
> > > to the sensor resolution from PipelineHandlerRPi::generateConfiguration().
> > >
> > > [1] https://bugs.libcamera.org/show_bug.cgi?id=105
> > >
> > > Fixes: f16acb275c85 ("pipeline: raspberrypi: Restrict the advertised maximum ISP output resolution")
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index b5c687da467f..220a5749c0d9 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -639,8 +639,11 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,
> > >                          */
> > >                         for (const auto &format : fmts) {
> > >                                 PixelFormat pf = format.first.toPixelFormat();
> > > -                               if (pf.isValid())
> > > -                                       deviceFormats[pf].emplace_back(sensorSize);
> > > +                               if (pf.isValid()) {
> > > +                                       const SizeRange &ispSizes = format.second[0];
> > > +                                       deviceFormats[pf].emplace_back(ispSizes.min, sensorSize,
> > > +                                                                      ispSizes.hStep, ispSizes.vStep);
> > > +                               }
> >
> > Yes, looks fine to me.
> >
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> >
> > Just one related question... I wonder if we should munge those sizes
> > (and step values) so that applications only ever see values where the
> > width and stride match.
> >
> > If I do that then cheese (which uses gstreamer) actually produces
> > non-garbled images. Hurray (never seen that before)! Though trying to
> > run cheese using v4l2-compat.so seems to fail whatever I try. *sigh*
>
> I think if you try to run cheese with the v4l2-compat layer, while you
> have the gstreamer plugin available they'll conflict, as we can only
> have one CameraManager and you'll end up with two ...
>
> Perhaps that could be fixed somehow in the future if they could both end
> up mapping to the same one in the same process contexts ...
>
> This patch seems good though, I interpret it as exposing all the size
> options available from the isp min, only up to the sensor resolution
> which seems quite reasonable.

Would you have a view on the alignment question? (i.e. only
advertising sizes where the width happens to be such that the stride
matches it exactly with no padding bytes)

Thanks
David

>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> --
> Kieran
>
>
> >
> > David
> >
> > >                         }
> > >                 }
> > >
> > > --
> > > 2.25.1
> > >