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

[Naushir Patuck]

Hi David,

On Wed, 12 Jan 2022 at 10:20, David Plowman <david.plowman at raspberrypi.com>
wrote:

> 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()?
>

I don't think that would be possible.  We only get to know the user
requested flips in the configure(), so validate() would only ever return
the un-flipped (user point of view) ordering.

Naush


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