[{"id":26808,"web_url":"https://patchwork.libcamera.org/comment/26808/","msgid":"<20230403081751.wby3eihzgty4m5rs@uno.localdomain>","date":"2023-04-03T08:17:51","subject":"Re: [libcamera-devel] [PATCH] pipeline: simple: set controlInfo_ to\n\tsensor_->controls()","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Mattijs\n\nOn Fri, Mar 31, 2023 at 05:09:58PM +0200, Mattijs Korpershoek via libcamera-devel wrote:\n> Each pipeline has a ControlInfoMap to list the set of controls supported\n> by the camera.\n>\n> ControlInfoMap() default constructor -used by simple pipeline-,\n> initializes its idmap_ member to nullptr.\n>\n> Other methods such as ControlInfoMap::find() don't check for idmap_'s\n> validity so the following code (from find()) crashes:\n>\n> auto iter = idmap_->find(id);\n> auto iter = nullptr->find(id);\n>\n> Fix this by assigning the sensor's control info map.\n>\n\nEh, I wish it was that simple :)\n\nThe issue of missing controls for Simple is plaguing it since a long\ntime and it's a rather complex one.\n\nFirst of all, you're here registering controls straight as they come\nfrom the v4l2-subdev v4l2-controls interface, which means you're\nexposing to applications the V4L2 controls numeric identifiers.\nLibcamera aims instead to expose to applications its own defined\ncontrols (control_ids.yaml) for sake of applications portability\nacross platforms.\n\nThis also requires that the controls' valid ranges and values should\nbe abstracted away from the platform details, and it's up to the\npipeline-handler/IPA to re-scale them in values that make sense for\nthe hardware in use.\n\nFor platforms with an ISP, most controls come from the IPA module\nwhich register both controls to drive the algorithms (enable/disable,\nmode of operations etc) and controls to manually tune the parameters\nof the image processing pipeline. When these controls directly apply\nto the sensor (as ExposureTime and AnalogueGain, in example) you can\nsee how the CameraSensorHelper hierarchy helps translating the\ncontrols to sensor-consumable values in the IPA modules.\n\nThe closest example to Simple we have is the uvcvideo pipeline, where\nyou can see how controls from the v4l2 interface are translated to\nlibcamera generic controls.\nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp#n582\n\nFor Simple there have been a few attempts in the past and recently\nthere was desire to re-tackle the issue for supporting auto-focus on\nthe Pinephone platform but I've lost track of the development on the\nfron (cc Adam and Rafael)\nhttps://patchwork.libcamera.org/patch/15078/#21709\n\n\n> Signed-off-by: Mattijs Korpershoek \n> ---\n> Alternatively, we could null check everywhere in controls.cpp or\n> get rid of the default constructor for ControlInfoMap.\n> ---\n> src/libcamera/pipeline/simple/simple.cpp | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 2423ec10c878..4db3dc2812b0 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -537,6 +537,7 @@ int SimpleCameraData::init()\n> \t}\n>\n> \tproperties_ = sensor_->properties();\n> +\tcontrolInfo_ = sensor_->controls();\n>\n> \treturn 0;\n> }\n>\n> ---\n> base-commit: ac7511dc4c594f567ddff27ccc02c30bf6c00bfd\n> change-id: 20230331-simple-controls-fd92853c7cff\n>\n> Best regards,\n> --\n> Mattijs Korpershoek \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 3B9C1C326B\n\tfor ;\n\tMon, 3 Apr 2023 08:17:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A36F862722;\n\tMon, 3 Apr 2023 10:17:56 +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 75CD861EC6\n\tfor ;\n\tMon, 3 Apr 2023 10:17:55 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 001B76CF;\n\tMon, 3 Apr 2023 10:17:54 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680509876;\n\tbh=ilESng6TFHGen768AHnweNvo6tXOG2Mi92Pi+gYUuRw=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=uNviSh0vDlhWZFAAC9YkxaVHVhymCatclm7tSwV10JN9Cf07j2TXSHiEbs2UeMbJC\n\t1I5lehIqBPBrjgKXuJlcjaaMpNfM5q54BoTLg6ev24J87fGg01rVapbtnQezRX65np\n\tRIFJv0oLiJ+oIdxzObg35SQ65ILkPRQ5u8SL3EJ/FqxzR8NO71ICL1bVKGgMVlS4Kg\n\tafNBzALZrdBpuqktnZi5h3ziOnzFq2ROR3wGx7wd5o2XGR/5ToFZ+QJJZCVwyw6ZJ2\n\t3UKcEUT/VSAe+GtYRoGY63Z2NzdLnOkxgzjRHYYVUJkrleNrk5zA49n5CACy91fssM\n\tDO1bJ1dJFGPuQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1680509875;\n\tbh=ilESng6TFHGen768AHnweNvo6tXOG2Mi92Pi+gYUuRw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QDTnS8nC1dajfCG/YlPDLRhYpx+R3vLgaZ6cVGzLwXYC33lXbmojw+iJYBzejy765\n\tSOr1Tm1WmgygWU3zQ5IEuxKBprNC5c7/zT3wiI/KV+qarSHtBKMxDc360jqxKHFIXH\n\ttYHk5pUmsb8IHm+E9diKipgARZLqphS3plXsAJsg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"QDTnS8nC\"; dkim-atps=neutral","Date":"Mon, 3 Apr 2023 10:17:51 +0200","To":"Mattijs Korpershoek ","Message-ID":"<20230403081751.wby3eihzgty4m5rs@uno.localdomain>","References":"<20230331-simple-controls-v1-1-f7b8327d2c26@baylibre.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230331-simple-controls-v1-1-f7b8327d2c26@baylibre.com>","Subject":"Re: [libcamera-devel] [PATCH] pipeline: simple: set controlInfo_ to\n\tsensor_->controls()","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, adam@piggz.co.uk","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26833,"web_url":"https://patchwork.libcamera.org/comment/26833/","msgid":"<87v8ic9fp6.fsf@baylibre.com>","date":"2023-04-04T09:09:25","subject":"Re: [libcamera-devel] [PATCH] pipeline: simple: set controlInfo_ to\n\tsensor_->controls()","submitter":{"id":153,"url":"https://patchwork.libcamera.org/api/people/153/","name":"Mattijs Korpershoek","email":"mkorpershoek@baylibre.com"},"content":"Hi Jacopo,\n\nThank you for your review and detailed reponse.\n\nOn lun., avril 03, 2023 at 10:17, Jacopo Mondi wrote:\n\n> Hi Mattijs\n>\n> On Fri, Mar 31, 2023 at 05:09:58PM +0200, Mattijs Korpershoek via libcamera-devel wrote:\n>> Each pipeline has a ControlInfoMap to list the set of controls supported\n>> by the camera.\n>>\n>> ControlInfoMap() default constructor -used by simple pipeline-,\n>> initializes its idmap_ member to nullptr.\n>>\n>> Other methods such as ControlInfoMap::find() don't check for idmap_'s\n>> validity so the following code (from find()) crashes:\n>>\n>> auto iter = idmap_->find(id);\n>> auto iter = nullptr->find(id);\n>>\n>> Fix this by assigning the sensor's control info map.\n>>\n>\n> Eh, I wish it was that simple :)\n\nHmm, it fixed my nullptr crash, so I figured, it's \"good enough\". Seems\nI was terribly wrong :)\n\n>\n> The issue of missing controls for Simple is plaguing it since a long\n> time and it's a rather complex one.\n>\n> First of all, you're here registering controls straight as they come\n> from the v4l2-subdev v4l2-controls interface, which means you're\n> exposing to applications the V4L2 controls numeric identifiers.\n> Libcamera aims instead to expose to applications its own defined\n> controls (control_ids.yaml) for sake of applications portability\n> across platforms.\n>\n> This also requires that the controls' valid ranges and values should\n> be abstracted away from the platform details, and it's up to the\n> pipeline-handler/IPA to re-scale them in values that make sense for\n> the hardware in use.\n>\n> For platforms with an ISP, most controls come from the IPA module\n> which register both controls to drive the algorithms (enable/disable,\n> mode of operations etc) and controls to manually tune the parameters\n> of the image processing pipeline. When these controls directly apply\n> to the sensor (as ExposureTime and AnalogueGain, in example) you can\n> see how the CameraSensorHelper hierarchy helps translating the\n> controls to sensor-consumable values in the IPA modules.\n>\n> The closest example to Simple we have is the uvcvideo pipeline, where\n> you can see how controls from the v4l2 interface are translated to\n> libcamera generic controls.\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp#n582\n>\n> For Simple there have been a few attempts in the past and recently\n> there was desire to re-tackle the issue for supporting auto-focus on\n> the Pinephone platform but I've lost track of the development on the\n> fron (cc Adam and Rafael)\n> https://patchwork.libcamera.org/patch/15078/#21709\n\nUnderstood. For the time being, I don't feel confident enough to\nmake the controls in Simple pipeline behave as they should. Especially\nif others are already working on this.\n\nI do want to fix the nullptr dereference I'm facing when using Simple\nPipeline along with the provided Android camera HAL.\n\nHere is a stacktrace (from Android) when this happens:\nhttp://codepad.org/CiLLcPNW\n\nIs adding some null checks in ControlInfoMap::find() an acceptable\nsolution?\n\n>\n>\n>> Signed-off-by: Mattijs Korpershoek \n>> ---\n>> Alternatively, we could null check everywhere in controls.cpp or\n>> get rid of the default constructor for ControlInfoMap.\n>> ---\n>> src/libcamera/pipeline/simple/simple.cpp | 1 +\n>> 1 file changed, 1 insertion(+)\n>>\n>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> index 2423ec10c878..4db3dc2812b0 100644\n>> --- a/src/libcamera/pipeline/simple/simple.cpp\n>> +++ b/src/libcamera/pipeline/simple/simple.cpp\n>> @@ -537,6 +537,7 @@ int SimpleCameraData::init()\n>> \t}\n>>\n>> \tproperties_ = sensor_->properties();\n>> +\tcontrolInfo_ = sensor_->controls();\n>>\n>> \treturn 0;\n>> }\n>>\n>> ---\n>> base-commit: ac7511dc4c594f567ddff27ccc02c30bf6c00bfd\n>> change-id: 20230331-simple-controls-fd92853c7cff\n>>\n>> Best regards,\n>> --\n>> Mattijs Korpershoek \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 5C782C326B\n\tfor ;\n\tTue, 4 Apr 2023 09:09:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 83A0762724;\n\tTue, 4 Apr 2023 11:09:28 +0200 (CEST)","from mail-wr1-x42d.google.com (mail-wr1-x42d.google.com\n\t[IPv6:2a00:1450:4864:20::42d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C92E8626DA\n\tfor ;\n\tTue, 4 Apr 2023 11:09:26 +0200 (CEST)","by mail-wr1-x42d.google.com with SMTP id t4so26760362wra.7\n\tfor ;\n\tTue, 04 Apr 2023 02:09:26 -0700 (PDT)","from localhost ([82.66.159.240]) by smtp.gmail.com with ESMTPSA id\n\tn5-20020adffe05000000b002d97529b3bbsm11797730wrr.96.2023.04.04.02.09.25\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 04 Apr 2023 02:09:26 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680599368;\n\tbh=UjlhCrS/cVeka9Mg2AQhFmr1QgprylvZ9/3MJmm2rM0=;\n\th=To:In-Reply-To:References:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=yOp+eEoQ1qlKmzUJVLNXx87zNCf9jrk65RRuxMtN10MguD+kcke3ZA6PVtqJEtZZg\n\t4tooJda8xHX5O9DWV631DxENxlbCwRyhXCDXUddzfwMwcVvDN6tyMcYYknVZgUDnpO\n\tP4/BjOFhFh+cZ9Cx7xEsbbph4zVGDiYp7FLSssRxbwEeoUu7+7jBzpMq+/L+F3fHw2\n\tC2SwXRC0bBId9ZTrJNSBjPitRB8lPPrGqAAvULwn125U98fgkoiXbe0+DFubowsBin\n\t6DZ5N2XgBEoJGqLwgSg5h5jHvyvTeGfbZqKGs3cKveQsWSm4Y9a0wg3wHzUavw4/B6\n\tkWeyIHLbc+rmA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=baylibre-com.20210112.gappssmtp.com; s=20210112; t=1680599366;\n\th=mime-version:message-id:date:references:in-reply-to:subject:cc:to\n\t:from:from:to:cc:subject:date:message-id:reply-to;\n\tbh=XUq1MZUkKd5Vj36S5a8szW/t/V25GzERnmMepcXs2dM=;\n\tb=aYO3HEZ46NYU0TtHjCjQjE5IRmh697BZ+RoP8dOMvJgGvqtJh/NT5Ro3nwcCRcZSok\n\tcy2DaSs9TvwVpZ+/xiWy21iKL1QA4UIGagajZBfVfmLPL3ZDynFxQlSTaQRA9O7SzP+Y\n\tAXSiViLSSaRhfUqKcRgOFKfMiA2nvBc/sMBdFo7UtVNf4IzajEBSz2Sj0zfrVdSo99RI\n\tRKB7tlcoyLx+Lx4QYIoxukRIQkp8gcjVBXsrxIMlTeq+d5bFSFSOahWkrr9NO/RsQVSy\n\tgID/XfmL66B1Cb9ECo6UXYCxyrHMW6GwF4Ny3mJxyxGcgi+4qV62FN2ZR0kDDo76nffg\n\tHk+Q=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=baylibre-com.20210112.gappssmtp.com\n\theader.i=@baylibre-com.20210112.gappssmtp.com header.b=\"aYO3HEZ4\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1680599366;\n\th=mime-version:message-id:date:references:in-reply-to:subject:cc:to\n\t:from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; \n\tbh=XUq1MZUkKd5Vj36S5a8szW/t/V25GzERnmMepcXs2dM=;\n\tb=3TgeHZpl49a26fKRe9jeB2rAf0L3RxAao94tcmnFvvIeNXizdo7MRQT9yafQ/XmG8v\n\tZ1bGEmr2Cb6EuUUPIfl76afJXJJa2F+VhugjkUWKhuaBVaRdGvGV4t9RqKTR1BJ4VS/e\n\tXctR5unDXw3U+2Z9E0pEpwfOcxGvFTcdZ8CC2UwGaD/8zWFoMHjLPGC0q98JpWu5cCUg\n\taCmi7SINa/2gVQpmVK7tisemCbD9ORUx3QzLOa3UsuQoPVG9eFd1HoRmBcswgrZnilz5\n\tPXaYYQlqfZI1OFVmYoc9pzK14avGTcs19ytU6z+DlU4DSRjfGuVj3tXzFSJdaEMEiDmN\n\tfUdQ==","X-Gm-Message-State":"AAQBX9cMBz6JBiVVCwNvbv/U56z0xI4y3lr4WLhOBYtL14qzNnLhd/rv\n\tQZgHM3NA2QpqadVU06/e12m8KQ==","X-Google-Smtp-Source":"AKy350acutVJqiEZDkWTwyvw8yzSSTLwhZPQyBI63swctwm4+oOspWAGUJNohAXamfLa8zL/g5s+iw==","X-Received":"by 2002:adf:f705:0:b0:2dc:ea38:d9fa with SMTP id\n\tr5-20020adff705000000b002dcea38d9famr871778wrp.32.1680599366377; \n\tTue, 04 Apr 2023 02:09:26 -0700 (PDT)","To":"Jacopo Mondi ","In-Reply-To":"<20230403081751.wby3eihzgty4m5rs@uno.localdomain>","References":"<20230331-simple-controls-v1-1-f7b8327d2c26@baylibre.com>\n\t<20230403081751.wby3eihzgty4m5rs@uno.localdomain>","Date":"Tue, 04 Apr 2023 11:09:25 +0200","Message-ID":"<87v8ic9fp6.fsf@baylibre.com>","MIME-Version":"1.0","Content-Type":"text/plain","Subject":"Re: [libcamera-devel] [PATCH] pipeline: simple: set controlInfo_ to\n\tsensor_->controls()","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","From":"Mattijs Korpershoek via libcamera-devel\n\t","Reply-To":"Mattijs Korpershoek ","Cc":"libcamera-devel@lists.libcamera.org, adam@piggz.co.uk","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26840,"web_url":"https://patchwork.libcamera.org/comment/26840/","msgid":"<20230404104319.qiaazsty5lr2tlsu@uno.localdomain>","date":"2023-04-04T10:43:19","subject":"Re: [libcamera-devel] [PATCH] pipeline: simple: set controlInfo_ to\n\tsensor_->controls()","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Mattijs\n\nOn Tue, Apr 04, 2023 at 11:09:25AM +0200, Mattijs Korpershoek wrote:\n> Hi Jacopo,\n>\n> Thank you for your review and detailed reponse.\n>\n> On lun., avril 03, 2023 at 10:17, Jacopo Mondi wrote:\n>\n> > Hi Mattijs\n> >\n> > On Fri, Mar 31, 2023 at 05:09:58PM +0200, Mattijs Korpershoek via libcamera-devel wrote:\n> >> Each pipeline has a ControlInfoMap to list the set of controls supported\n> >> by the camera.\n> >>\n> >> ControlInfoMap() default constructor -used by simple pipeline-,\n> >> initializes its idmap_ member to nullptr.\n> >>\n> >> Other methods such as ControlInfoMap::find() don't check for idmap_'s\n> >> validity so the following code (from find()) crashes:\n> >>\n> >> auto iter = idmap_->find(id);\n> >> auto iter = nullptr->find(id);\n> >>\n> >> Fix this by assigning the sensor's control info map.\n> >>\n> >\n> > Eh, I wish it was that simple :)\n>\n> Hmm, it fixed my nullptr crash, so I figured, it's \"good enough\". Seems\n> I was terribly wrong :)\n>\n\nDon't worry, many people got fooled by this, me included :)\n\n> >\n> > The issue of missing controls for Simple is plaguing it since a long\n> > time and it's a rather complex one.\n> >\n> > First of all, you're here registering controls straight as they come\n> > from the v4l2-subdev v4l2-controls interface, which means you're\n> > exposing to applications the V4L2 controls numeric identifiers.\n> > Libcamera aims instead to expose to applications its own defined\n> > controls (control_ids.yaml) for sake of applications portability\n> > across platforms.\n> >\n> > This also requires that the controls' valid ranges and values should\n> > be abstracted away from the platform details, and it's up to the\n> > pipeline-handler/IPA to re-scale them in values that make sense for\n> > the hardware in use.\n> >\n> > For platforms with an ISP, most controls come from the IPA module\n> > which register both controls to drive the algorithms (enable/disable,\n> > mode of operations etc) and controls to manually tune the parameters\n> > of the image processing pipeline. When these controls directly apply\n> > to the sensor (as ExposureTime and AnalogueGain, in example) you can\n> > see how the CameraSensorHelper hierarchy helps translating the\n> > controls to sensor-consumable values in the IPA modules.\n> >\n> > The closest example to Simple we have is the uvcvideo pipeline, where\n> > you can see how controls from the v4l2 interface are translated to\n> > libcamera generic controls.\n> > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp#n582\n> >\n> > For Simple there have been a few attempts in the past and recently\n> > there was desire to re-tackle the issue for supporting auto-focus on\n> > the Pinephone platform but I've lost track of the development on the\n> > fron (cc Adam and Rafael)\n> > https://patchwork.libcamera.org/patch/15078/#21709\n>\n> Understood. For the time being, I don't feel confident enough to\n> make the controls in Simple pipeline behave as they should. Especially\n> if others are already working on this.\n>\n> I do want to fix the nullptr dereference I'm facing when using Simple\n> Pipeline along with the provided Android camera HAL.\n>\n> Here is a stacktrace (from Android) when this happens:\n> http://codepad.org/CiLLcPNW\n>\n\nThis puzzles me a bit...\n\nIndeed the camera controls for Simple are not populated, but I wasn't\nexpecting a segfault, but rather an assertion failure.\n\nThe call trace points to\n\nint CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n\nwhere the request's controls are populated translating Android\ncontrols to libcamera's ones.\n\nThe request list is populated with the Camera's validator when\nrequest is created\n\nRequest::Request(Camera *camera, uint64_t cookie)\n\t: Extensible(std::make_unique(camera)),\n\t cookie_(cookie), status_(RequestPending)\n{\n\tcontrols_ = new ControlList(controls::controls,\n\t\t\t\t camera->_d()->validator());\n\nI presume the call to\n\n\tcontrols.set(controls::ScalerCrop, cropRegion);\n\nin CameraDevice goes through\n\n\ttemplate\n\tvoid set(const Control &ctrl, const V &value)\n\t{\n\t\tControlValue *val = find(ctrl.id());\n\t\tif (!val)\n\t\t\treturn;\n\n\t\tval->set(value);\n\t}\n\nwhich calls\n\nControlValue *ControlList::find(unsigned int id)\n{\n\tif (validator_ && !validator_->validate(id)) {\n\nand finally\n\nbool CameraControlValidator::validate(unsigned int id) const\n{\n\tconst ControlInfoMap &controls = camera_->controls();\n\treturn controls.find(id) != controls.end();\n}\n\nwith the offendin call being\n\n\treturn controls.find(id) != controls.end();\n\nIf you look at the implementation of\n\nControlInfoMap::iterator ControlInfoMap::find(unsigned int id)\n{\n\tauto iter = idmap_->find(id);\n\tif (iter == idmap_->end())\n\t\treturn end();\n\n\treturn find(iter->second);\n}\n\nyou'll see that the default constructor for ControlInfoMap doesn't\nintialized idmap_ which is initialized at class declaration time as\n\n\tconst ControlIdMap *idmap_ = nullptr;\n\nTo sum it up, a Camera is constructed with an unsafe controlInfo_\n\n\n> Is adding some null checks in ControlInfoMap::find() an acceptable\n> solution?\n>\n> >\n> >\n> >> Signed-off-by: Mattijs Korpershoek \n> >> ---\n> >> Alternatively, we could null check everywhere in controls.cpp or\n> >> get rid of the default constructor for ControlInfoMap.\n> >> ---\n\nAnd if I would have read this, I would have spare the above. Well,\nwe're on the same page at least :)\n\nI tried rather hard to make idmap_ an instance intead of a pointer.\n\nThis however has two consequences:\n1) operator= cannot be default-generated, and in the implementation I\n tried I was not able to correctly copy the Map in. I'm afraid the only\n solution here would be to use composition instead of having\n ControlInfoMap inherit from unordered_map\n\n2) ControlInfo might needs to reference the global controls::controls id\n map, which might be expensive to copy in if idmap_ is now made an\n instance. This is also relevant for control serialization, which\n happens on IPC borders and is in a rather hot path\n\nI'm afraid we'll have to protect the ControlInfoMap class methods with\nan\n if (!idmap_)\n return end();\n\nWhat do you think ?\n\n\n> >> src/libcamera/pipeline/simple/simple.cpp | 1 +\n> >> 1 file changed, 1 insertion(+)\n> >>\n> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> >> index 2423ec10c878..4db3dc2812b0 100644\n> >> --- a/src/libcamera/pipeline/simple/simple.cpp\n> >> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> >> @@ -537,6 +537,7 @@ int SimpleCameraData::init()\n> >> \t}\n> >>\n> >> \tproperties_ = sensor_->properties();\n> >> +\tcontrolInfo_ = sensor_->controls();\n> >>\n> >> \treturn 0;\n> >> }\n> >>\n> >> ---\n> >> base-commit: ac7511dc4c594f567ddff27ccc02c30bf6c00bfd\n> >> change-id: 20230331-simple-controls-fd92853c7cff\n> >>\n> >> Best regards,\n> >> --\n> >> Mattijs Korpershoek \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 D1547C0F2A\n\tfor ;\n\tTue, 4 Apr 2023 10:43:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2535262724;\n\tTue, 4 Apr 2023 12:43:23 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 37298626DB\n\tfor ;\n\tTue, 4 Apr 2023 12:43:22 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BDEB57F8;\n\tTue, 4 Apr 2023 12:43:21 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680605003;\n\tbh=5yVvytQSsjBBvuLbbmb0XLcGXwc0hrAHCnJ5V5t2Dqw=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=yXCiusSECTvZaVA2Z6EYCJMKxETT630Ud8YjbReclHSNLdWsjOIixDcCkbkiUFTkw\n\tF1ap0Hx8QMS7i4fjqLQkayLr2DrKMTxCAKfKHa91kMkheq5ZVem0n4gXdhDrXdx7PP\n\thFvYF0rpmjPVNf6EdL0OHaWrd/ZLNWDms0wsWtgvobSOFJj91XGXMDsDePmLgVBSAc\n\tOZ49OwJfQ3a2YS3FXrBXljZ9Gp97j7yGeeOC/FVYCN86ZqUdPJicV67jGGBjD9bQzj\n\th6DjxlewaVz3g5siqc7kL9go5xC+QlDChQOR1eJtJ/T/5sa7f25iqzozV4h91xlCiC\n\tnfYzJ6jEczNvg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1680605001;\n\tbh=5yVvytQSsjBBvuLbbmb0XLcGXwc0hrAHCnJ5V5t2Dqw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WX7IBvDKXgDGeIGiG9AL94U6y/dKs6nC07UxYUP5H1HRw8NmIAYPAC7e6V4N8w14J\n\trK9ixRTSez8xlvMGuMEzzVhngBr/tWnf1MOuqydz6IBhZS/2zGXATfDbgpKpvX86XK\n\ta4tz+lNOeVJV69geXPU+kifOZKCbQSdsIDqla8oA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"WX7IBvDK\"; dkim-atps=neutral","Date":"Tue, 4 Apr 2023 12:43:19 +0200","To":"Mattijs Korpershoek ","Message-ID":"<20230404104319.qiaazsty5lr2tlsu@uno.localdomain>","References":"<20230331-simple-controls-v1-1-f7b8327d2c26@baylibre.com>\n\t<20230403081751.wby3eihzgty4m5rs@uno.localdomain>\n\t<87v8ic9fp6.fsf@baylibre.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<87v8ic9fp6.fsf@baylibre.com>","Subject":"Re: [libcamera-devel] [PATCH] pipeline: simple: set controlInfo_ to\n\tsensor_->controls()","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org, adam@piggz.co.uk","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26843,"web_url":"https://patchwork.libcamera.org/comment/26843/","msgid":"<87sfdfam6e.fsf@baylibre.com>","date":"2023-04-04T12:04:09","subject":"Re: [libcamera-devel] [PATCH] pipeline: simple: set controlInfo_ to\n\tsensor_->controls()","submitter":{"id":153,"url":"https://patchwork.libcamera.org/api/people/153/","name":"Mattijs Korpershoek","email":"mkorpershoek@baylibre.com"},"content":"Hi Jacopo,\n\nOn mar., avril 04, 2023 at 12:43, Jacopo Mondi wrote:\n\n> Hi Mattijs\n>\n> On Tue, Apr 04, 2023 at 11:09:25AM +0200, Mattijs Korpershoek wrote:\n>> Hi Jacopo,\n>>\n>> Thank you for your review and detailed reponse.\n>>\n>> On lun., avril 03, 2023 at 10:17, Jacopo Mondi wrote:\n>>\n>> > Hi Mattijs\n>> >\n>> > On Fri, Mar 31, 2023 at 05:09:58PM +0200, Mattijs Korpershoek via libcamera-devel wrote:\n>> >> Each pipeline has a ControlInfoMap to list the set of controls supported\n>> >> by the camera.\n>> >>\n>> >> ControlInfoMap() default constructor -used by simple pipeline-,\n>> >> initializes its idmap_ member to nullptr.\n>> >>\n>> >> Other methods such as ControlInfoMap::find() don't check for idmap_'s\n>> >> validity so the following code (from find()) crashes:\n>> >>\n>> >> auto iter = idmap_->find(id);\n>> >> auto iter = nullptr->find(id);\n>> >>\n>> >> Fix this by assigning the sensor's control info map.\n>> >>\n>> >\n>> > Eh, I wish it was that simple :)\n>>\n>> Hmm, it fixed my nullptr crash, so I figured, it's \"good enough\". Seems\n>> I was terribly wrong :)\n>>\n>\n> Don't worry, many people got fooled by this, me included :)\n>\n>> >\n>> > The issue of missing controls for Simple is plaguing it since a long\n>> > time and it's a rather complex one.\n>> >\n>> > First of all, you're here registering controls straight as they come\n>> > from the v4l2-subdev v4l2-controls interface, which means you're\n>> > exposing to applications the V4L2 controls numeric identifiers.\n>> > Libcamera aims instead to expose to applications its own defined\n>> > controls (control_ids.yaml) for sake of applications portability\n>> > across platforms.\n>> >\n>> > This also requires that the controls' valid ranges and values should\n>> > be abstracted away from the platform details, and it's up to the\n>> > pipeline-handler/IPA to re-scale them in values that make sense for\n>> > the hardware in use.\n>> >\n>> > For platforms with an ISP, most controls come from the IPA module\n>> > which register both controls to drive the algorithms (enable/disable,\n>> > mode of operations etc) and controls to manually tune the parameters\n>> > of the image processing pipeline. When these controls directly apply\n>> > to the sensor (as ExposureTime and AnalogueGain, in example) you can\n>> > see how the CameraSensorHelper hierarchy helps translating the\n>> > controls to sensor-consumable values in the IPA modules.\n>> >\n>> > The closest example to Simple we have is the uvcvideo pipeline, where\n>> > you can see how controls from the v4l2 interface are translated to\n>> > libcamera generic controls.\n>> > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp#n582\n>> >\n>> > For Simple there have been a few attempts in the past and recently\n>> > there was desire to re-tackle the issue for supporting auto-focus on\n>> > the Pinephone platform but I've lost track of the development on the\n>> > fron (cc Adam and Rafael)\n>> > https://patchwork.libcamera.org/patch/15078/#21709\n>>\n>> Understood. For the time being, I don't feel confident enough to\n>> make the controls in Simple pipeline behave as they should. Especially\n>> if others are already working on this.\n>>\n>> I do want to fix the nullptr dereference I'm facing when using Simple\n>> Pipeline along with the provided Android camera HAL.\n>>\n>> Here is a stacktrace (from Android) when this happens:\n>> http://codepad.org/CiLLcPNW\n>>\n>\n> This puzzles me a bit...\n>\n> Indeed the camera controls for Simple are not populated, but I wasn't\n> expecting a segfault, but rather an assertion failure.\n>\n> The call trace points to\n>\n> int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n>\n> where the request's controls are populated translating Android\n> controls to libcamera's ones.\n>\n> The request list is populated with the Camera's validator when\n> request is created\n>\n> Request::Request(Camera *camera, uint64_t cookie)\n> \t: Extensible(std::make_unique(camera)),\n> \t cookie_(cookie), status_(RequestPending)\n> {\n> \tcontrols_ = new ControlList(controls::controls,\n> \t\t\t\t camera->_d()->validator());\n>\n> I presume the call to\n>\n> \tcontrols.set(controls::ScalerCrop, cropRegion);\n>\n> in CameraDevice goes through\n>\n> \ttemplate\n> \tvoid set(const Control &ctrl, const V &value)\n> \t{\n> \t\tControlValue *val = find(ctrl.id());\n> \t\tif (!val)\n> \t\t\treturn;\n>\n> \t\tval->set(value);\n> \t}\n>\n> which calls\n>\n> ControlValue *ControlList::find(unsigned int id)\n> {\n> \tif (validator_ && !validator_->validate(id)) {\n>\n> and finally\n>\n> bool CameraControlValidator::validate(unsigned int id) const\n> {\n> \tconst ControlInfoMap &controls = camera_->controls();\n> \treturn controls.find(id) != controls.end();\n> }\n>\n> with the offendin call being\n>\n> \treturn controls.find(id) != controls.end();\n>\n> If you look at the implementation of\n>\n> ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)\n> {\n> \tauto iter = idmap_->find(id);\n> \tif (iter == idmap_->end())\n> \t\treturn end();\n>\n> \treturn find(iter->second);\n> }\n>\n> you'll see that the default constructor for ControlInfoMap doesn't\n> intialized idmap_ which is initialized at class declaration time as\n>\n> \tconst ControlIdMap *idmap_ = nullptr;\n>\n> To sum it up, a Camera is constructed with an unsafe controlInfo_\n>\n>\n>> Is adding some null checks in ControlInfoMap::find() an acceptable\n>> solution?\n>>\n>> >\n>> >\n>> >> Signed-off-by: Mattijs Korpershoek \n>> >> ---\n>> >> Alternatively, we could null check everywhere in controls.cpp or\n>> >> get rid of the default constructor for ControlInfoMap.\n>> >> ---\n>\n> And if I would have read this, I would have spare the above. Well,\n> we're on the same page at least :)\n\nI agree, we are on the same page. Thank you for the write-up though,\nit's a way better explanation than what I've done in the commit message.\n\n>\n> I tried rather hard to make idmap_ an instance intead of a pointer.\n>\n> This however has two consequences:\n> 1) operator= cannot be default-generated, and in the implementation I\n> tried I was not able to correctly copy the Map in. I'm afraid the only\n> solution here would be to use composition instead of having\n> ControlInfoMap inherit from unordered_map\n>\n> 2) ControlInfo might needs to reference the global controls::controls id\n> map, which might be expensive to copy in if idmap_ is now made an\n> instance. This is also relevant for control serialization, which\n> happens on IPC borders and is in a rather hot path\n>\n> I'm afraid we'll have to protect the ControlInfoMap class methods with\n> an\n> if (!idmap_)\n> return end();\n>\n> What do you think ?\n\nI think this is the most trivial solution and i'm willing to implement\nthis and submit it.\n\nAnother alternative I can think of is not providing a default\nconstructor for ControlInfoMap but that seems difficult/also not\nnecessarily a good idea.\n\nI will test and submit the trivial solution in the coming days. Thanks\nagain for your help.\n\nMattijs\n\n>\n>\n>> >> src/libcamera/pipeline/simple/simple.cpp | 1 +\n>> >> 1 file changed, 1 insertion(+)\n>> >>\n>> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> >> index 2423ec10c878..4db3dc2812b0 100644\n>> >> --- a/src/libcamera/pipeline/simple/simple.cpp\n>> >> +++ b/src/libcamera/pipeline/simple/simple.cpp\n>> >> @@ -537,6 +537,7 @@ int SimpleCameraData::init()\n>> >> \t}\n>> >>\n>> >> \tproperties_ = sensor_->properties();\n>> >> +\tcontrolInfo_ = sensor_->controls();\n>> >>\n>> >> \treturn 0;\n>> >> }\n>> >>\n>> >> ---\n>> >> base-commit: ac7511dc4c594f567ddff27ccc02c30bf6c00bfd\n>> >> change-id: 20230331-simple-controls-fd92853c7cff\n>> >>\n>> >> Best regards,\n>> >> --\n>> >> Mattijs Korpershoek \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 2324EC326B\n\tfor ;\n\tTue, 4 Apr 2023 12:04:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E8296271C;\n\tTue, 4 Apr 2023 14:04:13 +0200 (CEST)","from mail-wr1-x432.google.com (mail-wr1-x432.google.com\n\t[IPv6:2a00:1450:4864:20::432])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A7F59626DB\n\tfor ;\n\tTue, 4 Apr 2023 14:04:11 +0200 (CEST)","by mail-wr1-x432.google.com with SMTP id y14so32568383wrq.4\n\tfor ;\n\tTue, 04 Apr 2023 05:04:11 -0700 (PDT)","from localhost ([82.66.159.240]) by smtp.gmail.com with ESMTPSA id\n\td16-20020a5d4f90000000b002d51d10a3fasm12215220wru.55.2023.04.04.05.04.10\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 04 Apr 2023 05:04:10 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680609853;\n\tbh=K7WeBGZBaOdLo/JCJgI3vR3k+0Ie2UbBHOi3cjK85eM=;\n\th=To:In-Reply-To:References:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=CBA3T7mjpJfMeOjFDp5dtgVqZsnDjINviRwWCfj6ztEiiQLSYq0WZ7WfOfm8xcrwc\n\t5fhoIj1Zp55+3iODZQgjtZu3R6aL3ixigi4Ja3p7xPakuUNIUYMSTt6EpgW2uZHyAP\n\tWDtAfg89z7ckOQfeRh1y9PuiPFW5YJgAeKFJSzNn5VJ3D72q9xA7O40pMdHjeZOAWy\n\t56zDxCMT8c2s5LNfN2WhDwae5K5oXWXFJ1a/To17AeKx2DYfKjru/09tVWGkK5JbIW\n\tkMRclH8W/DuF1UmySBXgKvCEj82ks4bQCj6hwAOm0D7P31nqrMezKAIL2GNMzN2Wkq\n\tURdLXNGqnzWVw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=baylibre-com.20210112.gappssmtp.com; s=20210112; t=1680609851;\n\th=mime-version:message-id:date:references:in-reply-to:subject:cc:to\n\t:from:from:to:cc:subject:date:message-id:reply-to;\n\tbh=KbQFC0B0cQnl37Xvr7I6ipi0TmvJdYqaB6bH+RUdXqo=;\n\tb=wOGdkIYr93JzC0FyAT6UmVr2At6778xE5wrajn9xL3kqNWFF5rYDrbRTqb761MatWw\n\tF7/9kqG2qrV2Gm3THaBQiw3KVYK9ZCp+rXrsDjHZbQV61f/1esQIm+oP6c5bB/aZzuir\n\tX71oOGL/RCWjSKlcD1ZBGrK9HSgNAe3hC7ztwh3LHM7im6N70YW3piUEUvxG8H643ulN\n\thvZ261JDfmDRdj3Ft5Sm040ffAW4EQeWqnHa9va7+hmG8YiH6wXktAYAXvh0X56GXwk7\n\ttcJVod1j599lAa2m7hNJbC2USQuHkjIMjJGKEV9FvFWMjQpk/7PnS4KNbDTFmlgPb+4S\n\tG/TQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=baylibre-com.20210112.gappssmtp.com\n\theader.i=@baylibre-com.20210112.gappssmtp.com header.b=\"wOGdkIYr\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1680609851;\n\th=mime-version:message-id:date:references:in-reply-to:subject:cc:to\n\t:from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; \n\tbh=KbQFC0B0cQnl37Xvr7I6ipi0TmvJdYqaB6bH+RUdXqo=;\n\tb=2tWYfE824fCoOA1lurFr34yGwK3VJLRWruwZy1MRF8opTCPUO/+BddlPCr8Yad47vq\n\t/+36fTi+zUBwfKzvvZXyWd6WT6M83rjtWPghxv44toGxLB33Ytj7oxQp+xdPN+L+GGIo\n\tr3AWUZqoOm13FAauD8/bLBWnkdzfmlv8yg/g6QI6G94pS0FkHfzp8qTUz6SR4wmdKmxx\n\t5rzYywm7cU87uAuOkGUfTKkCd6tiJgX5gVHNxWquIuFiROKcndEU6iwTptSK75/ASlum\n\teC6msF53JH/s6HKNPGVBU0QKZSn/p+dPDHJJdDTOf+W6M0jeVOp8MLmwRf4rCmFVnvn/\n\t04/w==","X-Gm-Message-State":"AAQBX9cT4Eoewtmh+U+TWcjs8LaucZz8vMpt+feBcBZ8u7A+kCjCWhjw\n\tA8BEISVK3uU3WodSgaSDsM3X1A==","X-Google-Smtp-Source":"AKy350YF0Y8da7euJpKMIVDOa4AnLHHCuTX6AN0VNNUoRcLCltjmMiolxwrHKBw9wfgg3YGqyemCdw==","X-Received":"by 2002:adf:f350:0:b0:2cf:ea5f:6de with SMTP id\n\te16-20020adff350000000b002cfea5f06demr1216938wrp.14.1680609851141; \n\tTue, 04 Apr 2023 05:04:11 -0700 (PDT)","To":"Jacopo Mondi ","In-Reply-To":"<20230404104319.qiaazsty5lr2tlsu@uno.localdomain>","References":"<20230331-simple-controls-v1-1-f7b8327d2c26@baylibre.com>\n\t<20230403081751.wby3eihzgty4m5rs@uno.localdomain>\n\t<87v8ic9fp6.fsf@baylibre.com>\n\t<20230404104319.qiaazsty5lr2tlsu@uno.localdomain>","Date":"Tue, 04 Apr 2023 14:04:09 +0200","Message-ID":"<87sfdfam6e.fsf@baylibre.com>","MIME-Version":"1.0","Content-Type":"text/plain","Subject":"Re: [libcamera-devel] [PATCH] pipeline: simple: set controlInfo_ to\n\tsensor_->controls()","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","From":"Mattijs Korpershoek via libcamera-devel\n\t","Reply-To":"Mattijs Korpershoek ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org, adam@piggz.co.uk","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26844,"web_url":"https://patchwork.libcamera.org/comment/26844/","msgid":"<87jzyrd3u5.fsf@baylibre.com>","date":"2023-04-04T16:12:02","subject":"Re: [libcamera-devel] [PATCH] pipeline: simple: set controlInfo_ to\n\tsensor_->controls()","submitter":{"id":153,"url":"https://patchwork.libcamera.org/api/people/153/","name":"Mattijs Korpershoek","email":"mkorpershoek@baylibre.com"},"content":"On mar., avril 04, 2023 at 14:04, Mattijs Korpershoek wrote:\n\n> Hi Jacopo,\n>\n> On mar., avril 04, 2023 at 12:43, Jacopo Mondi wrote:\n>\n>> Hi Mattijs\n>>\n>> On Tue, Apr 04, 2023 at 11:09:25AM +0200, Mattijs Korpershoek wrote:\n>>> Hi Jacopo,\n>>>\n>>> Thank you for your review and detailed reponse.\n>>>\n>>> On lun., avril 03, 2023 at 10:17, Jacopo Mondi wrote:\n>>>\n>>> > Hi Mattijs\n>>> >\n>>> > On Fri, Mar 31, 2023 at 05:09:58PM +0200, Mattijs Korpershoek via libcamera-devel wrote:\n>>> >> Each pipeline has a ControlInfoMap to list the set of controls supported\n>>> >> by the camera.\n>>> >>\n>>> >> ControlInfoMap() default constructor -used by simple pipeline-,\n>>> >> initializes its idmap_ member to nullptr.\n>>> >>\n>>> >> Other methods such as ControlInfoMap::find() don't check for idmap_'s\n>>> >> validity so the following code (from find()) crashes:\n>>> >>\n>>> >> auto iter = idmap_->find(id);\n>>> >> auto iter = nullptr->find(id);\n>>> >>\n>>> >> Fix this by assigning the sensor's control info map.\n>>> >>\n>>> >\n>>> > Eh, I wish it was that simple :)\n>>>\n>>> Hmm, it fixed my nullptr crash, so I figured, it's \"good enough\". Seems\n>>> I was terribly wrong :)\n>>>\n>>\n>> Don't worry, many people got fooled by this, me included :)\n>>\n>>> >\n>>> > The issue of missing controls for Simple is plaguing it since a long\n>>> > time and it's a rather complex one.\n>>> >\n>>> > First of all, you're here registering controls straight as they come\n>>> > from the v4l2-subdev v4l2-controls interface, which means you're\n>>> > exposing to applications the V4L2 controls numeric identifiers.\n>>> > Libcamera aims instead to expose to applications its own defined\n>>> > controls (control_ids.yaml) for sake of applications portability\n>>> > across platforms.\n>>> >\n>>> > This also requires that the controls' valid ranges and values should\n>>> > be abstracted away from the platform details, and it's up to the\n>>> > pipeline-handler/IPA to re-scale them in values that make sense for\n>>> > the hardware in use.\n>>> >\n>>> > For platforms with an ISP, most controls come from the IPA module\n>>> > which register both controls to drive the algorithms (enable/disable,\n>>> > mode of operations etc) and controls to manually tune the parameters\n>>> > of the image processing pipeline. When these controls directly apply\n>>> > to the sensor (as ExposureTime and AnalogueGain, in example) you can\n>>> > see how the CameraSensorHelper hierarchy helps translating the\n>>> > controls to sensor-consumable values in the IPA modules.\n>>> >\n>>> > The closest example to Simple we have is the uvcvideo pipeline, where\n>>> > you can see how controls from the v4l2 interface are translated to\n>>> > libcamera generic controls.\n>>> > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp#n582\n>>> >\n>>> > For Simple there have been a few attempts in the past and recently\n>>> > there was desire to re-tackle the issue for supporting auto-focus on\n>>> > the Pinephone platform but I've lost track of the development on the\n>>> > fron (cc Adam and Rafael)\n>>> > https://patchwork.libcamera.org/patch/15078/#21709\n>>>\n>>> Understood. For the time being, I don't feel confident enough to\n>>> make the controls in Simple pipeline behave as they should. Especially\n>>> if others are already working on this.\n>>>\n>>> I do want to fix the nullptr dereference I'm facing when using Simple\n>>> Pipeline along with the provided Android camera HAL.\n>>>\n>>> Here is a stacktrace (from Android) when this happens:\n>>> http://codepad.org/CiLLcPNW\n>>>\n>>\n>> This puzzles me a bit...\n>>\n>> Indeed the camera controls for Simple are not populated, but I wasn't\n>> expecting a segfault, but rather an assertion failure.\n>>\n>> The call trace points to\n>>\n>> int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n>>\n>> where the request's controls are populated translating Android\n>> controls to libcamera's ones.\n>>\n>> The request list is populated with the Camera's validator when\n>> request is created\n>>\n>> Request::Request(Camera *camera, uint64_t cookie)\n>> \t: Extensible(std::make_unique(camera)),\n>> \t cookie_(cookie), status_(RequestPending)\n>> {\n>> \tcontrols_ = new ControlList(controls::controls,\n>> \t\t\t\t camera->_d()->validator());\n>>\n>> I presume the call to\n>>\n>> \tcontrols.set(controls::ScalerCrop, cropRegion);\n>>\n>> in CameraDevice goes through\n>>\n>> \ttemplate\n>> \tvoid set(const Control &ctrl, const V &value)\n>> \t{\n>> \t\tControlValue *val = find(ctrl.id());\n>> \t\tif (!val)\n>> \t\t\treturn;\n>>\n>> \t\tval->set(value);\n>> \t}\n>>\n>> which calls\n>>\n>> ControlValue *ControlList::find(unsigned int id)\n>> {\n>> \tif (validator_ && !validator_->validate(id)) {\n>>\n>> and finally\n>>\n>> bool CameraControlValidator::validate(unsigned int id) const\n>> {\n>> \tconst ControlInfoMap &controls = camera_->controls();\n>> \treturn controls.find(id) != controls.end();\n>> }\n>>\n>> with the offendin call being\n>>\n>> \treturn controls.find(id) != controls.end();\n>>\n>> If you look at the implementation of\n>>\n>> ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)\n>> {\n>> \tauto iter = idmap_->find(id);\n>> \tif (iter == idmap_->end())\n>> \t\treturn end();\n>>\n>> \treturn find(iter->second);\n>> }\n>>\n>> you'll see that the default constructor for ControlInfoMap doesn't\n>> intialized idmap_ which is initialized at class declaration time as\n>>\n>> \tconst ControlIdMap *idmap_ = nullptr;\n>>\n>> To sum it up, a Camera is constructed with an unsafe controlInfo_\n>>\n>>\n>>> Is adding some null checks in ControlInfoMap::find() an acceptable\n>>> solution?\n>>>\n>>> >\n>>> >\n>>> >> Signed-off-by: Mattijs Korpershoek \n>>> >> ---\n>>> >> Alternatively, we could null check everywhere in controls.cpp or\n>>> >> get rid of the default constructor for ControlInfoMap.\n>>> >> ---\n>>\n>> And if I would have read this, I would have spare the above. Well,\n>> we're on the same page at least :)\n>\n> I agree, we are on the same page. Thank you for the write-up though,\n> it's a way better explanation than what I've done in the commit message.\n>\n>>\n>> I tried rather hard to make idmap_ an instance intead of a pointer.\n>>\n>> This however has two consequences:\n>> 1) operator= cannot be default-generated, and in the implementation I\n>> tried I was not able to correctly copy the Map in. I'm afraid the only\n>> solution here would be to use composition instead of having\n>> ControlInfoMap inherit from unordered_map\n>>\n>> 2) ControlInfo might needs to reference the global controls::controls id\n>> map, which might be expensive to copy in if idmap_ is now made an\n>> instance. This is also relevant for control serialization, which\n>> happens on IPC borders and is in a rather hot path\n>>\n>> I'm afraid we'll have to protect the ControlInfoMap class methods with\n>> an\n>> if (!idmap_)\n>> return end();\n>>\n>> What do you think ?\n>\n> I think this is the most trivial solution and i'm willing to implement\n> this and submit it.\n>\n> Another alternative I can think of is not providing a default\n> constructor for ControlInfoMap but that seems difficult/also not\n> necessarily a good idea.\n>\n> I will test and submit the trivial solution in the coming days. Thanks\n> again for your help.\n\nI've submitted this here:\nhttps://lists.libcamera.org/pipermail/libcamera-devel/2023-April/037443.html\n\n>\n> Mattijs\n>\n>>\n>>\n>>> >> src/libcamera/pipeline/simple/simple.cpp | 1 +\n>>> >> 1 file changed, 1 insertion(+)\n>>> >>\n>>> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>>> >> index 2423ec10c878..4db3dc2812b0 100644\n>>> >> --- a/src/libcamera/pipeline/simple/simple.cpp\n>>> >> +++ b/src/libcamera/pipeline/simple/simple.cpp\n>>> >> @@ -537,6 +537,7 @@ int SimpleCameraData::init()\n>>> >> \t}\n>>> >>\n>>> >> \tproperties_ = sensor_->properties();\n>>> >> +\tcontrolInfo_ = sensor_->controls();\n>>> >>\n>>> >> \treturn 0;\n>>> >> }\n>>> >>\n>>> >> ---\n>>> >> base-commit: ac7511dc4c594f567ddff27ccc02c30bf6c00bfd\n>>> >> change-id: 20230331-simple-controls-fd92853c7cff\n>>> >>\n>>> >> Best regards,\n>>> >> --\n>>> >> Mattijs Korpershoek \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 95E3BC0F2A\n\tfor ;\n\tTue, 4 Apr 2023 16:12:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 611296271C;\n\tTue, 4 Apr 2023 18:12:05 +0200 (CEST)","from mail-wr1-x42b.google.com (mail-wr1-x42b.google.com\n\t[IPv6:2a00:1450:4864:20::42b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1F2CE626DB\n\tfor ;\n\tTue, 4 Apr 2023 18:12:04 +0200 (CEST)","by mail-wr1-x42b.google.com with SMTP id m2so33391074wrh.6\n\tfor ;\n\tTue, 04 Apr 2023 09:12:04 -0700 (PDT)","from localhost ([2a01:cb19:85e6:1900:2bf7:7388:731d:c4e1])\n\tby smtp.gmail.com with ESMTPSA id\n\ti12-20020a5d55cc000000b002c3f81c51b6sm12613001wrw.90.2023.04.04.09.12.03\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 04 Apr 2023 09:12:03 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680624725;\n\tbh=3WXaY0WBBYGhbqifZ3ClDpfGdqJQ2KWpVuK37EeTKV8=;\n\th=To:In-Reply-To:References:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ZnK3ySbyxe0CTDQ7n4HiwJjD8WinOX4gjIbWT0PR7bNFck56Lr5gY7C72yDY97/wj\n\t1o0KY3gPP/q0XHb3A75Dmc1oUlLgRaWOzHSRn12ImAzMClMOXl3DZi0hLSskpnm4Cu\n\tRpE9M5gHq7yuDUrQuJCKfhqHj16GOA8UmG8KiQ5OGnEGs6WnMt/obeAgq8g0uWFBbE\n\to/1TD7vq8Z+QQV9JcVn+56FtIWaS/GO1bnlfhbEfm1reb7T4eHpbeEKWQeOIXLmFDn\n\tI4xiX0E0RxhOms0WcZ5fV6T1L1s6n9WifBfpiMMEMRPFIweFY8VNcZvpOoE5VE3Nvu\n\tDehtOC9CbBHkg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=baylibre-com.20210112.gappssmtp.com; s=20210112; t=1680624724;\n\th=mime-version:message-id:date:references:in-reply-to:subject:cc:to\n\t:from:from:to:cc:subject:date:message-id:reply-to;\n\tbh=iJSjqH5Z6ZwHnAgarpLnjxF5xzS5aXXFI7Uk77wIrII=;\n\tb=PRreONTjzLvyrv45PTTonwHBGZ1WWHnSq15uV+mLkt4o+KkNKcfnl3F8dlbf6eY/qx\n\tNnR3wrdQXh9+BaSoTNzI6GgrmezIZMWftdY9MExDa2ol3Z6zmqypk435hAxBSIn5FAXP\n\tTFkjlVLuS2u9dTT4zxNDx5BIRWjfRltSRJHJaiHwXCJH+dDmYAf9Fn2041FjxXQN+lR2\n\tQEDymXxQJAsIBGhI4l6AQV/GJ0ewf9AFFV9NSbpjurZn4W8nvlkGIJBdRqTufZ1Z4Teo\n\tvlmWU5iig+TwKjbXyoPce4PdIfueQSIN28Gll698mwBzRi5ZOiEf3c2JGkn0wfoMYzFM\n\tmhtQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=baylibre-com.20210112.gappssmtp.com\n\theader.i=@baylibre-com.20210112.gappssmtp.com header.b=\"PRreONTj\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1680624724;\n\th=mime-version:message-id:date:references:in-reply-to:subject:cc:to\n\t:from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; \n\tbh=iJSjqH5Z6ZwHnAgarpLnjxF5xzS5aXXFI7Uk77wIrII=;\n\tb=0KobfJZfeImfKbUBQr95XXYlvAJM5bqtJevmGDEAN1G7lGEXcCp8YPoWmbe/BygVUn\n\tRimkZ6w24UQ5A7zNQ5p5zoI0ZUTqEjxcj8HhHLD+sFKJ5IAl77KCRYs+/PrHkvhCzKLz\n\ts2vDv/HkP7wzYpWIO9eNW2qN2sS3Euarp+xyGfw7sKA5/wOgRhA8XvtYlYSK7cEeAJZq\n\tyalUlZzLkClMzlnIQkhxb6BE6qsZrF6oH9jspJUJLkTpA2lQcwEAMVthNhtnOZf0cBgw\n\t87QrCCYA9CMyGUTInXiEx/wA+BW7R+/IvBIpJg7BoIaRlZCjgYWjNpuf9uUOUe1NhXJf\n\tVafA==","X-Gm-Message-State":"AAQBX9fWjdwigXo4N4BE7dWsQQRsMAhlIp+g5GAUuiDJFc5MCptBJ6No\n\tnZBi/97R9FGN87M4ZOYHqXACAA==","X-Google-Smtp-Source":"AKy350bKxt43RYylgz1IAzysqlaZGcCGMCcQncFbJfty+KdrRC6mgflgVQfblCqwyyrZ80BCEanLNw==","X-Received":"by 2002:adf:f4d2:0:b0:2da:f0b5:2595 with SMTP id\n\th18-20020adff4d2000000b002daf0b52595mr2142960wrp.0.1680624723646; \n\tTue, 04 Apr 2023 09:12:03 -0700 (PDT)","To":"Jacopo Mondi ","In-Reply-To":"<87sfdfam6e.fsf@baylibre.com>","References":"<20230331-simple-controls-v1-1-f7b8327d2c26@baylibre.com>\n\t<20230403081751.wby3eihzgty4m5rs@uno.localdomain>\n\t<87v8ic9fp6.fsf@baylibre.com>\n\t<20230404104319.qiaazsty5lr2tlsu@uno.localdomain>\n\t<87sfdfam6e.fsf@baylibre.com>","Date":"Tue, 04 Apr 2023 18:12:02 +0200","Message-ID":"<87jzyrd3u5.fsf@baylibre.com>","MIME-Version":"1.0","Content-Type":"text/plain","Subject":"Re: [libcamera-devel] [PATCH] pipeline: simple: set controlInfo_ to\n\tsensor_->controls()","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","From":"Mattijs Korpershoek via libcamera-devel\n\t","Reply-To":"Mattijs Korpershoek ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org, adam@piggz.co.uk","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]