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

[Kieran Bingham]

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?

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.

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

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