[libcamera-devel] [PATCH] pipeline: raspberrypi: Fix calculation of sensor's native Bayer order
David Plowman
david.plowman at raspberrypi.com
Wed Jan 12 11:20:18 CET 2022
Hi Naush
Thanks for the reply.
On Wed, 12 Jan 2022 at 10:07, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> Hi David,
>
> Thank you for your work.
>
> On Wed, 12 Jan 2022 at 09:28, David Plowman <david.plowman at raspberrypi.com> wrote:
>>
>> This bug crept in when the pipeline handler was converted to use media
>> controller.
>>
>> Previously the sensor's hflip and vflip were being cleared before
>> querying the sensor for its "native" Bayer order. Now, though, the
>> sensor's available formats are cached before we can clear these bits.
>>
>> Instead, we deduce the transform equivalent to the current hflip and
>> vflip settings, and apply its inverse to the Bayer formats that we now
>> have, thereby giving us the untransformed Bayer order that we want.
>>
>> The practical consequence of this was that the Bayer order stored in
>> DNG files was frequently wrong.
>>
>> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
>> Fixes: 83a512816189 ("pipeline: raspberrypi: Convert the pipeline handler to use media controller")
>> ---
>> .../pipeline/raspberrypi/raspberrypi.cpp | 21 ++++++++++++-------
>> 1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> index 49d7ff23..c1fb9666 100644
>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> @@ -1279,20 +1279,24 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
>> * Thirdly, what is the "native" Bayer order, when no transforms are
>> * applied?
>> *
>> - * As part of answering the final question, we reset the camera to
>> - * no transform at all.
>> + * We note that the format list has already been populated with
>> + * whatever flips are currently set, so to answer the final question
>> + * we get the current Bayer order and undo the transform implied by
>> + * the current flip settings.
>> */
>> const V4L2Subdevice *sensor = data->sensor_->device();
>> const struct v4l2_query_ext_ctrl *hflipCtrl = sensor->controlInfo(V4L2_CID_HFLIP);
>> + Transform currentTransform = Transform::Identity;
>> if (hflipCtrl) {
>> /* We assume it will support vflips too... */
>> data->supportsFlips_ = true;
>> data->flipsAlterBayerOrder_ = hflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>>
>> - ControlList ctrls(data->sensor_->controls());
>> - ctrls.set(V4L2_CID_HFLIP, 0);
>> - ctrls.set(V4L2_CID_VFLIP, 0);
>> - data->setSensorControls(ctrls);
>> + ControlList ctrls = data->sensor_->getControls({ V4L2_CID_HFLIP, V4L2_CID_VFLIP });
>> + if (ctrls.get(V4L2_CID_HFLIP).get<int32_t>())
>> + currentTransform ^= Transform::HFlip;
>> + if (ctrls.get(V4L2_CID_VFLIP).get<int32_t>())
>> + currentTransform ^= Transform::VFlip;
>> }
>>
>> /* Look for a valid Bayer format. */
>> @@ -1307,7 +1311,10 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
>> LOG(RPI, Error) << "No Bayer format found";
>> return -EINVAL;
>> }
>> - data->nativeBayerOrder_ = bayerFormat.order;
>> + /* Applying the inverse transform will give us the native order. */
>> + BayerFormat nativeBayerFormat = bayerFormat.transform(-currentTransform);
>> + data->nativeBayerOrder_ = nativeBayerFormat.order;
>> + LOG(RPI, Debug) << "Native Bayer format is " << nativeBayerFormat.toString();
>>
>> /*
>> * List the available streams an application may request. At present, we
>
>
> I think I understand the logic of the changes and it all looks good. However, I wonder if the
> following (entirely untested) change would also give us the correct Bayer order accounting for
> flips after configure()?
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 5ee713fe66a6..02b31d787b55 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -761,6 +761,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> if (isRaw(cfg.pixelFormat)) {
> cfg.setStream(&data->unicam_[Unicam::Image]);
> data->unicam_[Unicam::Image].setExternal(true);
> + cfg.pixelFormat = unicamFormat.fourcc.toPixelFormat();
> continue;
> }
That's pretty interesting. I wonder, is there any way to do this
earlier, my impression is that we need to set the pixel format
correctly in validate(), rather than wait for configure()?
David
>
>
>>
>> --
>> 2.30.2
>>
More information about the libcamera-devel
mailing list