[{"id":19626,"web_url":"https://patchwork.libcamera.org/comment/19626/","msgid":"","date":"2021-09-13T00:19:38","subject":"Re: [libcamera-devel] [PATCH v2 5/9] android: camera_stream: Query\n\tpost-processing status","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Fri, Sep 10, 2021 at 12:36:34PM +0530, Umang Jain wrote:\n> CameraStream takes care of any post-processing required via the\n> CameraStream::process(). Since the post-processor will be moved\n> to a separate thread (in subsequent commits), the caller of\n> PostProcessor::process() will notify the associated CameraStream\n> instance about the post-processing completion via processComplete\n> signal. The CameraStream class should handle this event to query\n> the status and take next steps of reporting it back to upper layers\n> (i.e. CameraDevice).\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_stream.cpp | 13 +++++++++++++\n> src/android/camera_stream.h | 13 ++++++++++++-\n> 2 files changed, 25 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index d1c54797..996779c4 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -81,6 +81,9 @@ int CameraStream::configure()\n> \t\tint ret = postProcessor_->configure(configuration(), output);\n> \t\tif (ret)\n> \t\t\treturn ret;\n> +\n> +\t\tpostProcessor_->processComplete.connect(\n> +\t\t\tthis, &CameraStream::postProcessingComplete);\n> \t}\n> \n> \tif (allocator_) {\n> @@ -123,6 +126,16 @@ int CameraStream::process(const FrameBuffer *source,\n> \t\t\t\t resultMetadata, context);\n> }\n> \n> +void CameraStream::postProcessingComplete(PostProcessor::Status status,\n> +\t\t\t\t\t const Camera3RequestDescriptor *context)\n> +{\n> +\tProcessStatus processStatus;\n> +\tif (status == PostProcessor::Status::Succeeded)\n> +\t\tprocessStatus = ProcessStatus::Succeeded;\n> +\telse\n> +\t\tprocessStatus = ProcessStatus::Failed;\n> +}\n\nI'd squash this patch with 6/9, it doesn't do much on its own, and fails\nto compile due to the unused processStatus variable.\n\n> +\n> FrameBuffer *CameraStream::getBuffer()\n> {\n> \tif (!allocator_)\n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index d589f699..be076995 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -13,15 +13,18 @@\n> \n> #include \n> \n> +#include \n\nI don't think you need the signal header here.\n\n> +\n> #include \n> #include \n> #include \n> #include \n> #include \n> \n> +#include \"post_processor.h\"\n> +\n> class CameraDevice;\n> class CameraMetadata;\n> -class PostProcessor;\n> \n> struct Camera3RequestDescriptor;\n> \n> @@ -128,7 +131,15 @@ public:\n> \tlibcamera::FrameBuffer *getBuffer();\n> \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n> \n> +\tenum ProcessStatus {\n> +\t\tSucceeded,\n> +\t\tFailed,\n> +\t};\n\nDo we need this enum, could we use PostProcessor::Status everywhere ?\n\n> +\n> private:\n> +\tvoid postProcessingComplete(PostProcessor::Status status,\n> +\t\t\t\t const Camera3RequestDescriptor *context);\n> +\n> \tCameraDevice *const cameraDevice_;\n> \tconst libcamera::CameraConfiguration *config_;\n> \tconst Type type_;","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 23745BDC71\n\tfor ;\n\tMon, 13 Sep 2021 00:20:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 877D969184;\n\tMon, 13 Sep 2021 02:20:04 +0200 (CEST)","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 7355A60250\n\tfor ;\n\tMon, 13 Sep 2021 02:20:02 +0200 (CEST)","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 D93F49E;\n\tMon, 13 Sep 2021 02:20:01 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"pUj7oJ4m\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631492402;\n\tbh=l6VVtj1FPXKMQ16BTyitDVn+Nd9T0FvWzEUxYUz0294=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pUj7oJ4mdAZfIWuxlS1Sxx9hW5/DsWgU/w7KbyvyYvAUUQtJPt+HMgaE6+2a8FSpm\n\t3Cq/NvzI0AuXkEvH7/JhU3NdRUG1EYLA341d2djDl0Aon5uXqgKPlan/aGJtyaUmJ4\n\t/gEiGKy+GLLhcaB1rA3r+4gLX/bmNmFaF9T8u6kg=","Date":"Mon, 13 Sep 2021 03:19:38 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210910070638.467294-1-umang.jain@ideasonboard.com>\n\t<20210910070638.467294-6-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210910070638.467294-6-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 5/9] android: camera_stream: Query\n\tpost-processing status","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19633,"web_url":"https://patchwork.libcamera.org/comment/19633/","msgid":"","date":"2021-09-13T06:25:35","subject":"Re: [libcamera-devel] [PATCH v2 5/9] android: camera_stream: Query\n\tpost-processing status","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 9/13/21 5:49 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Fri, Sep 10, 2021 at 12:36:34PM +0530, Umang Jain wrote:\n>> CameraStream takes care of any post-processing required via the\n>> CameraStream::process(). Since the post-processor will be moved\n>> to a separate thread (in subsequent commits), the caller of\n>> PostProcessor::process() will notify the associated CameraStream\n>> instance about the post-processing completion via processComplete\n>> signal. The CameraStream class should handle this event to query\n>> the status and take next steps of reporting it back to upper layers\n>> (i.e. CameraDevice).\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/camera_stream.cpp | 13 +++++++++++++\n>> src/android/camera_stream.h | 13 ++++++++++++-\n>> 2 files changed, 25 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n>> index d1c54797..996779c4 100644\n>> --- a/src/android/camera_stream.cpp\n>> +++ b/src/android/camera_stream.cpp\n>> @@ -81,6 +81,9 @@ int CameraStream::configure()\n>> \t\tint ret = postProcessor_->configure(configuration(), output);\n>> \t\tif (ret)\n>> \t\t\treturn ret;\n>> +\n>> +\t\tpostProcessor_->processComplete.connect(\n>> +\t\t\tthis, &CameraStream::postProcessingComplete);\n>> \t}\n>> \n>> \tif (allocator_) {\n>> @@ -123,6 +126,16 @@ int CameraStream::process(const FrameBuffer *source,\n>> \t\t\t\t resultMetadata, context);\n>> }\n>> \n>> +void CameraStream::postProcessingComplete(PostProcessor::Status status,\n>> +\t\t\t\t\t const Camera3RequestDescriptor *context)\n>> +{\n>> +\tProcessStatus processStatus;\n>> +\tif (status == PostProcessor::Status::Succeeded)\n>> +\t\tprocessStatus = ProcessStatus::Succeeded;\n>> +\telse\n>> +\t\tprocessStatus = ProcessStatus::Failed;\n>> +}\n> I'd squash this patch with 6/9, it doesn't do much on its own, and fails\n> to compile due to the unused processStatus variable.\n>\n>> +\n>> FrameBuffer *CameraStream::getBuffer()\n>> {\n>> \tif (!allocator_)\n>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n>> index d589f699..be076995 100644\n>> --- a/src/android/camera_stream.h\n>> +++ b/src/android/camera_stream.h\n>> @@ -13,15 +13,18 @@\n>> \n>> #include \n>> \n>> +#include \n> I don't think you need the signal header here.\n>\n>> +\n>> #include \n>> #include \n>> #include \n>> #include \n>> #include \n>> \n>> +#include \"post_processor.h\"\n>> +\n>> class CameraDevice;\n>> class CameraMetadata;\n>> -class PostProcessor;\n>> \n>> struct Camera3RequestDescriptor;\n>> \n>> @@ -128,7 +131,15 @@ public:\n>> \tlibcamera::FrameBuffer *getBuffer();\n>> \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n>> \n>> +\tenum ProcessStatus {\n>> +\t\tSucceeded,\n>> +\t\tFailed,\n>> +\t};\n> Do we need this enum, could we use PostProcessor::Status everywhere ?\n\n\nYes, that would be my preference as well. But I think is if we do, we \nwill have PostProcessor:: in CameraDevice, which currently doesn't \ninteract with PostProcessor layer atleast directly. And I would like to \nkeep it that way.\n\n\nMaybe keep the status enum in CameraStream class and use it both in \nPostProcessor and CameraDevice. That way we achieve Status:: stuff \neverywhere probably.\n\n\n>\n>> +\n>> private:\n>> +\tvoid postProcessingComplete(PostProcessor::Status status,\n>> +\t\t\t\t const Camera3RequestDescriptor *context);\n>> +\n>> \tCameraDevice *const cameraDevice_;\n>> \tconst libcamera::CameraConfiguration *config_;\n>> \tconst Type type_;","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 3618FBDC71\n\tfor ;\n\tMon, 13 Sep 2021 06:25:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6D1DC69184;\n\tMon, 13 Sep 2021 08:25:42 +0200 (CEST)","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 E6E5C60131\n\tfor ;\n\tMon, 13 Sep 2021 08:25:40 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.152])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 199C29E;\n\tMon, 13 Sep 2021 08:25:39 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"aOrcBWGO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631514340;\n\tbh=t+GUWkDrANGNZG3NQTRIRlq1sXr3JKFAZa5Wbct3i4A=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=aOrcBWGOUjK49uNpia7iT7KGOJaNqEhfxBfUBPFt5/tQPZ82KJL1ZF+IFkRqJlLX3\n\tN/ahrIfqgguwe9PDEJeFyV8cVuU/ZeL69pg0xPJMCo4J4ozSnI+3b7+XQUAQsv+rMu\n\tTy1otJTF0vb5qCRjVEFqhtTdvg8KUh5KlWKPAlc0=","To":"Laurent Pinchart ","References":"<20210910070638.467294-1-umang.jain@ideasonboard.com>\n\t<20210910070638.467294-6-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"","Date":"Mon, 13 Sep 2021 11:55:35 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2 5/9] android: camera_stream: Query\n\tpost-processing status","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19692,"web_url":"https://patchwork.libcamera.org/comment/19692/","msgid":"","date":"2021-09-15T00:44:58","subject":"Re: [libcamera-devel] [PATCH v2 5/9] android: camera_stream: Query\n\tpost-processing status","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Mon, Sep 13, 2021 at 11:55:35AM +0530, Umang Jain wrote:\n> On 9/13/21 5:49 AM, Laurent Pinchart wrote:\n> > On Fri, Sep 10, 2021 at 12:36:34PM +0530, Umang Jain wrote:\n> >> CameraStream takes care of any post-processing required via the\n> >> CameraStream::process(). Since the post-processor will be moved\n> >> to a separate thread (in subsequent commits), the caller of\n> >> PostProcessor::process() will notify the associated CameraStream\n> >> instance about the post-processing completion via processComplete\n> >> signal. The CameraStream class should handle this event to query\n> >> the status and take next steps of reporting it back to upper layers\n> >> (i.e. CameraDevice).\n> >>\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> src/android/camera_stream.cpp | 13 +++++++++++++\n> >> src/android/camera_stream.h | 13 ++++++++++++-\n> >> 2 files changed, 25 insertions(+), 1 deletion(-)\n> >>\n> >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> >> index d1c54797..996779c4 100644\n> >> --- a/src/android/camera_stream.cpp\n> >> +++ b/src/android/camera_stream.cpp\n> >> @@ -81,6 +81,9 @@ int CameraStream::configure()\n> >> \t\tint ret = postProcessor_->configure(configuration(), output);\n> >> \t\tif (ret)\n> >> \t\t\treturn ret;\n> >> +\n> >> +\t\tpostProcessor_->processComplete.connect(\n> >> +\t\t\tthis, &CameraStream::postProcessingComplete);\n> >> \t}\n> >> \n> >> \tif (allocator_) {\n> >> @@ -123,6 +126,16 @@ int CameraStream::process(const FrameBuffer *source,\n> >> \t\t\t\t resultMetadata, context);\n> >> }\n> >> \n> >> +void CameraStream::postProcessingComplete(PostProcessor::Status status,\n> >> +\t\t\t\t\t const Camera3RequestDescriptor *context)\n> >> +{\n> >> +\tProcessStatus processStatus;\n> >> +\tif (status == PostProcessor::Status::Succeeded)\n> >> +\t\tprocessStatus = ProcessStatus::Succeeded;\n> >> +\telse\n> >> +\t\tprocessStatus = ProcessStatus::Failed;\n> >> +}\n> >\n> > I'd squash this patch with 6/9, it doesn't do much on its own, and fails\n> > to compile due to the unused processStatus variable.\n> >\n> >> +\n> >> FrameBuffer *CameraStream::getBuffer()\n> >> {\n> >> \tif (!allocator_)\n> >> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> >> index d589f699..be076995 100644\n> >> --- a/src/android/camera_stream.h\n> >> +++ b/src/android/camera_stream.h\n> >> @@ -13,15 +13,18 @@\n> >> \n> >> #include \n> >> \n> >> +#include \n> >\n> > I don't think you need the signal header here.\n> >\n> >> +\n> >> #include \n> >> #include \n> >> #include \n> >> #include \n> >> #include \n> >> \n> >> +#include \"post_processor.h\"\n> >> +\n> >> class CameraDevice;\n> >> class CameraMetadata;\n> >> -class PostProcessor;\n> >> \n> >> struct Camera3RequestDescriptor;\n> >> \n> >> @@ -128,7 +131,15 @@ public:\n> >> \tlibcamera::FrameBuffer *getBuffer();\n> >> \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n> >> \n> >> +\tenum ProcessStatus {\n> >> +\t\tSucceeded,\n> >> +\t\tFailed,\n> >> +\t};\n> >\n> > Do we need this enum, could we use PostProcessor::Status everywhere ?\n> \n> Yes, that would be my preference as well. But I think is if we do, we \n> will have PostProcessor:: in CameraDevice, which currently doesn't \n> interact with PostProcessor layer atleast directly. And I would like to \n> keep it that way.\n\nI was expecting that objection :-)\n\n> Maybe keep the status enum in CameraStream class and use it both in \n> PostProcessor and CameraDevice. That way we achieve Status:: stuff \n> everywhere probably.\n\nHow about adding the enum to Camera3RequestDescriptor ? It's a request\ncompletion status after all, so it would make sense to centralize it\nthere.\n\n> >> +\n> >> private:\n> >> +\tvoid postProcessingComplete(PostProcessor::Status status,\n> >> +\t\t\t\t const Camera3RequestDescriptor *context);\n> >> +\n> >> \tCameraDevice *const cameraDevice_;\n> >> \tconst libcamera::CameraConfiguration *config_;\n> >> \tconst Type type_;","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 1BDFBBDB1D\n\tfor ;\n\tWed, 15 Sep 2021 00:45:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 86AC069189;\n\tWed, 15 Sep 2021 02:45:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2638260249\n\tfor ;\n\tWed, 15 Sep 2021 02:45:24 +0200 (CEST)","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 935CB2A5;\n\tWed, 15 Sep 2021 02:45:23 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ZD0Ctv08\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631666723;\n\tbh=C6pU3b9ERJ7f9NZkakzcxO1X0X6G1crfJXDofHnn5aU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZD0Ctv08nVyahBMs+ZLLPqp6enP6yZWlPftPPIFPg5NJyuw3RjJvBwOypKgO54ikQ\n\tJ9vIwdaODhQ40M+SdgpbZamDjGoqLiQmHkNnc0n+LFotgoT13h8WvpOnpL15jee7/T\n\t01ya5gepLvdKdj1jE6k8r/74RCDxJR3StxdiXIL4=","Date":"Wed, 15 Sep 2021 03:44:58 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210910070638.467294-1-umang.jain@ideasonboard.com>\n\t<20210910070638.467294-6-umang.jain@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 5/9] android: camera_stream: Query\n\tpost-processing status","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]