[{"id":36315,"web_url":"https://patchwork.libcamera.org/comment/36315/","msgid":"<66323c5e-1c76-4d05-adf3-eee52d01aa12@ideasonboard.com>","date":"2025-10-16T14:16:34","subject":"Re: [PATCH v1 27/33] libcamera: rkisp1: Handle requested orientation\n\tusing dewarper","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Stefan\n\nOn 30/09/2025 13:26, Stefan Klug wrote:\n> When the dewarper is present it can handle arbitrary orientations\n> specified in the requested camera configuration. In that case handle all\n> transformations inside the dewarper (even if the sensor supports some of\n> them) because that makes it easier to handle coordinates for lens\n> dewarping inside the dewarper.\n> \n> Signed-off-by: Stefan Klug \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 79 +++++++++++++++++++-----\n> 1 file changed, 63 insertions(+), 16 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index e8902ce66b70..1046930857e1 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -550,11 +550,6 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\tstatus = Adjusted;\n> \t}\n> \n> -\tOrientation requestedOrientation = orientation;\n> -\tcombinedTransform_ = data_->sensor_->computeTransform(&orientation);\n> -\tif (orientation != requestedOrientation)\n> -\t\tstatus = Adjusted;\n> -\n> \t/*\n> \t * Simultaneous capture of raw and processed streams isn't possible. If\n> \t * there is any raw stream, cap the number of streams to one.\n> @@ -570,6 +565,12 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\t}\n> \t}\n> \n> +\t/*\n> +\t * If the dewarper supports orientation adjustments, apply that completely\n> +\t * there. Even if the sensor supports flips, it is beneficial to do that\n> +\t * in the dewarper so that lens dewarping happens on the unflipped image\n> +\t */\n> +\tbool transposeAfterIsp = false;\n> \tbool useDewarper = false;\n> \tif (pipe->dewarper_) {\n> \t\t/*\n> @@ -580,6 +581,14 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\t\t PixelFormatInfo::ColourEncodingRAW;\n> \t\tif (!isRaw)\n> \t\t\tuseDewarper = true;\n> +\t\tcombinedTransform_ = orientation / data_->sensor_->mountingOrientation();\n> +\t\tif (!!(combinedTransform_ & Transform::Transpose))\n> +\t\t\ttransposeAfterIsp = true;\n> +\t} else {\n> +\t\tOrientation requestedOrientation = orientation;\n> +\t\tcombinedTransform_ = data_->sensor_->computeTransform(&orientation);\n> +\t\tif (orientation != requestedOrientation)\n> +\t\t\tstatus = Adjusted;\n> \t}\n> \n> \t/*\n> @@ -610,6 +619,9 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\t (expectedStatus == Valid && ret == Adjusted))\n> \t\t\treturn false;\n> \n> +\t\tif (transposeAfterIsp)\n> +\t\t\ttryCfg.size.transpose();\n> +\n> \t\tif (useDewarper) {\n> \t\t\t/*\n> \t\t\t * The dewarper output is independent of the ISP path.\n> @@ -694,6 +706,9 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\tmaxSize = std::max(maxSize, cfg.size);\n> \t}\n> \n> +\tif (transposeAfterIsp)\n> +\t\tmaxSize.transpose();\n> +\n> \tstd::vector mbusCodes;\n> \n> \tif (rawFormat.isValid()) {\n> @@ -739,6 +754,22 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> \tif (roles.empty())\n> \t\treturn config;\n> \n> +\tTransform transform = Transform::Identity;\n> +\tSize previewSize = kRkISP1PreviewSize;\n> +\tbool transposeAfterIsp = false;\n> +\tif (data->canUseDewarper_) {\n> +\t\ttransform = Orientation::Rotate0 / data->sensor_->mountingOrientation();\n> +\t\tif (!!(transform & Transform::Transpose))\n> +\t\t\ttransposeAfterIsp = true;\n> +\t}\n> +\n> +\t/*\n> +\t * In case of a transpose transform we need to create a path for the\n> +\t * transposed size.\n> +\t */\n> +\tif (transposeAfterIsp)\n> +\t\tpreviewSize.transpose();\n> +\n> \t/*\n> \t * As the ISP can't output different color spaces for the main and self\n> \t * path, pick a sensible default color space based on the role of the\n> @@ -767,7 +798,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> \t\t\tif (!colorSpace)\n> \t\t\t\tcolorSpace = ColorSpace::Sycc;\n> \n> -\t\t\tsize = kRkISP1PreviewSize;\n> +\t\t\tsize = previewSize;\n> \t\t\tbreak;\n> \n> \t\tcase StreamRole::VideoRecording:\n> @@ -775,7 +806,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> \t\t\tif (!colorSpace)\n> \t\t\t\tcolorSpace = ColorSpace::Rec709;\n> \n> -\t\t\tsize = kRkISP1PreviewSize;\n> +\t\t\tsize = previewSize;\n> \t\t\tbreak;\n> \n> \t\tcase StreamRole::Raw:\n> @@ -817,6 +848,9 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> \t\tif (!cfg.pixelFormat.isValid())\n> \t\t\treturn nullptr;\n> \n> +\t\tif (transposeAfterIsp && role != StreamRole::Raw)\n> +\t\t\tcfg.size.transpose();\n> +\n> \t\tcfg.colorSpace = colorSpace;\n> \t\tcfg.bufferCount = kRkISP1MinBufferCount;\n> \t\tconfig->addConfiguration(cfg);\n> @@ -839,6 +873,19 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \tif (ret)\n> \t\treturn ret;\n> \n> +\tconst PixelFormat &streamFormat = config->at(0).pixelFormat;\n> +\tconst PixelFormatInfo &info = PixelFormatInfo::info(streamFormat);\n> +\tisRaw_ = info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> +\tdata->usesDewarper_ = data->canUseDewarper_ && !isRaw_;\n\nThis change is I think the root cause of a segfault I get using this series in ::updateControls(). \nWe check for data->usesDewarper_ which until this patch is set (below) with:\n\n > -\tdata->usesDewarper_ = dewarper_ && !isRaw_;\n\nbut following this change usesDewarper_ can be set to true even if PipelineHandlerRkISP1::dewarper_ \nis null.\n\nPerhaps:\n\ndata->usesDewarper_ = dewarper_ && data->canUseDewarper_ && !isRaw_;\n\n? Or else a similar check when data->canUseDewarper_ is initialised.\n\nThanks\nDan\n\n> +\n> +\tTransform transform = config->combinedTransform();\n> +\tbool transposeAfterIsp = false;\n> +\tif (data->usesDewarper_) {\n> +\t\tif (!!(transform & Transform::Transpose))\n> +\t\t\ttransposeAfterIsp = true;\n> +\t\ttransform = Transform::Identity;\n> +\t}\n> +\n> \t/*\n> \t * Configure the format on the sensor output and propagate it through\n> \t * the pipeline.\n> @@ -848,10 +895,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \n> \tif (config->sensorConfig)\n> \t\tret = sensor->applyConfiguration(*config->sensorConfig,\n> -\t\t\t\t\t\t config->combinedTransform(),\n> +\t\t\t\t\t\t transform,\n> \t\t\t\t\t\t &format);\n> \telse\n> -\t\tret = sensor->setFormat(&format, config->combinedTransform());\n> +\t\tret = sensor->setFormat(&format, transform);\n> \n> \tif (ret < 0)\n> \t\treturn ret;\n> @@ -878,10 +925,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \t\t<< \" crop \" << inputCrop;\n> \n> \tRectangle outputCrop = inputCrop;\n> -\tconst PixelFormat &streamFormat = config->at(0).pixelFormat;\n> -\tconst PixelFormatInfo &info = PixelFormatInfo::info(streamFormat);\n> -\tisRaw_ = info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> -\tdata->usesDewarper_ = dewarper_ && !isRaw_;\n> \n> \t/* YUYV8_2X8 is required on the ISP source path pad for YUV output. */\n> \tif (!isRaw_)\n> @@ -973,15 +1016,19 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \t\t\t\t\tinputCrop, sensorInfo.analogCrop);\n> \t\t\t\tauto &vertexMap = dewarper_->vertexMap(cfg.stream());\n> \t\t\t\tvertexMap.setSensorCrop(sensorCrop);\n> +\t\t\t\tvertexMap.setTransform(config->combinedTransform());\n> \t\t\t\tdata->properties_.set(properties::ScalerCropMaximum, sensorCrop);\n> \n> \t\t\t\t/*\n> \t\t\t\t * Apply a default sensor crop that keeps the\n> \t\t\t\t * aspect ratio.\n> \t\t\t\t */\n> -\t\t\t\tSize crop = format.size.boundedToAspectRatio(cfg.size);\n> -\t\t\t\tvertexMap.setScalerCrop(crop.centeredTo(\n> -\t\t\t\t\tRectangle(format.size).center()));\n> +\t\t\t\tSize size = cfg.size;\n> +\t\t\t\tif (transposeAfterIsp)\n> +\t\t\t\t\tsize.transpose();\n> +\t\t\t\tsize = sensorCrop.size().boundedToAspectRatio(size);\n> +\t\t\t\tvertexMap.setScalerCrop(\n> +\t\t\t\t\tsize.centeredTo(sensorCrop.center()));\n> \t\t\t}\n> \n> \t\t\tret = mainPath_.configure(ispCfg, format);","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 AF448C3259\n\tfor ;\n\tThu, 16 Oct 2025 14:16:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EAD3960683;\n\tThu, 16 Oct 2025 16:16:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C0826605D7\n\tfor ;\n\tThu, 16 Oct 2025 16:16:36 +0200 (CEST)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E2CB2558\n\tfor ;\n\tThu, 16 Oct 2025 16:14:56 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GrN0V1HI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1760624097;\n\tbh=yIwjJnDzh3ooz5SEG706Gkc+GS0ytMMSYtecqNaA6wY=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=GrN0V1HI02U0esL1femB2w7txXMgLcKfXZoAlobNli4Mb8spnYd4P3HTkniVjwWuu\n\toY9QNXnbuLifM16XZMN40V1qy8xSkGF6LhfSLld8f2gKSLZuwurmIT2w46WpManPNx\n\te/A5FAVQCadengYFDJiIpM1UMPfsUYkKKLRiGBBk=","Message-ID":"<66323c5e-1c76-4d05-adf3-eee52d01aa12@ideasonboard.com>","Date":"Thu, 16 Oct 2025 15:16:34 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 27/33] libcamera: rkisp1: Handle requested orientation\n\tusing dewarper","To":"libcamera-devel@lists.libcamera.org","References":"<20250930122726.1837524-1-stefan.klug@ideasonboard.com>\n\t<20250930122726.1837524-28-stefan.klug@ideasonboard.com>","Content-Language":"en-US","From":"Dan Scally ","In-Reply-To":"<20250930122726.1837524-28-stefan.klug@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36363,"web_url":"https://patchwork.libcamera.org/comment/36363/","msgid":"<176096740668.336133.2219066694299853548@localhost>","date":"2025-10-20T13:36:46","subject":"Re: [PATCH v1 27/33] libcamera: rkisp1: Handle requested orientation\n\tusing dewarper","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Dan,\n\nThank you for the review.\n\nQuoting Dan Scally (2025-10-16 16:16:34)\n> Hi Stefan\n> \n> On 30/09/2025 13:26, Stefan Klug wrote:\n> > When the dewarper is present it can handle arbitrary orientations\n> > specified in the requested camera configuration. In that case handle all\n> > transformations inside the dewarper (even if the sensor supports some of\n> > them) because that makes it easier to handle coordinates for lens\n> > dewarping inside the dewarper.\n> > \n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 79 +++++++++++++++++++-----\n> > 1 file changed, 63 insertions(+), 16 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index e8902ce66b70..1046930857e1 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -550,11 +550,6 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > status = Adjusted;\n> > }\n> > \n> > - Orientation requestedOrientation = orientation;\n> > - combinedTransform_ = data_->sensor_->computeTransform(&orientation);\n> > - if (orientation != requestedOrientation)\n> > - status = Adjusted;\n> > -\n> > /*\n> > * Simultaneous capture of raw and processed streams isn't possible. If\n> > * there is any raw stream, cap the number of streams to one.\n> > @@ -570,6 +565,12 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > }\n> > }\n> > \n> > + /*\n> > + * If the dewarper supports orientation adjustments, apply that completely\n> > + * there. Even if the sensor supports flips, it is beneficial to do that\n> > + * in the dewarper so that lens dewarping happens on the unflipped image\n> > + */\n> > + bool transposeAfterIsp = false;\n> > bool useDewarper = false;\n> > if (pipe->dewarper_) {\n> > /*\n> > @@ -580,6 +581,14 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > PixelFormatInfo::ColourEncodingRAW;\n> > if (!isRaw)\n> > useDewarper = true;\n> > + combinedTransform_ = orientation / data_->sensor_->mountingOrientation();\n> > + if (!!(combinedTransform_ & Transform::Transpose))\n> > + transposeAfterIsp = true;\n> > + } else {\n> > + Orientation requestedOrientation = orientation;\n> > + combinedTransform_ = data_->sensor_->computeTransform(&orientation);\n> > + if (orientation != requestedOrientation)\n> > + status = Adjusted;\n> > }\n> > \n> > /*\n> > @@ -610,6 +619,9 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > (expectedStatus == Valid && ret == Adjusted))\n> > return false;\n> > \n> > + if (transposeAfterIsp)\n> > + tryCfg.size.transpose();\n> > +\n> > if (useDewarper) {\n> > /*\n> > * The dewarper output is independent of the ISP path.\n> > @@ -694,6 +706,9 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > maxSize = std::max(maxSize, cfg.size);\n> > }\n> > \n> > + if (transposeAfterIsp)\n> > + maxSize.transpose();\n> > +\n> > std::vector mbusCodes;\n> > \n> > if (rawFormat.isValid()) {\n> > @@ -739,6 +754,22 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> > if (roles.empty())\n> > return config;\n> > \n> > + Transform transform = Transform::Identity;\n> > + Size previewSize = kRkISP1PreviewSize;\n> > + bool transposeAfterIsp = false;\n> > + if (data->canUseDewarper_) {\n> > + transform = Orientation::Rotate0 / data->sensor_->mountingOrientation();\n> > + if (!!(transform & Transform::Transpose))\n> > + transposeAfterIsp = true;\n> > + }\n> > +\n> > + /*\n> > + * In case of a transpose transform we need to create a path for the\n> > + * transposed size.\n> > + */\n> > + if (transposeAfterIsp)\n> > + previewSize.transpose();\n> > +\n> > /*\n> > * As the ISP can't output different color spaces for the main and self\n> > * path, pick a sensible default color space based on the role of the\n> > @@ -767,7 +798,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> > if (!colorSpace)\n> > colorSpace = ColorSpace::Sycc;\n> > \n> > - size = kRkISP1PreviewSize;\n> > + size = previewSize;\n> > break;\n> > \n> > case StreamRole::VideoRecording:\n> > @@ -775,7 +806,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> > if (!colorSpace)\n> > colorSpace = ColorSpace::Rec709;\n> > \n> > - size = kRkISP1PreviewSize;\n> > + size = previewSize;\n> > break;\n> > \n> > case StreamRole::Raw:\n> > @@ -817,6 +848,9 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> > if (!cfg.pixelFormat.isValid())\n> > return nullptr;\n> > \n> > + if (transposeAfterIsp && role != StreamRole::Raw)\n> > + cfg.size.transpose();\n> > +\n> > cfg.colorSpace = colorSpace;\n> > cfg.bufferCount = kRkISP1MinBufferCount;\n> > config->addConfiguration(cfg);\n> > @@ -839,6 +873,19 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > if (ret)\n> > return ret;\n> > \n> > + const PixelFormat &streamFormat = config->at(0).pixelFormat;\n> > + const PixelFormatInfo &info = PixelFormatInfo::info(streamFormat);\n> > + isRaw_ = info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> > + data->usesDewarper_ = data->canUseDewarper_ && !isRaw_;\n> \n> This change is I think the root cause of a segfault I get using this series in ::updateControls(). \n> We check for data->usesDewarper_ which until this patch is set (below) with:\n> \n> > - data->usesDewarper_ = dewarper_ && !isRaw_;\n> \n> but following this change usesDewarper_ can be set to true even if PipelineHandlerRkISP1::dewarper_ \n> is null.\n\nI think I'm missing something here. usesDewarper_ can only be true is\ncanUseDewarper_ is true. And canUseDewarper_ is only set to true in\nloadTuningFile() which contains a guard\n\nif (!pipe()->dewarper_)\n\t/* Nothing to do without dewarper */\n\treturn 0;\n\nSo I fail to understand how canUseDewarper_ is true in your case.\n\nAs a side note: I had some corner cases especially when sensor orientations != 0 are\ninvolved where the generated/validated configurations couldn't be\napplied to a camera. Might that be the case on your side?\n\nBest regards,\nStefan\n\n> \n> Perhaps:\n> \n> data->usesDewarper_ = dewarper_ && data->canUseDewarper_ && !isRaw_;\n> \n> ? Or else a similar check when data->canUseDewarper_ is initialised.\n> \n> Thanks\n> Dan\n> \n> > +\n> > + Transform transform = config->combinedTransform();\n> > + bool transposeAfterIsp = false;\n> > + if (data->usesDewarper_) {\n> > + if (!!(transform & Transform::Transpose))\n> > + transposeAfterIsp = true;\n> > + transform = Transform::Identity;\n> > + }\n> > +\n> > /*\n> > * Configure the format on the sensor output and propagate it through\n> > * the pipeline.\n> > @@ -848,10 +895,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > \n> > if (config->sensorConfig)\n> > ret = sensor->applyConfiguration(*config->sensorConfig,\n> > - config->combinedTransform(),\n> > + transform,\n> > &format);\n> > else\n> > - ret = sensor->setFormat(&format, config->combinedTransform());\n> > + ret = sensor->setFormat(&format, transform);\n> > \n> > if (ret < 0)\n> > return ret;\n> > @@ -878,10 +925,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > << \" crop \" << inputCrop;\n> > \n> > Rectangle outputCrop = inputCrop;\n> > - const PixelFormat &streamFormat = config->at(0).pixelFormat;\n> > - const PixelFormatInfo &info = PixelFormatInfo::info(streamFormat);\n> > - isRaw_ = info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> > - data->usesDewarper_ = dewarper_ && !isRaw_;\n> > \n> > /* YUYV8_2X8 is required on the ISP source path pad for YUV output. */\n> > if (!isRaw_)\n> > @@ -973,15 +1016,19 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > inputCrop, sensorInfo.analogCrop);\n> > auto &vertexMap = dewarper_->vertexMap(cfg.stream());\n> > vertexMap.setSensorCrop(sensorCrop);\n> > + vertexMap.setTransform(config->combinedTransform());\n> > data->properties_.set(properties::ScalerCropMaximum, sensorCrop);\n> > \n> > /*\n> > * Apply a default sensor crop that keeps the\n> > * aspect ratio.\n> > */\n> > - Size crop = format.size.boundedToAspectRatio(cfg.size);\n> > - vertexMap.setScalerCrop(crop.centeredTo(\n> > - Rectangle(format.size).center()));\n> > + Size size = cfg.size;\n> > + if (transposeAfterIsp)\n> > + size.transpose();\n> > + size = sensorCrop.size().boundedToAspectRatio(size);\n> > + vertexMap.setScalerCrop(\n> > + size.centeredTo(sensorCrop.center()));\n> > }\n> > \n> > ret = mainPath_.configure(ispCfg, format);\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 1C02EBE080\n\tfor ;\n\tMon, 20 Oct 2025 13:36:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4ADDA6074E;\n\tMon, 20 Oct 2025 15:36:51 +0200 (CEST)","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 3EDFB60728\n\tfor ;\n\tMon, 20 Oct 2025 15:36:50 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:b5d:2477:3cd5:195f])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 7C5132B3;\n\tMon, 20 Oct 2025 15:35:07 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XFgsZdid\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1760967307;\n\tbh=tTN1+xd9uWO6ynnVob0DbxEKFUr2OjuyffuoRZ1RRGI=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=XFgsZdidkZ8zsJUYUMCosZcv2tsfgjaLLOyTUnkVHzOs0Zxm4MEtbQGW0hxAVhzOT\n\trCWuTsKy8zPkpzjccPgI9J5c0St9wZRoWni2dQu5pZtwQRs8BCbkeYFJuJJZRzLPBg\n\tiqRny61jqtFNYGHn96yQgim7xnTsdUv9mS42xjiw=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<66323c5e-1c76-4d05-adf3-eee52d01aa12@ideasonboard.com>","References":"<20250930122726.1837524-1-stefan.klug@ideasonboard.com>\n\t<20250930122726.1837524-28-stefan.klug@ideasonboard.com>\n\t<66323c5e-1c76-4d05-adf3-eee52d01aa12@ideasonboard.com>","Subject":"Re: [PATCH v1 27/33] libcamera: rkisp1: Handle requested orientation\n\tusing dewarper","From":"Stefan Klug ","To":"Dan Scally ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 20 Oct 2025 15:36:46 +0200","Message-ID":"<176096740668.336133.2219066694299853548@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36364,"web_url":"https://patchwork.libcamera.org/comment/36364/","msgid":"<6b6a7d6c-8c14-4d8a-bff7-0a708c69c294@ideasonboard.com>","date":"2025-10-20T13:49:45","subject":"Re: [PATCH v1 27/33] libcamera: rkisp1: Handle requested orientation\n\tusing dewarper","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Stefan\n\nOn 20/10/2025 14:36, Stefan Klug wrote:\n> Hi Dan,\n> \n> Thank you for the review.\n> \n> Quoting Dan Scally (2025-10-16 16:16:34)\n>> Hi Stefan\n>>\n>> On 30/09/2025 13:26, Stefan Klug wrote:\n>>> When the dewarper is present it can handle arbitrary orientations\n>>> specified in the requested camera configuration. In that case handle all\n>>> transformations inside the dewarper (even if the sensor supports some of\n>>> them) because that makes it easier to handle coordinates for lens\n>>> dewarping inside the dewarper.\n>>>\n>>> Signed-off-by: Stefan Klug \n>>> ---\n>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 79 +++++++++++++++++++-----\n>>> 1 file changed, 63 insertions(+), 16 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> index e8902ce66b70..1046930857e1 100644\n>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> @@ -550,11 +550,6 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>>> status = Adjusted;\n>>> }\n>>> \n>>> - Orientation requestedOrientation = orientation;\n>>> - combinedTransform_ = data_->sensor_->computeTransform(&orientation);\n>>> - if (orientation != requestedOrientation)\n>>> - status = Adjusted;\n>>> -\n>>> /*\n>>> * Simultaneous capture of raw and processed streams isn't possible. If\n>>> * there is any raw stream, cap the number of streams to one.\n>>> @@ -570,6 +565,12 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>>> }\n>>> }\n>>> \n>>> + /*\n>>> + * If the dewarper supports orientation adjustments, apply that completely\n>>> + * there. Even if the sensor supports flips, it is beneficial to do that\n>>> + * in the dewarper so that lens dewarping happens on the unflipped image\n>>> + */\n>>> + bool transposeAfterIsp = false;\n>>> bool useDewarper = false;\n>>> if (pipe->dewarper_) {\n>>> /*\n>>> @@ -580,6 +581,14 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>>> PixelFormatInfo::ColourEncodingRAW;\n>>> if (!isRaw)\n>>> useDewarper = true;\n>>> + combinedTransform_ = orientation / data_->sensor_->mountingOrientation();\n>>> + if (!!(combinedTransform_ & Transform::Transpose))\n>>> + transposeAfterIsp = true;\n>>> + } else {\n>>> + Orientation requestedOrientation = orientation;\n>>> + combinedTransform_ = data_->sensor_->computeTransform(&orientation);\n>>> + if (orientation != requestedOrientation)\n>>> + status = Adjusted;\n>>> }\n>>> \n>>> /*\n>>> @@ -610,6 +619,9 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>>> (expectedStatus == Valid && ret == Adjusted))\n>>> return false;\n>>> \n>>> + if (transposeAfterIsp)\n>>> + tryCfg.size.transpose();\n>>> +\n>>> if (useDewarper) {\n>>> /*\n>>> * The dewarper output is independent of the ISP path.\n>>> @@ -694,6 +706,9 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>>> maxSize = std::max(maxSize, cfg.size);\n>>> }\n>>> \n>>> + if (transposeAfterIsp)\n>>> + maxSize.transpose();\n>>> +\n>>> std::vector mbusCodes;\n>>> \n>>> if (rawFormat.isValid()) {\n>>> @@ -739,6 +754,22 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n>>> if (roles.empty())\n>>> return config;\n>>> \n>>> + Transform transform = Transform::Identity;\n>>> + Size previewSize = kRkISP1PreviewSize;\n>>> + bool transposeAfterIsp = false;\n>>> + if (data->canUseDewarper_) {\n>>> + transform = Orientation::Rotate0 / data->sensor_->mountingOrientation();\n>>> + if (!!(transform & Transform::Transpose))\n>>> + transposeAfterIsp = true;\n>>> + }\n>>> +\n>>> + /*\n>>> + * In case of a transpose transform we need to create a path for the\n>>> + * transposed size.\n>>> + */\n>>> + if (transposeAfterIsp)\n>>> + previewSize.transpose();\n>>> +\n>>> /*\n>>> * As the ISP can't output different color spaces for the main and self\n>>> * path, pick a sensible default color space based on the role of the\n>>> @@ -767,7 +798,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n>>> if (!colorSpace)\n>>> colorSpace = ColorSpace::Sycc;\n>>> \n>>> - size = kRkISP1PreviewSize;\n>>> + size = previewSize;\n>>> break;\n>>> \n>>> case StreamRole::VideoRecording:\n>>> @@ -775,7 +806,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n>>> if (!colorSpace)\n>>> colorSpace = ColorSpace::Rec709;\n>>> \n>>> - size = kRkISP1PreviewSize;\n>>> + size = previewSize;\n>>> break;\n>>> \n>>> case StreamRole::Raw:\n>>> @@ -817,6 +848,9 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n>>> if (!cfg.pixelFormat.isValid())\n>>> return nullptr;\n>>> \n>>> + if (transposeAfterIsp && role != StreamRole::Raw)\n>>> + cfg.size.transpose();\n>>> +\n>>> cfg.colorSpace = colorSpace;\n>>> cfg.bufferCount = kRkISP1MinBufferCount;\n>>> config->addConfiguration(cfg);\n>>> @@ -839,6 +873,19 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>>> if (ret)\n>>> return ret;\n>>> \n>>> + const PixelFormat &streamFormat = config->at(0).pixelFormat;\n>>> + const PixelFormatInfo &info = PixelFormatInfo::info(streamFormat);\n>>> + isRaw_ = info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n>>> + data->usesDewarper_ = data->canUseDewarper_ && !isRaw_;\n>>\n>> This change is I think the root cause of a segfault I get using this series in ::updateControls().\n>> We check for data->usesDewarper_ which until this patch is set (below) with:\n>>\n>> > - data->usesDewarper_ = dewarper_ && !isRaw_;\n>>\n>> but following this change usesDewarper_ can be set to true even if PipelineHandlerRkISP1::dewarper_\n>> is null.\n> \n> I think I'm missing something here. usesDewarper_ can only be true is\n> canUseDewarper_ is true. And canUseDewarper_ is only set to true in\n> loadTuningFile() which contains a guard\n> \n> if (!pipe()->dewarper_)\n> \t/* Nothing to do without dewarper */\n> \treturn 0;\n> \n> So I fail to understand how canUseDewarper_ is true in your case.\n\nHmmmmm you're right. OK, back to the debugger I go. Sorry about the noise.\n> As a side note: I had some corner cases especially when sensor orientations != 0 are\n> involved where the generated/validated configurations couldn't be\n> applied to a camera. Might that be the case on your side?\n\nI don't think so; I get a segfault in ::updateControls() and verified that dewarper_ was null...I \nthought I had seen why but I will look again and come back to you.\n\nThanks!\nDan\n\n> \n> Best regards,\n> Stefan\n> \n>>\n>> Perhaps:\n>>\n>> data->usesDewarper_ = dewarper_ && data->canUseDewarper_ && !isRaw_;\n>>\n>> ? Or else a similar check when data->canUseDewarper_ is initialised.\n>>\n>> Thanks\n>> Dan\n>>\n>>> +\n>>> + Transform transform = config->combinedTransform();\n>>> + bool transposeAfterIsp = false;\n>>> + if (data->usesDewarper_) {\n>>> + if (!!(transform & Transform::Transpose))\n>>> + transposeAfterIsp = true;\n>>> + transform = Transform::Identity;\n>>> + }\n>>> +\n>>> /*\n>>> * Configure the format on the sensor output and propagate it through\n>>> * the pipeline.\n>>> @@ -848,10 +895,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>>> \n>>> if (config->sensorConfig)\n>>> ret = sensor->applyConfiguration(*config->sensorConfig,\n>>> - config->combinedTransform(),\n>>> + transform,\n>>> &format);\n>>> else\n>>> - ret = sensor->setFormat(&format, config->combinedTransform());\n>>> + ret = sensor->setFormat(&format, transform);\n>>> \n>>> if (ret < 0)\n>>> return ret;\n>>> @@ -878,10 +925,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>>> << \" crop \" << inputCrop;\n>>> \n>>> Rectangle outputCrop = inputCrop;\n>>> - const PixelFormat &streamFormat = config->at(0).pixelFormat;\n>>> - const PixelFormatInfo &info = PixelFormatInfo::info(streamFormat);\n>>> - isRaw_ = info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n>>> - data->usesDewarper_ = dewarper_ && !isRaw_;\n>>> \n>>> /* YUYV8_2X8 is required on the ISP source path pad for YUV output. */\n>>> if (!isRaw_)\n>>> @@ -973,15 +1016,19 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>>> inputCrop, sensorInfo.analogCrop);\n>>> auto &vertexMap = dewarper_->vertexMap(cfg.stream());\n>>> vertexMap.setSensorCrop(sensorCrop);\n>>> + vertexMap.setTransform(config->combinedTransform());\n>>> data->properties_.set(properties::ScalerCropMaximum, sensorCrop);\n>>> \n>>> /*\n>>> * Apply a default sensor crop that keeps the\n>>> * aspect ratio.\n>>> */\n>>> - Size crop = format.size.boundedToAspectRatio(cfg.size);\n>>> - vertexMap.setScalerCrop(crop.centeredTo(\n>>> - Rectangle(format.size).center()));\n>>> + Size size = cfg.size;\n>>> + if (transposeAfterIsp)\n>>> + size.transpose();\n>>> + size = sensorCrop.size().boundedToAspectRatio(size);\n>>> + vertexMap.setScalerCrop(\n>>> + size.centeredTo(sensorCrop.center()));\n>>> }\n>>> \n>>> ret = mainPath_.configure(ispCfg, format);\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 DFC7AC3259\n\tfor ;\n\tMon, 20 Oct 2025 13:49:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3404360758;\n\tMon, 20 Oct 2025 15:49:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B39506074A\n\tfor ;\n\tMon, 20 Oct 2025 15:49:48 +0200 (CEST)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E60566F9;\n\tMon, 20 Oct 2025 15:48:05 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"q4Xqd84G\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1760968086;\n\tbh=O2sWZr37Om4ZLhrOGHYDcmQiu/dqwnE2DdQQ2iBFBqQ=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=q4Xqd84GrRAzC4mxK2QOACiluuUrnjTatHaMDUOAy2icayKwUt3EU5+uVEiYedT6y\n\tTDUzk+uVMntukeDwM3rT8Lv6cTpWOv/ODcApQ9WkCWku90QqbtwDKOmrKyws+NqzUB\n\tgXP0AtLFDALQ9DYKHeM4alZgd6f8PJRVsq7xhrHw=","Message-ID":"<6b6a7d6c-8c14-4d8a-bff7-0a708c69c294@ideasonboard.com>","Date":"Mon, 20 Oct 2025 14:49:45 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 27/33] libcamera: rkisp1: Handle requested orientation\n\tusing dewarper","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250930122726.1837524-1-stefan.klug@ideasonboard.com>\n\t<20250930122726.1837524-28-stefan.klug@ideasonboard.com>\n\t<66323c5e-1c76-4d05-adf3-eee52d01aa12@ideasonboard.com>\n\t<176096740668.336133.2219066694299853548@localhost>","Content-Language":"en-US","From":"Dan Scally ","In-Reply-To":"<176096740668.336133.2219066694299853548@localhost>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]