[libcamera-devel] [PATCH] pipeline: raspberrypi: Fix calculation of sensor's native Bayer order

[David Plowman]

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