[{"id":19624,"web_url":"https://patchwork.libcamera.org/comment/19624/","msgid":"","date":"2021-09-13T00:01:53","subject":"Re: [libcamera-devel] [PATCH v2 3/9] android: post_processor: Plumb\n\tdown the process() with context","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:32PM +0530, Umang Jain wrote:\n> The context required for post processing of a camera request is saved\n> via Camera3RequestDescriptor. The context needs to be plumbed to\n> PostProcessor::process() which should be passed a const pointer to\n> Camera3RequestDescriptor. In subsequent commits, we will prepare\n> the post-processor to run asynchrnoously hence, it will require the\n> context pointer to be emitted back to the upper layers, to identify for\n> which request the post-processing has been completed.\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 3 ++-\n> src/android/camera_stream.cpp | 6 ++++--\n> src/android/camera_stream.h | 5 ++++-\n> src/android/jpeg/post_processor_jpeg.cpp | 3 ++-\n> src/android/jpeg/post_processor_jpeg.h | 3 ++-\n> src/android/post_processor.h | 5 ++++-\n> src/android/yuv/post_processor_yuv.cpp | 3 ++-\n> src/android/yuv/post_processor_yuv.h | 3 ++-\n> 8 files changed, 22 insertions(+), 9 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 0840c056..84549d04 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1150,7 +1150,8 @@ void CameraDevice::requestComplete(Request *request)\n> \n> \t\tint ret = cameraStream->process(src, *buffer.buffer,\n> \t\t\t\t\t\tdescriptor.settings_,\n\nNow that the request metadata can be accessed through the descriptor\npointer, could you please drop this argument ?\n\n> -\t\t\t\t\t\tresultMetadata.get());\n> +\t\t\t\t\t\tresultMetadata.get(),\n> +\t\t\t\t\t\t&descriptor);\n\nI'd move the descriptor parameter before the metadata. I think it would\nalso make sense to store the result metadata in\nCamera3RequestDescriptor, but that can go to another patch.\n\n> \t\t/*\n> \t\t * Return the FrameBuffer to the CameraStream now that we're\n> \t\t * done processing it.\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index 0fed5382..d1c54797 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -101,7 +101,8 @@ int CameraStream::configure()\n> int CameraStream::process(const FrameBuffer *source,\n> \t\t\t buffer_handle_t camera3Dest,\n> \t\t\t const CameraMetadata &requestMetadata,\n> -\t\t\t CameraMetadata *resultMetadata)\n> +\t\t\t CameraMetadata *resultMetadata,\n> +\t\t\t const Camera3RequestDescriptor *context)\n> {\n> \tif (!postProcessor_)\n> \t\treturn 0;\n> @@ -118,7 +119,8 @@ int CameraStream::process(const FrameBuffer *source,\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\treturn postProcessor_->process(source, &dest, requestMetadata, resultMetadata);\n> +\treturn postProcessor_->process(source, &dest, requestMetadata,\n> +\t\t\t\t resultMetadata, context);\n> }\n> \n> FrameBuffer *CameraStream::getBuffer()\n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index 5c232cb6..d589f699 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -23,6 +23,8 @@ class CameraDevice;\n> class CameraMetadata;\n> class PostProcessor;\n> \n> +struct Camera3RequestDescriptor;\n> +\n> class CameraStream\n> {\n> public:\n> @@ -121,7 +123,8 @@ public:\n> \tint process(const libcamera::FrameBuffer *source,\n> \t\t buffer_handle_t camera3Dest,\n> \t\t const CameraMetadata &requestMetadata,\n> -\t\t CameraMetadata *resultMetadata);\n> +\t\t CameraMetadata *resultMetadata,\n> +\t\t const Camera3RequestDescriptor *context);\n> \tlibcamera::FrameBuffer *getBuffer();\n> \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n> \n> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> index cb45f86b..741eeb21 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -100,7 +100,8 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer *source,\n> int PostProcessorJpeg::process(const FrameBuffer *source,\n> \t\t\t CameraBuffer *destination,\n> \t\t\t const CameraMetadata &requestMetadata,\n> -\t\t\t CameraMetadata *resultMetadata)\n> +\t\t\t CameraMetadata *resultMetadata,\n> +\t\t\t const Camera3RequestDescriptor *context)\n> {\n> \tif (!encoder_)\n> \t\treturn 0;\n> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> index c4b2e9ef..72c38fdb 100644\n> --- a/src/android/jpeg/post_processor_jpeg.h\n> +++ b/src/android/jpeg/post_processor_jpeg.h\n> @@ -25,7 +25,8 @@ public:\n> \tint process(const libcamera::FrameBuffer *source,\n> \t\t CameraBuffer *destination,\n> \t\t const CameraMetadata &requestMetadata,\n> -\t\t CameraMetadata *resultMetadata) override;\n> +\t\t CameraMetadata *resultMetadata,\n> +\t\t const Camera3RequestDescriptor *context) override;\n> \n> private:\n> \tvoid generateThumbnail(const libcamera::FrameBuffer *source,\n> diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> index 61dfb6d4..1213e7e6 100644\n> --- a/src/android/post_processor.h\n> +++ b/src/android/post_processor.h\n> @@ -14,6 +14,8 @@\n> \n> class CameraMetadata;\n> \n> +struct Camera3RequestDescriptor;\n> +\n> class PostProcessor\n> {\n> public:\n> @@ -24,7 +26,8 @@ public:\n> \tvirtual int process(const libcamera::FrameBuffer *source,\n> \t\t\t CameraBuffer *destination,\n> \t\t\t const CameraMetadata &requestMetadata,\n> -\t\t\t CameraMetadata *resultMetadata) = 0;\n> +\t\t\t CameraMetadata *resultMetadata,\n> +\t\t\t const Camera3RequestDescriptor *context) = 0;\n> };\n> \n> #endif /* __ANDROID_POST_PROCESSOR_H__ */\n> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp\n> index 0a874886..13a78abe 100644\n> --- a/src/android/yuv/post_processor_yuv.cpp\n> +++ b/src/android/yuv/post_processor_yuv.cpp\n> @@ -52,7 +52,8 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg,\n> int PostProcessorYuv::process(const FrameBuffer *source,\n> \t\t\t CameraBuffer *destination,\n> \t\t\t [[maybe_unused]] const CameraMetadata &requestMetadata,\n> -\t\t\t [[maybe_unused]] CameraMetadata *metadata)\n> +\t\t\t [[maybe_unused]] CameraMetadata *metadata,\n> +\t\t\t [[maybe_unused]] const Camera3RequestDescriptor *context)\n> {\n> \tif (!isValidBuffers(source, *destination))\n> \t\treturn -EINVAL;\n> diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h\n> index 44a04113..38a30f91 100644\n> --- a/src/android/yuv/post_processor_yuv.h\n> +++ b/src/android/yuv/post_processor_yuv.h\n> @@ -23,7 +23,8 @@ public:\n> \tint process(const libcamera::FrameBuffer *source,\n> \t\t CameraBuffer *destination,\n> \t\t const CameraMetadata &requestMetadata,\n> -\t\t CameraMetadata *metadata) override;\n> +\t\t CameraMetadata *metadata,\n> +\t\t const Camera3RequestDescriptor *context) override;\n\nI'd name the parameter \"request\" instead of \"context\", \"context\" is more\nvague.\n\nWith those changes,\n\nReviewed-by: Laurent Pinchart \n\n> \n> private:\n> \tbool isValidBuffers(const libcamera::FrameBuffer *source,","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 6306ABDC71\n\tfor ;\n\tMon, 13 Sep 2021 00:02:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A7E0369185;\n\tMon, 13 Sep 2021 02:02:19 +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 23D4E69182\n\tfor ;\n\tMon, 13 Sep 2021 02:02:18 +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 985049E;\n\tMon, 13 Sep 2021 02:02:17 +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=\"F0/9CeLd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631491337;\n\tbh=EtIt2hwQrGcR2PVP13EvFKGpZL2gvqm4grgbnaFpxEk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=F0/9CeLdhjSJ3MpJjXTHXRBZjcYJNuGsGhTRqEF+irYoi4gSiexT9V5/iAk5Z0VwH\n\teosVNLZ4JuSKGGgQhOmRcsfL3hunhsnLoe+EClndi/3rqOpak1Hcjd+cktAAefTawF\n\t7OuP+MhGP+nsru1vsWIA/JtVZyiBJiZOxvtEoDgg=","Date":"Mon, 13 Sep 2021 03:01:53 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210910070638.467294-1-umang.jain@ideasonboard.com>\n\t<20210910070638.467294-4-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210910070638.467294-4-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 3/9] android: post_processor: Plumb\n\tdown the process() with context","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":19632,"web_url":"https://patchwork.libcamera.org/comment/19632/","msgid":"<0235b818-dc34-2f13-6c69-9a7c85820602@ideasonboard.com>","date":"2021-09-13T05:43:53","subject":"Re: [libcamera-devel] [PATCH v2 3/9] android: post_processor: Plumb\n\tdown the process() with context","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:31 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Fri, Sep 10, 2021 at 12:36:32PM +0530, Umang Jain wrote:\n>> The context required for post processing of a camera request is saved\n>> via Camera3RequestDescriptor. The context needs to be plumbed to\n>> PostProcessor::process() which should be passed a const pointer to\n>> Camera3RequestDescriptor. In subsequent commits, we will prepare\n>> the post-processor to run asynchrnoously hence, it will require the\n>> context pointer to be emitted back to the upper layers, to identify for\n>> which request the post-processing has been completed.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/camera_device.cpp | 3 ++-\n>> src/android/camera_stream.cpp | 6 ++++--\n>> src/android/camera_stream.h | 5 ++++-\n>> src/android/jpeg/post_processor_jpeg.cpp | 3 ++-\n>> src/android/jpeg/post_processor_jpeg.h | 3 ++-\n>> src/android/post_processor.h | 5 ++++-\n>> src/android/yuv/post_processor_yuv.cpp | 3 ++-\n>> src/android/yuv/post_processor_yuv.h | 3 ++-\n>> 8 files changed, 22 insertions(+), 9 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index 0840c056..84549d04 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -1150,7 +1150,8 @@ void CameraDevice::requestComplete(Request *request)\n>> \n>> \t\tint ret = cameraStream->process(src, *buffer.buffer,\n>> \t\t\t\t\t\tdescriptor.settings_,\n> Now that the request metadata can be accessed through the descriptor\n> pointer, could you please drop this argument ?\n\n\nSince it's const reference and only read down in the function chain, I \nthink that would make sense to drop it. I'll double it though if it's \nnot meant to be read anywhere.\n\n>\n>> -\t\t\t\t\t\tresultMetadata.get());\n>> +\t\t\t\t\t\tresultMetadata.get(),\n>> +\t\t\t\t\t\t&descriptor);\n> I'd move the descriptor parameter before the metadata. I think it would\n> also make sense to store the result metadata in\n> Camera3RequestDescriptor, but that can go to another patch.\n\n\nResultMetadata seems to be populated/modified in the JPEG \npost-processor. It won't make sense to access it through the descriptor \npointer since it's a const *.\n\nThe idea here is to not do any access / modification through the \ndescriptor pointer passed (you see I have passed it as const). Which \never member (even part of Camera3RequestDescriptor struct) that needs \nmodification as such, should be passed explicitly in the process() \nfunction argument. Does it make sense?\n\n\n>\n>> \t\t/*\n>> \t\t * Return the FrameBuffer to the CameraStream now that we're\n>> \t\t * done processing it.\n>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n>> index 0fed5382..d1c54797 100644\n>> --- a/src/android/camera_stream.cpp\n>> +++ b/src/android/camera_stream.cpp\n>> @@ -101,7 +101,8 @@ int CameraStream::configure()\n>> int CameraStream::process(const FrameBuffer *source,\n>> \t\t\t buffer_handle_t camera3Dest,\n>> \t\t\t const CameraMetadata &requestMetadata,\n>> -\t\t\t CameraMetadata *resultMetadata)\n>> +\t\t\t CameraMetadata *resultMetadata,\n>> +\t\t\t const Camera3RequestDescriptor *context)\n>> {\n>> \tif (!postProcessor_)\n>> \t\treturn 0;\n>> @@ -118,7 +119,8 @@ int CameraStream::process(const FrameBuffer *source,\n>> \t\treturn -EINVAL;\n>> \t}\n>> \n>> -\treturn postProcessor_->process(source, &dest, requestMetadata, resultMetadata);\n>> +\treturn postProcessor_->process(source, &dest, requestMetadata,\n>> +\t\t\t\t resultMetadata, context);\n>> }\n>> \n>> FrameBuffer *CameraStream::getBuffer()\n>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n>> index 5c232cb6..d589f699 100644\n>> --- a/src/android/camera_stream.h\n>> +++ b/src/android/camera_stream.h\n>> @@ -23,6 +23,8 @@ class CameraDevice;\n>> class CameraMetadata;\n>> class PostProcessor;\n>> \n>> +struct Camera3RequestDescriptor;\n>> +\n>> class CameraStream\n>> {\n>> public:\n>> @@ -121,7 +123,8 @@ public:\n>> \tint process(const libcamera::FrameBuffer *source,\n>> \t\t buffer_handle_t camera3Dest,\n>> \t\t const CameraMetadata &requestMetadata,\n>> -\t\t CameraMetadata *resultMetadata);\n>> +\t\t CameraMetadata *resultMetadata,\n>> +\t\t const Camera3RequestDescriptor *context);\n>> \tlibcamera::FrameBuffer *getBuffer();\n>> \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n>> \n>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n>> index cb45f86b..741eeb21 100644\n>> --- a/src/android/jpeg/post_processor_jpeg.cpp\n>> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n>> @@ -100,7 +100,8 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer *source,\n>> int PostProcessorJpeg::process(const FrameBuffer *source,\n>> \t\t\t CameraBuffer *destination,\n>> \t\t\t const CameraMetadata &requestMetadata,\n>> -\t\t\t CameraMetadata *resultMetadata)\n>> +\t\t\t CameraMetadata *resultMetadata,\n>> +\t\t\t const Camera3RequestDescriptor *context)\n>> {\n>> \tif (!encoder_)\n>> \t\treturn 0;\n>> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n>> index c4b2e9ef..72c38fdb 100644\n>> --- a/src/android/jpeg/post_processor_jpeg.h\n>> +++ b/src/android/jpeg/post_processor_jpeg.h\n>> @@ -25,7 +25,8 @@ public:\n>> \tint process(const libcamera::FrameBuffer *source,\n>> \t\t CameraBuffer *destination,\n>> \t\t const CameraMetadata &requestMetadata,\n>> -\t\t CameraMetadata *resultMetadata) override;\n>> +\t\t CameraMetadata *resultMetadata,\n>> +\t\t const Camera3RequestDescriptor *context) override;\n>> \n>> private:\n>> \tvoid generateThumbnail(const libcamera::FrameBuffer *source,\n>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n>> index 61dfb6d4..1213e7e6 100644\n>> --- a/src/android/post_processor.h\n>> +++ b/src/android/post_processor.h\n>> @@ -14,6 +14,8 @@\n>> \n>> class CameraMetadata;\n>> \n>> +struct Camera3RequestDescriptor;\n>> +\n>> class PostProcessor\n>> {\n>> public:\n>> @@ -24,7 +26,8 @@ public:\n>> \tvirtual int process(const libcamera::FrameBuffer *source,\n>> \t\t\t CameraBuffer *destination,\n>> \t\t\t const CameraMetadata &requestMetadata,\n>> -\t\t\t CameraMetadata *resultMetadata) = 0;\n>> +\t\t\t CameraMetadata *resultMetadata,\n>> +\t\t\t const Camera3RequestDescriptor *context) = 0;\n>> };\n>> \n>> #endif /* __ANDROID_POST_PROCESSOR_H__ */\n>> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp\n>> index 0a874886..13a78abe 100644\n>> --- a/src/android/yuv/post_processor_yuv.cpp\n>> +++ b/src/android/yuv/post_processor_yuv.cpp\n>> @@ -52,7 +52,8 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg,\n>> int PostProcessorYuv::process(const FrameBuffer *source,\n>> \t\t\t CameraBuffer *destination,\n>> \t\t\t [[maybe_unused]] const CameraMetadata &requestMetadata,\n>> -\t\t\t [[maybe_unused]] CameraMetadata *metadata)\n>> +\t\t\t [[maybe_unused]] CameraMetadata *metadata,\n>> +\t\t\t [[maybe_unused]] const Camera3RequestDescriptor *context)\n>> {\n>> \tif (!isValidBuffers(source, *destination))\n>> \t\treturn -EINVAL;\n>> diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h\n>> index 44a04113..38a30f91 100644\n>> --- a/src/android/yuv/post_processor_yuv.h\n>> +++ b/src/android/yuv/post_processor_yuv.h\n>> @@ -23,7 +23,8 @@ public:\n>> \tint process(const libcamera::FrameBuffer *source,\n>> \t\t CameraBuffer *destination,\n>> \t\t const CameraMetadata &requestMetadata,\n>> -\t\t CameraMetadata *metadata) override;\n>> +\t\t CameraMetadata *metadata,\n>> +\t\t const Camera3RequestDescriptor *context) override;\n> I'd name the parameter \"request\" instead of \"context\", \"context\" is more\n> vague.\n>\n> With those changes,\n>\n> Reviewed-by: Laurent Pinchart \n>\n>> \n>> private:\n>> \tbool isValidBuffers(const libcamera::FrameBuffer *source,","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 58D49BDB1D\n\tfor ;\n\tMon, 13 Sep 2021 05:44:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7FADA69184;\n\tMon, 13 Sep 2021 07:44:00 +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 EF68160131\n\tfor ;\n\tMon, 13 Sep 2021 07:43:58 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.152])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E8D049E;\n\tMon, 13 Sep 2021 07:43:57 +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=\"Fx1Gy1HY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631511838;\n\tbh=uC1HGjPAyG+NdHoT1QyiCv9+BIwBil2FN8ZgNaXdyLM=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=Fx1Gy1HYxFkVFsb8x3tCIDHLJ/SDPW0c+C1BLaKFvDjChxEtkwbJuJiG5XMz4hfgQ\n\tf9O0a8eLFdPA0OSs5BoGgy3RQpRygaJ10ni/c1f4fY1u6I2kETQw31bm4g3Mgq/jWJ\n\tK7ZQ1HXLLzlNGL+7gmzRXnOa0jWSaXYf8UHHuEdI=","To":"Laurent Pinchart ","References":"<20210910070638.467294-1-umang.jain@ideasonboard.com>\n\t<20210910070638.467294-4-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"<0235b818-dc34-2f13-6c69-9a7c85820602@ideasonboard.com>","Date":"Mon, 13 Sep 2021 11:13:53 +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 3/9] android: post_processor: Plumb\n\tdown the process() with context","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":19691,"web_url":"https://patchwork.libcamera.org/comment/19691/","msgid":"","date":"2021-09-15T00:42:58","subject":"Re: [libcamera-devel] [PATCH v2 3/9] android: post_processor: Plumb\n\tdown the process() with context","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:13:53AM +0530, Umang Jain wrote:\n> On 9/13/21 5:31 AM, Laurent Pinchart wrote:\n> > On Fri, Sep 10, 2021 at 12:36:32PM +0530, Umang Jain wrote:\n> >> The context required for post processing of a camera request is saved\n> >> via Camera3RequestDescriptor. The context needs to be plumbed to\n> >> PostProcessor::process() which should be passed a const pointer to\n> >> Camera3RequestDescriptor. In subsequent commits, we will prepare\n> >> the post-processor to run asynchrnoously hence, it will require the\n> >> context pointer to be emitted back to the upper layers, to identify for\n> >> which request the post-processing has been completed.\n> >>\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> src/android/camera_device.cpp | 3 ++-\n> >> src/android/camera_stream.cpp | 6 ++++--\n> >> src/android/camera_stream.h | 5 ++++-\n> >> src/android/jpeg/post_processor_jpeg.cpp | 3 ++-\n> >> src/android/jpeg/post_processor_jpeg.h | 3 ++-\n> >> src/android/post_processor.h | 5 ++++-\n> >> src/android/yuv/post_processor_yuv.cpp | 3 ++-\n> >> src/android/yuv/post_processor_yuv.h | 3 ++-\n> >> 8 files changed, 22 insertions(+), 9 deletions(-)\n> >>\n> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >> index 0840c056..84549d04 100644\n> >> --- a/src/android/camera_device.cpp\n> >> +++ b/src/android/camera_device.cpp\n> >> @@ -1150,7 +1150,8 @@ void CameraDevice::requestComplete(Request *request)\n> >> \n> >> \t\tint ret = cameraStream->process(src, *buffer.buffer,\n> >> \t\t\t\t\t\tdescriptor.settings_,\n> >\n> > Now that the request metadata can be accessed through the descriptor\n> > pointer, could you please drop this argument ?\n> \n> Since it's const reference and only read down in the function chain, I \n> think that would make sense to drop it. I'll double it though if it's \n> not meant to be read anywhere.\n> \n> >> -\t\t\t\t\t\tresultMetadata.get());\n> >> +\t\t\t\t\t\tresultMetadata.get(),\n> >> +\t\t\t\t\t\t&descriptor);\n> >\n> > I'd move the descriptor parameter before the metadata. I think it would\n> > also make sense to store the result metadata in\n> > Camera3RequestDescriptor, but that can go to another patch.\n> \n> ResultMetadata seems to be populated/modified in the JPEG \n> post-processor. It won't make sense to access it through the descriptor \n> pointer since it's a const *.\n> \n> The idea here is to not do any access / modification through the \n> descriptor pointer passed (you see I have passed it as const). Which \n> ever member (even part of Camera3RequestDescriptor struct) that needs \n> modification as such, should be passed explicitly in the process() \n> function argument. Does it make sense?\n\nIt's a good point. I'm torn here, between the options of passing a\nnon-const pointer to the descriptor (pros: we only need only pointer,\ncons: the stream and post-processor could then modify anything in the\ndescriptor) and passing a const descriptor and a non-const metadata\n(pros and cons are the opposite as above). If it's only two pointers I\ncould live with it, but if we add other non-const members of descriptors\nto the list of modifiable data, I'd prefer a non-const pointer to the\ndescriptor.\n\nI think it's reasonable in this case, given that we're dealing with an\ninternal API, to expect the camera, stream and post-processor to\ncollaborate to some extent, and not access data in a way they're not\nsupposed to. I'd consider this coupling to be especially tight when it\ncomes to the camera and stream class, so I'd be tempted to pass the\ndescriptor as non-const to CameraStream::process() and drop the result\nmetadata pointer from there, even if we keep them separate for the\npost-processor.\n\n> >> \t\t/*\n> >> \t\t * Return the FrameBuffer to the CameraStream now that we're\n> >> \t\t * done processing it.\n> >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> >> index 0fed5382..d1c54797 100644\n> >> --- a/src/android/camera_stream.cpp\n> >> +++ b/src/android/camera_stream.cpp\n> >> @@ -101,7 +101,8 @@ int CameraStream::configure()\n> >> int CameraStream::process(const FrameBuffer *source,\n> >> \t\t\t buffer_handle_t camera3Dest,\n> >> \t\t\t const CameraMetadata &requestMetadata,\n> >> -\t\t\t CameraMetadata *resultMetadata)\n> >> +\t\t\t CameraMetadata *resultMetadata,\n> >> +\t\t\t const Camera3RequestDescriptor *context)\n> >> {\n> >> \tif (!postProcessor_)\n> >> \t\treturn 0;\n> >> @@ -118,7 +119,8 @@ int CameraStream::process(const FrameBuffer *source,\n> >> \t\treturn -EINVAL;\n> >> \t}\n> >> \n> >> -\treturn postProcessor_->process(source, &dest, requestMetadata, resultMetadata);\n> >> +\treturn postProcessor_->process(source, &dest, requestMetadata,\n> >> +\t\t\t\t resultMetadata, context);\n> >> }\n> >> \n> >> FrameBuffer *CameraStream::getBuffer()\n> >> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> >> index 5c232cb6..d589f699 100644\n> >> --- a/src/android/camera_stream.h\n> >> +++ b/src/android/camera_stream.h\n> >> @@ -23,6 +23,8 @@ class CameraDevice;\n> >> class CameraMetadata;\n> >> class PostProcessor;\n> >> \n> >> +struct Camera3RequestDescriptor;\n> >> +\n> >> class CameraStream\n> >> {\n> >> public:\n> >> @@ -121,7 +123,8 @@ public:\n> >> \tint process(const libcamera::FrameBuffer *source,\n> >> \t\t buffer_handle_t camera3Dest,\n> >> \t\t const CameraMetadata &requestMetadata,\n> >> -\t\t CameraMetadata *resultMetadata);\n> >> +\t\t CameraMetadata *resultMetadata,\n> >> +\t\t const Camera3RequestDescriptor *context);\n> >> \tlibcamera::FrameBuffer *getBuffer();\n> >> \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n> >> \n> >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> >> index cb45f86b..741eeb21 100644\n> >> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> >> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> >> @@ -100,7 +100,8 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer *source,\n> >> int PostProcessorJpeg::process(const FrameBuffer *source,\n> >> \t\t\t CameraBuffer *destination,\n> >> \t\t\t const CameraMetadata &requestMetadata,\n> >> -\t\t\t CameraMetadata *resultMetadata)\n> >> +\t\t\t CameraMetadata *resultMetadata,\n> >> +\t\t\t const Camera3RequestDescriptor *context)\n> >> {\n> >> \tif (!encoder_)\n> >> \t\treturn 0;\n> >> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> >> index c4b2e9ef..72c38fdb 100644\n> >> --- a/src/android/jpeg/post_processor_jpeg.h\n> >> +++ b/src/android/jpeg/post_processor_jpeg.h\n> >> @@ -25,7 +25,8 @@ public:\n> >> \tint process(const libcamera::FrameBuffer *source,\n> >> \t\t CameraBuffer *destination,\n> >> \t\t const CameraMetadata &requestMetadata,\n> >> -\t\t CameraMetadata *resultMetadata) override;\n> >> +\t\t CameraMetadata *resultMetadata,\n> >> +\t\t const Camera3RequestDescriptor *context) override;\n> >> \n> >> private:\n> >> \tvoid generateThumbnail(const libcamera::FrameBuffer *source,\n> >> diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> >> index 61dfb6d4..1213e7e6 100644\n> >> --- a/src/android/post_processor.h\n> >> +++ b/src/android/post_processor.h\n> >> @@ -14,6 +14,8 @@\n> >> \n> >> class CameraMetadata;\n> >> \n> >> +struct Camera3RequestDescriptor;\n> >> +\n> >> class PostProcessor\n> >> {\n> >> public:\n> >> @@ -24,7 +26,8 @@ public:\n> >> \tvirtual int process(const libcamera::FrameBuffer *source,\n> >> \t\t\t CameraBuffer *destination,\n> >> \t\t\t const CameraMetadata &requestMetadata,\n> >> -\t\t\t CameraMetadata *resultMetadata) = 0;\n> >> +\t\t\t CameraMetadata *resultMetadata,\n> >> +\t\t\t const Camera3RequestDescriptor *context) = 0;\n> >> };\n> >> \n> >> #endif /* __ANDROID_POST_PROCESSOR_H__ */\n> >> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp\n> >> index 0a874886..13a78abe 100644\n> >> --- a/src/android/yuv/post_processor_yuv.cpp\n> >> +++ b/src/android/yuv/post_processor_yuv.cpp\n> >> @@ -52,7 +52,8 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg,\n> >> int PostProcessorYuv::process(const FrameBuffer *source,\n> >> \t\t\t CameraBuffer *destination,\n> >> \t\t\t [[maybe_unused]] const CameraMetadata &requestMetadata,\n> >> -\t\t\t [[maybe_unused]] CameraMetadata *metadata)\n> >> +\t\t\t [[maybe_unused]] CameraMetadata *metadata,\n> >> +\t\t\t [[maybe_unused]] const Camera3RequestDescriptor *context)\n> >> {\n> >> \tif (!isValidBuffers(source, *destination))\n> >> \t\treturn -EINVAL;\n> >> diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h\n> >> index 44a04113..38a30f91 100644\n> >> --- a/src/android/yuv/post_processor_yuv.h\n> >> +++ b/src/android/yuv/post_processor_yuv.h\n> >> @@ -23,7 +23,8 @@ public:\n> >> \tint process(const libcamera::FrameBuffer *source,\n> >> \t\t CameraBuffer *destination,\n> >> \t\t const CameraMetadata &requestMetadata,\n> >> -\t\t CameraMetadata *metadata) override;\n> >> +\t\t CameraMetadata *metadata,\n> >> +\t\t const Camera3RequestDescriptor *context) override;\n> >\n> > I'd name the parameter \"request\" instead of \"context\", \"context\" is more\n> > vague.\n> >\n> > With those changes,\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> >> private:\n> >> \tbool isValidBuffers(const libcamera::FrameBuffer *source,","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 61E70BDB1D\n\tfor ;\n\tWed, 15 Sep 2021 00:43:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 827E069189;\n\tWed, 15 Sep 2021 02:43: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 07BDD60249\n\tfor ;\n\tWed, 15 Sep 2021 02:43: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 6D5202A5;\n\tWed, 15 Sep 2021 02:43: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=\"FnCsi5sV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631666603;\n\tbh=idrmdaS7KjTKnYzAhdnweDWBVaGn7HcbBJvOJG3YuXo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FnCsi5sVXXiLmyTaAUigN79tFN1SNBh52O/63X1QgD8m8Rt98d1QIu8HcJXootajs\n\tQyGnRWDSJKxPZkp4xlS5pvlvtT9Jcs0q/3cRQuTkWDkF5P8+OvU8AllILlrepUYqB9\n\tD1jld8COjw53Vb5pz5o/UVsWktJmVX3S5YP9zX+I=","Date":"Wed, 15 Sep 2021 03:42:58 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210910070638.467294-1-umang.jain@ideasonboard.com>\n\t<20210910070638.467294-4-umang.jain@ideasonboard.com>\n\t\n\t<0235b818-dc34-2f13-6c69-9a7c85820602@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<0235b818-dc34-2f13-6c69-9a7c85820602@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 3/9] android: post_processor: Plumb\n\tdown the process() with context","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\" "}}]