[{"id":1335,"web_url":"https://patchwork.libcamera.org/comment/1335/","msgid":"<20190414200844.GM1980@bigcity.dyn.berto.se>","date":"2019-04-14T20:08:44","subject":"Re: [libcamera-devel] [PATCH v4 08/12] libcamera: camera: Refuse\n\tempty requests","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-09 21:25:44 +0200, Jacopo Mondi wrote:\n> Requests that do not contain any Stream should not be forwarded to\n> the pipeline handlers. Return -EINVAL if the request does not contain\n> any Stream to capture from.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/camera.cpp | 4 ++++\n> 1 file changed, 4 insertions(+)\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index cb392ca3b7e7..bd73ff69c3da 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -728,6 +728,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 does not contain any stream to capture from\n> */\n> int Camera::queueRequest(Request *request)\n> {\n> @@ -743,6 +744,9 @@ int Camera::queueRequest(Request *request)\n> \t\treturn ret;\n> \t}\n> \n> +\tif (request->empty())\n> +\t\treturn -EINVAL;\n> +\n\nShould this be moved inside Request::prepare() ?\n\n> \treturn pipe_->queueRequest(this, request);\n> }\n> \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-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4840360DBE\n\tfor ;\n\tSun, 14 Apr 2019 22:08:46 +0200 (CEST)","by mail-lj1-x241.google.com with SMTP id r24so13664335ljg.3\n\tfor ;\n\tSun, 14 Apr 2019 13:08:46 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tl22sm9519385lja.97.2019.04.14.13.08.44\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tSun, 14 Apr 2019 13:08:44 -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=PBASQKaljzrHTwLSsoNmcpCfqHuK81NamlRHVnUU3M4=;\n\tb=egSjKHRkQGz/cb9Zd00VLP+WAnYZ00PGgT5kYlpeOzaRI2M6fhWDcTw9meGYON8mNd\n\tDxAQypGbt1sI1eh4cqgS4FRIr25wwKT45BJ+ioHvNua4C7WnMW2pUGMOR+nzOXz0hLd6\n\t+ztXONR5YdPrttgLcf7FuY2Zqg9jYIcnSi4DZArwPbh2LVvipRnDrKorrHNcDhYWNoKq\n\tBbf9kVW4b9g7gVcuezdH4L3+DqCts6G2zc6Eqr78qtRujgkszAU+ryZ3gy8nKXGJWzON\n\tlcnfuknCtAv4EdquKVvGfrUICKDZpEpncea+W31ffllXaR4zoG2cnDkwudKuix3eLBDu\n\tsY+Q==","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=PBASQKaljzrHTwLSsoNmcpCfqHuK81NamlRHVnUU3M4=;\n\tb=RAd5RSv93COyicR6fM1P1kRoxB2PkEJ+CkirFQiN6Ub/jc8Gtj5Ww8b8qEU4xYRhBA\n\t+6b0Re1Knor1h2GbW+mNrdkigW5XFvn04yIkKWnL2D96clS4vVMF9T8tco/Kcsmpnz/5\n\taa+MUkcBTPBs9xnQyEH5FmXHMWeF/AXjbdeSLXu8PAKpeCc8nUOmKMpQN6VGQgtCod/D\n\tF7lYWgwPpkG5nepb3aAkbU/2WmgUXxK/tlPBWOvtuR6HG+4nYvX8m0wHSFmQrbFu1ysf\n\tUsi2sCbpTkN9SknpmhEOPF4n28EsMTDpX6qaN8BHE5gSIngATe7epAIX4SrSASHEuNPU\n\tqkbA==","X-Gm-Message-State":"APjAAAWtV/LaJPpjhK0OTI7F8Y1cju3sFMVYS4mmSHiDT2+k4IUjtdiT\n\t62AH9xWNLqrULCPw7aVi+HFR+rqsw8I=","X-Google-Smtp-Source":"APXvYqw9M/EgWhKN1M/66mO3tN2nSTdbnF5fnTcSEqiTAnWPXsU4YagXOTSd6B13CjlXGtoXzi6eYA==","X-Received":"by 2002:a2e:219:: with SMTP id 25mr36772421ljc.34.1555272525703; \n\tSun, 14 Apr 2019 13:08:45 -0700 (PDT)","Date":"Sun, 14 Apr 2019 22:08:44 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190414200844.GM1980@bigcity.dyn.berto.se>","References":"<20190409192548.20325-1-jacopo@jmondi.org>\n\t<20190409192548.20325-9-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":"<20190409192548.20325-9-jacopo@jmondi.org>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH v4 08/12] libcamera: camera: Refuse\n\tempty requests","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":"Sun, 14 Apr 2019 20:08:46 -0000"}},{"id":1347,"web_url":"https://patchwork.libcamera.org/comment/1347/","msgid":"<20190415124035.x767x7f65y24uvdb@uno.localdomain>","date":"2019-04-15T12:40:36","subject":"Re: [libcamera-devel] [PATCH v4 08/12] libcamera: camera: Refuse\n\tempty requests","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Sun, Apr 14, 2019 at 10:08:44PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2019-04-09 21:25:44 +0200, Jacopo Mondi wrote:\n> > Requests that do not contain any Stream should not be forwarded to\n> > the pipeline handlers. Return -EINVAL if the request does not contain\n> > any Stream to capture from.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/camera.cpp | 4 ++++\n> > 1 file changed, 4 insertions(+)\n> >\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index cb392ca3b7e7..bd73ff69c3da 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -728,6 +728,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 does not contain any stream to capture from\n> > */\n> > int Camera::queueRequest(Request *request)\n> > {\n> > @@ -743,6 +744,9 @@ int Camera::queueRequest(Request *request)\n> > \t\treturn ret;\n> > \t}\n> >\n> > +\tif (request->empty())\n> > +\t\treturn -EINVAL;\n> > +\n>\n> Should this be moved inside Request::prepare() ?\n>\n\nI considered doing that, but this sees to me like something that\nshould live in a 'validate()' method more than in 'prepare()'.\n\nAs long as this is only validation we have I think it could live\nhere, to be later moved to a 'validate()' method if we see any need\nfor that. What do you think?\n\nThanks\n j\n\n> > \treturn pipe_->queueRequest(this, request);\n> > }\n> >\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\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6AC91600F9\n\tfor ;\n\tMon, 15 Apr 2019 14:39:45 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id DD45910000B;\n\tMon, 15 Apr 2019 12:39:44 +0000 (UTC)"],"Date":"Mon, 15 Apr 2019 14:40:36 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190415124035.x767x7f65y24uvdb@uno.localdomain>","References":"<20190409192548.20325-1-jacopo@jmondi.org>\n\t<20190409192548.20325-9-jacopo@jmondi.org>\n\t<20190414200844.GM1980@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"nutbhcsxf53uo2pg\"","Content-Disposition":"inline","In-Reply-To":"<20190414200844.GM1980@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v4 08/12] libcamera: camera: Refuse\n\tempty requests","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 12:39:51 -0000"}},{"id":1352,"web_url":"https://patchwork.libcamera.org/comment/1352/","msgid":"<20190415141929.GJ4809@pendragon.ideasonboard.com>","date":"2019-04-15T14:19:29","subject":"Re: [libcamera-devel] [PATCH v4 08/12] libcamera: camera: Refuse\n\tempty requests","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Mon, Apr 15, 2019 at 02:40:36PM +0200, Jacopo Mondi wrote:\n> On Sun, Apr 14, 2019 at 10:08:44PM +0200, Niklas Söderlund wrote:\n> > On 2019-04-09 21:25:44 +0200, Jacopo Mondi wrote:\n> > > Requests that do not contain any Stream should not be forwarded to\n> > > the pipeline handlers. Return -EINVAL if the request does not contain\n> > > any Stream to capture from.\n\nWhen we'll add support for controls this will not necessarily hold true\nanymore (although I'm not sure what the use cases would be). In any case\nI don't think it's a reason to block this patch.\n\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/camera.cpp | 4 ++++\n> > > 1 file changed, 4 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index cb392ca3b7e7..bd73ff69c3da 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -728,6 +728,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 does not contain any stream to capture from\n\nIn the future I'm sure we'll have other types of invalid requests,\nshould we already just state that the request is invalid without\ndetailing why ? It may make sense to add a new paragraph to the\ndocumentation of this function to explain what a valid request is (or\nmove that to the documentation of Request::valid() if you decide to go\nthat route).\n\n> > > */\n> > > int Camera::queueRequest(Request *request)\n> > > {\n> > > @@ -743,6 +744,9 @@ int Camera::queueRequest(Request *request)\n> > > \t\treturn ret;\n> > > \t}\n> > >\n> > > +\tif (request->empty())\n\nI think you should log a debug message.\n\n> > > +\t\treturn -EINVAL;\n> > > +\n> >\n> > Should this be moved inside Request::prepare() ?\n> \n> I considered doing that, but this sees to me like something that\n> should live in a 'validate()' method more than in 'prepare()'.\n> \n> As long as this is only validation we have I think it could live\n> here, to be later moved to a 'validate()' method if we see any need\n> for that. What do you think?\n\nMaybe you could rename Request::empty() to Request::valid() ?\n\n> > > \treturn pipe_->queueRequest(this, request);\n> > > }\n> > >","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9ABD8600F9\n\tfor ;\n\tMon, 15 Apr 2019 16:19:38 +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 0670D333;\n\tMon, 15 Apr 2019 16:19:37 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555337978;\n\tbh=v2dT51LnhWmldqF0a67GSCx36TErJUgWLmvlpNO3++0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CyIGFTp1YdLoh/CmHa2QLI6JfNYx1QYtKzYIMmrGD62WP+NTK1RJfYtj/PcvPpyWO\n\t1si6060gLr1bEtDLfOTadDxJrPAt9tls1I9DdV3AMiCAFEnTSD/qGEowmWvx9S6cap\n\tsveCaJIP2Y+9uGSWoK5wgDg1Vys0+IDCMENx8bBU=","Date":"Mon, 15 Apr 2019 17:19:29 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190415141929.GJ4809@pendragon.ideasonboard.com>","References":"<20190409192548.20325-1-jacopo@jmondi.org>\n\t<20190409192548.20325-9-jacopo@jmondi.org>\n\t<20190414200844.GM1980@bigcity.dyn.berto.se>\n\t<20190415124035.x767x7f65y24uvdb@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190415124035.x767x7f65y24uvdb@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 08/12] libcamera: camera: Refuse\n\tempty requests","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 14:19:38 -0000"}},{"id":1355,"web_url":"https://patchwork.libcamera.org/comment/1355/","msgid":"<20190415145550.cegcfvxsrihtsc7v@uno.localdomain>","date":"2019-04-15T14:55:50","subject":"Re: [libcamera-devel] [PATCH v4 08/12] libcamera: camera: Refuse\n\tempty requests","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Apr 15, 2019 at 05:19:29PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Mon, Apr 15, 2019 at 02:40:36PM +0200, Jacopo Mondi wrote:\n> > On Sun, Apr 14, 2019 at 10:08:44PM +0200, Niklas Söderlund wrote:\n> > > On 2019-04-09 21:25:44 +0200, Jacopo Mondi wrote:\n> > > > Requests that do not contain any Stream should not be forwarded to\n> > > > the pipeline handlers. Return -EINVAL if the request does not contain\n> > > > any Stream to capture from.\n>\n> When we'll add support for controls this will not necessarily hold true\n> anymore (although I'm not sure what the use cases would be). In any case\n> I don't think it's a reason to block this patch.\n>\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/libcamera/camera.cpp | 4 ++++\n> > > > 1 file changed, 4 insertions(+)\n> > > >\n> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > index cb392ca3b7e7..bd73ff69c3da 100644\n> > > > --- a/src/libcamera/camera.cpp\n> > > > +++ b/src/libcamera/camera.cpp\n> > > > @@ -728,6 +728,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 does not contain any stream to capture from\n>\n> In the future I'm sure we'll have other types of invalid requests,\n> should we already just state that the request is invalid without\n> detailing why ? It may make sense to add a new paragraph to the\n> documentation of this function to explain what a valid request is (or\n> move that to the documentation of Request::valid() if you decide to go\n> that route).\n>\n> > > > */\n> > > > int Camera::queueRequest(Request *request)\n> > > > {\n> > > > @@ -743,6 +744,9 @@ int Camera::queueRequest(Request *request)\n> > > > \t\treturn ret;\n> > > > \t}\n> > > >\n> > > > +\tif (request->empty())\n>\n> I think you should log a debug message.\n>\n> > > > +\t\treturn -EINVAL;\n> > > > +\n> > >\n> > > Should this be moved inside Request::prepare() ?\n> >\n> > I considered doing that, but this sees to me like something that\n> > should live in a 'validate()' method more than in 'prepare()'.\n> >\n> > As long as this is only validation we have I think it could live\n> > here, to be later moved to a 'validate()' method if we see any need\n> > for that. What do you think?\n>\n> Maybe you could rename Request::empty() to Request::valid() ?\n>\n\nThis would be very convienent, but I'm not sure a request should be\nself-validating honestly... Although a Request::valid() function might\nmake sense to perform the basic checks, I think there should be a\nvalidation performed by the Camera class too (and posibly a hook to\npipeline handler too? as some control might be valid for one\nimplementation but not for the other? That's for later thuough...)\n\nI'll change empty() to valid() and perform a first validity check,\nsuch as making sure the request has at least one Stream where to\ncapture from...\n\nThanks\n j\n\n> > > > \treturn pipe_->queueRequest(this, request);\n> > > > }\n> > > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["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 1B803600F9\n\tfor ;\n\tMon, 15 Apr 2019 16:55:00 +0200 (CEST)","from uno.localdomain (unknown [31.159.28.121])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id D593D1C0016;\n\tMon, 15 Apr 2019 14:54:58 +0000 (UTC)"],"X-Originating-IP":"31.159.28.121","Date":"Mon, 15 Apr 2019 16:55:50 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190415145550.cegcfvxsrihtsc7v@uno.localdomain>","References":"<20190409192548.20325-1-jacopo@jmondi.org>\n\t<20190409192548.20325-9-jacopo@jmondi.org>\n\t<20190414200844.GM1980@bigcity.dyn.berto.se>\n\t<20190415124035.x767x7f65y24uvdb@uno.localdomain>\n\t<20190415141929.GJ4809@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"cew2ewvzpfj74mpw\"","Content-Disposition":"inline","In-Reply-To":"<20190415141929.GJ4809@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v4 08/12] libcamera: camera: Refuse\n\tempty requests","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 14:55:00 -0000"}},{"id":1359,"web_url":"https://patchwork.libcamera.org/comment/1359/","msgid":"<20190415153738.GE17083@pendragon.ideasonboard.com>","date":"2019-04-15T15:37:38","subject":"Re: [libcamera-devel] [PATCH v4 08/12] libcamera: camera: Refuse\n\tempty requests","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Apr 15, 2019 at 04:55:50PM +0200, Jacopo Mondi wrote:\n> On Mon, Apr 15, 2019 at 05:19:29PM +0300, Laurent Pinchart wrote:\n> > On Mon, Apr 15, 2019 at 02:40:36PM +0200, Jacopo Mondi wrote:\n> >> On Sun, Apr 14, 2019 at 10:08:44PM +0200, Niklas Söderlund wrote:\n> >>> On 2019-04-09 21:25:44 +0200, Jacopo Mondi wrote:\n> >>>> Requests that do not contain any Stream should not be forwarded to\n> >>>> the pipeline handlers. Return -EINVAL if the request does not contain\n> >>>> any Stream to capture from.\n> >\n> > When we'll add support for controls this will not necessarily hold true\n> > anymore (although I'm not sure what the use cases would be). In any case\n> > I don't think it's a reason to block this patch.\n> >\n> >>>> Signed-off-by: Jacopo Mondi \n> >>>> ---\n> >>>> src/libcamera/camera.cpp | 4 ++++\n> >>>> 1 file changed, 4 insertions(+)\n> >>>>\n> >>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> >>>> index cb392ca3b7e7..bd73ff69c3da 100644\n> >>>> --- a/src/libcamera/camera.cpp\n> >>>> +++ b/src/libcamera/camera.cpp\n> >>>> @@ -728,6 +728,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 does not contain any stream to capture from\n> >\n> > In the future I'm sure we'll have other types of invalid requests,\n> > should we already just state that the request is invalid without\n> > detailing why ? It may make sense to add a new paragraph to the\n> > documentation of this function to explain what a valid request is (or\n> > move that to the documentation of Request::valid() if you decide to go\n> > that route).\n> >\n> >>>> */\n> >>>> int Camera::queueRequest(Request *request)\n> >>>> {\n> >>>> @@ -743,6 +744,9 @@ int Camera::queueRequest(Request *request)\n> >>>> \t\treturn ret;\n> >>>> \t}\n> >>>>\n> >>>> +\tif (request->empty())\n> >\n> > I think you should log a debug message.\n> >\n> >>>> +\t\treturn -EINVAL;\n> >>>> +\n> >>>\n> >>> Should this be moved inside Request::prepare() ?\n> >>\n> >> I considered doing that, but this sees to me like something that\n> >> should live in a 'validate()' method more than in 'prepare()'.\n> >>\n> >> As long as this is only validation we have I think it could live\n> >> here, to be later moved to a 'validate()' method if we see any need\n> >> for that. What do you think?\n> >\n> > Maybe you could rename Request::empty() to Request::valid() ?\n> >\n> \n> This would be very convienent, but I'm not sure a request should be\n> self-validating honestly... Although a Request::valid() function might\n> make sense to perform the basic checks, I think there should be a\n> validation performed by the Camera class too (and posibly a hook to\n> pipeline handler too? as some control might be valid for one\n> implementation but not for the other? That's for later thuough...)\n> \n> I'll change empty() to valid() and perform a first validity check,\n> such as making sure the request has at least one Stream where to\n> capture from...\n\nActually, after reading the rest of the series, I realized you use\n->empty() is various other places, and those usages don't match a\nvalid() function. I would thus perform the validate check in\nCamera::queueRequest() as done in this patch, with an additional\nparagraph in the function documentation to explain what a valid request\nis, and keep the existing semantics for the empty() method. I would\nhowever rename empty() to something else, as what we really want to\ncheck is if the request still has uncompleted buffers. Maybe\nisReadyToComplete() ? It's a bit long, I'm sure a better name exists.\n\n> >>>> \treturn pipe_->queueRequest(this, request);\n> >>>> }\n> >>>>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F1B15600F9\n\tfor ;\n\tMon, 15 Apr 2019 17:37:47 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6148E333;\n\tMon, 15 Apr 2019 17:37:47 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555342667;\n\tbh=bD+l0a7Ujy0xsPRlzdMA5RiNVgx+IJYNGttrot72lmI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iiNybh/Y2ZhMqu/AuuEnN0u4hT2c+HL51Wst0FCyCZYCMJYLDPDbgpih8uGhyRzij\n\tJmV9Du1/SkDljpyexOPMHH11OBpkqXF5CllECZ9m+bQ4FVJY7n++A6SBJ/K37f7+mM\n\tbB4kE7fwOwqkeWN5H5c0gbx/e6M9CYPMdMdoq564=","Date":"Mon, 15 Apr 2019 18:37:38 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190415153738.GE17083@pendragon.ideasonboard.com>","References":"<20190409192548.20325-1-jacopo@jmondi.org>\n\t<20190409192548.20325-9-jacopo@jmondi.org>\n\t<20190414200844.GM1980@bigcity.dyn.berto.se>\n\t<20190415124035.x767x7f65y24uvdb@uno.localdomain>\n\t<20190415141929.GJ4809@pendragon.ideasonboard.com>\n\t<20190415145550.cegcfvxsrihtsc7v@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190415145550.cegcfvxsrihtsc7v@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 08/12] libcamera: camera: Refuse\n\tempty requests","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 15:37:48 -0000"}}]