[libcamera-devel] [PATCH] pipeline: raspberrypi: Fix calculation of sensor's native Bayer order
David Plowman
david.plowman at raspberrypi.com
Thu Jan 13 11:12:37 CET 2022
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.
>
>
>
> > > >> }
> > > >>
> > > >> /* 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
> > > >>
> > >
More information about the libcamera-devel
mailing list