[{"id":16369,"web_url":"https://patchwork.libcamera.org/comment/16369/","msgid":"","date":"2021-04-20T00:32:52","subject":"Re: [libcamera-devel] [PATCH] libcamera: V4L2Device: Return the\n\trequested Controls in getContrls()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Thu, Apr 15, 2021 at 12:07:30PM +0900, Hirokazu Honda wrote:\n> Originally V4L2Device::getControls() returns all the available\n> controls while requested control values are acquired by\n> VIDIOC_G_EXT_CTRLS. V4L2Device::getControls() should rather\n> return the request controls only. This fixes the bug.\n\nThese types of internal bugs should be caught by unit tests. If the\nissue hasn't been noticed before, it means a test is missing, and our\npolicy is to submit the additional test (or a patch extending an\nexisting test) to catch the problem in the same series as the fix.\n\nThis being said, this patch actually breaks a unit test:\n\n30/61 libcamera:v4l2_videodevice / controls FAIL 0.02s (exit status 255 or signal 127 SIGinvalid)\n\nI'll thus take this as an opportunity to say that unit tests should be\nrun before submitting patches :-) You can simply run \"ninja test\". The\nvivid, vim2m and vimc drivers need to be loaded.\n\nThe reason is that the controls_ parameter passed to the ControlList\nconstructor isn't a ControlList, but a ControlInfoMap. The code doesn't\ncopy controls_ in ctrls, it references the ControlInfoMap which contains\ninformation about all supported controls by the device.\n\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/libcamera/v4l2_device.cpp | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index decd19ef..d4a9bb75 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -177,7 +177,7 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> \tif (count == 0)\n> \t\treturn {};\n> \n> -\tControlList ctrls{ controls_ };\n> +\tControlList ctrls;\n> \n> \t/*\n> \t * Start by filling the ControlList. This can't be combined with filling","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 41171BD816\n\tfor ;\n\tTue, 20 Apr 2021 00:32:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B5AA468824;\n\tTue, 20 Apr 2021 02:32:58 +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 DA874602CA\n\tfor ;\n\tTue, 20 Apr 2021 02:32:56 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 46D7D470;\n\tTue, 20 Apr 2021 02:32:56 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"R5F5rmAD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618878776;\n\tbh=db+QTW/JxqlOm8tUCsh+p6G1l/XKkIpAUwuhY1azFe8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=R5F5rmADb0OJRGv0z3C795DYHPfV0KySIwEie76msHRfofNF0s7NkEqqLCw2ya2ab\n\tmyu0Ueocs5hgVugd+ty9FoILorudtzRueUQ+ETsz3wGFuJ/RXAlUkNZiavXO7asmYF\n\t36EFImyytHrQeAaqLAAaXyBl2RnGozgau8NQMuiw=","Date":"Tue, 20 Apr 2021 03:32:52 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210415030730.3333809-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210415030730.3333809-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: V4L2Device: Return the\n\trequested Controls in getContrls()","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16430,"web_url":"https://patchwork.libcamera.org/comment/16430/","msgid":"","date":"2021-04-21T08:40:47","subject":"Re: [libcamera-devel] [PATCH] libcamera: V4L2Device: Return the\n\trequested Controls in getContrls()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Thanks Laurent for reviewing.\n\nOn Tue, Apr 20, 2021 at 9:33 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Thu, Apr 15, 2021 at 12:07:30PM +0900, Hirokazu Honda wrote:\n> > Originally V4L2Device::getControls() returns all the available\n> > controls while requested control values are acquired by\n> > VIDIOC_G_EXT_CTRLS. V4L2Device::getControls() should rather\n> > return the request controls only. This fixes the bug.\n>\n> These types of internal bugs should be caught by unit tests. If the\n> issue hasn't been noticed before, it means a test is missing, and our\n> policy is to submit the additional test (or a patch extending an\n> existing test) to catch the problem in the same series as the fix.\n>\n> This being said, this patch actually breaks a unit test:\n>\n> 30/61 libcamera:v4l2_videodevice / controls FAIL 0.02s (exit status 255 or signal 127 SIGinvalid)\n>\n> I'll thus take this as an opportunity to say that unit tests should be\n> run before submitting patches :-) You can simply run \"ninja test\". The\n> vivid, vim2m and vimc drivers need to be loaded.\n>\n> The reason is that the controls_ parameter passed to the ControlList\n> constructor isn't a ControlList, but a ControlInfoMap. The code doesn't\n> copy controls_ in ctrls, it references the ControlInfoMap which contains\n> information about all supported controls by the device.\n>\n\nI read ControlList code and it doesn't generate control maps.\nThis patch should be abandoned.\n\n\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/libcamera/v4l2_device.cpp | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index decd19ef..d4a9bb75 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -177,7 +177,7 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > if (count == 0)\n> > return {};\n> >\n> > - ControlList ctrls{ controls_ };\n> > + ControlList ctrls;\n> >\n> > /*\n> > * Start by filling the ControlList. This can't be combined with filling\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 0E80ABDB16\n\tfor ;\n\tWed, 21 Apr 2021 08:41:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 446EC68843;\n\tWed, 21 Apr 2021 10:40:59 +0200 (CEST)","from mail-ej1-x631.google.com (mail-ej1-x631.google.com\n\t[IPv6:2a00:1450:4864:20::631])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4B946602C8\n\tfor ;\n\tWed, 21 Apr 2021 10:40:58 +0200 (CEST)","by mail-ej1-x631.google.com with SMTP id w3so62356744ejc.4\n\tfor ;\n\tWed, 21 Apr 2021 01:40:58 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"b/xERPfu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=8hBg1CSe9BT05A87sn7Ia0St4HW8RkB5U1iDNFp3X7k=;\n\tb=b/xERPfup1kzjUoLbyCxArI7GNeVLXBtp3wprMGB9VbYep9Jzt2Mkw7zZQ2B+3r7rv\n\toAnSA40DOcwfPBfYPvrYPD7PvE8+ZeNmd6D4mnlXc/SMLmeVYIXyWGpkcKLNADoXIYk6\n\tOmEsNT7mGsuOaBcDV74HYJvuQ+amxutacMrGE=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=8hBg1CSe9BT05A87sn7Ia0St4HW8RkB5U1iDNFp3X7k=;\n\tb=s0ddNuTxsMhPlr209WL81cuNcKPMkMBRJ0+Zl6hVMzkPPd4/B739Vl0LK0ad8nk9iV\n\tdeI/FEIie+4k9dvPAZ9toKxrMxMMkCk5urcZ6ZlnCoQiedU1ETRJlojbRqlwZ4/0nkaA\n\tCXZRsdx0yvvWXss4A/l6Cb0d6MikETM0pSzAjFTJmuoadBnxOPRKKngj6LErk1lXweTI\n\tMqsPj7wMf4DPocldGS+Bgtka/vBx5+DYIHYORVkb6hhyB0MGFtJsbwixy1KNYQPewIAZ\n\tRRfMty9JJnjt8XySVID/59sP14kEvBWiPBDFrNlvox/N8NJgKSlZ0ucESqFWq7yFilcQ\n\tOZDw==","X-Gm-Message-State":"AOAM5339ubu+5zrVOTo+ua2aDSPJYHBQfdiKIHjOkvBWjQX8tDgsDa5L\n\tLS1/EmlZSuvw+tuaRtCpG/YVAzDPBhttSWqp4SbfWg==","X-Google-Smtp-Source":"ABdhPJyTy2Hvc/GZda3qWu/2LovSgoMMwLlPX0kuJkZ213qt3E33bJiXGoyKMkOS96shhe6KPbfYckLM7WgPOOK975o=","X-Received":"by 2002:a17:906:349b:: with SMTP id\n\tg27mr31304121ejb.306.1618994457797; \n\tWed, 21 Apr 2021 01:40:57 -0700 (PDT)","MIME-Version":"1.0","References":"<20210415030730.3333809-1-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Wed, 21 Apr 2021 17:40:47 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH] libcamera: V4L2Device: Return the\n\trequested Controls in getContrls()","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]