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

[David Plowman]

HI Kieran

On Fri, 28 Aug 2020 at 16:26, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Hi David,
>
> On 28/08/2020 16:14, David Plowman wrote:
> > Hi Kieran
> >
> > On Fri, 28 Aug 2020 at 16:02, Kieran Bingham
> > <kieran.bingham at ideasonboard.com> wrote:
> >>
> >> Hi David,
> >>
> >> 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?

Correct. If you have an RGGB sensor and ask it to do h and v flips you
end up with BGGR.

>
> Are we intending to support those transforms on a per-frame basis? or
> just a per-stream basis.

Per-stream. Actually, they're per-camera because that's where the
transformations are being applied. There's no possibility to change
any of this per frame.

>
> 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.
>
> 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 think the problem with this is that we don't know what the camera
will do when we ask for a transform. I think all the drivers we've
ever been involved with will change the Bayer order, but others might
not. They might fiddle with some 1-pixel wide cropping to keep the
Bayer order the same.

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

Yes, I think this is the crux of the matter.

David

>
> --
> Kieran
>
>
>
> >
> > David
> >
> >>
> >>> 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
> >> --
> >> Kieran
>
> --
> Regards
> --
> Kieran