[{"id":20019,"web_url":"https://patchwork.libcamera.org/comment/20019/","msgid":"<20211001165402.l6bxt3y37kggew7v@uno.localdomain>","date":"2021-10-01T16:54:02","subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: pipeline: uvcvideo:\n\tSupport the new AE controls","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Fri, Oct 01, 2021 at 07:33:20PM +0900, Paul Elder wrote:\n> Add support for the new AE controls in the uvcvideo pipeline handler.\n>\n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=42\n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=43\n> Signed-off-by: Paul Elder \n>\n> ---\n> Changes in v2:\n> - fix the rebase error where some uvc stuff was in rasberrypi\n> ---\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 36 +++++++++++++++++---\n> 1 file changed, 32 insertions(+), 4 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 264f5370..3a9c3b8d 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -268,7 +268,9 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> \t\tcid = V4L2_CID_CONTRAST;\n> \telse if (id == controls::Saturation)\n> \t\tcid = V4L2_CID_SATURATION;\n> -\telse if (id == controls::AeEnable)\n> +\telse if (id == controls::ExposureTimeMode)\n> +\t\tcid = V4L2_CID_EXPOSURE_AUTO;\n> +\telse if (id == controls::AnalogueGainMode)\n> \t\tcid = V4L2_CID_EXPOSURE_AUTO;\n\nShouldn't AnalogueGainMode map to V4L2_CID_AUTOGAIN ?\n\n> \telse if (id == controls::ExposureTime)\n> \t\tcid = V4L2_CID_EXPOSURE_ABSOLUTE;\n> @@ -302,9 +304,33 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> \t}\n>\n> \tcase V4L2_CID_EXPOSURE_AUTO: {\n> -\t\tint32_t ivalue = value.get()\n> +\t\tbool exposureSet = controls->contains(V4L2_CID_EXPOSURE_AUTO);\n\nUh, I don't get this. We add V4L2_CID_EXPOSURE_AUTO to controls at the\nend of the function, don't we ?\n> +\n> +\t\t/* \\todo Make this nicer. */\n\nI don't get what happens down here, maybe a most explicative comment\nmight help ?\n\n> +\t\tint32_t ivalue;\n> +\t\tif (id == controls::ExposureTimeMode && exposureSet) {\n> +\t\t\tint32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO).get();\n> +\t\t\tivalue = value.get() == controls::ExposureTimeModeAuto\n> +\t\t\t ? (exposureMode == V4L2_EXPOSURE_SHUTTER_PRIORITY\n> +\t\t\t\t ? V4L2_EXPOSURE_AUTO\n> +\t\t\t\t : V4L2_EXPOSURE_APERTURE_PRIORITY)\n> +\t\t\t : V4L2_EXPOSURE_MANUAL;\n> +\t\t} else if (id == controls::ExposureTimeMode && !exposureSet) {\n> +\t\t\tivalue = value.get() == controls::ExposureTimeModeAuto\n> \t\t\t ? V4L2_EXPOSURE_APERTURE_PRIORITY\n> \t\t\t : V4L2_EXPOSURE_MANUAL;\n> +\t\t} else if (id == controls::AnalogueGainMode && exposureSet) {\n> +\t\t\tint32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO).get();\n> +\t\t\tivalue = value.get() == controls::AnalogueGainModeAuto\n> +\t\t\t ? (exposureMode == V4L2_EXPOSURE_APERTURE_PRIORITY\n> +\t\t\t\t ? V4L2_EXPOSURE_AUTO\n> +\t\t\t\t : V4L2_EXPOSURE_SHUTTER_PRIORITY)\n> +\t\t\t : V4L2_EXPOSURE_MANUAL;\n> +\t\t} else if (id == controls::AnalogueGainMode && !exposureSet) {\n> +\t\t\tivalue = value.get() == controls::AnalogueGainModeAuto\n> +\t\t\t ? V4L2_EXPOSURE_SHUTTER_PRIORITY\n> +\t\t\t : V4L2_EXPOSURE_MANUAL;\n> +\t\t}\n> \t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, ivalue);\n> \t\tbreak;\n> \t}\n> @@ -559,7 +585,7 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n> \t\tid = &controls::Saturation;\n> \t\tbreak;\n> \tcase V4L2_CID_EXPOSURE_AUTO:\n> -\t\tid = &controls::AeEnable;\n> +\t\tid = &controls::ExposureTimeMode;\n> \t\tbreak;\n> \tcase V4L2_CID_EXPOSURE_ABSOLUTE:\n> \t\tid = &controls::ExposureTime;\n> @@ -610,7 +636,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n> \t\tbreak;\n>\n> \tcase V4L2_CID_EXPOSURE_AUTO:\n> -\t\tinfo = ControlInfo{ false, true, true };\n> +\t\tinfo = ControlInfo{ { controls::ExposureTimeModeAuto,\n> +\t\t\t\t controls::ExposureTimeModeDisabled },\n> +\t\t\t\t controls::ExposureTimeModeDisabled };\n\n> \t\tbreak;\n>\n> \tcase V4L2_CID_EXPOSURE_ABSOLUTE:\n> --\n> 2.27.0\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 AD47ABDC71\n\tfor ;\n\tFri, 1 Oct 2021 16:53:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1B2EF691AA;\n\tFri, 1 Oct 2021 18:53:18 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EC252691A6\n\tfor ;\n\tFri, 1 Oct 2021 18:53:16 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 5D81B1C0007;\n\tFri, 1 Oct 2021 16:53:16 +0000 (UTC)"],"Date":"Fri, 1 Oct 2021 18:54:02 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20211001165402.l6bxt3y37kggew7v@uno.localdomain>","References":"<20211001103325.1077590-1-paul.elder@ideasonboard.com>\n\t<20211001103325.1077590-3-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211001103325.1077590-3-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: pipeline: uvcvideo:\n\tSupport the new AE 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20026,"web_url":"https://patchwork.libcamera.org/comment/20026/","msgid":"<20211004081106.GD4221@pyrite.rasen.tech>","date":"2021-10-04T08:11:06","subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: pipeline: uvcvideo:\n\tSupport the new AE controls","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Oct 01, 2021 at 06:54:02PM +0200, Jacopo Mondi wrote:\n> Hi Paul,\n> \n> On Fri, Oct 01, 2021 at 07:33:20PM +0900, Paul Elder wrote:\n> > Add support for the new AE controls in the uvcvideo pipeline handler.\n> >\n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=42\n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=43\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > Changes in v2:\n> > - fix the rebase error where some uvc stuff was in rasberrypi\n> > ---\n> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 36 +++++++++++++++++---\n> > 1 file changed, 32 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index 264f5370..3a9c3b8d 100644\n> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > @@ -268,7 +268,9 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> > \t\tcid = V4L2_CID_CONTRAST;\n> > \telse if (id == controls::Saturation)\n> > \t\tcid = V4L2_CID_SATURATION;\n> > -\telse if (id == controls::AeEnable)\n> > +\telse if (id == controls::ExposureTimeMode)\n> > +\t\tcid = V4L2_CID_EXPOSURE_AUTO;\n> > +\telse if (id == controls::AnalogueGainMode)\n> > \t\tcid = V4L2_CID_EXPOSURE_AUTO;\n> \n> Shouldn't AnalogueGainMode map to V4L2_CID_AUTOGAIN ?\n\nOh probably. I'm not yet familiar with V4L2 controls.\n\n> \n> > \telse if (id == controls::ExposureTime)\n> > \t\tcid = V4L2_CID_EXPOSURE_ABSOLUTE;\n> > @@ -302,9 +304,33 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> > \t}\n> >\n> > \tcase V4L2_CID_EXPOSURE_AUTO: {\n> > -\t\tint32_t ivalue = value.get()\n> > +\t\tbool exposureSet = controls->contains(V4L2_CID_EXPOSURE_AUTO);\n> \n> Uh, I don't get this. We add V4L2_CID_EXPOSURE_AUTO to controls at the\n> end of the function, don't we ?\n\nOh you're right. I think I thought I needed and exception and then\ndidn't and forgot to fix it back.\n\n> > +\n> > +\t\t/* \\todo Make this nicer. */\n> \n> I don't get what happens down here, maybe a most explicative comment\n> might help ?\n\nAh :D\n\nI thought that V4L2_CID_EXPOSURE_AUTO was \"the\" AE control, as it had\nboth aperture priority and shutter priority.\n\nThe end goal of this complex block is to:\nexposure | gain || value\n---------+--------++------\nauto | auto || auto\nauto | manual || aperture priority\nauto | none || aperture priority\nmanual | auto || shutter priority\nnone | auto || shutter priority\nmanual | manual || manual\n\n(I'm pretty sure, this is so disgusting that even I lost track...)\n\n\nPaul\n\n> \n> > +\t\tint32_t ivalue;\n> > +\t\tif (id == controls::ExposureTimeMode && exposureSet) {\n> > +\t\t\tint32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO).get();\n> > +\t\t\tivalue = value.get() == controls::ExposureTimeModeAuto\n> > +\t\t\t ? (exposureMode == V4L2_EXPOSURE_SHUTTER_PRIORITY\n> > +\t\t\t\t ? V4L2_EXPOSURE_AUTO\n> > +\t\t\t\t : V4L2_EXPOSURE_APERTURE_PRIORITY)\n> > +\t\t\t : V4L2_EXPOSURE_MANUAL;\n> > +\t\t} else if (id == controls::ExposureTimeMode && !exposureSet) {\n> > +\t\t\tivalue = value.get() == controls::ExposureTimeModeAuto\n> > \t\t\t ? V4L2_EXPOSURE_APERTURE_PRIORITY\n> > \t\t\t : V4L2_EXPOSURE_MANUAL;\n> > +\t\t} else if (id == controls::AnalogueGainMode && exposureSet) {\n> > +\t\t\tint32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO).get();\n> > +\t\t\tivalue = value.get() == controls::AnalogueGainModeAuto\n> > +\t\t\t ? (exposureMode == V4L2_EXPOSURE_APERTURE_PRIORITY\n> > +\t\t\t\t ? V4L2_EXPOSURE_AUTO\n> > +\t\t\t\t : V4L2_EXPOSURE_SHUTTER_PRIORITY)\n> > +\t\t\t : V4L2_EXPOSURE_MANUAL;\n> > +\t\t} else if (id == controls::AnalogueGainMode && !exposureSet) {\n> > +\t\t\tivalue = value.get() == controls::AnalogueGainModeAuto\n> > +\t\t\t ? V4L2_EXPOSURE_SHUTTER_PRIORITY\n> > +\t\t\t : V4L2_EXPOSURE_MANUAL;\n> > +\t\t}\n> > \t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, ivalue);\n> > \t\tbreak;\n> > \t}\n> > @@ -559,7 +585,7 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n> > \t\tid = &controls::Saturation;\n> > \t\tbreak;\n> > \tcase V4L2_CID_EXPOSURE_AUTO:\n> > -\t\tid = &controls::AeEnable;\n> > +\t\tid = &controls::ExposureTimeMode;\n> > \t\tbreak;\n> > \tcase V4L2_CID_EXPOSURE_ABSOLUTE:\n> > \t\tid = &controls::ExposureTime;\n> > @@ -610,7 +636,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n> > \t\tbreak;\n> >\n> > \tcase V4L2_CID_EXPOSURE_AUTO:\n> > -\t\tinfo = ControlInfo{ false, true, true };\n> > +\t\tinfo = ControlInfo{ { controls::ExposureTimeModeAuto,\n> > +\t\t\t\t controls::ExposureTimeModeDisabled },\n> > +\t\t\t\t controls::ExposureTimeModeDisabled };\n> \n> > \t\tbreak;\n> >\n> > \tcase V4L2_CID_EXPOSURE_ABSOLUTE:\n> > --\n> > 2.27.0\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 32C62BDC71\n\tfor ;\n\tMon, 4 Oct 2021 08:11:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8E2F9691B9;\n\tMon, 4 Oct 2021 10:11:16 +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 CF16D602DC\n\tfor ;\n\tMon, 4 Oct 2021 10:11:14 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 705445A1;\n\tMon, 4 Oct 2021 10:11:13 +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=\"tKODvtnR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1633335074;\n\tbh=vxmL5peDEF3PU8+afA9l/FeOpIgnzkHd0ZypcvwQvqQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tKODvtnR/OWdDOVXrn7cd2sK+x32LQCdFm6HK5C200phgGU3qqaPbEyAxeUmAN8fB\n\twqjARfqlsqNdC5Qymhml3e76GF2aD97XxYyWIwoLJPX+zTDYbO3WRTmLJ0N7PgImys\n\tnnwunGcYOjp0YWzFeqcDPxnomrjvp+dwsDe+lY8c=","Date":"Mon, 4 Oct 2021 17:11:06 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20211004081106.GD4221@pyrite.rasen.tech>","References":"<20211001103325.1077590-1-paul.elder@ideasonboard.com>\n\t<20211001103325.1077590-3-paul.elder@ideasonboard.com>\n\t<20211001165402.l6bxt3y37kggew7v@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20211001165402.l6bxt3y37kggew7v@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: pipeline: uvcvideo:\n\tSupport the new AE 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]