[libcamera-devel] [PATCH v4 2/7] libcamera: pipeline: raspberrypi: Set sensor orientation during validate

[Laurent Pinchart]

Hello,

On Fri, Aug 28, 2020 at 04:26:05PM +0100, Kieran Bingham wrote:
> On 28/08/2020 16:14, David Plowman wrote:
> > On Fri, 28 Aug 2020 at 16:02, Kieran Bingham wrote:
> >> On 28/08/2020 15:41, David Plowman wrote:
> >>> We set the sensor orientation (h and v flips) during validate as this
> >>> will in general affect the Bayer order output by the sensor. Doing it
> >>> here means that the correct raw format gets advertised in any raw
> >>> streams that the application requested.
> >>
> >> Eeep - I'm not sure if we could do this in validate().
> >>
> >> Validation should not actually make any change to the hardware, but it
> >> should check that the configuration can be applied correctly, and make
> >> any changes that would be necessary to support a correct (and 'valid')
> >> configuration to be applied through the ->configure()
> > 
> > Yes, that's an interesting one. The reason for doing it here is so
> > that the Bayer format comes out correctly for any raw streams that
> > were requested, and we're relying on the camera driver to give us the
> > true Bayer order.
> > 
> > Of course, the camera driver doesn't *have* to change the Bayer order
> > when you transform it (it might do 1-pixel shits to maintain the
> 
> haha, I hadn't noticed the typo until your follow up post ;-)
> 
> > original Bayer order), so the puzzle then is...  how would you know?
> > 
> > Another solution I toyed with - and indeed implemented first - was to
> > do it in the configure() method, but then I had to dig around and find
> > any raw stream configurations and update the pixelFormat post facto.
> > This involves changing stream formats after validate(), which seemed
> > bad to me too... but do we prefer it?
> 
> Ayeee - no, we can't change a format after validate either.
> Oh dear have we deadlocked...
> 
> So - is the issue that the rotation/transform affects the pixel format?
> 
> Are we intending to support those transforms on a per-frame basis? or
> just a per-stream basis.
> 
> As long as it's per-stream - Can we just store the state of the expected
> rotation in the RPiCameraConfiguration, so that we can use it at
> configure time appropriately? Or is it that we will need to determine
> what adjustment will be made to the pixelformat based on the transform.

The latter. We need to report to the user which raw format will be used
based on the transformation.

> And if that's the case, I assume it's the kernel driver which is going
> to tell us what the new format will be. I fear we might have to
> duplicate the determination of the format up here, and report the
> adjustment.
> 
> I guess what we're lacking is the ability to do a full atomic 'try' of
> the state of the device at the driver level ...

Correct, and I doubt V4L2 will ever give us that.

I think we need to adjust the format in the pipeline handler, instead of
querying the device. We however need to know if h/v flip will affect the
bayer pattern, or if the sensor will compensate by shifting the crop
rectangle.

One option is to query the sensor driver at match time, to see if the
media bus format changes when the h/v flip controls are modified. A bit
of a hassle, but shouldn't be too complicated. Another option would be
to ignore all this and consider that a proper sensor driver will never
shift the crop rectangle behind the scene. We have the option to
establish a set of rules for a sensor driver to be supported by
libcamera.

> >>> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> >>> ---
> >>>  .../pipeline/raspberrypi/raspberrypi.cpp       | 18 +++++++++++-------
> >>>  1 file changed, 11 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> index 42c9caa..7aace71 100644
> >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> @@ -400,6 +400,17 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> >>>       if (config_.empty())
> >>>               return Invalid;
> >>>
> >>> +     /*
> >>> +      * Configure the H/V flip controls based on the sensor rotation. We do
> >>> +      * this here so that the sensor has the correct Bayer format that will
> >>> +      * get advertised in the configuration of any raw streams.
> >>> +      */
> >>> +     ControlList ctrls(data_->unicam_[Unicam::Image].dev()->controls());
> >>> +     int32_t rotation = data_->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));
> >>> +     data_->unicam_[Unicam::Image].dev()->setControls(&ctrls);
> >>> +
> >>>       unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0;
> >>>       std::pair<int, Size> outSize[2];
> >>>       Size maxSize;
> >>> @@ -1164,13 +1175,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