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

[Kieran Bingham]

Hi David,

Quoting David Plowman (2022-01-07 15:37:10)
> 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)

I would say if there are known limitations, then it is the
responsibility of the PH to ensure it only reports what is supported.
Though in this case, it's a little bit dubious as the padding bytes are
supported, but I think we're likely not conveying the stride correctly
to the gstreamer element perhaps.

So I think that's all worth some investigation, but on top. I'll create
a bugzilla entry to track it, as I think this patch could be merged
without waiting for that.

  https://bugs.libcamera.org/show_bug.cgi?id=108

Either we should correctly convey the stride information to gstreamer,
and it should make use of it accordingly, or we should not support that
size. (or if the limitation is on gstreamer, then it should not use a
size it can't support).

--
Kieran


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