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

[Kieran Bingham]

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.


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


> 
> David
> 
> >                         }
> >                 }
> >
> > --
> > 2.25.1
> >