<div dir="ltr"><div dir="ltr">Hi David,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 12 Jan 2022 at 10:20, 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 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>> wrote:<br>
><br>
> Hi David,<br>
><br>
> Thank you for your work.<br>
><br>
> On Wed, 12 Jan 2022 at 09:28, David Plowman <<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 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 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 PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me<br>
>> * Thirdly, what is the "native" Bayer order, when no transforms are<br>
>> * applied?<br>
>> *<br>
>> - * As part of answering the final question, we reset the camera 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 question<br>
>> + * we get the current Bayer order and undo the transform implied by<br>
>> + * the current flip settings.<br>
>> */<br>
>> const V4L2Subdevice *sensor = data->sensor_->device();<br>
>> const struct v4l2_query_ext_ctrl *hflipCtrl = 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 & 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({ 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>
>><br>
>> /* Look for a valid Bayer format. */<br>
>> @@ -1307,7 +1311,10 @@ int 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 order. */<br>
>> + BayerFormat nativeBayerFormat = bayerFormat.transform(-currentTransform);<br>
>> + data->nativeBayerOrder_ = nativeBayerFormat.order;<br>
>> + LOG(RPI, Debug) << "Native Bayer format is " << nativeBayerFormat.toString();<br>
>><br>
>> /*<br>
>> * List the available streams an application may request. At present, we<br>
><br>
><br>
> I think I understand the logic of the changes and it all looks good. However, I wonder if the<br>
> following (entirely untested) change would also give us the correct Bayer order accounting for<br>
> flips after configure()?<br>
><br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp 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, CameraConfiguration *config)<br>
> if (isRaw(cfg.pixelFormat)) {<br>
> cfg.setStream(&data->unicam_[Unicam::Image]);<br>
> data->unicam_[Unicam::Image].setExternal(true);<br>
> + cfg.pixelFormat = 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></blockquote><div><br></div><div>I don't think that would be possible. We only get to know the user</div><div>requested flips in the configure(), so validate() would only ever return</div><div>the un-flipped (user point of view) ordering.</div><div><br></div><div>Naush</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>
>> 2.30.2<br>
>><br>
</blockquote></div></div>