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

[Laurent Pinchart]

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>

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.

> > 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