[{"id":22018,"web_url":"https://patchwork.libcamera.org/comment/22018/","msgid":"","date":"2022-01-12T10:06:50","subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Fix\n\tcalculation of sensor's native Bayer order","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nThank you for your work.\n\nOn Wed, 12 Jan 2022 at 09:28, David Plowman \nwrote:\n\n> This bug crept in when the pipeline handler was converted to use media\n> controller.\n>\n> Previously the sensor's hflip and vflip were being cleared before\n> querying the sensor for its \"native\" Bayer order. Now, though, the\n> sensor's available formats are cached before we can clear these bits.\n>\n> Instead, we deduce the transform equivalent to the current hflip and\n> vflip settings, and apply its inverse to the Bayer formats that we now\n> have, thereby giving us the untransformed Bayer order that we want.\n>\n> The practical consequence of this was that the Bayer order stored in\n> DNG files was frequently wrong.\n>\n> Signed-off-by: David Plowman \n> Fixes: 83a512816189 (\"pipeline: raspberrypi: Convert the pipeline handler\n> to use media controller\")\n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 21 ++++++++++++-------\n> 1 file changed, 14 insertions(+), 7 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 49d7ff23..c1fb9666 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1279,20 +1279,24 @@ int PipelineHandlerRPi::registerCamera(MediaDevice\n> *unicam, MediaDevice *isp, Me\n> * Thirdly, what is the \"native\" Bayer order, when no transforms\n> are\n> * applied?\n> *\n> - * As part of answering the final question, we reset the camera to\n> - * no transform at all.\n> + * We note that the format list has already been populated with\n> + * whatever flips are currently set, so to answer the final\n> question\n> + * we get the current Bayer order and undo the transform implied by\n> + * the current flip settings.\n> */\n> const V4L2Subdevice *sensor = data->sensor_->device();\n> const struct v4l2_query_ext_ctrl *hflipCtrl =\n> sensor->controlInfo(V4L2_CID_HFLIP);\n> + Transform currentTransform = Transform::Identity;\n> if (hflipCtrl) {\n> /* We assume it will support vflips too... */\n> data->supportsFlips_ = true;\n> data->flipsAlterBayerOrder_ = hflipCtrl->flags &\n> V4L2_CTRL_FLAG_MODIFY_LAYOUT;\n>\n> - ControlList ctrls(data->sensor_->controls());\n> - ctrls.set(V4L2_CID_HFLIP, 0);\n> - ctrls.set(V4L2_CID_VFLIP, 0);\n> - data->setSensorControls(ctrls);\n> + ControlList ctrls = data->sensor_->getControls({\n> V4L2_CID_HFLIP, V4L2_CID_VFLIP });\n> + if (ctrls.get(V4L2_CID_HFLIP).get())\n> + currentTransform ^= Transform::HFlip;\n> + if (ctrls.get(V4L2_CID_VFLIP).get())\n> + currentTransform ^= Transform::VFlip;\n> }\n>\n> /* Look for a valid Bayer format. */\n> @@ -1307,7 +1311,10 @@ int PipelineHandlerRPi::registerCamera(MediaDevice\n> *unicam, MediaDevice *isp, Me\n> LOG(RPI, Error) << \"No Bayer format found\";\n> return -EINVAL;\n> }\n> - data->nativeBayerOrder_ = bayerFormat.order;\n> + /* Applying the inverse transform will give us the native order. */\n> + BayerFormat nativeBayerFormat =\n> bayerFormat.transform(-currentTransform);\n> + data->nativeBayerOrder_ = nativeBayerFormat.order;\n> + LOG(RPI, Debug) << \"Native Bayer format is \" <<\n> nativeBayerFormat.toString();\n>\n> /*\n> * List the available streams an application may request. At\n> present, we\n>\n\nI think I understand the logic of the changes and it all looks good.\nHowever, I wonder if the\nfollowing (entirely untested) change would also give us the correct Bayer\norder accounting for\nflips after configure()?\n\ndiff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\nb/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\nindex 5ee713fe66a6..02b31d787b55 100644\n--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n@@ -761,6 +761,7 @@ int PipelineHandlerRPi::configure(Camera *camera,\nCameraConfiguration *config)\n if (isRaw(cfg.pixelFormat)) {\n cfg.setStream(&data->unicam_[Unicam::Image]);\n data->unicam_[Unicam::Image].setExternal(true);\n+ cfg.pixelFormat =\nunicamFormat.fourcc.toPixelFormat();\n continue;\n }\n\n\n\n> --\n> 2.30.2\n>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 66F06BF415\n\tfor ;\n\tWed, 12 Jan 2022 10:07:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ABCD960921;\n\tWed, 12 Jan 2022 11:07:08 +0100 (CET)","from mail-lf1-x12e.google.com (mail-lf1-x12e.google.com\n\t[IPv6:2a00:1450:4864:20::12e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 78C386017F\n\tfor ;\n\tWed, 12 Jan 2022 11:07:07 +0100 (CET)","by mail-lf1-x12e.google.com with SMTP id o15so6167929lfo.11\n\tfor ;\n\tWed, 12 Jan 2022 02:07:07 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"DiOjBEHf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=fp4Zcsq2MAd18Y8oytsg8d35FSZ0SH9mB+xHg9Y5nio=;\n\tb=DiOjBEHfZddvjZ4VfvHOJwEujdP3Ky6zesfCysJzCWqpfcCBn5Aj3bsbcnkIuSe8W5\n\tghRNBfpRFrqnPeloPQo+bvkOwzra4ZnQEY22IQqoZqPExOsjDpxe4UGRGNz25fesPD0g\n\tx/cIa5eQk44lxFONBz/Z5zUrq5zotZzLhJ85COZiCH66O30cqvKEDJi6GpfZPHLfHLs3\n\tRxrzp1SkssvTqGAg6sBJEGldMhcNHloUGEhx01Rh6YBx+ZHzSqIsrjSnc5+Q4Kl9U1Es\n\tm4JSMRcE9QdtBkbVXuAe0HBZvxOCTVx6iwPEcoAUmaP5yYRWqBjqfaOpGTqZlcXaTokJ\n\td2tA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=fp4Zcsq2MAd18Y8oytsg8d35FSZ0SH9mB+xHg9Y5nio=;\n\tb=3C+ejkhfBKscRGMaLHzuklA6VOIBYrOgBDvmmfxYvK0W7O+l+B8YKyGpDRasvzyRcT\n\tYEWf8i1d/i3S0SzjQ2e/PyAZmt23aetf53meOrykzLZz8Brgg1CHucvx2idzQeoNwX1i\n\tRfZBN4Rc656LPK5deEuxPsGeoOMVuPQkS4mATomFMNwOIni5ZX42lOyk2wOpIqBGUJJk\n\tqF37Wv/saC0SZba/dSj09sinm9dbdIblu0VdiZiAUSChJyXaovx+e8au/Doi6UOGM/qE\n\tgOlOcOubfkNjJmtoFrQ2mnLAHeHUh9ymHIcERSvrnj+P7GFbwD5uFEVrDf82ihGoqcaW\n\tik2w==","X-Gm-Message-State":"AOAM531MxuItd2OcQXNd1eBtmi+FLRDFJgmVYO4n+dAT2h7mNgTT/13a\n\tt9k/msJ8fgDo/HHCLOaGISZTAWFcm7H69SCrUD8jOA==","X-Google-Smtp-Source":"ABdhPJxoi5kh8RI0KF+YtbOYOh06r+sZMyT8sghmUvDAHxlEg7CaaxTCNLLo38viphcdAYyHSuhDHAUp+20MJalpzWQ=","X-Received":"by 2002:a2e:7007:: with SMTP id l7mr5878198ljc.280.1641982026640;\n\tWed, 12 Jan 2022 02:07:06 -0800 (PST)","MIME-Version":"1.0","References":"<20220112092758.4726-1-david.plowman@raspberrypi.com>","In-Reply-To":"<20220112092758.4726-1-david.plowman@raspberrypi.com>","From":"Naushir Patuck ","Date":"Wed, 12 Jan 2022 10:06:50 +0000","Message-ID":"","To":"David Plowman ","Content-Type":"multipart/alternative; boundary=\"0000000000006d079505d55fbb96\"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Fix\n\tcalculation of sensor's native Bayer order","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22019,"web_url":"https://patchwork.libcamera.org/comment/22019/","msgid":"","date":"2022-01-12T10:20:18","subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Fix\n\tcalculation of sensor's native Bayer order","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for the reply.\n\nOn Wed, 12 Jan 2022 at 10:07, Naushir Patuck wrote:\n>\n> Hi David,\n>\n> Thank you for your work.\n>\n> On Wed, 12 Jan 2022 at 09:28, David Plowman wrote:\n>>\n>> This bug crept in when the pipeline handler was converted to use media\n>> controller.\n>>\n>> Previously the sensor's hflip and vflip were being cleared before\n>> querying the sensor for its \"native\" Bayer order. Now, though, the\n>> sensor's available formats are cached before we can clear these bits.\n>>\n>> Instead, we deduce the transform equivalent to the current hflip and\n>> vflip settings, and apply its inverse to the Bayer formats that we now\n>> have, thereby giving us the untransformed Bayer order that we want.\n>>\n>> The practical consequence of this was that the Bayer order stored in\n>> DNG files was frequently wrong.\n>>\n>> Signed-off-by: David Plowman \n>> Fixes: 83a512816189 (\"pipeline: raspberrypi: Convert the pipeline handler to use media controller\")\n>> ---\n>> .../pipeline/raspberrypi/raspberrypi.cpp | 21 ++++++++++++-------\n>> 1 file changed, 14 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> index 49d7ff23..c1fb9666 100644\n>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> @@ -1279,20 +1279,24 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n>> * Thirdly, what is the \"native\" Bayer order, when no transforms are\n>> * applied?\n>> *\n>> - * As part of answering the final question, we reset the camera to\n>> - * no transform at all.\n>> + * We note that the format list has already been populated with\n>> + * whatever flips are currently set, so to answer the final question\n>> + * we get the current Bayer order and undo the transform implied by\n>> + * the current flip settings.\n>> */\n>> const V4L2Subdevice *sensor = data->sensor_->device();\n>> const struct v4l2_query_ext_ctrl *hflipCtrl = sensor->controlInfo(V4L2_CID_HFLIP);\n>> + Transform currentTransform = Transform::Identity;\n>> if (hflipCtrl) {\n>> /* We assume it will support vflips too... */\n>> data->supportsFlips_ = true;\n>> data->flipsAlterBayerOrder_ = hflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT;\n>>\n>> - ControlList ctrls(data->sensor_->controls());\n>> - ctrls.set(V4L2_CID_HFLIP, 0);\n>> - ctrls.set(V4L2_CID_VFLIP, 0);\n>> - data->setSensorControls(ctrls);\n>> + ControlList ctrls = data->sensor_->getControls({ V4L2_CID_HFLIP, V4L2_CID_VFLIP });\n>> + if (ctrls.get(V4L2_CID_HFLIP).get())\n>> + currentTransform ^= Transform::HFlip;\n>> + if (ctrls.get(V4L2_CID_VFLIP).get())\n>> + currentTransform ^= Transform::VFlip;\n>> }\n>>\n>> /* Look for a valid Bayer format. */\n>> @@ -1307,7 +1311,10 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n>> LOG(RPI, Error) << \"No Bayer format found\";\n>> return -EINVAL;\n>> }\n>> - data->nativeBayerOrder_ = bayerFormat.order;\n>> + /* Applying the inverse transform will give us the native order. */\n>> + BayerFormat nativeBayerFormat = bayerFormat.transform(-currentTransform);\n>> + data->nativeBayerOrder_ = nativeBayerFormat.order;\n>> + LOG(RPI, Debug) << \"Native Bayer format is \" << nativeBayerFormat.toString();\n>>\n>> /*\n>> * List the available streams an application may request. At present, we\n>\n>\n> I think I understand the logic of the changes and it all looks good. However, I wonder if the\n> following (entirely untested) change would also give us the correct Bayer order accounting for\n> flips after configure()?\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 5ee713fe66a6..02b31d787b55 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -761,6 +761,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> if (isRaw(cfg.pixelFormat)) {\n> cfg.setStream(&data->unicam_[Unicam::Image]);\n> data->unicam_[Unicam::Image].setExternal(true);\n> + cfg.pixelFormat = unicamFormat.fourcc.toPixelFormat();\n> continue;\n> }\n\nThat's pretty interesting. I wonder, is there any way to do this\nearlier, my impression is that we need to set the pixel format\ncorrectly in validate(), rather than wait for configure()?\n\nDavid\n\n>\n>\n>>\n>> --\n>> 2.30.2\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E3D78BE080\n\tfor ;\n\tWed, 12 Jan 2022 10:20:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 32491604F4;\n\tWed, 12 Jan 2022 11:20:31 +0100 (CET)","from mail-wr1-x435.google.com (mail-wr1-x435.google.com\n\t[IPv6:2a00:1450:4864:20::435])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3A7AE6017F\n\tfor ;\n\tWed, 12 Jan 2022 11:20:29 +0100 (CET)","by mail-wr1-x435.google.com with SMTP id t28so3299389wrb.4\n\tfor ;\n\tWed, 12 Jan 2022 02:20:29 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"hRG8X6rB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=pmYXqgwAa61M5dfJUSavkHWsVm9OR7y2RUBVkMAxi6g=;\n\tb=hRG8X6rB3lA8TcFQg/JnP3a0kV2dGvudjzNFh/hhydibZoiTEvBzFap7ZpDKXewvmt\n\tHW4Rrn8j0vHSYTrv2QkNBYyULonHkJSqFysvQPgHGyMr+SlHmbsvk1IxjCAQ4xldDCTf\n\tcwYvBsyAA2J/uonW/KIAxC1Ndv5jrd8WtdZfaOIrwwID4ac0FORu0lpXQ5zr0dKCGr/u\n\tl7n00elD1lb6XMrlssD/kDTGS9brIAreg4rEiHDNayEilTng+Y8PIKkOdvxItXrPRB2I\n\tgkyb22zKhy9ipEOiOQfHGCGTkoLKF+Qn3+4HSBAI2Db5VC5WJgsCycYGyqBH2V0GBCqv\n\tiSag==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=pmYXqgwAa61M5dfJUSavkHWsVm9OR7y2RUBVkMAxi6g=;\n\tb=K8nA0RRTYvKNgZ4i5/MdD9SYbBdBMY7SErPsT1mqGdifMsI7HDemB9CIYgTvV4xmQd\n\tjO2HeGs375SkYgGr/sRLIppN8D3wdRkSKe0TvqqmU/G/se3kqnl9hN8jPT59MYNuRdoR\n\tkXv9R2azTuXFfDW3cLhMtsoxaiUeizo16cfOq0Ml8egJOAseldOzWZp7zVGh4V1gEIjb\n\tdgkkf6dWi9RER2hWcBtCMAIJIW0v6mDnkT0lz88PF6zz95t2T6c0DI5P2m6OeYZSyD/4\n\t+QVgBZXfb9Qzk9AwE7pQdOA4hbto1LI7ujabg7qbFNsZF4Cu/lkoTXCq1Nru6vJ1UYrD\n\trJsA==","X-Gm-Message-State":"AOAM531DS0fUpoCT9tzxH1dqkgDY6M6rn0BGmPHWuo0e5W9GW2NhPmEJ\n\tXup7v1l0LkiXDPhly5wIvMQjqGMtuCrqeBGSnkHA6JnklkNfJw==","X-Google-Smtp-Source":"ABdhPJyNZXy/n5JIDvu3t7tuAaH8AsQY5rqLevjGFPaGllCo19ourY/BC/aFSw8nswyq2SBSLj8crt6wAa2nD7PI/ow=","X-Received":"by 2002:adf:ea0d:: with SMTP id\n\tq13mr7121121wrm.597.1641982828776; \n\tWed, 12 Jan 2022 02:20:28 -0800 (PST)","MIME-Version":"1.0","References":"<20220112092758.4726-1-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Wed, 12 Jan 2022 10:20:18 +0000","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Fix\n\tcalculation of sensor's native Bayer order","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22020,"web_url":"https://patchwork.libcamera.org/comment/22020/","msgid":"","date":"2022-01-12T10:24:02","subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Fix\n\tcalculation of sensor's native Bayer order","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nOn Wed, 12 Jan 2022 at 10:20, David Plowman \nwrote:\n\n> Hi Naush\n>\n> Thanks for the reply.\n>\n> On Wed, 12 Jan 2022 at 10:07, Naushir Patuck \n> wrote:\n> >\n> > Hi David,\n> >\n> > Thank you for your work.\n> >\n> > On Wed, 12 Jan 2022 at 09:28, David Plowman <\n> david.plowman@raspberrypi.com> wrote:\n> >>\n> >> This bug crept in when the pipeline handler was converted to use media\n> >> controller.\n> >>\n> >> Previously the sensor's hflip and vflip were being cleared before\n> >> querying the sensor for its \"native\" Bayer order. Now, though, the\n> >> sensor's available formats are cached before we can clear these bits.\n> >>\n> >> Instead, we deduce the transform equivalent to the current hflip and\n> >> vflip settings, and apply its inverse to the Bayer formats that we now\n> >> have, thereby giving us the untransformed Bayer order that we want.\n> >>\n> >> The practical consequence of this was that the Bayer order stored in\n> >> DNG files was frequently wrong.\n> >>\n> >> Signed-off-by: David Plowman \n> >> Fixes: 83a512816189 (\"pipeline: raspberrypi: Convert the pipeline\n> handler to use media controller\")\n> >> ---\n> >> .../pipeline/raspberrypi/raspberrypi.cpp | 21 ++++++++++++-------\n> >> 1 file changed, 14 insertions(+), 7 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> index 49d7ff23..c1fb9666 100644\n> >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> @@ -1279,20 +1279,24 @@ int\n> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> >> * Thirdly, what is the \"native\" Bayer order, when no\n> transforms are\n> >> * applied?\n> >> *\n> >> - * As part of answering the final question, we reset the camera\n> to\n> >> - * no transform at all.\n> >> + * We note that the format list has already been populated with\n> >> + * whatever flips are currently set, so to answer the final\n> question\n> >> + * we get the current Bayer order and undo the transform\n> implied by\n> >> + * the current flip settings.\n> >> */\n> >> const V4L2Subdevice *sensor = data->sensor_->device();\n> >> const struct v4l2_query_ext_ctrl *hflipCtrl =\n> sensor->controlInfo(V4L2_CID_HFLIP);\n> >> + Transform currentTransform = Transform::Identity;\n> >> if (hflipCtrl) {\n> >> /* We assume it will support vflips too... */\n> >> data->supportsFlips_ = true;\n> >> data->flipsAlterBayerOrder_ = hflipCtrl->flags &\n> V4L2_CTRL_FLAG_MODIFY_LAYOUT;\n> >>\n> >> - ControlList ctrls(data->sensor_->controls());\n> >> - ctrls.set(V4L2_CID_HFLIP, 0);\n> >> - ctrls.set(V4L2_CID_VFLIP, 0);\n> >> - data->setSensorControls(ctrls);\n> >> + ControlList ctrls = data->sensor_->getControls({\n> V4L2_CID_HFLIP, V4L2_CID_VFLIP });\n> >> + if (ctrls.get(V4L2_CID_HFLIP).get())\n> >> + currentTransform ^= Transform::HFlip;\n> >> + if (ctrls.get(V4L2_CID_VFLIP).get())\n> >> + currentTransform ^= Transform::VFlip;\n> >> }\n> >>\n> >> /* Look for a valid Bayer format. */\n> >> @@ -1307,7 +1311,10 @@ int\n> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> >> LOG(RPI, Error) << \"No Bayer format found\";\n> >> return -EINVAL;\n> >> }\n> >> - data->nativeBayerOrder_ = bayerFormat.order;\n> >> + /* Applying the inverse transform will give us the native\n> order. */\n> >> + BayerFormat nativeBayerFormat =\n> bayerFormat.transform(-currentTransform);\n> >> + data->nativeBayerOrder_ = nativeBayerFormat.order;\n> >> + LOG(RPI, Debug) << \"Native Bayer format is \" <<\n> nativeBayerFormat.toString();\n> >>\n> >> /*\n> >> * List the available streams an application may request. At\n> present, we\n> >\n> >\n> > I think I understand the logic of the changes and it all looks good.\n> However, I wonder if the\n> > following (entirely untested) change would also give us the correct\n> Bayer order accounting for\n> > flips after configure()?\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 5ee713fe66a6..02b31d787b55 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -761,6 +761,7 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n> > if (isRaw(cfg.pixelFormat)) {\n> > cfg.setStream(&data->unicam_[Unicam::Image]);\n> > data->unicam_[Unicam::Image].setExternal(true);\n> > + cfg.pixelFormat =\n> unicamFormat.fourcc.toPixelFormat();\n> > continue;\n> > }\n>\n> That's pretty interesting. I wonder, is there any way to do this\n> earlier, my impression is that we need to set the pixel format\n> correctly in validate(), rather than wait for configure()?\n>\n\nI don't think that would be possible. We only get to know the user\nrequested flips in the configure(), so validate() would only ever return\nthe un-flipped (user point of view) ordering.\n\nNaush\n\n\n>\n> David\n>\n> >\n> >\n> >>\n> >> --\n> >> 2.30.2\n> >>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 14D5BBF415\n\tfor ;\n\tWed, 12 Jan 2022 10:24:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 52C2260921;\n\tWed, 12 Jan 2022 11:24:21 +0100 (CET)","from mail-lf1-x136.google.com (mail-lf1-x136.google.com\n\t[IPv6:2a00:1450:4864:20::136])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6EBC96017F\n\tfor ;\n\tWed, 12 Jan 2022 11:24:19 +0100 (CET)","by mail-lf1-x136.google.com with SMTP id s30so6389726lfo.7\n\tfor ;\n\tWed, 12 Jan 2022 02:24:19 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"ofDpCLMV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=986miRPxBMx/DL3Gt4LvJNvEnKfoNJlFJzyOMlEggEQ=;\n\tb=ofDpCLMVn8LLqgaf5XQwH506gU6M8tI6iPku3lcIjfPnrMv0/RjTOvEF6/7DI/nVXl\n\tRET/V03k75YUPsoQD/KTceCg14fFTguzA7ZxV8ZOYc5vPjFp+bJxjc15jpbpSCqXPTzq\n\tgaOwb6K6TtVGLmgWp3HybdU4Ul05HLU0CmpAKhwcq4mUFvJW84MTXBkzMdXwhqEQAMOE\n\t+60HcwME6ihcraNkOrT83TJtXjnbX+wuqEJgGf+qUNOnpm0hsIj4yzW3JkCcSbkrBxKO\n\tHvgM75/4hd45y6XX2LizepoIkXiJfD1LNdpMG1aLcKYj4PtCGVLyrc1zvFn7NaHvWmKo\n\tuYcA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=986miRPxBMx/DL3Gt4LvJNvEnKfoNJlFJzyOMlEggEQ=;\n\tb=70LmS7x3ZOcoYzpVd/hozcEUxk2r4Qio47ND7Dyqk52tgA6xtUOgxdvNqchdB5/WeB\n\tcKw7lewJqUbJX188U4bcGnyKz5GLdfF5nMV9BTJJ7hs6pti7iMqo9HbIApkSja1RvbKb\n\tvnYpWElHVP7i6re8KGm2kLU4WRfe1ZD0hrguOd+D0y9EUdIaf6PkZYplfIVeGSytmZrw\n\tKgzG2+eOn92ocThNSiRZDAYvyFgpiUstXdqonF/SGjH6z0YnX8p1/GqC/od8JqqwWd1I\n\t1Cz9tVwl0TeOYrHMdxMQg24KhbEn+JEwFCQzT+be4ifXGWtZp4kCyNPAZBeLOGNCkc1h\n\txGlQ==","X-Gm-Message-State":"AOAM5317zshYJRzQkSqCJDPy564u/tpmMxQuVHHfJLiPsF+4hWKAdH+S\n\tE8dUSx6kwCVHiplM+TDIPO2IUwS2kRvj3cTNultCMQ==","X-Google-Smtp-Source":"ABdhPJxdEHb7Us3qNcyY7ZyedwxdRQHOT4djKrS0/qXI8u+c3XAilHlo7toaRvkZY8qPSiJayyq1A4jiFMhKtrEYDZA=","X-Received":"by 2002:a05:6512:3056:: with SMTP id\n\tb22mr4578724lfb.611.1641983058627; \n\tWed, 12 Jan 2022 02:24:18 -0800 (PST)","MIME-Version":"1.0","References":"<20220112092758.4726-1-david.plowman@raspberrypi.com>\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Wed, 12 Jan 2022 10:24:02 +0000","Message-ID":"","To":"David Plowman ","Content-Type":"multipart/alternative; boundary=\"000000000000efe3f705d55ff88d\"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Fix\n\tcalculation of sensor's native Bayer order","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22021,"web_url":"https://patchwork.libcamera.org/comment/22021/","msgid":"<164206727892.10801.13264424264443266391@Monstersaurus>","date":"2022-01-13T09:47:58","subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Fix\n\tcalculation of sensor's native Bayer order","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck (2022-01-12 10:24:02)\n> Hi David,\n> \n> On Wed, 12 Jan 2022 at 10:20, David Plowman \n> wrote:\n> \n> > Hi Naush\n> >\n> > Thanks for the reply.\n> >\n> > On Wed, 12 Jan 2022 at 10:07, Naushir Patuck \n> > wrote:\n> > >\n> > > Hi David,\n> > >\n> > > Thank you for your work.\n> > >\n> > > On Wed, 12 Jan 2022 at 09:28, David Plowman <\n> > david.plowman@raspberrypi.com> wrote:\n> > >>\n> > >> This bug crept in when the pipeline handler was converted to use media\n> > >> controller.\n> > >>\n> > >> Previously the sensor's hflip and vflip were being cleared before\n> > >> querying the sensor for its \"native\" Bayer order. Now, though, the\n> > >> sensor's available formats are cached before we can clear these bits.\n> > >>\n> > >> Instead, we deduce the transform equivalent to the current hflip and\n> > >> vflip settings, and apply its inverse to the Bayer formats that we now\n> > >> have, thereby giving us the untransformed Bayer order that we want.\n> > >>\n> > >> The practical consequence of this was that the Bayer order stored in\n> > >> DNG files was frequently wrong.\n> > >>\n> > >> Signed-off-by: David Plowman \n> > >> Fixes: 83a512816189 (\"pipeline: raspberrypi: Convert the pipeline\n> > handler to use media controller\")\n> > >> ---\n> > >> .../pipeline/raspberrypi/raspberrypi.cpp | 21 ++++++++++++-------\n> > >> 1 file changed, 14 insertions(+), 7 deletions(-)\n> > >>\n> > >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> index 49d7ff23..c1fb9666 100644\n> > >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> @@ -1279,20 +1279,24 @@ int\n> > PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > >> * Thirdly, what is the \"native\" Bayer order, when no\n> > transforms are\n> > >> * applied?\n> > >> *\n> > >> - * As part of answering the final question, we reset the camera\n> > to\n> > >> - * no transform at all.\n> > >> + * We note that the format list has already been populated with\n> > >> + * whatever flips are currently set, so to answer the final\n> > question\n> > >> + * we get the current Bayer order and undo the transform\n> > implied by\n> > >> + * the current flip settings.\n> > >> */\n> > >> const V4L2Subdevice *sensor = data->sensor_->device();\n> > >> const struct v4l2_query_ext_ctrl *hflipCtrl =\n> > sensor->controlInfo(V4L2_CID_HFLIP);\n> > >> + Transform currentTransform = Transform::Identity;\n> > >> if (hflipCtrl) {\n> > >> /* We assume it will support vflips too... */\n> > >> data->supportsFlips_ = true;\n> > >> data->flipsAlterBayerOrder_ = hflipCtrl->flags &\n> > V4L2_CTRL_FLAG_MODIFY_LAYOUT;\n> > >>\n> > >> - ControlList ctrls(data->sensor_->controls());\n> > >> - ctrls.set(V4L2_CID_HFLIP, 0);\n> > >> - ctrls.set(V4L2_CID_VFLIP, 0);\n> > >> - data->setSensorControls(ctrls);\n> > >> + ControlList ctrls = data->sensor_->getControls({\n> > V4L2_CID_HFLIP, V4L2_CID_VFLIP });\n> > >> + if (ctrls.get(V4L2_CID_HFLIP).get())\n> > >> + currentTransform ^= Transform::HFlip;\n> > >> + if (ctrls.get(V4L2_CID_VFLIP).get())\n> > >> + currentTransform ^= Transform::VFlip;\n\nI'm worried that this might leave us in an inconsistent state at\nstartup.\n\nI can see we definitely need to account for the flips when mapping the\ncurrent bayerFormat.order to a native order, and I think that sounds\nreasonable, so my only concern is if we need to 'reset' the HFLIP/VFLIP\ncontrols to known defaults at startup, either after we've handled this\nnative bayer ordering, or ... perhaps we should do it when we construct\nthe CameraSensor class ... before we read the formats?\n\n\n\n> > >> }\n> > >>\n> > >> /* Look for a valid Bayer format. */\n> > >> @@ -1307,7 +1311,10 @@ int\n> > PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > >> LOG(RPI, Error) << \"No Bayer format found\";\n> > >> return -EINVAL;\n> > >> }\n> > >> - data->nativeBayerOrder_ = bayerFormat.order;\n> > >> + /* Applying the inverse transform will give us the native\n> > order. */\n> > >> + BayerFormat nativeBayerFormat =\n> > bayerFormat.transform(-currentTransform);\n> > >> + data->nativeBayerOrder_ = nativeBayerFormat.order;\n> > >> + LOG(RPI, Debug) << \"Native Bayer format is \" <<\n> > nativeBayerFormat.toString();\n> > >>\n> > >> /*\n> > >> * List the available streams an application may request. At\n> > present, we\n> > >\n> > >\n> > > I think I understand the logic of the changes and it all looks good.\n> > However, I wonder if the\n> > > following (entirely untested) change would also give us the correct\n> > Bayer order accounting for\n> > > flips after configure()?\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 5ee713fe66a6..02b31d787b55 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -761,6 +761,7 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> > CameraConfiguration *config)\n> > > if (isRaw(cfg.pixelFormat)) {\n> > > cfg.setStream(&data->unicam_[Unicam::Image]);\n> > > data->unicam_[Unicam::Image].setExternal(true);\n> > > + cfg.pixelFormat =\n> > unicamFormat.fourcc.toPixelFormat();\n> > > continue;\n> > > }\n> >\n> > That's pretty interesting. I wonder, is there any way to do this\n> > earlier, my impression is that we need to set the pixel format\n> > correctly in validate(), rather than wait for configure()?\n> >\n> \n> I don't think that would be possible. We only get to know the user\n> requested flips in the configure(), so validate() would only ever return\n> the un-flipped (user point of view) ordering.\n> \n\nAlso, if I understand this correctly it's trying to determine the 'constant'\nnative bayer order of the sensor. If it is consistent, and doesn't\nchange with any configuration, then I would expect this to be done when\nthe camera is created/registered as this patch does.\n\nValidate should be able to identify everything that would be affected by\nthe configuration when configure is called with that configuration.\n\n--\nKieran\n\n\n> Naush\n> \n> \n> >\n> > David\n> >\n> > >\n> > >\n> > >>\n> > >> --\n> > >> 2.30.2\n> > >>\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A0D51BE086\n\tfor ;\n\tThu, 13 Jan 2022 09:48:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CB960604F8;\n\tThu, 13 Jan 2022 10:48:03 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9E7C86017E\n\tfor ;\n\tThu, 13 Jan 2022 10:48:01 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 221EB97;\n\tThu, 13 Jan 2022 10:48:01 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"g2rizRXe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1642067281;\n\tbh=oAOCeBBMsuLnv8xFA8GKd6MOf0rKfxuhfTiPGjFqKkw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=g2rizRXeqXZs78hygor/dybCV2kdTgD2jN6VJGquAubgsJ1COIA73Ov7SSW7ug51j\n\tkxP37UIxuIw+cItGZwsTWXDGmUg81osS3HppXTmSdS/lGIBt8FMKtBd1KJSD0/yQzY\n\twbb5Q7z5wEl5qBjYFCaazftPJZetUu4aAVEAsAKs=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220112092758.4726-1-david.plowman@raspberrypi.com>\n\t\n\t\n\t","From":"Kieran Bingham ","To":"David Plowman ,\n\tNaushir Patuck ","Date":"Thu, 13 Jan 2022 09:47:58 +0000","Message-ID":"<164206727892.10801.13264424264443266391@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Fix\n\tcalculation of sensor's native Bayer order","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22022,"web_url":"https://patchwork.libcamera.org/comment/22022/","msgid":"","date":"2022-01-13T10:12:37","subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Fix\n\tcalculation of sensor's native Bayer order","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Kieran, Naush\n\nOn Thu, 13 Jan 2022 at 09:48, Kieran Bingham\n wrote:\n>\n> Quoting Naushir Patuck (2022-01-12 10:24:02)\n> > Hi David,\n> >\n> > On Wed, 12 Jan 2022 at 10:20, David Plowman \n> > wrote:\n> >\n> > > Hi Naush\n> > >\n> > > Thanks for the reply.\n> > >\n> > > On Wed, 12 Jan 2022 at 10:07, Naushir Patuck \n> > > wrote:\n> > > >\n> > > > Hi David,\n> > > >\n> > > > Thank you for your work.\n> > > >\n> > > > On Wed, 12 Jan 2022 at 09:28, David Plowman <\n> > > david.plowman@raspberrypi.com> wrote:\n> > > >>\n> > > >> This bug crept in when the pipeline handler was converted to use media\n> > > >> controller.\n> > > >>\n> > > >> Previously the sensor's hflip and vflip were being cleared before\n> > > >> querying the sensor for its \"native\" Bayer order. Now, though, the\n> > > >> sensor's available formats are cached before we can clear these bits.\n> > > >>\n> > > >> Instead, we deduce the transform equivalent to the current hflip and\n> > > >> vflip settings, and apply its inverse to the Bayer formats that we now\n> > > >> have, thereby giving us the untransformed Bayer order that we want.\n> > > >>\n> > > >> The practical consequence of this was that the Bayer order stored in\n> > > >> DNG files was frequently wrong.\n> > > >>\n> > > >> Signed-off-by: David Plowman \n> > > >> Fixes: 83a512816189 (\"pipeline: raspberrypi: Convert the pipeline\n> > > handler to use media controller\")\n> > > >> ---\n> > > >> .../pipeline/raspberrypi/raspberrypi.cpp | 21 ++++++++++++-------\n> > > >> 1 file changed, 14 insertions(+), 7 deletions(-)\n> > > >>\n> > > >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > >> index 49d7ff23..c1fb9666 100644\n> > > >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > >> @@ -1279,20 +1279,24 @@ int\n> > > PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > >> * Thirdly, what is the \"native\" Bayer order, when no\n> > > transforms are\n> > > >> * applied?\n> > > >> *\n> > > >> - * As part of answering the final question, we reset the camera\n> > > to\n> > > >> - * no transform at all.\n> > > >> + * We note that the format list has already been populated with\n> > > >> + * whatever flips are currently set, so to answer the final\n> > > question\n> > > >> + * we get the current Bayer order and undo the transform\n> > > implied by\n> > > >> + * the current flip settings.\n> > > >> */\n> > > >> const V4L2Subdevice *sensor = data->sensor_->device();\n> > > >> const struct v4l2_query_ext_ctrl *hflipCtrl =\n> > > sensor->controlInfo(V4L2_CID_HFLIP);\n> > > >> + Transform currentTransform = Transform::Identity;\n> > > >> if (hflipCtrl) {\n> > > >> /* We assume it will support vflips too... */\n> > > >> data->supportsFlips_ = true;\n> > > >> data->flipsAlterBayerOrder_ = hflipCtrl->flags &\n> > > V4L2_CTRL_FLAG_MODIFY_LAYOUT;\n> > > >>\n> > > >> - ControlList ctrls(data->sensor_->controls());\n> > > >> - ctrls.set(V4L2_CID_HFLIP, 0);\n> > > >> - ctrls.set(V4L2_CID_VFLIP, 0);\n> > > >> - data->setSensorControls(ctrls);\n> > > >> + ControlList ctrls = data->sensor_->getControls({\n> > > V4L2_CID_HFLIP, V4L2_CID_VFLIP });\n> > > >> + if (ctrls.get(V4L2_CID_HFLIP).get())\n> > > >> + currentTransform ^= Transform::HFlip;\n> > > >> + if (ctrls.get(V4L2_CID_VFLIP).get())\n> > > >> + currentTransform ^= Transform::VFlip;\n>\n> I'm worried that this might leave us in an inconsistent state at\n> startup.\n>\n> I can see we definitely need to account for the flips when mapping the\n> current bayerFormat.order to a native order, and I think that sounds\n> reasonable, so my only concern is if we need to 'reset' the HFLIP/VFLIP\n> controls to known defaults at startup, either after we've handled this\n> native bayer ordering, or ... perhaps we should do it when we construct\n> the CameraSensor class ... before we read the formats?\n\nI don't think anything should end up inconsistent - whatever the\ncurrent h/vflip settings it should (and in my testing does) come out\nwith the correct native Bayer order every time. But I agree, maybe\nthere's a place where we can clear the flips before we cache those\nformats? That would definitely make things easier here.\n\nI assume the Bayer order of the cached formats can't matter - as the\ncamera may get flipped later anyway and everything carries on working.\n\n>\n>\n>\n> > > >> }\n> > > >>\n> > > >> /* Look for a valid Bayer format. */\n> > > >> @@ -1307,7 +1311,10 @@ int\n> > > PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > >> LOG(RPI, Error) << \"No Bayer format found\";\n> > > >> return -EINVAL;\n> > > >> }\n> > > >> - data->nativeBayerOrder_ = bayerFormat.order;\n> > > >> + /* Applying the inverse transform will give us the native\n> > > order. */\n> > > >> + BayerFormat nativeBayerFormat =\n> > > bayerFormat.transform(-currentTransform);\n> > > >> + data->nativeBayerOrder_ = nativeBayerFormat.order;\n> > > >> + LOG(RPI, Debug) << \"Native Bayer format is \" <<\n> > > nativeBayerFormat.toString();\n> > > >>\n> > > >> /*\n> > > >> * List the available streams an application may request. At\n> > > present, we\n> > > >\n> > > >\n> > > > I think I understand the logic of the changes and it all looks good.\n> > > However, I wonder if the\n> > > > following (entirely untested) change would also give us the correct\n> > > Bayer order accounting for\n> > > > flips after configure()?\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index 5ee713fe66a6..02b31d787b55 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -761,6 +761,7 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> > > CameraConfiguration *config)\n> > > > if (isRaw(cfg.pixelFormat)) {\n> > > > cfg.setStream(&data->unicam_[Unicam::Image]);\n> > > > data->unicam_[Unicam::Image].setExternal(true);\n> > > > + cfg.pixelFormat =\n> > > unicamFormat.fourcc.toPixelFormat();\n> > > > continue;\n> > > > }\n> > >\n> > > That's pretty interesting. I wonder, is there any way to do this\n> > > earlier, my impression is that we need to set the pixel format\n> > > correctly in validate(), rather than wait for configure()?\n> > >\n> >\n> > I don't think that would be possible. We only get to know the user\n> > requested flips in the configure(), so validate() would only ever return\n> > the un-flipped (user point of view) ordering.\n> >\n>\n> Also, if I understand this correctly it's trying to determine the 'constant'\n> native bayer order of the sensor. If it is consistent, and doesn't\n> change with any configuration, then I would expect this to be done when\n> the camera is created/registered as this patch does.\n>\n> Validate should be able to identify everything that would be affected by\n> the configuration when configure is called with that configuration.\n\nYes. If we can clear the flips and do everything in registerCamera\nthat would seem ideal...\n\nDavid\n\n>\n> --\n> Kieran\n>\n>\n> > Naush\n> >\n> >\n> > >\n> > > David\n> > >\n> > > >\n> > > >\n> > > >>\n> > > >> --\n> > > >> 2.30.2\n> > > >>\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id ADB09BF415\n\tfor ;\n\tThu, 13 Jan 2022 10:12:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 038FA604F8;\n\tThu, 13 Jan 2022 11:12:52 +0100 (CET)","from mail-wr1-x436.google.com (mail-wr1-x436.google.com\n\t[IPv6:2a00:1450:4864:20::436])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3D0586017E\n\tfor ;\n\tThu, 13 Jan 2022 11:12:50 +0100 (CET)","by mail-wr1-x436.google.com with SMTP id a5so9148862wrh.5\n\tfor ;\n\tThu, 13 Jan 2022 02:12:50 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"OhBbhobR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=W7uqdx1fbDMnP7irMkXB5x3KSaRnqLzb6GKArRGexRo=;\n\tb=OhBbhobRxjd9hF3JcmFStt0NH40pI8HuQnBgrXeKIhath+lq3ML9XLr9P05glEh3F/\n\tgEvonkT06uRUQ70Ihedy5y3X3p1xSm2oHW77LH4Z9BxVECQU7wNX67reeDQJfGUQczBx\n\tZ+CdbJKTIVjJ6SnoTktYRb1hQBt8n3mb3/7sAuyINt84SxagFpWgS819tJe9ATfPR4Vs\n\txpaIdYhcccUzlbtRy+Mbw+IBGz2iB03Ngz2YJqJl2DXr3aqPRlD0u/m2YicksAk1JkE3\n\tv8/L3XxDBzhdGXY0U6GJKmOBU3uDToiQfOcYPy0hfYgTM5mjWcng+9aecP9dsjdPZ6At\n\tRCNQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=W7uqdx1fbDMnP7irMkXB5x3KSaRnqLzb6GKArRGexRo=;\n\tb=PCYwm1prLPn+y90YKV+oefkFlC9HOd6CV82PogqGKkMtsqSDp9+zIW7YzBzjN4cVLe\n\tJ6kh8an1KLY6zaGfvngt3Dc9CnKL/PTRvhdy/ID0cYWJEUsd38t1KGtIv1y5ZevCDTYo\n\trJfDdY9GDAMSZErNtKATOwsLPhg3xqGCMq9rVqxrpk8aghWI8SjvC3QSma2v7WZ5eeBD\n\t2PkuseQBLBoMojFF8g+3FkkptgO6Avbs3gDCUXCE2kvVUI0xkHrVgnhi0nOJh66Qhk3B\n\t9J/auyzIb55eMQ1TPiKPXVFNilVxPaYT6e6KjIjc+BjHst5V54+o3yp6CyPMwMGo7Iw8\n\tkmJQ==","X-Gm-Message-State":"AOAM532iwa0vlYInimkYoq14DzfbcWuVnTilKtHb9CaKTPgo/q9nAMlP\n\t3jtP2u+YSAkDTzLCEM8DgN8Ejs8Q3eHrbikQAEtPXQ==","X-Google-Smtp-Source":"ABdhPJwYn6D4VCkdiY1EdUmg4PcCBEGcZgvXBR3Y1K69UEtvdh4Yrx/koWcAeMtCq3RulxhAKheuTypfCc9tVrjdKQ4=","X-Received":"by 2002:a5d:6d05:: with SMTP id e5mr3281735wrq.3.1642068769731; \n\tThu, 13 Jan 2022 02:12:49 -0800 (PST)","MIME-Version":"1.0","References":"<20220112092758.4726-1-david.plowman@raspberrypi.com>\n\t\n\t\n\t\n\t<164206727892.10801.13264424264443266391@Monstersaurus>","In-Reply-To":"<164206727892.10801.13264424264443266391@Monstersaurus>","From":"David Plowman ","Date":"Thu, 13 Jan 2022 10:12:37 +0000","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Fix\n\tcalculation of sensor's native Bayer order","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22023,"web_url":"https://patchwork.libcamera.org/comment/22023/","msgid":"","date":"2022-01-13T10:30:39","subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Fix\n\tcalculation of sensor's native Bayer order","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi again\n\nOn Thu, 13 Jan 2022 at 10:12, David Plowman\n wrote:\n>\n> Hi Kieran, Naush\n>\n> On Thu, 13 Jan 2022 at 09:48, Kieran Bingham\n> wrote:\n> >\n> > Quoting Naushir Patuck (2022-01-12 10:24:02)\n> > > Hi David,\n> > >\n> > > On Wed, 12 Jan 2022 at 10:20, David Plowman \n> > > wrote:\n> > >\n> > > > Hi Naush\n> > > >\n> > > > Thanks for the reply.\n> > > >\n> > > > On Wed, 12 Jan 2022 at 10:07, Naushir Patuck \n> > > > wrote:\n> > > > >\n> > > > > Hi David,\n> > > > >\n> > > > > Thank you for your work.\n> > > > >\n> > > > > On Wed, 12 Jan 2022 at 09:28, David Plowman <\n> > > > david.plowman@raspberrypi.com> wrote:\n> > > > >>\n> > > > >> This bug crept in when the pipeline handler was converted to use media\n> > > > >> controller.\n> > > > >>\n> > > > >> Previously the sensor's hflip and vflip were being cleared before\n> > > > >> querying the sensor for its \"native\" Bayer order. Now, though, the\n> > > > >> sensor's available formats are cached before we can clear these bits.\n> > > > >>\n> > > > >> Instead, we deduce the transform equivalent to the current hflip and\n> > > > >> vflip settings, and apply its inverse to the Bayer formats that we now\n> > > > >> have, thereby giving us the untransformed Bayer order that we want.\n> > > > >>\n> > > > >> The practical consequence of this was that the Bayer order stored in\n> > > > >> DNG files was frequently wrong.\n> > > > >>\n> > > > >> Signed-off-by: David Plowman \n> > > > >> Fixes: 83a512816189 (\"pipeline: raspberrypi: Convert the pipeline\n> > > > handler to use media controller\")\n> > > > >> ---\n> > > > >> .../pipeline/raspberrypi/raspberrypi.cpp | 21 ++++++++++++-------\n> > > > >> 1 file changed, 14 insertions(+), 7 deletions(-)\n> > > > >>\n> > > > >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > >> index 49d7ff23..c1fb9666 100644\n> > > > >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > >> @@ -1279,20 +1279,24 @@ int\n> > > > PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > > >> * Thirdly, what is the \"native\" Bayer order, when no\n> > > > transforms are\n> > > > >> * applied?\n> > > > >> *\n> > > > >> - * As part of answering the final question, we reset the camera\n> > > > to\n> > > > >> - * no transform at all.\n> > > > >> + * We note that the format list has already been populated with\n> > > > >> + * whatever flips are currently set, so to answer the final\n> > > > question\n> > > > >> + * we get the current Bayer order and undo the transform\n> > > > implied by\n> > > > >> + * the current flip settings.\n> > > > >> */\n> > > > >> const V4L2Subdevice *sensor = data->sensor_->device();\n> > > > >> const struct v4l2_query_ext_ctrl *hflipCtrl =\n> > > > sensor->controlInfo(V4L2_CID_HFLIP);\n> > > > >> + Transform currentTransform = Transform::Identity;\n> > > > >> if (hflipCtrl) {\n> > > > >> /* We assume it will support vflips too... */\n> > > > >> data->supportsFlips_ = true;\n> > > > >> data->flipsAlterBayerOrder_ = hflipCtrl->flags &\n> > > > V4L2_CTRL_FLAG_MODIFY_LAYOUT;\n> > > > >>\n> > > > >> - ControlList ctrls(data->sensor_->controls());\n> > > > >> - ctrls.set(V4L2_CID_HFLIP, 0);\n> > > > >> - ctrls.set(V4L2_CID_VFLIP, 0);\n> > > > >> - data->setSensorControls(ctrls);\n> > > > >> + ControlList ctrls = data->sensor_->getControls({\n> > > > V4L2_CID_HFLIP, V4L2_CID_VFLIP });\n> > > > >> + if (ctrls.get(V4L2_CID_HFLIP).get())\n> > > > >> + currentTransform ^= Transform::HFlip;\n> > > > >> + if (ctrls.get(V4L2_CID_VFLIP).get())\n> > > > >> + currentTransform ^= Transform::VFlip;\n> >\n> > I'm worried that this might leave us in an inconsistent state at\n> > startup.\n> >\n> > I can see we definitely need to account for the flips when mapping the\n> > current bayerFormat.order to a native order, and I think that sounds\n> > reasonable, so my only concern is if we need to 'reset' the HFLIP/VFLIP\n> > controls to known defaults at startup, either after we've handled this\n> > native bayer ordering, or ... perhaps we should do it when we construct\n> > the CameraSensor class ... before we read the formats?\n>\n> I don't think anything should end up inconsistent - whatever the\n> current h/vflip settings it should (and in my testing does) come out\n> with the correct native Bayer order every time. But I agree, maybe\n> there's a place where we can clear the flips before we cache those\n> formats? That would definitely make things easier here.\n>\n> I assume the Bayer order of the cached formats can't matter - as the\n> camera may get flipped later anyway and everything carries on working.\n\nIt looks like it fills that list of formats in CameraSensor::init(). I\nwonder if that should clear the flip bits first (where they exist)?\nWould that be acceptable?\n\nDavid\n\n>\n> >\n> >\n> >\n> > > > >> }\n> > > > >>\n> > > > >> /* Look for a valid Bayer format. */\n> > > > >> @@ -1307,7 +1311,10 @@ int\n> > > > PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > > >> LOG(RPI, Error) << \"No Bayer format found\";\n> > > > >> return -EINVAL;\n> > > > >> }\n> > > > >> - data->nativeBayerOrder_ = bayerFormat.order;\n> > > > >> + /* Applying the inverse transform will give us the native\n> > > > order. */\n> > > > >> + BayerFormat nativeBayerFormat =\n> > > > bayerFormat.transform(-currentTransform);\n> > > > >> + data->nativeBayerOrder_ = nativeBayerFormat.order;\n> > > > >> + LOG(RPI, Debug) << \"Native Bayer format is \" <<\n> > > > nativeBayerFormat.toString();\n> > > > >>\n> > > > >> /*\n> > > > >> * List the available streams an application may request. At\n> > > > present, we\n> > > > >\n> > > > >\n> > > > > I think I understand the logic of the changes and it all looks good.\n> > > > However, I wonder if the\n> > > > > following (entirely untested) change would also give us the correct\n> > > > Bayer order accounting for\n> > > > > flips after configure()?\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > index 5ee713fe66a6..02b31d787b55 100644\n> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > @@ -761,6 +761,7 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> > > > CameraConfiguration *config)\n> > > > > if (isRaw(cfg.pixelFormat)) {\n> > > > > cfg.setStream(&data->unicam_[Unicam::Image]);\n> > > > > data->unicam_[Unicam::Image].setExternal(true);\n> > > > > + cfg.pixelFormat =\n> > > > unicamFormat.fourcc.toPixelFormat();\n> > > > > continue;\n> > > > > }\n> > > >\n> > > > That's pretty interesting. I wonder, is there any way to do this\n> > > > earlier, my impression is that we need to set the pixel format\n> > > > correctly in validate(), rather than wait for configure()?\n> > > >\n> > >\n> > > I don't think that would be possible. We only get to know the user\n> > > requested flips in the configure(), so validate() would only ever return\n> > > the un-flipped (user point of view) ordering.\n> > >\n> >\n> > Also, if I understand this correctly it's trying to determine the 'constant'\n> > native bayer order of the sensor. If it is consistent, and doesn't\n> > change with any configuration, then I would expect this to be done when\n> > the camera is created/registered as this patch does.\n> >\n> > Validate should be able to identify everything that would be affected by\n> > the configuration when configure is called with that configuration.\n>\n> Yes. If we can clear the flips and do everything in registerCamera\n> that would seem ideal...\n>\n> David\n>\n> >\n> > --\n> > Kieran\n> >\n> >\n> > > Naush\n> > >\n> > >\n> > > >\n> > > > David\n> > > >\n> > > > >\n> > > > >\n> > > > >>\n> > > > >> --\n> > > > >> 2.30.2\n> > > > >>\n> > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 004CFBF415\n\tfor ;\n\tThu, 13 Jan 2022 10:30:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4DAD6604F8;\n\tThu, 13 Jan 2022 11:30:53 +0100 (CET)","from mail-wr1-x430.google.com (mail-wr1-x430.google.com\n\t[IPv6:2a00:1450:4864:20::430])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D63AF6017E\n\tfor ;\n\tThu, 13 Jan 2022 11:30:51 +0100 (CET)","by mail-wr1-x430.google.com with SMTP id e9so9260117wra.2\n\tfor ;\n\tThu, 13 Jan 2022 02:30:51 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"NYS/pL/g\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=dF/cxDV1JvytJztAFlIxWEZbFbAjtbQKoLxtHugPzRU=;\n\tb=NYS/pL/gMOqtobz0SvMR2amouAbdFCdzVP1nhPkd9V4RqAYP26tvLNfgup1HR2RO/T\n\t1WyWz7SMCY/h5rmkK243jF99i4o1+b2qXVS9CLX3ueHG4ES874NvkUGjbKDVkELYUOnh\n\tcKvmx/q0XE5DgPlrKnNAKzFEHmYDB5DoAhZb84NzhZw9w/N0ArPBaSBDVBwg4UTT7On2\n\t4guxAEOMI/voAvrof/DfTTaVbJR8/rx5QSrub6EnCQomxl8UupvveRTFYtW2OkirMTxu\n\t2jg7YvHkRLJ1M8tEwwLoOWvrpmJ0mHmWiPYGjLpX6Hyyu8fPjMfC2pAzffo7zwJm1fh6\n\tL7Sg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=dF/cxDV1JvytJztAFlIxWEZbFbAjtbQKoLxtHugPzRU=;\n\tb=n9X4OaST8fl9fjynb2Qh3olXyznYFbno7RTyYyL1nuq1y8/RnF/wVfK/8mXhXRua90\n\tewiwX9tixjrYPyck0ctC/CZJVy+R3HijwMSHz2ojxwDgMVkudqDaNH7O24MKMfdT12Wj\n\tEwSYGtqa+JhO8jqXtJZGMRyxaKZ5qZIMUrwBL1nyqSO6Z7rzEB/Z/oW+ItBlD81BwDNd\n\tca1gVV3ry9BAA3KKC/fk8X7NrezBx9DcBhfL3BqiNn1V2IGYWbDJcZvCfaoMIZWUEZ0v\n\tBDkEvJQsCcJ8XAmWZPtg5A4XCnvtw8pl7dx1aPT9XPsjEEINMtMrr+YPxRJhz2rA5wAD\n\tFNgw==","X-Gm-Message-State":"AOAM5318Jb6mHnL6KfxFfqW/9uYzzjKTJlD1e9pCniQK8huHD5lWZ3Jn\n\t/Ul0zFAO9w5kSiIy5wo4Nq9XEq+DDsKPz48dkHSjww==","X-Google-Smtp-Source":"ABdhPJwtqWCqJ9qGOQp8F3QRPqiO+o4mPYnWWE++z9S1Xinusuom/mnDyA9yK3mQzMth3fcekK5y0kaAXs/5tOWfT3Y=","X-Received":"by 2002:adf:d1c8:: with SMTP id b8mr2170357wrd.317.1642069851359;\n\tThu, 13 Jan 2022 02:30:51 -0800 (PST)","MIME-Version":"1.0","References":"<20220112092758.4726-1-david.plowman@raspberrypi.com>\n\t\n\t\n\t\n\t<164206727892.10801.13264424264443266391@Monstersaurus>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Thu, 13 Jan 2022 10:30:39 +0000","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Fix\n\tcalculation of sensor's native Bayer order","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22024,"web_url":"https://patchwork.libcamera.org/comment/22024/","msgid":"","date":"2022-01-13T10:32:26","subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Fix\n\tcalculation of sensor's native Bayer order","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Thu, 13 Jan 2022 at 10:30, David Plowman \nwrote:\n\n> Hi again\n>\n> On Thu, 13 Jan 2022 at 10:12, David Plowman\n> wrote:\n> >\n> > Hi Kieran, Naush\n> >\n> > On Thu, 13 Jan 2022 at 09:48, Kieran Bingham\n> > wrote:\n> > >\n> > > Quoting Naushir Patuck (2022-01-12 10:24:02)\n> > > > Hi David,\n> > > >\n> > > > On Wed, 12 Jan 2022 at 10:20, David Plowman <\n> david.plowman@raspberrypi.com>\n> > > > wrote:\n> > > >\n> > > > > Hi Naush\n> > > > >\n> > > > > Thanks for the reply.\n> > > > >\n> > > > > On Wed, 12 Jan 2022 at 10:07, Naushir Patuck <\n> naush@raspberrypi.com>\n> > > > > wrote:\n> > > > > >\n> > > > > > Hi David,\n> > > > > >\n> > > > > > Thank you for your work.\n> > > > > >\n> > > > > > On Wed, 12 Jan 2022 at 09:28, David Plowman <\n> > > > > david.plowman@raspberrypi.com> wrote:\n> > > > > >>\n> > > > > >> This bug crept in when the pipeline handler was converted to\n> use media\n> > > > > >> controller.\n> > > > > >>\n> > > > > >> Previously the sensor's hflip and vflip were being cleared\n> before\n> > > > > >> querying the sensor for its \"native\" Bayer order. Now, though,\n> the\n> > > > > >> sensor's available formats are cached before we can clear these\n> bits.\n> > > > > >>\n> > > > > >> Instead, we deduce the transform equivalent to the current\n> hflip and\n> > > > > >> vflip settings, and apply its inverse to the Bayer formats that\n> we now\n> > > > > >> have, thereby giving us the untransformed Bayer order that we\n> want.\n> > > > > >>\n> > > > > >> The practical consequence of this was that the Bayer order\n> stored in\n> > > > > >> DNG files was frequently wrong.\n> > > > > >>\n> > > > > >> Signed-off-by: David Plowman \n> > > > > >> Fixes: 83a512816189 (\"pipeline: raspberrypi: Convert the\n> pipeline\n> > > > > handler to use media controller\")\n> > > > > >> ---\n> > > > > >> .../pipeline/raspberrypi/raspberrypi.cpp | 21\n> ++++++++++++-------\n> > > > > >> 1 file changed, 14 insertions(+), 7 deletions(-)\n> > > > > >>\n> > > > > >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > >> index 49d7ff23..c1fb9666 100644\n> > > > > >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > >> @@ -1279,20 +1279,24 @@ int\n> > > > > PipelineHandlerRPi::registerCamera(MediaDevice *unicam,\n> MediaDevice *isp, Me\n> > > > > >> * Thirdly, what is the \"native\" Bayer order, when no\n> > > > > transforms are\n> > > > > >> * applied?\n> > > > > >> *\n> > > > > >> - * As part of answering the final question, we reset\n> the camera\n> > > > > to\n> > > > > >> - * no transform at all.\n> > > > > >> + * We note that the format list has already been\n> populated with\n> > > > > >> + * whatever flips are currently set, so to answer the\n> final\n> > > > > question\n> > > > > >> + * we get the current Bayer order and undo the transform\n> > > > > implied by\n> > > > > >> + * the current flip settings.\n> > > > > >> */\n> > > > > >> const V4L2Subdevice *sensor = data->sensor_->device();\n> > > > > >> const struct v4l2_query_ext_ctrl *hflipCtrl =\n> > > > > sensor->controlInfo(V4L2_CID_HFLIP);\n> > > > > >> + Transform currentTransform = Transform::Identity;\n> > > > > >> if (hflipCtrl) {\n> > > > > >> /* We assume it will support vflips too... */\n> > > > > >> data->supportsFlips_ = true;\n> > > > > >> data->flipsAlterBayerOrder_ = hflipCtrl->flags &\n> > > > > V4L2_CTRL_FLAG_MODIFY_LAYOUT;\n> > > > > >>\n> > > > > >> - ControlList ctrls(data->sensor_->controls());\n> > > > > >> - ctrls.set(V4L2_CID_HFLIP, 0);\n> > > > > >> - ctrls.set(V4L2_CID_VFLIP, 0);\n> > > > > >> - data->setSensorControls(ctrls);\n> > > > > >> + ControlList ctrls = data->sensor_->getControls({\n> > > > > V4L2_CID_HFLIP, V4L2_CID_VFLIP });\n> > > > > >> + if (ctrls.get(V4L2_CID_HFLIP).get())\n> > > > > >> + currentTransform ^= Transform::HFlip;\n> > > > > >> + if (ctrls.get(V4L2_CID_VFLIP).get())\n> > > > > >> + currentTransform ^= Transform::VFlip;\n> > >\n> > > I'm worried that this might leave us in an inconsistent state at\n> > > startup.\n> > >\n> > > I can see we definitely need to account for the flips when mapping the\n> > > current bayerFormat.order to a native order, and I think that sounds\n> > > reasonable, so my only concern is if we need to 'reset' the HFLIP/VFLIP\n> > > controls to known defaults at startup, either after we've handled this\n> > > native bayer ordering, or ... perhaps we should do it when we construct\n> > > the CameraSensor class ... before we read the formats?\n> >\n> > I don't think anything should end up inconsistent - whatever the\n> > current h/vflip settings it should (and in my testing does) come out\n> > with the correct native Bayer order every time. But I agree, maybe\n> > there's a place where we can clear the flips before we cache those\n> > formats? That would definitely make things easier here.\n> >\n> > I assume the Bayer order of the cached formats can't matter - as the\n> > camera may get flipped later anyway and everything carries on working.\n>\n> It looks like it fills that list of formats in CameraSensor::init(). I\n> wonder if that should clear the flip bits first (where they exist)?\n> Would that be acceptable?\n>\n\nI would be happy with that behaviour.\n\nNaush\n\n\n\n>\n> David\n>\n> >\n> > >\n> > >\n> > >\n> > > > > >> }\n> > > > > >>\n> > > > > >> /* Look for a valid Bayer format. */\n> > > > > >> @@ -1307,7 +1311,10 @@ int\n> > > > > PipelineHandlerRPi::registerCamera(MediaDevice *unicam,\n> MediaDevice *isp, Me\n> > > > > >> LOG(RPI, Error) << \"No Bayer format found\";\n> > > > > >> return -EINVAL;\n> > > > > >> }\n> > > > > >> - data->nativeBayerOrder_ = bayerFormat.order;\n> > > > > >> + /* Applying the inverse transform will give us the\n> native\n> > > > > order. */\n> > > > > >> + BayerFormat nativeBayerFormat =\n> > > > > bayerFormat.transform(-currentTransform);\n> > > > > >> + data->nativeBayerOrder_ = nativeBayerFormat.order;\n> > > > > >> + LOG(RPI, Debug) << \"Native Bayer format is \" <<\n> > > > > nativeBayerFormat.toString();\n> > > > > >>\n> > > > > >> /*\n> > > > > >> * List the available streams an application may\n> request. At\n> > > > > present, we\n> > > > > >\n> > > > > >\n> > > > > > I think I understand the logic of the changes and it all looks\n> good.\n> > > > > However, I wonder if the\n> > > > > > following (entirely untested) change would also give us the\n> correct\n> > > > > Bayer order accounting for\n> > > > > > flips after configure()?\n> > > > > >\n> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > index 5ee713fe66a6..02b31d787b55 100644\n> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > @@ -761,6 +761,7 @@ int PipelineHandlerRPi::configure(Camera\n> *camera,\n> > > > > CameraConfiguration *config)\n> > > > > > if (isRaw(cfg.pixelFormat)) {\n> > > > > >\n> cfg.setStream(&data->unicam_[Unicam::Image]);\n> > > > > >\n> data->unicam_[Unicam::Image].setExternal(true);\n> > > > > > + cfg.pixelFormat =\n> > > > > unicamFormat.fourcc.toPixelFormat();\n> > > > > > continue;\n> > > > > > }\n> > > > >\n> > > > > That's pretty interesting. I wonder, is there any way to do this\n> > > > > earlier, my impression is that we need to set the pixel format\n> > > > > correctly in validate(), rather than wait for configure()?\n> > > > >\n> > > >\n> > > > I don't think that would be possible. We only get to know the user\n> > > > requested flips in the configure(), so validate() would only ever\n> return\n> > > > the un-flipped (user point of view) ordering.\n> > > >\n> > >\n> > > Also, if I understand this correctly it's trying to determine the\n> 'constant'\n> > > native bayer order of the sensor. If it is consistent, and doesn't\n> > > change with any configuration, then I would expect this to be done when\n> > > the camera is created/registered as this patch does.\n> > >\n> > > Validate should be able to identify everything that would be affected\n> by\n> > > the configuration when configure is called with that configuration.\n> >\n> > Yes. If we can clear the flips and do everything in registerCamera\n> > that would seem ideal...\n> >\n> > David\n> >\n> > >\n> > > --\n> > > Kieran\n> > >\n> > >\n> > > > Naush\n> > > >\n> > > >\n> > > > >\n> > > > > David\n> > > > >\n> > > > > >\n> > > > > >\n> > > > > >>\n> > > > > >> --\n> > > > > >> 2.30.2\n> > > > > >>\n> > > > >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 941CBBF415\n\tfor ;\n\tThu, 13 Jan 2022 10:32:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E217C604F8;\n\tThu, 13 Jan 2022 11:32:47 +0100 (CET)","from mail-lf1-x134.google.com (mail-lf1-x134.google.com\n\t[IPv6:2a00:1450:4864:20::134])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A6EF26017E\n\tfor ;\n\tThu, 13 Jan 2022 11:32:46 +0100 (CET)","by mail-lf1-x134.google.com with SMTP id m1so17895585lfq.4\n\tfor ;\n\tThu, 13 Jan 2022 02:32:46 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"JikeDoZk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=Fki5AhuhANheXvKIsh9/I6sRNsH2j8NQ3ACsUX1gnaM=;\n\tb=JikeDoZkAeZQMMxFkMLfZlhBDsCuMm6FcJUo2Ct2/3gDpCvQa597nty/N7JDCKZCG2\n\t2jyUm73BFvSTcO16ttI3W0kRlR/48EhLOKGvTN2t5a9GZ2jxrD9RuhKKEc0/L5qB0zdU\n\tjz/oa/JYGa+DH9PYVNlaszvVKmaWZy/1+l7uDd13inRh/j9JKihJyD7TBGcTeaUcTOs3\n\tGAQ/rd1VmveplwDs0NwN46HnO+QEaFPkUmyB29JKeTykmyUkzEERFEzqYJRoM/DxfAl2\n\tHQfO0nFpZsXym3nzdGQ0xO+f435JD9z7HRu4ILlmN+llwVrmBcPwQtYkiZ5rkrrhH4FK\n\t17MQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Fki5AhuhANheXvKIsh9/I6sRNsH2j8NQ3ACsUX1gnaM=;\n\tb=LlZVL0760w+Qk2Hj91dURGEAVtma+93AnfDA9XdXVkcbsXb7/JvofdRHv3X3o7LX2d\n\tfcs2PYOpWwCW9pEQwMImnHDUUxe8K0f/2UiGSTzDhbxuKnDbjC/LZW/fJQH3FuaBkFz/\n\tVX0GWOYjcaBKzrDwOh38btEvc5DfbP53m5wY2sRd8GLND4wfG4hmPLsoW2alu4BufTYi\n\tJ4ccZhX2Z3Lm1Ipym4+9bIsXB28QYBMsSvRLYIyisFHvJYeSY5nzczyh98rJx/3NQ8Fr\n\tm8rvFE5lxFcUoTJnKLF558tfJzPvEbQGmpf+w3OjgOJQjGIf3TOOZAUpXAaS7n1VxhOE\n\tmlKg==","X-Gm-Message-State":"AOAM531abLHhZqmnTAvbdJuo+nro8HzPWDqkcNuk5nuBGBrQURS7MNzN\n\t0VZd9090H4ChUXojqt/H7LEcRYy62sLpd9mIThnJ+Q==","X-Google-Smtp-Source":"ABdhPJyE2VXxmLWpAmgATyWGByBjNESvzFZr38KKrMnre3MLIOx1Xya+hjviJjwA1TPFDyTfzRgwUqbywJBzDYkFt0E=","X-Received":"by 2002:a05:651c:510:: with SMTP id\n\to16mr2720461ljp.372.1642069965682; \n\tThu, 13 Jan 2022 02:32:45 -0800 (PST)","MIME-Version":"1.0","References":"<20220112092758.4726-1-david.plowman@raspberrypi.com>\n\t\n\t\n\t\n\t<164206727892.10801.13264424264443266391@Monstersaurus>\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Thu, 13 Jan 2022 10:32:26 +0000","Message-ID":"","To":"David Plowman ","Content-Type":"multipart/alternative; boundary=\"00000000000000514f05d57435c8\"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Fix\n\tcalculation of sensor's native Bayer order","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]