[{"id":14598,"web_url":"https://patchwork.libcamera.org/comment/14598/","msgid":"","date":"2021-01-19T12:33:40","subject":"Re: [libcamera-devel] [PATCH 2/2] cam: Fix capturing an precis\n\tnumber of requests","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"On 2021-01-19 13:31:10 +0100, Niklas Söderlund wrote:\n> When moving request processing from the camera manager thread to the\n> main thread a subtle race condition where added when running with the\n> --capture=N option.\n> \n> Before the change the check of how many request had been queued was ran\n> in the camera manager thread and thus could not race with the producer\n> thread. After the change if requests are completed faster then they are\n> consumed (the consumer writes them to disk for example) the check may be\n> delayed and more then the expected number of request may be asked to\n> processed.\n> \n> Sebastian Fricke \n\nHum this should ofc be 'Reported-by: Sebastian ...'.\n\n> Fixes: 02eae70e15bdbb24 (\"cam: Move request processing to main thread\")\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/cam/capture.cpp | 12 ++++++------\n> 1 file changed, 6 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> index 113ea49d50046e5b..ef1601c716bfa137 100644\n> --- a/src/cam/capture.cpp\n> +++ b/src/cam/capture.cpp\n> @@ -157,6 +157,12 @@ void Capture::requestComplete(Request *request)\n> \tif (request->status() == Request::RequestCancelled)\n> \t\treturn;\n> \n> +\tcaptureCount_++;\n> +\tif (captureLimit_ && captureCount_ > captureLimit_) {\n> +\t\tloop_->exit(0);\n> +\t\treturn;\n> +\t}\n> +\n> \t/*\n> \t * Defer processing of the completed request to the event loop, to avoid\n> \t * blocking the camera manager thread.\n> @@ -206,12 +212,6 @@ void Capture::processRequest(Request *request)\n> \n> \tstd::cout << info.str() << std::endl;\n> \n> -\tcaptureCount_++;\n> -\tif (captureLimit_ && captureCount_ >= captureLimit_) {\n> -\t\tloop_->exit(0);\n> -\t\treturn;\n> -\t}\n> -\n> \trequest->reuse(Request::ReuseBuffers);\n> \tcamera_->queueRequest(request);\n> }\n> -- \n> 2.30.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 F21F0BD808\n\tfor ;\n\tTue, 19 Jan 2021 12:33:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8398C68143;\n\tTue, 19 Jan 2021 13:33:44 +0100 (CET)","from mail-lj1-x233.google.com (mail-lj1-x233.google.com\n\t[IPv6:2a00:1450:4864:20::233])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AAFF168140\n\tfor ;\n\tTue, 19 Jan 2021 13:33:43 +0100 (CET)","by mail-lj1-x233.google.com with SMTP id m10so21717268lji.1\n\tfor ;\n\tTue, 19 Jan 2021 04:33:43 -0800 (PST)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tj2sm2265581lfm.305.2021.01.19.04.33.41\n\tfor \n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 19 Jan 2021 04:33:41 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"GOFM44Wl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=cT3zc4thAJZxiiuSR40lM/JpnjuQZPk5iQdyzOQL6os=;\n\tb=GOFM44Wlj2qZwJ9wnECHWLKT6jbsuc0fzDn39Ts4/j3GA+NESR1b8mOfsT1KPJb5Y9\n\t+wUvCyjkH9Zb25YiErI5+VkB4jwCpvMNRC+kYEzxj7X/R13Fs6qTyUhKU4hKH3OTQZXB\n\tKwO9Kjpk3X0hNKys9SUvLSKiIDLprizui7bTx7a0zwpSof5H2XA29AuFrU8piE9QcXo7\n\t+mtR52NdT/wcawVcf4RKJWeHi7gQU2VrXuCXt41Cby/aF9rGkAouOMTx4tIONFIv3usS\n\tim3vuws7QzxtLcJqkSDH6tkD3nu0iyetiWO2DgQHHincc2XC9pl2cVx+goQZb5n/k5nj\n\tdqeA==","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:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=cT3zc4thAJZxiiuSR40lM/JpnjuQZPk5iQdyzOQL6os=;\n\tb=nnG1hThvOReTTmZZyFZog3i+7QTf3/cYM/EPsIEGR7RFJhfpi8r7aukQgT86RS5TZz\n\tX7rjPmmjO3hEdD5EISJOJzAlesZrC7OSXnRkZKJ83xLM/XGWpZXXYqquo3rtnEGXtGhU\n\tp0mpZnhro1B8jjXfj7KZMgtl/AiAgyLTwcs1jA4Wm+xC/+pHp8qFdFLDvRTGMTPFfMlj\n\tSlsJLUlZ31f+ldmGu5Pw8FM5NC5R/pnV7phA9lp5lEqrKdFanA+SmfcWcoKelWnhSkU5\n\tI+Z3hIgwb9/KHm41iHgrpeKMekVq2c0msae9JHtH2JIQdgyCrSRKF9CjS6ejaYWNywSz\n\tXRVg==","X-Gm-Message-State":"AOAM532EcvEY/E+7kDlXoh8fDF9Fs6I8u7H9xWqv6LNElZtCy4AQJtT/\n\tonbPdaPpFD5U4kYLz9P8h3cqO40V7W5RDg==","X-Google-Smtp-Source":"ABdhPJw35nn0v/coPp1G/hr6qQ0ZdlU9IhAbZUCEiqbYSa8dg+QxD1+ZTOLMn5YhFql4kkxxukPziw==","X-Received":"by 2002:a2e:8416:: with SMTP id\n\tz22mr1822755ljg.347.1611059623142; \n\tTue, 19 Jan 2021 04:33:43 -0800 (PST)","Date":"Tue, 19 Jan 2021 13:33:40 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"libcamera-devel@lists.libcamera.org","Message-ID":"","References":"<20210119123110.2976971-1-niklas.soderlund@ragnatech.se>\n\t<20210119123110.2976971-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210119123110.2976971-3-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 2/2] cam: Fix capturing an precis\n\tnumber of requests","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","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14600,"web_url":"https://patchwork.libcamera.org/comment/14600/","msgid":"","date":"2021-01-19T12:50:04","subject":"Re: [libcamera-devel] [PATCH 2/2] cam: Fix capturing an precis\n\tnumber of requests","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Tue, Jan 19, 2021 at 01:31:10PM +0100, Niklas Söderlund wrote:\n> When moving request processing from the camera manager thread to the\n> main thread a subtle race condition where added when running with the\n> --capture=N option.\n> \n> Before the change the check of how many request had been queued was ran\n> in the camera manager thread and thus could not race with the producer\n> thread. After the change if requests are completed faster then they are\n> consumed (the consumer writes them to disk for example) the check may be\n> delayed and more then the expected number of request may be asked to\n> processed.\n\nI'm not sure to see the problem. Capture::processRequest() is called\nasynchronously on request completion, but the calls are serialized. As\nwe don't requeue requests in Capture::requestComplete(), but in\nCapture::processRequest(), and only after the captureLimit_ check, we\nshould never queue more than captureLimit_ requests. With this change\nwe'll terminate the event loop early, which means that some completed\nrequests may fail to reach Capture::processRequest(), and won't be\nwritten to a file.\n\n> Sebastian Fricke \n> Fixes: 02eae70e15bdbb24 (\"cam: Move request processing to main thread\")\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/cam/capture.cpp | 12 ++++++------\n> 1 file changed, 6 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> index 113ea49d50046e5b..ef1601c716bfa137 100644\n> --- a/src/cam/capture.cpp\n> +++ b/src/cam/capture.cpp\n> @@ -157,6 +157,12 @@ void Capture::requestComplete(Request *request)\n> \tif (request->status() == Request::RequestCancelled)\n> \t\treturn;\n> \n> +\tcaptureCount_++;\n> +\tif (captureLimit_ && captureCount_ > captureLimit_) {\n> +\t\tloop_->exit(0);\n> +\t\treturn;\n> +\t}\n> +\n> \t/*\n> \t * Defer processing of the completed request to the event loop, to avoid\n> \t * blocking the camera manager thread.\n> @@ -206,12 +212,6 @@ void Capture::processRequest(Request *request)\n> \n> \tstd::cout << info.str() << std::endl;\n> \n> -\tcaptureCount_++;\n> -\tif (captureLimit_ && captureCount_ >= captureLimit_) {\n> -\t\tloop_->exit(0);\n> -\t\treturn;\n> -\t}\n> -\n> \trequest->reuse(Request::ReuseBuffers);\n> \tcamera_->queueRequest(request);\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 EB56DBD808\n\tfor ;\n\tTue, 19 Jan 2021 12:50:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 76B7F68145;\n\tTue, 19 Jan 2021 13:50:23 +0100 (CET)","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 AB0A868140\n\tfor ;\n\tTue, 19 Jan 2021 13:50:21 +0100 (CET)","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 20EC6813;\n\tTue, 19 Jan 2021 13:50:21 +0100 (CET)"],"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=\"mpiGiTab\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611060621;\n\tbh=m/YBLIjQGySMF6uzH5wXt5ZY46yXOEsIZs5YOXgZYKw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mpiGiTab9MH0TrZ/B7hcYMkm9fxaAahEcwGXw0mJ7uKrlytT/oHa8UsDjNOmU85QP\n\tOs+KuWehgkWvbuZBZZx3C2MfpsupOFDw+guESJQ8I8tDJECdqbfhx0i0pYGAn6zTcx\n\to6BrO1j9CK4gHzxLVi3i2uV33SKbIEPLb5ogvZBA=","Date":"Tue, 19 Jan 2021 14:50:04 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"","References":"<20210119123110.2976971-1-niklas.soderlund@ragnatech.se>\n\t<20210119123110.2976971-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210119123110.2976971-3-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 2/2] cam: Fix capturing an precis\n\tnumber of requests","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14601,"web_url":"https://patchwork.libcamera.org/comment/14601/","msgid":"","date":"2021-01-19T13:14:53","subject":"Re: [libcamera-devel] [PATCH 2/2] cam: Fix capturing an precis\n\tnumber of requests","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2021-01-19 14:50:04 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Tue, Jan 19, 2021 at 01:31:10PM +0100, Niklas Söderlund wrote:\n> > When moving request processing from the camera manager thread to the\n> > main thread a subtle race condition where added when running with the\n> > --capture=N option.\n> > \n> > Before the change the check of how many request had been queued was ran\n> > in the camera manager thread and thus could not race with the producer\n> > thread. After the change if requests are completed faster then they are\n> > consumed (the consumer writes them to disk for example) the check may be\n> > delayed and more then the expected number of request may be asked to\n> > processed.\n> \n> I'm not sure to see the problem. Capture::processRequest() is called\n> asynchronously on request completion, but the calls are serialized. As\n> we don't requeue requests in Capture::requestComplete(), but in\n> Capture::processRequest(), and only after the captureLimit_ check, we\n> should never queue more than captureLimit_ requests. With this change\n> we'll terminate the event loop early, which means that some completed\n> requests may fail to reach Capture::processRequest(), and won't be\n> written to a file.\n\nThe problem is that we have more then one request queued to libcamera at \na time. If the check is at the end of Capture::processRequest() and the \nprocessing time for each request is large more then N requests are \ncompleted and placed on the callback queue and process. Worst case is \nthat we write N + pipeline depth of frames to disk.\n\nOne could argue that the design in cam is bad and we should not count \nnumber of completed requests but rather how many we queue to the camera. \nI think this is something we could switch. But I feel a lot more \ncomfortable if we first had a compliance tool to verify that all \npipelines actually return N requests if N requests where queue ;-P\n\n> \n> > Sebastian Fricke \n> > Fixes: 02eae70e15bdbb24 (\"cam: Move request processing to main thread\")\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/cam/capture.cpp | 12 ++++++------\n> > 1 file changed, 6 insertions(+), 6 deletions(-)\n> > \n> > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> > index 113ea49d50046e5b..ef1601c716bfa137 100644\n> > --- a/src/cam/capture.cpp\n> > +++ b/src/cam/capture.cpp\n> > @@ -157,6 +157,12 @@ void Capture::requestComplete(Request *request)\n> > \tif (request->status() == Request::RequestCancelled)\n> > \t\treturn;\n> > \n> > +\tcaptureCount_++;\n> > +\tif (captureLimit_ && captureCount_ > captureLimit_) {\n> > +\t\tloop_->exit(0);\n> > +\t\treturn;\n> > +\t}\n> > +\n> > \t/*\n> > \t * Defer processing of the completed request to the event loop, to avoid\n> > \t * blocking the camera manager thread.\n> > @@ -206,12 +212,6 @@ void Capture::processRequest(Request *request)\n> > \n> > \tstd::cout << info.str() << std::endl;\n> > \n> > -\tcaptureCount_++;\n> > -\tif (captureLimit_ && captureCount_ >= captureLimit_) {\n> > -\t\tloop_->exit(0);\n> > -\t\treturn;\n> > -\t}\n> > -\n> > \trequest->reuse(Request::ReuseBuffers);\n> > \tcamera_->queueRequest(request);\n> > }\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 40967C0F1C\n\tfor ;\n\tTue, 19 Jan 2021 13:14:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A163068146;\n\tTue, 19 Jan 2021 14:14:56 +0100 (CET)","from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com\n\t[IPv6:2a00:1450:4864:20::22d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3638268140\n\tfor ;\n\tTue, 19 Jan 2021 14:14:55 +0100 (CET)","by mail-lj1-x22d.google.com with SMTP id p13so21891521ljg.2\n\tfor ;\n\tTue, 19 Jan 2021 05:14:55 -0800 (PST)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tt5sm2068294ljk.81.2021.01.19.05.14.53\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 19 Jan 2021 05:14:53 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"ZcJ4eALi\"; dkim-atps=neutral","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\tbh=M+8DzuOhSRBT7DXISxbqxLjtMEuLaKwwy2Xl3jq7gyI=;\n\tb=ZcJ4eALiWXsPqhGmd4VHKvnqVqqdPmaexyUDiZWEE5Qx9UI1rntOPCjpzTeUkUH+tW\n\t/MkPvo9NFa22HXeINw0nQw3B3pCLqBs7u9z6/dce3Ws1QInQ8ugWlLP4lVqLCj3F+6a0\n\teK8EvspQ9mURUEdNwVOwkhOtGy/0lw1LJBTHKF09XfGVCmOph8PWofyEoKoY5Z5a1J22\n\tUpFxdaPyLiaup+yAz3aiYfHCNrl/SOCuUi8qQ9M/A0SNrhhz44ENRP+BfiJCFypER9BK\n\tmW4Il7Qg7Bbae31GtKnoEGUUPditWRAocZkvMIdj6qV30mj+cnkUeKaV3DoYGioChvme\n\tkYIA==","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;\n\tbh=M+8DzuOhSRBT7DXISxbqxLjtMEuLaKwwy2Xl3jq7gyI=;\n\tb=Eiub/ezLz+0bdIemG5rnlygEZo0GHXJhVIaR9eoSyfRFkJQtFyupVXB/etb0Y8Qv89\n\tiBqr+OiO/xgXBoa0hB7E229otfoePo0I9YiEDa+yFYyPc4DoMtGoZC4p4qrzBhhGW0Gp\n\tC1uUq/NRXHbamxXS+A0D6LhuhuuNqHfAXa4sEWMg3+sgcq/+RHOc8a6/rNu4eN2x+q9+\n\tYACMqoKjIP4F++yetpb5yldzQwZf6CggAGlI0FyCRJeUURXyq7qMlBtmBhi7IAW3nNW/\n\tDOM4jyWd28ZWvms7Ekl2u3SnvUxam7AJYWo10Yt/DZ4vqsu7/yuOa2jABv21Ur/uAH0Y\n\tUwDg==","X-Gm-Message-State":"AOAM530qzxMYf3nVjdLUaNJQEP2Z3a/7rQ+1HDdMfBJLDNi5quCz3wJE\n\trK4u4xA5NWiMBvDdINjjBdFzDWG9ynkxJQ==","X-Google-Smtp-Source":"ABdhPJwmXv7UI23gK3kmDXfBZODNPzvkga4GvaTo6cM3m+RpEJkwivpcEuD9c/0NfJJW8pDbJCQDyA==","X-Received":"by 2002:a2e:9c83:: with SMTP id x3mr1908047lji.340.1611062094599;\n\tTue, 19 Jan 2021 05:14:54 -0800 (PST)","Date":"Tue, 19 Jan 2021 14:14:53 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"","References":"<20210119123110.2976971-1-niklas.soderlund@ragnatech.se>\n\t<20210119123110.2976971-3-niklas.soderlund@ragnatech.se>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/2] cam: Fix capturing an precis\n\tnumber of requests","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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14608,"web_url":"https://patchwork.libcamera.org/comment/14608/","msgid":"","date":"2021-01-19T16:08:26","subject":"Re: [libcamera-devel] [PATCH 2/2] cam: Fix capturing an precis\n\tnumber of requests","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Tue, Jan 19, 2021 at 02:14:53PM +0100, Niklas Söderlund wrote:\n> On 2021-01-19 14:50:04 +0200, Laurent Pinchart wrote:\n> > On Tue, Jan 19, 2021 at 01:31:10PM +0100, Niklas Söderlund wrote:\n> > > When moving request processing from the camera manager thread to the\n> > > main thread a subtle race condition where added when running with the\n> > > --capture=N option.\n> > > \n> > > Before the change the check of how many request had been queued was ran\n> > > in the camera manager thread and thus could not race with the producer\n> > > thread. After the change if requests are completed faster then they are\n> > > consumed (the consumer writes them to disk for example) the check may be\n> > > delayed and more then the expected number of request may be asked to\n> > > processed.\n> > \n> > I'm not sure to see the problem. Capture::processRequest() is called\n> > asynchronously on request completion, but the calls are serialized. As\n> > we don't requeue requests in Capture::requestComplete(), but in\n> > Capture::processRequest(), and only after the captureLimit_ check, we\n> > should never queue more than captureLimit_ requests. With this change\n> > we'll terminate the event loop early, which means that some completed\n> > requests may fail to reach Capture::processRequest(), and won't be\n> > written to a file.\n> \n> The problem is that we have more then one request queued to libcamera at \n> a time. If the check is at the end of Capture::processRequest() and the \n> processing time for each request is large more then N requests are \n> completed and placed on the callback queue and process. Worst case is \n> that we write N + pipeline depth of frames to disk.\n> \n> One could argue that the design in cam is bad and we should not count \n> number of completed requests but rather how many we queue to the camera. \n> I think this is something we could switch. But I feel a lot more \n> comfortable if we first had a compliance tool to verify that all \n> pipelines actually return N requests if N requests where queue ;-P\n\nI was also going to mention counting the number of buffers queued :-) We\nshould stop requeuing requests once we reach captureLimit_ queued\nrequests, and we should stop the event loop once we reach captureLimit_\ncompleted requests. As we prequeue a fixed number of requests at the\nbeginning we may not need to have two counters, but two counters may not\nhurt either.\n\n> > > Sebastian Fricke \n> > > Fixes: 02eae70e15bdbb24 (\"cam: Move request processing to main thread\")\n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > src/cam/capture.cpp | 12 ++++++------\n> > > 1 file changed, 6 insertions(+), 6 deletions(-)\n> > > \n> > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> > > index 113ea49d50046e5b..ef1601c716bfa137 100644\n> > > --- a/src/cam/capture.cpp\n> > > +++ b/src/cam/capture.cpp\n> > > @@ -157,6 +157,12 @@ void Capture::requestComplete(Request *request)\n> > > \tif (request->status() == Request::RequestCancelled)\n> > > \t\treturn;\n> > > \n> > > +\tcaptureCount_++;\n> > > +\tif (captureLimit_ && captureCount_ > captureLimit_) {\n> > > +\t\tloop_->exit(0);\n> > > +\t\treturn;\n> > > +\t}\n> > > +\n> > > \t/*\n> > > \t * Defer processing of the completed request to the event loop, to avoid\n> > > \t * blocking the camera manager thread.\n> > > @@ -206,12 +212,6 @@ void Capture::processRequest(Request *request)\n> > > \n> > > \tstd::cout << info.str() << std::endl;\n> > > \n> > > -\tcaptureCount_++;\n> > > -\tif (captureLimit_ && captureCount_ >= captureLimit_) {\n> > > -\t\tloop_->exit(0);\n> > > -\t\treturn;\n> > > -\t}\n> > > -\n> > > \trequest->reuse(Request::ReuseBuffers);\n> > > \tcamera_->queueRequest(request);\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 19DA8C0F1C\n\tfor ;\n\tTue, 19 Jan 2021 16:08:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9D03068160;\n\tTue, 19 Jan 2021 17:08:44 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B876E6814E\n\tfor ;\n\tTue, 19 Jan 2021 17:08:43 +0100 (CET)","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 3CC3C813;\n\tTue, 19 Jan 2021 17:08:43 +0100 (CET)"],"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=\"jpqLfQmy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611072523;\n\tbh=fIfIsLgRjfVpAvApuF+sVNmvqYXeM+XBqCSN8+51izM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jpqLfQmy1laGv+cxrS1Qt4RitIANzok8jJacfnsl44ZadFRJxoUFa9U9UMzPULeFf\n\tlCcuj1SpEW97Kcuij+0X9LqfMkSYVAP6FHI+0W0mgG0xuCdla1td8rpycvCK0KvJ2A\n\t+CpGMBooP2mVs36fmscxcUjJaHUAmAI/sLOexYS4=","Date":"Tue, 19 Jan 2021 18:08:26 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"","References":"<20210119123110.2976971-1-niklas.soderlund@ragnatech.se>\n\t<20210119123110.2976971-3-niklas.soderlund@ragnatech.se>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/2] cam: Fix capturing an precis\n\tnumber of requests","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]