[{"id":920,"web_url":"https://patchwork.libcamera.org/comment/920/","msgid":"<20190227130432.GE4813@pendragon.ideasonboard.com>","date":"2019-02-27T13:04:32","subject":"Re: [libcamera-devel] [PATCH 4/8] cam: fix order camera is operated\n\ton","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, Feb 26, 2019 at 03:18:53AM +0100, Niklas Söderlund wrote:\n> Upcoming enforcing of order the camera shall be operate on is not\n> compatible with the cam utility. Requests shall be queued after the\n> camera is started, not before.\n\nWhat are the practical implications of this on camera operation, from an\napplication point of view ? In particular, when will the acquisition be\nstarted after this change ? Will the pipeline handlers be required to\nstart acquisition on the sensor at start() time, throwing away all\nimages until requests are queued ? Or will acquisition start when the\nfirst request is queued ? Sensors can take a long time to start, so it's\nimportant to document the exact behaviour.\n\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/cam/main.cpp | 18 +++++++++++-------\n> 1 file changed, 11 insertions(+), 7 deletions(-)\n> \n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index 4c2df583fe8e99b7..8df8844c33a2d052 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -133,6 +133,7 @@ static int capture()\n> \tint ret;\n> \n> \tstd::vector streams = camera->streams();\n> +\tstd::vector requests;\n> \n> \tret = configureStreams(camera.get(), streams);\n> \tif (ret < 0) {\n> @@ -169,21 +170,24 @@ static int capture()\n> \t\t\tgoto out;\n> \t\t}\n> \n> -\t\tret = camera->queueRequest(request);\n> -\t\tif (ret < 0) {\n> -\t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> -\t\t\tgoto out;\n> -\t\t}\n> +\t\trequests.push_back(request);\n> \t}\n> \n> -\tstd::cout << \"Capture until user interrupts by SIGINT\" << std::endl;\n> -\n> \tret = camera->start();\n> \tif (ret) {\n> \t\tstd::cout << \"Failed to start capture\" << std::endl;\n> \t\tgoto out;\n> \t}\n> \n> +\tfor (Request *request : requests) {\n> +\t\tret = camera->queueRequest(request);\n> +\t\tif (ret < 0) {\n> +\t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> +\t\t\tgoto out;\n> +\t\t}\n> +\t}\n> +\n> +\tstd::cout << \"Capture until user interrupts by SIGINT\" << std::endl;\n> \tret = loop->exec();\n> \n> \tret = camera->stop();","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 182CC600FD\n\tfor ;\n\tWed, 27 Feb 2019 14:04:38 +0100 (CET)","from pendragon.ideasonboard.com (unknown [185.25.200.246])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 92E7B49;\n\tWed, 27 Feb 2019 14:04:37 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1551272677;\n\tbh=+S+nuttCqZnJYwnyBJ1B4QudQiVE1IeNpNbWk/hScP8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ikG6DJg2Ao4Wb9YTrhuwlHkHuTtwz8Hxmm/nd2UAAeHJMA/ztoJzxM+Wt3Q7BbKF6\n\tRSmvuM7UE3USqgBYF/UrfLRkE0nuvLd3/SPAS9LD7oS5mxqKu6e4ZsK/DGh9IdkMH0\n\tGFqWhf74McDhLPx9zIGKvZKh6xp/Qa9kQT//oCdM=","Date":"Wed, 27 Feb 2019 15:04:32 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190227130432.GE4813@pendragon.ideasonboard.com>","References":"<20190226021857.28255-1-niklas.soderlund@ragnatech.se>\n\t<20190226021857.28255-5-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190226021857.28255-5-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 4/8] cam: fix order camera is operated\n\ton","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":"Wed, 27 Feb 2019 13:04:38 -0000"}},{"id":935,"web_url":"https://patchwork.libcamera.org/comment/935/","msgid":"<20190227185128.GU899@bigcity.dyn.berto.se>","date":"2019-02-27T18:51:28","subject":"Re: [libcamera-devel] [PATCH 4/8] cam: fix order camera is operated\n\ton","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 2019-02-27 15:04:32 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Tue, Feb 26, 2019 at 03:18:53AM +0100, Niklas Söderlund wrote:\n> > Upcoming enforcing of order the camera shall be operate on is not\n> > compatible with the cam utility. Requests shall be queued after the\n> > camera is started, not before.\n> \n> What are the practical implications of this on camera operation, from an\n> application point of view ? In particular, when will the acquisition be\n> started after this change ? Will the pipeline handlers be required to\n> start acquisition on the sensor at start() time, throwing away all\n> images until requests are queued ? Or will acquisition start when the\n> first request is queued ? Sensors can take a long time to start, so it's\n> important to document the exact behaviour.\n\nMy understanding of the situation is that at start() time \nVIDIOC_STREAMON shall be called. Then it's currently not defined what \nother actions are expected by the camera or pipeline handler when \nprocessing start(). I agree this need to be defined.\n\nI see two possible scenarios.\n\n1. The library grantees that once a request have been queued it will \n eventually complete disregarding of more requests are queued or not.\n\n2. The library informs the application about the request queue depth it \n needs before requests starts to be processed.\n\nI'm hoping we can do scenario 1 but not sure if we can do that without \nugly scratch buffers in the library which I feel is a bit of a hack. \nWhatever scenario we pick the pipeline handler would need to know about \nthe request queue depth and act on that or report it to the application \nso it can act. The pipeline handlers do not know this property today so \nnot much we can do yet.\n\nIn any case I think this is outside the scope of this patch. This \nproblem exists disregarding if we only allow queueing requests after \nstart() or if allow it both before and after. In fact I thought of this \nchange as a first step to address this issue as we now define that \nrequests can only be queued after start().\n\n> \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/cam/main.cpp | 18 +++++++++++-------\n> > 1 file changed, 11 insertions(+), 7 deletions(-)\n> > \n> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > index 4c2df583fe8e99b7..8df8844c33a2d052 100644\n> > --- a/src/cam/main.cpp\n> > +++ b/src/cam/main.cpp\n> > @@ -133,6 +133,7 @@ static int capture()\n> > \tint ret;\n> > \n> > \tstd::vector streams = camera->streams();\n> > +\tstd::vector requests;\n> > \n> > \tret = configureStreams(camera.get(), streams);\n> > \tif (ret < 0) {\n> > @@ -169,21 +170,24 @@ static int capture()\n> > \t\t\tgoto out;\n> > \t\t}\n> > \n> > -\t\tret = camera->queueRequest(request);\n> > -\t\tif (ret < 0) {\n> > -\t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> > -\t\t\tgoto out;\n> > -\t\t}\n> > +\t\trequests.push_back(request);\n> > \t}\n> > \n> > -\tstd::cout << \"Capture until user interrupts by SIGINT\" << std::endl;\n> > -\n> > \tret = camera->start();\n> > \tif (ret) {\n> > \t\tstd::cout << \"Failed to start capture\" << std::endl;\n> > \t\tgoto out;\n> > \t}\n> > \n> > +\tfor (Request *request : requests) {\n> > +\t\tret = camera->queueRequest(request);\n> > +\t\tif (ret < 0) {\n> > +\t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> > +\t\t\tgoto out;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tstd::cout << \"Capture until user interrupts by SIGINT\" << std::endl;\n> > \tret = loop->exec();\n> > \n> > \tret = camera->stop();\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lf1-x141.google.com (mail-lf1-x141.google.com\n\t[IPv6:2a00:1450:4864:20::141])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 66B18610B3\n\tfor ;\n\tWed, 27 Feb 2019 19:51:30 +0100 (CET)","by mail-lf1-x141.google.com with SMTP id u21so13260423lfu.1\n\tfor ;\n\tWed, 27 Feb 2019 10:51:30 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tj16sm355753lfh.81.2019.02.27.10.51.28\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tWed, 27 Feb 2019 10:51:28 -0800 (PST)"],"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=biIO+Alg6XbbFBcZalh+uDiUbPqKy09Dzap27/4kkOk=;\n\tb=ePQmIlAu+cuIr7udUSnAYOv8llrxEzewonImjdoViUmzxu4RJFjU6JNLs0ODCQFrCL\n\tLfZfCBY93zLNNs7NfR24NEvbTg2WpuCjgpFyvPJESeSYvtvAGc+UWEcsxdCsIuyduqQV\n\tPBDr7vpBV9DPBOLW2qHFbfouy8glyTsakdX6jK8X+mwgYV3V7n5hCP3rWDEH9NlJdVmo\n\tzQ2n6JK3234S3C/DBUDLoQ7aqOJHlM189nyNHxRka0eZo6Dlkfo/xvqnFcAn826ioK6a\n\tvoh+MeC5GpTBKtv90HF7iz9YSfbZTqNYazxd4sVc6WPdKazPUP2/+v8sigow0gZMvGP/\n\tXWbA==","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=biIO+Alg6XbbFBcZalh+uDiUbPqKy09Dzap27/4kkOk=;\n\tb=VGcKy5z20dNegOM2kPhlASAcSsIVgRoIRO/0IKOqL47tI1XpfRrc4AbJWjYJHT5cD0\n\tJEwS6eWzXg6GQ4zoZpZl7SG3efy/laFX1YsRFhv3LBVQ8Y/dUtdri6QxYQ3cAXDLzXKL\n\t3mXWSn2pQ8257Tce5oRZxM1h7atB1Gbx3mp/13j7OmXwhwWzlro8L4hn6hxXAtPvZmpx\n\tjJoRwWC0jIieLfMhxHf3NooQlq55o376bpBsaGw3y4CXkoMnwmH6IROeB69H2ZHFLZb0\n\tWqOynztm1ibxrO813I/jCwbrGR49lNM1CUFWKMBzDP9MNkd7izAFRnfoLbzuhSUz0Y6p\n\toU3A==","X-Gm-Message-State":"AHQUAuYC7crw+SkOlMSA7xBEqPEAIQRoORmPokw91Y+BWOOFu7rMSruA\n\to7A4F9/PgqMeGOkNXKXpSj6+wAg3Imo=","X-Google-Smtp-Source":"AHgI3IaxUAwJhxfSy1nJyeFu3ckRt3wH0iyQnmLNN+Nz+VLUjQN9GExpbcGoN2VVcbf9ObkRgvnSUA==","X-Received":"by 2002:a19:d250:: with SMTP id j77mr1739688lfg.21.1551293489648;\n\tWed, 27 Feb 2019 10:51:29 -0800 (PST)","Date":"Wed, 27 Feb 2019 19:51:28 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190227185128.GU899@bigcity.dyn.berto.se>","References":"<20190226021857.28255-1-niklas.soderlund@ragnatech.se>\n\t<20190226021857.28255-5-niklas.soderlund@ragnatech.se>\n\t<20190227130432.GE4813@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190227130432.GE4813@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 4/8] cam: fix order camera is operated\n\ton","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":"Wed, 27 Feb 2019 18:51:30 -0000"}},{"id":938,"web_url":"https://patchwork.libcamera.org/comment/938/","msgid":"<20190227192916.GC13598@pendragon.ideasonboard.com>","date":"2019-02-27T19:29:16","subject":"Re: [libcamera-devel] [PATCH 4/8] cam: fix order camera is operated\n\ton","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Wed, Feb 27, 2019 at 07:51:28PM +0100, Niklas Söderlund wrote:\n> On 2019-02-27 15:04:32 +0200, Laurent Pinchart wrote:\n> > On Tue, Feb 26, 2019 at 03:18:53AM +0100, Niklas Söderlund wrote:\n> >> Upcoming enforcing of order the camera shall be operate on is not\n> >> compatible with the cam utility. Requests shall be queued after the\n> >> camera is started, not before.\n> > \n> > What are the practical implications of this on camera operation, from an\n> > application point of view ? In particular, when will the acquisition be\n> > started after this change ? Will the pipeline handlers be required to\n> > start acquisition on the sensor at start() time, throwing away all\n> > images until requests are queued ? Or will acquisition start when the\n> > first request is queued ? Sensors can take a long time to start, so it's\n> > important to document the exact behaviour.\n> \n> My understanding of the situation is that at start() time \n> VIDIOC_STREAMON shall be called. Then it's currently not defined what \n> other actions are expected by the camera or pipeline handler when \n> processing start(). I agree this need to be defined.\n> \n> I see two possible scenarios.\n> \n> 1. The library grantees that once a request have been queued it will \n> eventually complete disregarding of more requests are queued or not.\n\nI think we need to guarantee that once a request has been queued it will\ncomplete ASAP, regardless of if more requests are queued. Results may\nnot be optimal, but the request should complete nonetheless.\n\n> 2. The library informs the application about the request queue depth it \n> needs before requests starts to be processed.\n\nThat should be done too, as applications should try to fill the\nprocessing pipeline with request for optimal operation.\n\n> I'm hoping we can do scenario 1 but not sure if we can do that without \n> ugly scratch buffers in the library which I feel is a bit of a hack.\n\nScratch buffers may be needed, depending on the hardware, and I don't\nthink that would really be a hack, more the implementation of a best\neffort mode when the system becomes too slow to keep up.\n\n> Whatever scenario we pick the pipeline handler would need to know about \n> the request queue depth and act on that or report it to the application \n> so it can act. The pipeline handlers do not know this property today so \n> not much we can do yet.\n\nIt doesn't matter much today given our simple pipeline handlers :-) None\nof them require any particular queue depth. It will soon become an issue\nwe will need to tackle though.\n\n> In any case I think this is outside the scope of this patch. This \n> problem exists disregarding if we only allow queueing requests after \n> start() or if allow it both before and after. In fact I thought of this \n> change as a first step to address this issue as we now define that \n> requests can only be queued after start().\n\nMy point was that we need to document the behaviour related to stream\nstart. As explained, sensors can take a long time to start. Do we\nguarantee that this delay will be synchronous with the start() call, or\ndo we push it to the time when the first request(s) are queued ? In the\nformer case, can we guarantee that pipeline handlers will always be able\nto do so even without buffers queued ? If not, we have a problem, and\nneed to reconsider the queue-after-start order.\n\n> >> Signed-off-by: Niklas Söderlund \n> >> ---\n> >> src/cam/main.cpp | 18 +++++++++++-------\n> >> 1 file changed, 11 insertions(+), 7 deletions(-)\n> >> \n> >> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> >> index 4c2df583fe8e99b7..8df8844c33a2d052 100644\n> >> --- a/src/cam/main.cpp\n> >> +++ b/src/cam/main.cpp\n> >> @@ -133,6 +133,7 @@ static int capture()\n> >> \tint ret;\n> >> \n> >> \tstd::vector streams = camera->streams();\n> >> +\tstd::vector requests;\n> >> \n> >> \tret = configureStreams(camera.get(), streams);\n> >> \tif (ret < 0) {\n> >> @@ -169,21 +170,24 @@ static int capture()\n> >> \t\t\tgoto out;\n> >> \t\t}\n> >> \n> >> -\t\tret = camera->queueRequest(request);\n> >> -\t\tif (ret < 0) {\n> >> -\t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> >> -\t\t\tgoto out;\n> >> -\t\t}\n> >> +\t\trequests.push_back(request);\n> >> \t}\n> >> \n> >> -\tstd::cout << \"Capture until user interrupts by SIGINT\" << std::endl;\n> >> -\n> >> \tret = camera->start();\n> >> \tif (ret) {\n> >> \t\tstd::cout << \"Failed to start capture\" << std::endl;\n> >> \t\tgoto out;\n> >> \t}\n> >> \n> >> +\tfor (Request *request : requests) {\n> >> +\t\tret = camera->queueRequest(request);\n> >> +\t\tif (ret < 0) {\n> >> +\t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> >> +\t\t\tgoto out;\n> >> +\t\t}\n> >> +\t}\n> >> +\n> >> +\tstd::cout << \"Capture until user interrupts by SIGINT\" << std::endl;\n> >> \tret = loop->exec();\n> >> \n> >> \tret = camera->stop();","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 3F1FE600FD\n\tfor ;\n\tWed, 27 Feb 2019 20:29:30 +0100 (CET)","from pendragon.ideasonboard.com (unknown [83.145.195.18])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 68BC149;\n\tWed, 27 Feb 2019 20:29:29 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1551295769;\n\tbh=qKDX6H+8zRR0Gm6U47qHFBLkroj1RtNcjsyeCYolNPo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WNQks1ckwm8+VY23MplzfhPMhqWfnXDdmvl+Ol0V7CwqyAGb86bgR7KxBN9tO8okO\n\ttiQwecRDfK8x7cjr+9j0Mq8BYfhD7KPifiL6UOTtOUM8r/hAr4fJ31q6H8yP5m8tQC\n\tufeYbpvUhcH0Wsg8zcse+HV/bTFg6InIKkETZ2ho=","Date":"Wed, 27 Feb 2019 21:29:16 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190227192916.GC13598@pendragon.ideasonboard.com>","References":"<20190226021857.28255-1-niklas.soderlund@ragnatech.se>\n\t<20190226021857.28255-5-niklas.soderlund@ragnatech.se>\n\t<20190227130432.GE4813@pendragon.ideasonboard.com>\n\t<20190227185128.GU899@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":"<20190227185128.GU899@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 4/8] cam: fix order camera is operated\n\ton","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":"Wed, 27 Feb 2019 19:29:30 -0000"}}]