[libcamera-devel] [PATCH v4 2/7] libcamera: pipeline: raspberrypi: Set sensor orientation during validate
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Aug 28 18:17:24 CEST 2020
On Fri, Aug 28, 2020 at 05:13:09PM +0100, Dave Stevenson wrote:
> On Fri, 28 Aug 2020 at 17:03, Laurent Pinchart wrote:
> > On Fri, Aug 28, 2020 at 04:55:14PM +0100, Dave Stevenson wrote:
> >> On Fri, 28 Aug 2020 at 16:36, Laurent Pinchart wrote:
> >>> 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.
> >
> > Good point, that's easier than changing the controls and checking the
> > media bus format.
> >
> >> 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?
> >
> > I think it should, yes.
>
> You make writing sensor drivers such fun! ;-)
> At least with that one you don't have to fight the v4l2_ctrl framework.
Does it show that v4l2_subdev hasn't been designed with a good enough
understanding of camera sensors ? :-) Or, to be fairer, when previously
unsupported features of sensors had to be supported, the API and how it
was used by drivers never went through a coordinated design phase.
> >>>>>>> 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
More information about the libcamera-devel
mailing list