<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 13 Jan 2022 at 10:30, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi again<br>
<br>
On Thu, 13 Jan 2022 at 10:12, David Plowman<br>
<<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>> wrote:<br>
><br>
> Hi Kieran, Naush<br>
><br>
> On Thu, 13 Jan 2022 at 09:48, Kieran Bingham<br>
> <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>> wrote:<br>
> ><br>
> > Quoting Naushir Patuck (2022-01-12 10:24:02)<br>
> > > Hi David,<br>
> > ><br>
> > > On Wed, 12 Jan 2022 at 10:20, David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> > > wrote:<br>
> > ><br>
> > > > Hi Naush<br>
> > > ><br>
> > > > Thanks for the reply.<br>
> > > ><br>
> > > > On Wed, 12 Jan 2022 at 10:07, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > > wrote:<br>
> > > > ><br>
> > > > > Hi David,<br>
> > > > ><br>
> > > > > Thank you for your work.<br>
> > > > ><br>
> > > > > On Wed, 12 Jan 2022 at 09:28, David Plowman <<br>
> > > > <a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>> wrote:<br>
> > > > >><br>
> > > > >> This bug crept in when the pipeline handler was converted to use media<br>
> > > > >> controller.<br>
> > > > >><br>
> > > > >> Previously the sensor's hflip and vflip were being cleared before<br>
> > > > >> querying the sensor for its "native" Bayer order. Now, though, the<br>
> > > > >> sensor's available formats are cached before we can clear these bits.<br>
> > > > >><br>
> > > > >> Instead, we deduce the transform equivalent to the current hflip and<br>
> > > > >> vflip settings, and apply its inverse to the Bayer formats that we now<br>
> > > > >> have, thereby giving us the untransformed Bayer order that we want.<br>
> > > > >><br>
> > > > >> The practical consequence of this was that the Bayer order stored in<br>
> > > > >> DNG files was frequently wrong.<br>
> > > > >><br>
> > > > >> Signed-off-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> > > > >> Fixes: 83a512816189 ("pipeline: raspberrypi: Convert the pipeline<br>
> > > > handler to use media controller")<br>
> > > > >> ---<br>
> > > > >> .../pipeline/raspberrypi/raspberrypi.cpp | 21 ++++++++++++-------<br>
> > > > >> 1 file changed, 14 insertions(+), 7 deletions(-)<br>
> > > > >><br>
> > > > >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > > >> index 49d7ff23..c1fb9666 100644<br>
> > > > >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > > >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > > >> @@ -1279,20 +1279,24 @@ int<br>
> > > > PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me<br>
> > > > >> * Thirdly, what is the "native" Bayer order, when no<br>
> > > > transforms are<br>
> > > > >> * applied?<br>
> > > > >> *<br>
> > > > >> - * As part of answering the final question, we reset the camera<br>
> > > > to<br>
> > > > >> - * no transform at all.<br>
> > > > >> + * We note that the format list has already been populated with<br>
> > > > >> + * whatever flips are currently set, so to answer the final<br>
> > > > question<br>
> > > > >> + * we get the current Bayer order and undo the transform<br>
> > > > implied by<br>
> > > > >> + * the current flip settings.<br>
> > > > >> */<br>
> > > > >> const V4L2Subdevice *sensor = data->sensor_->device();<br>
> > > > >> const struct v4l2_query_ext_ctrl *hflipCtrl =<br>
> > > > sensor->controlInfo(V4L2_CID_HFLIP);<br>
> > > > >> + Transform currentTransform = Transform::Identity;<br>
> > > > >> if (hflipCtrl) {<br>
> > > > >> /* We assume it will support vflips too... */<br>
> > > > >> data->supportsFlips_ = true;<br>
> > > > >> data->flipsAlterBayerOrder_ = hflipCtrl->flags &<br>
> > > > V4L2_CTRL_FLAG_MODIFY_LAYOUT;<br>
> > > > >><br>
> > > > >> - ControlList ctrls(data->sensor_->controls());<br>
> > > > >> - ctrls.set(V4L2_CID_HFLIP, 0);<br>
> > > > >> - ctrls.set(V4L2_CID_VFLIP, 0);<br>
> > > > >> - data->setSensorControls(ctrls);<br>
> > > > >> + ControlList ctrls = data->sensor_->getControls({<br>
> > > > V4L2_CID_HFLIP, V4L2_CID_VFLIP });<br>
> > > > >> + if (ctrls.get(V4L2_CID_HFLIP).get<int32_t>())<br>
> > > > >> + currentTransform ^= Transform::HFlip;<br>
> > > > >> + if (ctrls.get(V4L2_CID_VFLIP).get<int32_t>())<br>
> > > > >> + currentTransform ^= Transform::VFlip;<br>
> ><br>
> > I'm worried that this might leave us in an inconsistent state at<br>
> > startup.<br>
> ><br>
> > I can see we definitely need to account for the flips when mapping the<br>
> > current bayerFormat.order to a native order, and I think that sounds<br>
> > reasonable, so my only concern is if we need to 'reset' the HFLIP/VFLIP<br>
> > controls to known defaults at startup, either after we've handled this<br>
> > native bayer ordering, or ... perhaps we should do it when we construct<br>
> > the CameraSensor class ... before we read the formats?<br>
><br>
> I don't think anything should end up inconsistent - whatever the<br>
> current h/vflip settings it should (and in my testing does) come out<br>
> with the correct native Bayer order every time. But I agree, maybe<br>
> there's a place where we can clear the flips before we cache those<br>
> formats? That would definitely make things easier here.<br>
><br>
> I assume the Bayer order of the cached formats can't matter - as the<br>
> camera may get flipped later anyway and everything carries on working.<br>
<br>
It looks like it fills that list of formats in CameraSensor::init(). I<br>
wonder if that should clear the flip bits first (where they exist)?<br>
Would that be acceptable?<br></blockquote><div><br></div><div>I would be happy with that behaviour.</div><div><br></div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
David<br>
<br>
><br>
> ><br>
> ><br>
> ><br>
> > > > >> }<br>
> > > > >><br>
> > > > >> /* Look for a valid Bayer format. */<br>
> > > > >> @@ -1307,7 +1311,10 @@ int<br>
> > > > PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me<br>
> > > > >> LOG(RPI, Error) << "No Bayer format found";<br>
> > > > >> return -EINVAL;<br>
> > > > >> }<br>
> > > > >> - data->nativeBayerOrder_ = bayerFormat.order;<br>
> > > > >> + /* Applying the inverse transform will give us the native<br>
> > > > order. */<br>
> > > > >> + BayerFormat nativeBayerFormat =<br>
> > > > bayerFormat.transform(-currentTransform);<br>
> > > > >> + data->nativeBayerOrder_ = nativeBayerFormat.order;<br>
> > > > >> + LOG(RPI, Debug) << "Native Bayer format is " <<<br>
> > > > nativeBayerFormat.toString();<br>
> > > > >><br>
> > > > >> /*<br>
> > > > >> * List the available streams an application may request. At<br>
> > > > present, we<br>
> > > > ><br>
> > > > ><br>
> > > > > I think I understand the logic of the changes and it all looks good.<br>
> > > > However, I wonder if the<br>
> > > > > following (entirely untested) change would also give us the correct<br>
> > > > Bayer order accounting for<br>
> > > > > flips after configure()?<br>
> > > > ><br>
> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > > > index 5ee713fe66a6..02b31d787b55 100644<br>
> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > > > @@ -761,6 +761,7 @@ int PipelineHandlerRPi::configure(Camera *camera,<br>
> > > > CameraConfiguration *config)<br>
> > > > > if (isRaw(cfg.pixelFormat)) {<br>
> > > > > cfg.setStream(&data->unicam_[Unicam::Image]);<br>
> > > > > data->unicam_[Unicam::Image].setExternal(true);<br>
> > > > > + cfg.pixelFormat =<br>
> > > > unicamFormat.fourcc.toPixelFormat();<br>
> > > > > continue;<br>
> > > > > }<br>
> > > ><br>
> > > > That's pretty interesting. I wonder, is there any way to do this<br>
> > > > earlier, my impression is that we need to set the pixel format<br>
> > > > correctly in validate(), rather than wait for configure()?<br>
> > > ><br>
> > ><br>
> > > I don't think that would be possible. We only get to know the user<br>
> > > requested flips in the configure(), so validate() would only ever return<br>
> > > the un-flipped (user point of view) ordering.<br>
> > ><br>
> ><br>
> > Also, if I understand this correctly it's trying to determine the 'constant'<br>
> > native bayer order of the sensor. If it is consistent, and doesn't<br>
> > change with any configuration, then I would expect this to be done when<br>
> > the camera is created/registered as this patch does.<br>
> ><br>
> > Validate should be able to identify everything that would be affected by<br>
> > the configuration when configure is called with that configuration.<br>
><br>
> Yes. If we can clear the flips and do everything in registerCamera<br>
> that would seem ideal...<br>
><br>
> David<br>
><br>
> ><br>
> > --<br>
> > Kieran<br>
> ><br>
> ><br>
> > > Naush<br>
> > ><br>
> > ><br>
> > > ><br>
> > > > David<br>
> > > ><br>
> > > > ><br>
> > > > ><br>
> > > > >><br>
> > > > >> --<br>
> > > > >> 2.30.2<br>
> > > > >><br>
> > > ><br>
</blockquote></div></div>