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