[libcamera-devel] [PATCH v4 2/7] libcamera: pipeline: raspberrypi: Set sensor orientation during validate
Dave Stevenson
dave.stevenson at raspberrypi.com
Fri Aug 28 17:55:14 CEST 2020
Hi Guys
On Fri, 28 Aug 2020 at 16:36, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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.
You have a very good indicator of that from the
V4L2_CTRL_FLAG_MODIFY_LAYOUT flag when querying controls. It would be
fair to deduce from that flag on flips that it is going to change the
bayer order. I've yet to see a Bayer sensor that offers transpose, but
there may be some out there.
IMX327 and IMX290 are two that don't alter the Bayer order when using
the on-sensor flip controls. Reading the datasheet for IMX327 (page
58), it explicitly states for the full frame mode the active lines are
"Line No during normal operation" 21 to 1100
"Line No during inverted operation" 1101 to 22
and
"Horizontal pixel output image normal operation" 13 to1932
"Horizontal pixel output image inverted operation" 1933 to 14.
I'm also aware of one further sensor (can't say what) that I know for
definite doesn't change the Bayer order. Exactly how it achieves that
one can only assume is by shifting the readout by one pixel.
Should the crop reported by G_SELECTION be shifted in response to the
flips then?
Dave
> > >>> 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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list