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

[Naushir Patuck]

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



> --
> 2.30.2
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220112/4b3380e3/attachment.htm>