[{"id":1377,"web_url":"https://patchwork.libcamera.org/comment/1377/","msgid":"<20190415235643.GC16492@bigcity.dyn.berto.se>","date":"2019-04-15T23:56:43","subject":"Re: [libcamera-devel] [PATCH v5 5/7] libcamera: camera: Validate\n\tRequest befor queueing it","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2019-04-16 01:18:57 +0200, Jacopo Mondi wrote:\n> Validate the Request before proceeding to prepare it at\n> Camera::queueRequest() time.\n> \n> For now limit the validation to making sure the Request contains at\n> least one Stream to capture from.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/camera.cpp | 15 +++++++++++++--\n> 1 file changed, 13 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 21caa24b90b5..e93e7b3b8477 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -715,8 +715,9 @@ Request *Camera::createRequest()\n> * \\param[in] request The request to queue to the camera\n> *\n> * This method queues a \\a request allocated with createRequest() to the camera\n> - * for capture. Once the request has been queued, the camera will notify its\n> - * completion through the \\ref requestCompleted signal.\n> + * for capture and validates it by making sure it contains at least one stream\n> + * where to capture from. Once the request has been queued, the camera will\n> + * notify its completion through the \\ref requestCompleted signal.\n> *\n> * Ownership of the request is transferred to the camera. It will be deleted\n> * automatically after it completes.\n> @@ -724,6 +725,7 @@ Request *Camera::createRequest()\n> * \\return 0 on success or a negative error code otherwise\n> * \\retval -ENODEV The camera has been disconnected from the system\n> * \\retval -EACCES The camera is not running so requests can't be queued\n> + * \\retval -EINVAL The request is not valid\n> */\n> int Camera::queueRequest(Request *request)\n> {\n> @@ -733,6 +735,15 @@ int Camera::queueRequest(Request *request)\n> \tif (!stateIs(CameraRunning))\n> \t\treturn -EACCES;\n> \n> +\t/*\n> +\t * \\todo: Centralize validation if it gets more complex and update\n> +\t * the documentation.\n> +\t */\n> +\tif (request->empty()) {\n> +\t\tLOG(Camera, Error) << \"Invalid request\";\n> +\t\treturn -EINVAL;\n> +\t}\n\nI know this is still debated and there are 3 different options on the \ntopic ;-)\n\nI still think this should be moved to Request::prepare(), as it can't be \na valid operation to prepare a request for capture if it contains no \nbuffers.\n\nI also know the name is under debate and empty() is a work in progress. \nLooking at what empty() actually do maybe name isWaiting(), isPending(),\nhavePending() or something indicating that it checks if the request have \nany pending buffers. completed() might even be an option.\n\n> +\n> \tint ret = request->prepare();\n> \tif (ret) {\n> \t\tLOG(Camera, Error) << \"Failed to prepare request\";\n> -- \n> 2.21.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lj1-x231.google.com (mail-lj1-x231.google.com\n\t[IPv6:2a00:1450:4864:20::231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AF8A060DBE\n\tfor ;\n\tTue, 16 Apr 2019 01:56:44 +0200 (CEST)","by mail-lj1-x231.google.com with SMTP id j89so17351242ljb.1\n\tfor ;\n\tMon, 15 Apr 2019 16:56:44 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tv10sm10086718lfe.3.2019.04.15.16.56.43\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tMon, 15 Apr 2019 16:56:43 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=DGeGzv/qyH3vkzPDsYbpBYNu1nHcd64R+AnD7pbz5U8=;\n\tb=V5yF2RYq0P0o26b46fDTYShXBJ0/aHUJOaEu0/OctddE4xjejumQR/ZGNVoXQzKusI\n\tspqdHMZs51ODQ/N1EM1OPmN5lgWQTGRO8OFq/7CvL/CseUJf+aB/nrMSCoQuWzEOHspc\n\teBi+t6UZzmEKCmo9alBAKYHhOUrm4lL0tGUKYmuT43HYw2qrZQtqd8CoiwNmDhDEwEN7\n\tOrTRmxgIoipJHVhqdc6y72zEbT0aGpUhHNVPhaYPcfC4KTb73wNMtFcL09r5N9nmUgik\n\tpXVGOrQkJ+g1BU+ifm/LhguQw8rmBd7LXhDSE1Jy1KL1Hv8pxP1fusOCG31xxqSegVnp\n\tYKJg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=DGeGzv/qyH3vkzPDsYbpBYNu1nHcd64R+AnD7pbz5U8=;\n\tb=nag5MOaFAsFYCfdAEXq4LppV28DVDb7Av0dG/KpRxWn3SVOH40Jm/0bMWmw1OmYuW2\n\tj+uBHmWRi1Fv97HONHQUsBs1Rc5ftm4g2gDnEF9UIFM1pQIXlgr3BaG9G/iSWvCMczmt\n\ttkg5qyelgpc8PGWgPlAcI7OKAOudrJ6KDJty2RI+S9+Z7umCdCzrO5iQdGXSY+fK2dgD\n\tGW/vSyJ/mr2p49txtMJZlgqZEFdRBg/f63TrOoM3RGeP76nKnlehCwZhlz2zPVQEP7wW\n\tFfwHkBEX0JaCDw6P1qtke9/KJEtGvt7U0nvWUJ2wyXtEiVfIFX3MZGwFa7f0W1v7v7Mw\n\tqhPg==","X-Gm-Message-State":"APjAAAWpaG3New8UZy6+Zb3tv7o6cSsliPdCjx+cSqza6prqFgNxLX6L\n\tbM/iari0d0L0eS9H4CCpC+alTz8diE4=","X-Google-Smtp-Source":"APXvYqzlLyGK77z4XaZ6bv0kkS5A9T7Yk8/U0jZOEFGAi34pFAuo6GhyDmBnO/oYGliAcYNXu8LW3A==","X-Received":"by 2002:a2e:97d3:: with SMTP id m19mr2610466ljj.63.1555372604062;\n\tMon, 15 Apr 2019 16:56:44 -0700 (PDT)","Date":"Tue, 16 Apr 2019 01:56:43 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190415235643.GC16492@bigcity.dyn.berto.se>","References":"<20190415231859.9747-1-jacopo@jmondi.org>\n\t<20190415231859.9747-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190415231859.9747-6-jacopo@jmondi.org>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH v5 5/7] libcamera: camera: Validate\n\tRequest befor queueing it","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 15 Apr 2019 23:56:45 -0000"}},{"id":1382,"web_url":"https://patchwork.libcamera.org/comment/1382/","msgid":"<20190416111537.GD4832@pendragon.ideasonboard.com>","date":"2019-04-16T11:15:37","subject":"Re: [libcamera-devel] [PATCH v5 5/7] libcamera: camera: Validate\n\tRequest befor queueing it","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello Jacopo,\n\nThank you for the patch.\n\nIn the subject line, s/befor/before/\n\nOn Tue, Apr 16, 2019 at 01:56:43AM +0200, Niklas Söderlund wrote:\n> On 2019-04-16 01:18:57 +0200, Jacopo Mondi wrote:\n> > Validate the Request before proceeding to prepare it at\n> > Camera::queueRequest() time.\n> > \n> > For now limit the validation to making sure the Request contains at\n> > least one Stream to capture from.\n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/camera.cpp | 15 +++++++++++++--\n> > 1 file changed, 13 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 21caa24b90b5..e93e7b3b8477 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -715,8 +715,9 @@ Request *Camera::createRequest()\n> > * \\param[in] request The request to queue to the camera\n> > *\n> > * This method queues a \\a request allocated with createRequest() to the camera\n> > - * for capture. Once the request has been queued, the camera will notify its\n> > - * completion through the \\ref requestCompleted signal.\n> > + * for capture and validates it by making sure it contains at least one stream\n> > + * where to capture from. Once the request has been queued, the camera will\n> > + * notify its completion through the \\ref requestCompleted signal.\n\n * This method validates and queues the \\a request to the camera for capture.\n * After allocating the request with createRequest(), the application shall\n * fill it with at least one capture buffer before queuing it. Requests that\n * contain no buffers are invalid and are rejected by this method without being\n * queued.\n *\n * Once the request has been queued, the camera will notify its completion\n * through the \\ref requestCompleted signal.\n\n> > *\n> > * Ownership of the request is transferred to the camera. It will be deleted\n> > * automatically after it completes.\n> > @@ -724,6 +725,7 @@ Request *Camera::createRequest()\n> > * \\return 0 on success or a negative error code otherwise\n> > * \\retval -ENODEV The camera has been disconnected from the system\n> > * \\retval -EACCES The camera is not running so requests can't be queued\n> > + * \\retval -EINVAL The request is not valid\n> > */\n> > int Camera::queueRequest(Request *request)\n> > {\n> > @@ -733,6 +735,15 @@ int Camera::queueRequest(Request *request)\n> > \tif (!stateIs(CameraRunning))\n> > \t\treturn -EACCES;\n> > \n> > +\t/*\n> > +\t * \\todo: Centralize validation if it gets more complex and update\n> > +\t * the documentation.\n> > +\t */\n\nIsn't the Camera class as central as it can get ? :-) I would drop the\ncomment.\n\n> > +\tif (request->empty()) {\n> > +\t\tLOG(Camera, Error) << \"Invalid request\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> \n> I know this is still debated and there are 3 different options on the \n> topic ;-)\n> \n> I still think this should be moved to Request::prepare(), as it can't be \n> a valid operation to prepare a request for capture if it contains no \n> buffers.\n\nI would be fine with that. I would then remove the error message below,\nand add an \"Invalid request due to missing buffers\" message to\nRequest::prepare().\n\n> I also know the name is under debate and empty() is a work in progress. \n> Looking at what empty() actually do maybe name isWaiting(), isPending(),\n> havePending() or something indicating that it checks if the request have \n> any pending buffers. completed() might even be an option.\n\ncompleted() isn't a good name, as the request doesn't complete\nautomatically when all buffers complete, pipeline handlers can delay\ncompletion. How about hasPendingBuffers() ? A tad longer than I would\nlike, but still manageable, and I think it conveys the right meaning.\n\n> > +\n> > \tint ret = request->prepare();\n> > \tif (ret) {\n> > \t\tLOG(Camera, Error) << \"Failed to prepare request\";","headers":{"Return-Path":"","Received":["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 0FA8A60DB4\n\tfor ;\n\tTue, 16 Apr 2019 13:15:47 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 664E1E2;\n\tTue, 16 Apr 2019 13:15:46 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555413346;\n\tbh=dv9jPM5v9ZaFgrKwZhtwcHZu2mgZmxqz/tcpg8jzoCA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vPh4XunJIPRcdNWrzFgdA9qog5vG8sweZSIoT4bmugTAJ5VXYA75bUDrr+ALTt4At\n\tZ3OxoJFvBsilsROn6Y+P0mIXKOB5do65jyzsGl+8CgqDPEC9GtHtrxzegfT6n9rruO\n\tYrskBQFYC35TOZeHNO27O9dxumb6RojihCCxx7Mg=","Date":"Tue, 16 Apr 2019 14:15:37 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190416111537.GD4832@pendragon.ideasonboard.com>","References":"<20190415231859.9747-1-jacopo@jmondi.org>\n\t<20190415231859.9747-6-jacopo@jmondi.org>\n\t<20190415235643.GC16492@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190415235643.GC16492@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v5 5/7] libcamera: camera: Validate\n\tRequest befor queueing it","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 16 Apr 2019 11:15:47 -0000"}}]