[libcamera-devel] [PATCH] pipeline: raspberrypi: Fix calculation of sensor's native Bayer order
Naushir Patuck
naush at raspberrypi.com
Thu Jan 13 11:32:26 CET 2022
On Thu, 13 Jan 2022 at 10:30, David Plowman <david.plowman at raspberrypi.com>
wrote:
> Hi again
>
> On Thu, 13 Jan 2022 at 10:12, David Plowman
> <david.plowman at raspberrypi.com> wrote:
> >
> > Hi Kieran, Naush
> >
> > On Thu, 13 Jan 2022 at 09:48, Kieran Bingham
> > <kieran.bingham at ideasonboard.com> wrote:
> > >
> > > 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?
> >
> > I don't think anything should end up inconsistent - whatever the
> > current h/vflip settings it should (and in my testing does) come out
> > with the correct native Bayer order every time. But I agree, maybe
> > there's a place where we can clear the flips before we cache those
> > formats? That would definitely make things easier here.
> >
> > I assume the Bayer order of the cached formats can't matter - as the
> > camera may get flipped later anyway and everything carries on working.
>
> It looks like it fills that list of formats in CameraSensor::init(). I
> wonder if that should clear the flip bits first (where they exist)?
> Would that be acceptable?
>
I would be happy with that behaviour.
Naush
>
> David
>
> >
> > >
> > >
> > >
> > > > > >> }
> > > > > >>
> > > > > >> /* 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.
> >
> > Yes. If we can clear the flips and do everything in registerCamera
> > that would seem ideal...
> >
> > David
> >
> > >
> > > --
> > > Kieran
> > >
> > >
> > > > Naush
> > > >
> > > >
> > > > >
> > > > > David
> > > > >
> > > > > >
> > > > > >
> > > > > >>
> > > > > >> --
> > > > > >> 2.30.2
> > > > > >>
> > > > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220113/a7ce85e6/attachment-0001.htm>
More information about the libcamera-devel
mailing list