[{"id":1376,"web_url":"https://patchwork.libcamera.org/comment/1376/","msgid":"<20190415234328.GB16492@bigcity.dyn.berto.se>","date":"2019-04-15T23:43:28","subject":"Re: [libcamera-devel] [PATCH v5 2/7] libcamera: camera: Propagate\n\tfreeBuffers() error","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:54 +0200, Jacopo Mondi wrote:\n> The error return code of PipelineHandler::freeBuffers() was not\n> propagate up to applications by the Camera class. Fix this by returning\n> the incremental error accumulated by multiple calls (one per Stream)\n> to freeBuffers().\n> \n> Do not return the error code of the call to freeBuffers() in the\n> allocateBuffers() method error path not to overwrite the original error\n> code returned from the allocation method.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/camera.cpp | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index bdf14b31d8ee..7f2dc904df16 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -671,6 +671,8 @@ int Camera::allocateBuffers()\n> */\n> int Camera::freeBuffers()\n> {\n> +\tint ret = 0;\n> +\n> \tif (!stateIs(CameraPrepared))\n> \t\treturn -EACCES;\n> \n> @@ -683,12 +685,12 @@ int Camera::freeBuffers()\n> \t\t * by the V4L2 device that has allocated them.\n> \t\t */\n> \t\tstream->bufferPool().destroyBuffers();\n> -\t\tpipe_->freeBuffers(this, stream);\n> +\t\tret |= pipe_->freeBuffers(this, stream);\n\nOr:ing in errors here seems very strange as we are dealing with the \nstandard error codes (EINVAL, EBUSY, etc). Would it not be saner to \nstore and return the (potential) first error while still running \nfreeBuffers() for all streams before returning?\n\nHow about something like this\n\n int ret = 0;\n\n for (Stream *stream : activeStreams_) {\n int status = pipe_->freeBuffers(this, stream);\n\n if (!ret)\n ret = status;\n }\n\n return ret;\n\n> \t}\n> \n> \tstate_ = CameraConfigured;\n> \n> -\treturn 0;\n> +\treturn ret;\n> }\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-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CCF8460DBE\n\tfor ;\n\tTue, 16 Apr 2019 01:43:29 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id t4so8810256ljc.2\n\tfor ;\n\tMon, 15 Apr 2019 16:43:29 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tm28sm1104758lfc.71.2019.04.15.16.43.28\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tMon, 15 Apr 2019 16:43:28 -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=DciQOa7by3agsnJ/55TZay/OTJa24PjuNkeiw6BQ31Q=;\n\tb=OD5NT0YhMMvYgPqhmjn9TF8T6JgrxyfBVZ3FoJN+LUC33PLCuC9le/bMX5i8lO/38F\n\tMFxyrrZbGI16Ih/xq13SR6jwiXd6JQ/VxVQkVxH3aGSJEueYVVNDILQySScQYweRD26r\n\tGMrw2Q6udngzog99X+FkGTtQUTSKeN0RQtcx1NSUH1I70/9fnKWN4NA/NLZMZaorHTXG\n\tVfqD2HjsQWF0cFKWhSqFhEWi24ZFn1HJJqFbITwBJoomcC6F2xMc6sZSrFX0oxrMOan7\n\t7j2l/R+zNc/wvDNvbzU4Y6yv76pt93jVuBRHIe4swl3fwQ+YS1JMMDCXjAoOJUdqcaB7\n\ts1og==","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=DciQOa7by3agsnJ/55TZay/OTJa24PjuNkeiw6BQ31Q=;\n\tb=e71jaYJHlHV/qWYJSkcbTLqt9dh1W6AL5EN3Z6WhGSjru7/2C/ESfC3RKYwSZJ0hX5\n\t3mEh6Gqx7cNve6V4E7ie9mtNR2BOsXfM+kVtAWS0iRQPY8yVbfRr/o3c399om/SpE49D\n\tJ7yhAmYWcbNvPbfy8a/tnfkzgNrnWI71CdXjWURFeZ7C7YN7OIEJOzyth5n8yv8ZqrsE\n\tp8cCphDwD+Rqx6Zvz2fiWLVnkZ+ZBBJgdt+t7QuVA4aab4rjpQLa/YU0JQAgPLnN62SA\n\tqobkAuqFVBCK3NWxt2i8sUvp/B9jafwKi4Ud/dSCg5BNGSFlfGut6PGJjDn5tuc/YQHA\n\tRBWg==","X-Gm-Message-State":"APjAAAXqboTVUs8AhQT5kYNzge3kT0U9RTrtLv5lVnIR6KDOns25PN9/\n\tVoYuQ4Ti9IVkO62ZemJjLehro3f9rZg=","X-Google-Smtp-Source":"APXvYqxAECT5EtGM8JL7yfgreDxvUjyE33HVCvP+WBAKF2JK2P3fdwTKDRZN8qjTu7cTcHRUNwxjHQ==","X-Received":"by 2002:a2e:1257:: with SMTP id\n\tt84mr43354700lje.115.1555371809182; \n\tMon, 15 Apr 2019 16:43:29 -0700 (PDT)","Date":"Tue, 16 Apr 2019 01:43:28 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190415234328.GB16492@bigcity.dyn.berto.se>","References":"<20190415231859.9747-1-jacopo@jmondi.org>\n\t<20190415231859.9747-3-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-3-jacopo@jmondi.org>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH v5 2/7] libcamera: camera: Propagate\n\tfreeBuffers() error","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:43:30 -0000"}},{"id":1380,"web_url":"https://patchwork.libcamera.org/comment/1380/","msgid":"<20190416105527.GB4832@pendragon.ideasonboard.com>","date":"2019-04-16T10:55:27","subject":"Re: [libcamera-devel] [PATCH v5 2/7] libcamera: camera: Propagate\n\tfreeBuffers() error","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Tue, Apr 16, 2019 at 01:43:28AM +0200, Niklas Söderlund wrote:\n> On 2019-04-16 01:18:54 +0200, Jacopo Mondi wrote:\n> > The error return code of PipelineHandler::freeBuffers() was not\n> > propagate up to applications by the Camera class. Fix this by returning\n> > the incremental error accumulated by multiple calls (one per Stream)\n> > to freeBuffers().\n> > \n> > Do not return the error code of the call to freeBuffers() in the\n> > allocateBuffers() method error path not to overwrite the original error\n> > code returned from the allocation method.\n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/camera.cpp | 6 ++++--\n> > 1 file changed, 4 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index bdf14b31d8ee..7f2dc904df16 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -671,6 +671,8 @@ int Camera::allocateBuffers()\n> > */\n> > int Camera::freeBuffers()\n> > {\n> > +\tint ret = 0;\n> > +\n> > \tif (!stateIs(CameraPrepared))\n> > \t\treturn -EACCES;\n> > \n> > @@ -683,12 +685,12 @@ int Camera::freeBuffers()\n> > \t\t * by the V4L2 device that has allocated them.\n> > \t\t */\n> > \t\tstream->bufferPool().destroyBuffers();\n> > -\t\tpipe_->freeBuffers(this, stream);\n> > +\t\tret |= pipe_->freeBuffers(this, stream);\n> \n> Or:ing in errors here seems very strange as we are dealing with the \n> standard error codes (EINVAL, EBUSY, etc). Would it not be saner to \n> store and return the (potential) first error while still running \n> freeBuffers() for all streams before returning?\n> \n> How about something like this\n> \n> int ret = 0;\n> \n> for (Stream *stream : activeStreams_) {\n> int status = pipe_->freeBuffers(this, stream);\n> \n> if (!ret)\n> ret = status;\n> }\n> \n> return ret;\n\nOr maybe merging this on top of the patch that refactors buffer\nallocation ? The loop will then be gone.\n\n> > \t}\n> > \n> > \tstate_ = CameraConfigured;\n> > \n> > -\treturn 0;\n> > +\treturn ret;\n> > }\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 15BED60DB4\n\tfor ;\n\tTue, 16 Apr 2019 12:55:36 +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 9C258E2;\n\tTue, 16 Apr 2019 12:55:35 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555412135;\n\tbh=A5UkdpU0suVjTPRonYPy/ZQi1gz6vugvlQhAI45mYYQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HT1YXeXUokLlHf6614x3N1j0JUwp1PeGbD4ZIvEdhlQ1vF0upI3zUy++jTaqWDwEu\n\tacdpUPHUyBn3cIGGJ8dyBm90IS5FjYYDwRv9a1I3YJWAdWIg2mCjEimVE5ATpRLiLX\n\tbrAKIn+a146gKTZLgJcxfMi/QqG5iToFhYTCrTIs=","Date":"Tue, 16 Apr 2019 13:55:27 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20190416105527.GB4832@pendragon.ideasonboard.com>","References":"<20190415231859.9747-1-jacopo@jmondi.org>\n\t<20190415231859.9747-3-jacopo@jmondi.org>\n\t<20190415234328.GB16492@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":"<20190415234328.GB16492@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v5 2/7] libcamera: camera: Propagate\n\tfreeBuffers() error","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 10:55:36 -0000"}},{"id":1385,"web_url":"https://patchwork.libcamera.org/comment/1385/","msgid":"<20190416125208.emwgmstkcxbx2ncf@uno.localdomain>","date":"2019-04-16T12:52:08","subject":"Re: [libcamera-devel] [PATCH v5 2/7] libcamera: camera: Propagate\n\tfreeBuffers() error","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi,\n\nOn Tue, Apr 16, 2019 at 01:55:27PM +0300, Laurent Pinchart wrote:\n> Hello,\n>\n> On Tue, Apr 16, 2019 at 01:43:28AM +0200, Niklas Söderlund wrote:\n> > On 2019-04-16 01:18:54 +0200, Jacopo Mondi wrote:\n> > > The error return code of PipelineHandler::freeBuffers() was not\n> > > propagate up to applications by the Camera class. Fix this by returning\n> > > the incremental error accumulated by multiple calls (one per Stream)\n> > > to freeBuffers().\n> > >\n> > > Do not return the error code of the call to freeBuffers() in the\n> > > allocateBuffers() method error path not to overwrite the original error\n> > > code returned from the allocation method.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/camera.cpp | 6 ++++--\n> > > 1 file changed, 4 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index bdf14b31d8ee..7f2dc904df16 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -671,6 +671,8 @@ int Camera::allocateBuffers()\n> > > */\n> > > int Camera::freeBuffers()\n> > > {\n> > > +\tint ret = 0;\n> > > +\n> > > \tif (!stateIs(CameraPrepared))\n> > > \t\treturn -EACCES;\n> > >\n> > > @@ -683,12 +685,12 @@ int Camera::freeBuffers()\n> > > \t\t * by the V4L2 device that has allocated them.\n> > > \t\t */\n> > > \t\tstream->bufferPool().destroyBuffers();\n> > > -\t\tpipe_->freeBuffers(this, stream);\n> > > +\t\tret |= pipe_->freeBuffers(this, stream);\n> >\n> > Or:ing in errors here seems very strange as we are dealing with the\n> > standard error codes (EINVAL, EBUSY, etc). Would it not be saner to\n> > store and return the (potential) first error while still running\n> > freeBuffers() for all streams before returning?\n> >\n> > How about something like this\n> >\n> > int ret = 0;\n> >\n> > for (Stream *stream : activeStreams_) {\n> > int status = pipe_->freeBuffers(this, stream);\n> >\n> > if (!ret)\n> > ret = status;\n> > }\n> >\n> > return ret;\n>\n> Or maybe merging this on top of the patch that refactors buffer\n> allocation ? The loop will then be gone.\n>\n\nAh! so you would like to have it like it was in v4? It made more sense\nto me as well, I think I've been asked to split it, so I did.\n\n\n> > > \t}\n> > >\n> > > \tstate_ = CameraConfigured;\n> > >\n> > > -\treturn 0;\n> > > +\treturn ret;\n> > > }\n> > >\n> > > /**\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 02FC560DB4\n\tfor ;\n\tTue, 16 Apr 2019 14:51:16 +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 relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 48177E0004;\n\tTue, 16 Apr 2019 12:51:16 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 16 Apr 2019 14:52:08 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190416125208.emwgmstkcxbx2ncf@uno.localdomain>","References":"<20190415231859.9747-1-jacopo@jmondi.org>\n\t<20190415231859.9747-3-jacopo@jmondi.org>\n\t<20190415234328.GB16492@bigcity.dyn.berto.se>\n\t<20190416105527.GB4832@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"2wescrrohm45aax2\"","Content-Disposition":"inline","In-Reply-To":"<20190416105527.GB4832@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v5 2/7] libcamera: camera: Propagate\n\tfreeBuffers() error","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 12:51:17 -0000"}}]