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

[Kieran Bingham]

Quoting Naushir Patuck (2022-01-12 10:24:02)
> 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;

I'm worried that this might leave us in an inconsistent state at
startup.

I can see we definitely need to account for the flips when mapping the
current bayerFormat.order to a native order, and I think that sounds
reasonable, so my only concern is if we need to 'reset' the HFLIP/VFLIP
controls to known defaults at startup, either after we've handled this
native bayer ordering, or ... perhaps we should do it when we construct
the CameraSensor class ... before we read the formats?



> > >>         }
> > >>
> > >>         /* 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.
> 

Also, if I understand this correctly it's trying to determine the 'constant'
native bayer order of the sensor. If it is consistent, and doesn't
change with any configuration, then I would expect this to be done when
the camera is created/registered as this patch does.

Validate should be able to identify everything that would be affected by
the configuration when configure is called with that configuration.

--
Kieran


> Naush
> 
> 
> >
> > David
> >
> > >
> > >
> > >>
> > >> --
> > >> 2.30.2
> > >>
> >