[libcamera-devel] [PATCH] pipeline: raspberrypi: Set sensor default orientation before configure()

[David Plowman]

Hi everyone

On Mon, 27 Jul 2020 at 09:07, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> Hi Laurent,
>
> On Sat, 25 Jul 2020 at 01:20, Laurent Pinchart
> <laurent.pinchart at ideasonboard.com> wrote:
> >
> > Hi Naush,
> >
> > On Tue, Jul 21, 2020 at 01:40:09PM +0100, Naushir Patuck wrote:
> > > On Tue, 21 Jul 2020 at 12:35, Kieran Bingham wrote:
> > > > On 21/07/2020 10:23, Naushir Patuck wrote:
> > > > > The default sensor orientation must be set early on in match() to ensure
> > > > > generateConfiguration() and configure() return out the correct Bayer
> > > > > ordering to the application. This is particularly important for RAW
> > > > > capture streams.
> > > > >
> > > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > > ---
> > > > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 +++++++-------
> > > > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > index bf1c7714..e9084afd 100644
> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > @@ -962,6 +962,13 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> > > > >       /* Initialize the camera properties. */
> > > > >       data->properties_ = data->sensor_->properties();
> > > > >
> > > > > +     /* Configure the H/V flip controls based on the sensor rotation. */
> > > > > +     ControlList ctrls(data->unicam_[Unicam::Image].dev()->controls());
> > > > > +     int32_t rotation = data->properties_.get(properties::Rotation);
> > > > > +     ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation));
> > > > > +     ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation));
> > > > > +     data->unicam_[Unicam::Image].dev()->setControls(&ctrls);
> > > > > +
> > > >
> > > > I wonder if this should be done in some CameraData initialisation, but
> > > > maybe that's just over abstracting and imagining supporting multiple
> > > > cameras on this pipeline, which /aren't/ supported.
> > >
> > > Yes, that's a good point.  It won't make a difference now, but if we
> > > were to have multiple cameras, we would have to move this code to
> > > another place.
> >
> > If it's easy to do I'd rather have the code moved already. Otherwise,
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> I could be wrong, but right now, I don't think there is another
> appropriate hook to place this in for multiple camera cases.  Kieran,
> please do correct me if I am wrong.
>
> >
> > On a separate but related topic, when we'll implement support for
> > transformations, the Bayer pattern will be affected. There are different
> > options to deal with that, but before starting that discussion, I'd like
> > to know if it would be feasible on Raspberry Pi platforms to adjust the
> > crop rectangle transparently by one pixel to keep the Bayer pattern
> > stable, or if that's not technically possible or not desired for
> > particular reasons.
>
> The BCM2835 ISP should be able to crop on an odd pixel boundary,
> effectively modifying the bayer order.  However, I have never tried
> this in anger, so I would fully expect it to have some underlying
> issues :-)

For what it's worth, and this is just a matter of personal taste, I'm
not keen on the idea of applying teeny 1-pixel crops just to preserve
the Bayer pattern. It means you can't rely on being able to read out
the full native resolution of the sensor, which seems a bit
unfortunate to me.

Best regards
David

>
> Regards,
> Naush
>
>
> >
> > > > So there's no reason for this to be Camera specific at the moment, but I
> > > > do wonder when we'll see a video-mux hooked up to the RaspberryPi (Too
> > > > much working with GMSL on my mind)
> > > >
> > > > So,
> > > >
> > > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > >
> > > > >       /*
> > > > >        * List the available output streams.
> > > > >        * Currently cannot do Unicam streams!
> > > > > @@ -1165,13 +1172,6 @@ int RPiCameraData::configureIPA()
> > > > >                                             { V4L2_CID_EXPOSURE, result.data[1] } });
> > > > >                       sensorMetadata_ = result.data[2];
> > > > >               }
> > > > > -
> > > > > -             /* Configure the H/V flip controls based on the sensor rotation. */
> > > > > -             ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
> > > > > -             int32_t rotation = sensor_->properties().get(properties::Rotation);
> > > > > -             ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation));
> > > > > -             ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation));
> > > > > -             unicam_[Unicam::Image].dev()->setControls(&ctrls);
> > > > >       }
> > > > >
> > > > >       if (result.operation & RPI_IPA_CONFIG_SENSOR) {
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel