[{"id":20409,"web_url":"https://patchwork.libcamera.org/comment/20409/","msgid":"","date":"2021-10-25T05:23:24","subject":"Re: [libcamera-devel] [PATCH v6 4/7] android: post_processor:\n\tConsolidate contextual information","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 Sat, Oct 23, 2021 at 04:02:59PM +0530, Umang Jain wrote:\n> Save and provide the context for post-processor of a camera stream\n> via Camera3RequestDescriptor::StreamBuffer. We extend the structure\n> to include source and destination buffers for the post processor, along\n> with CameraStream::Type::Internal buffer pointer (if any). In addition\n> to that, a back pointer to Camera3RequestDescriptor is convienient to\n> get access to overall descriptor (status, metadata settings etc.)\n> \n> Also, migrate PostProcessor::process() signature to use\n> Camera3RequestDescriptor::StreamBuffer only. This will be helpful\n> when we move to async post-processing in subsequent commits.\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 11 ++++++-----\n> src/android/camera_request.cpp | 3 ++-\n> src/android/camera_request.h | 5 +++++\n> src/android/camera_stream.cpp | 14 ++++++++------\n> src/android/camera_stream.h | 3 +--\n> src/android/jpeg/post_processor_jpeg.cpp | 12 +++++++-----\n> src/android/jpeg/post_processor_jpeg.h | 4 +---\n> src/android/post_processor.h | 7 ++-----\n> src/android/yuv/post_processor_yuv.cpp | 7 ++++---\n> src/android/yuv/post_processor_yuv.h | 4 +---\n> 10 files changed, 37 insertions(+), 33 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index f4d4fb09..2a98a2e6 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -953,6 +953,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\t\t * once it has been processed.\n> \t\t\t */\n> \t\t\tframeBuffer = cameraStream->getBuffer();\n> +\t\t\tbuffer.internalBuffer = frameBuffer;\n> \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n> \t\t\tbreak;\n> \t\t}\n> @@ -1134,14 +1135,14 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t\tcontinue;\n> \t\t}\n> \n\nShould we store src in buffer here instead of in\nCameraStream::process(), to make the signature of all process()\nfunctions the same ?\n\n> -\t\tint ret = stream->process(*src, buffer, descriptor);\n> +\t\tint ret = stream->process(*src, buffer);\n> \n> \t\t/*\n> -\t\t * Return the FrameBuffer to the CameraStream now that we're\n> -\t\t * done processing it.\n> +\t\t * If the framebuffer is internal to CameraStream return it back\n> +\t\t * now that we're done processing it.\n> \t\t */\n> -\t\tif (stream->type() == CameraStream::Type::Internal)\n> -\t\t\tstream->putBuffer(src);\n> +\t\tif (buffer.internalBuffer)\n> +\t\t\tstream->putBuffer(buffer.internalBuffer);\n> \n> \t\tif (ret) {\n> \t\t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n> diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp\n> index 16cf266f..fd927167 100644\n> --- a/src/android/camera_request.cpp\n> +++ b/src/android/camera_request.cpp\n> @@ -36,7 +36,8 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> \t\t\tstatic_cast(buffer.stream->priv);\n> \n> \t\tbuffers_.push_back({ stream, buffer.buffer, nullptr,\n> -\t\t\t\t buffer.acquire_fence, Status::Success });\n> +\t\t\t\t buffer.acquire_fence, Status::Success,\n> +\t\t\t\t nullptr, nullptr, nullptr, this });\n\nA constructor will be nice at some point :-)\n\n> \t}\n> \n> \t/* Clone the controls associated with the camera3 request. */\n> diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> index 904886da..c4bc5d6e 100644\n> --- a/src/android/camera_request.h\n> +++ b/src/android/camera_request.h\n> @@ -17,6 +17,7 @@\n> \n> #include \n> \n> +#include \"camera_buffer.h\"\n\nCan you forward-declare CameraBuffer instead of including the header\nhere ?\n\n> #include \"camera_metadata.h\"\n> #include \"camera_worker.h\"\n> \n> @@ -36,6 +37,10 @@ public:\n> \t\tstd::unique_ptr frameBuffer;\n> \t\tint fence;\n> \t\tStatus status;\n> +\t\tlibcamera::FrameBuffer *internalBuffer;\n> +\t\tstd::unique_ptr destBuffer;\n\nMaybe dstBuffer to match srcBuffer ? I'd also place srcBuffer first to\nmatch the usual source -> destination logical order, but that's a\ndetail.\n\n> +\t\tconst libcamera::FrameBuffer *srcBuffer;\n> +\t\tCamera3RequestDescriptor *request;\n> \t};\n> \n> \tCamera3RequestDescriptor(libcamera::Camera *camera,\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index ca25c4db..0e268cdf 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -147,8 +147,7 @@ int CameraStream::waitFence(int fence)\n> }\n> \n> int CameraStream::process(const FrameBuffer &source,\n> -\t\t\t Camera3RequestDescriptor::StreamBuffer &dest,\n> -\t\t\t Camera3RequestDescriptor *request)\n> +\t\t\t Camera3RequestDescriptor::StreamBuffer &dest)\n> {\n> \t/* Handle waiting on fences on the destination buffer. */\n> \tif (dest.fence != -1) {\n> @@ -170,14 +169,17 @@ int CameraStream::process(const FrameBuffer &source,\n> \t * separate thread.\n> \t */\n> \tconst StreamConfiguration &output = configuration();\n> -\tCameraBuffer destBuffer(*dest.camera3Buffer, output.pixelFormat,\n> -\t\t\t\toutput.size, PROT_READ | PROT_WRITE);\n> -\tif (!destBuffer.isValid()) {\n> +\tdest.destBuffer = std::make_unique(\n> +\t\t*dest.camera3Buffer, output.pixelFormat, output.size,\n> +\t\tPROT_READ | PROT_WRITE);\n> +\tif (!dest.destBuffer->isValid()) {\n> \t\tLOG(HAL, Error) << \"Failed to create destination buffer\";\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\treturn postProcessor_->process(source, &destBuffer, request);\n> +\tdest.srcBuffer = &source;\n> +\n> +\treturn postProcessor_->process(&dest);\n> }\n> \n> FrameBuffer *CameraStream::getBuffer()\n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index f242336e..e9c320b1 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -122,8 +122,7 @@ public:\n> \n> \tint configure();\n> \tint process(const libcamera::FrameBuffer &source,\n> -\t\t Camera3RequestDescriptor::StreamBuffer &dest,\n> -\t\t Camera3RequestDescriptor *request);\n> +\t\t Camera3RequestDescriptor::StreamBuffer &dest);\n\nYou're passing a reference here, and a pointer in\nPostProcessor::process(). Could you pick one of the two and use it\nconsistently ?\n\nReviewed-by: Laurent Pinchart \n\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 49483836..da71f113 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -98,15 +98,17 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> \t}\n> }\n> \n> -int PostProcessorJpeg::process(const FrameBuffer &source,\n> -\t\t\t CameraBuffer *destination,\n> -\t\t\t Camera3RequestDescriptor *request)\n> +int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)\n> {\n> \tASSERT(encoder_);\n> +\n> +\tconst FrameBuffer &source = *streamBuffer->srcBuffer;\n> +\tCameraBuffer *destination = streamBuffer->destBuffer.get();\n> +\n> \tASSERT(destination->numPlanes() == 1);\n> \n> -\tconst CameraMetadata &requestMetadata = request->settings_;\n> -\tCameraMetadata *resultMetadata = request->resultMetadata_.get();\n> +\tconst CameraMetadata &requestMetadata = streamBuffer->request->settings_;\n> +\tCameraMetadata *resultMetadata = streamBuffer->request->resultMetadata_.get();\n> \tcamera_metadata_ro_entry_t entry;\n> \tint ret;\n> \n> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> index 0184d77e..92385548 100644\n> --- a/src/android/jpeg/post_processor_jpeg.h\n> +++ b/src/android/jpeg/post_processor_jpeg.h\n> @@ -22,9 +22,7 @@ public:\n> \n> \tint configure(const libcamera::StreamConfiguration &incfg,\n> \t\t const libcamera::StreamConfiguration &outcfg) override;\n> -\tint process(const libcamera::FrameBuffer &source,\n> -\t\t CameraBuffer *destination,\n> -\t\t Camera3RequestDescriptor *request) override;\n> +\tint process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) 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 27eaef88..128161c8 100644\n> --- a/src/android/post_processor.h\n> +++ b/src/android/post_processor.h\n> @@ -11,8 +11,7 @@\n> #include \n> \n> #include \"camera_buffer.h\"\n> -\n> -class Camera3RequestDescriptor;\n> +#include \"camera_request.h\"\n> \n> class PostProcessor\n> {\n> @@ -21,9 +20,7 @@ public:\n> \n> \tvirtual int configure(const libcamera::StreamConfiguration &inCfg,\n> \t\t\t const libcamera::StreamConfiguration &outCfg) = 0;\n> -\tvirtual int process(const libcamera::FrameBuffer &source,\n> -\t\t\t CameraBuffer *destination,\n> -\t\t\t Camera3RequestDescriptor *request) = 0;\n> +\tvirtual int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 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 8110a1f1..eeb8f1f4 100644\n> --- a/src/android/yuv/post_processor_yuv.cpp\n> +++ b/src/android/yuv/post_processor_yuv.cpp\n> @@ -49,10 +49,11 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg,\n> \treturn 0;\n> }\n> \n> -int PostProcessorYuv::process(const FrameBuffer &source,\n> -\t\t\t CameraBuffer *destination,\n> -\t\t\t [[maybe_unused]] Camera3RequestDescriptor *request)\n> +int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)\n> {\n> +\tconst FrameBuffer &source = *streamBuffer->srcBuffer;\n> +\tCameraBuffer *destination = streamBuffer->destBuffer.get();\n> +\n> \tif (!isValidBuffers(source, *destination))\n> \t\treturn -EINVAL;\n> \n> diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h\n> index a4e0ff5d..5954e11b 100644\n> --- a/src/android/yuv/post_processor_yuv.h\n> +++ b/src/android/yuv/post_processor_yuv.h\n> @@ -18,9 +18,7 @@ public:\n> \n> \tint configure(const libcamera::StreamConfiguration &incfg,\n> \t\t const libcamera::StreamConfiguration &outcfg) override;\n> -\tint process(const libcamera::FrameBuffer &source,\n> -\t\t CameraBuffer *destination,\n> -\t\t Camera3RequestDescriptor *request) override;\n> +\tint process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) override;\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 31DE1BDB1C\n\tfor ;\n\tMon, 25 Oct 2021 05:23:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8DEC360237;\n\tMon, 25 Oct 2021 07:23:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 786F460237\n\tfor ;\n\tMon, 25 Oct 2021 07:23:46 +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 A6FB4E0A;\n\tMon, 25 Oct 2021 07:23:45 +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=\"ZjLDYvGx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635139425;\n\tbh=VRQR6aC5AKpXXj2eDMkZEpDToS1UewNWMVrR+8zBD2I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZjLDYvGxVRx4SVuB44ssI62YPkS60Ul7mgmI/mJCksoOtKM0WnfGhbIVQDB3VddqO\n\tWq0yCdQ2NmmUKEThWkgmMRsNE0n2xjAApKFZEKW4jfsteSWeMsyAHTmMeGUZumxFvm\n\t1bTIVZePLP8+mX5uF5jbd0zKjAJp3McqBhlyHv2A=","Date":"Mon, 25 Oct 2021 08:23:24 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211023103302.152769-1-umang.jain@ideasonboard.com>\n\t<20211023103302.152769-5-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211023103302.152769-5-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v6 4/7] android: post_processor:\n\tConsolidate contextual information","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":20430,"web_url":"https://patchwork.libcamera.org/comment/20430/","msgid":"","date":"2021-10-25T11:13:14","subject":"Re: [libcamera-devel] [PATCH v6 4/7] android: post_processor:\n\tConsolidate contextual information","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang, thank you for the patch.\n\nOn Mon, Oct 25, 2021 at 2:23 PM Laurent Pinchart\n wrote:\n>\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Sat, Oct 23, 2021 at 04:02:59PM +0530, Umang Jain wrote:\n> > Save and provide the context for post-processor of a camera stream\n> > via Camera3RequestDescriptor::StreamBuffer. We extend the structure\n> > to include source and destination buffers for the post processor, along\n> > with CameraStream::Type::Internal buffer pointer (if any). In addition\n> > to that, a back pointer to Camera3RequestDescriptor is convienient to\n> > get access to overall descriptor (status, metadata settings etc.)\n> >\n> > Also, migrate PostProcessor::process() signature to use\n> > Camera3RequestDescriptor::StreamBuffer only. This will be helpful\n> > when we move to async post-processing in subsequent commits.\n> >\n> > Signed-off-by: Umang Jain \n> > ---\n> > src/android/camera_device.cpp | 11 ++++++-----\n> > src/android/camera_request.cpp | 3 ++-\n> > src/android/camera_request.h | 5 +++++\n> > src/android/camera_stream.cpp | 14 ++++++++------\n> > src/android/camera_stream.h | 3 +--\n> > src/android/jpeg/post_processor_jpeg.cpp | 12 +++++++-----\n> > src/android/jpeg/post_processor_jpeg.h | 4 +---\n> > src/android/post_processor.h | 7 ++-----\n> > src/android/yuv/post_processor_yuv.cpp | 7 ++++---\n> > src/android/yuv/post_processor_yuv.h | 4 +---\n> > 10 files changed, 37 insertions(+), 33 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index f4d4fb09..2a98a2e6 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -953,6 +953,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > * once it has been processed.\n> > */\n> > frameBuffer = cameraStream->getBuffer();\n> > + buffer.internalBuffer = frameBuffer;\n> > LOG(HAL, Debug) << ss.str() << \" (internal)\";\n> > break;\n> > }\n> > @@ -1134,14 +1135,14 @@ void CameraDevice::requestComplete(Request *request)\n> > continue;\n> > }\n> >\n>\n> Should we store src in buffer here instead of in\n> CameraStream::process(), to make the signature of all process()\n> functions the same ?\n>\n> > - int ret = stream->process(*src, buffer, descriptor);\n> > + int ret = stream->process(*src, buffer);\n> >\n> > /*\n> > - * Return the FrameBuffer to the CameraStream now that we're\n> > - * done processing it.\n> > + * If the framebuffer is internal to CameraStream return it back\n> > + * now that we're done processing it.\n> > */\n> > - if (stream->type() == CameraStream::Type::Internal)\n> > - stream->putBuffer(src);\n> > + if (buffer.internalBuffer)\n> > + stream->putBuffer(buffer.internalBuffer);\n> >\n> > if (ret) {\n> > buffer.status = Camera3RequestDescriptor::Status::Error;\n> > diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp\n> > index 16cf266f..fd927167 100644\n> > --- a/src/android/camera_request.cpp\n> > +++ b/src/android/camera_request.cpp\n> > @@ -36,7 +36,8 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> > static_cast(buffer.stream->priv);\n> >\n> > buffers_.push_back({ stream, buffer.buffer, nullptr,\n> > - buffer.acquire_fence, Status::Success });\n> > + buffer.acquire_fence, Status::Success,\n> > + nullptr, nullptr, nullptr, this });\n>\n> A constructor will be nice at some point :-)\n>\n> > }\n> >\n> > /* Clone the controls associated with the camera3 request. */\n> > diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> > index 904886da..c4bc5d6e 100644\n> > --- a/src/android/camera_request.h\n> > +++ b/src/android/camera_request.h\n> > @@ -17,6 +17,7 @@\n> >\n> > #include \n> >\n> > +#include \"camera_buffer.h\"\n>\n> Can you forward-declare CameraBuffer instead of including the header\n> here ?\n>\n> > #include \"camera_metadata.h\"\n> > #include \"camera_worker.h\"\n> >\n> > @@ -36,6 +37,10 @@ public:\n> > std::unique_ptr frameBuffer;\n> > int fence;\n> > Status status;\n> > + libcamera::FrameBuffer *internalBuffer;\n> > + std::unique_ptr destBuffer;\n>\n> Maybe dstBuffer to match srcBuffer ? I'd also place srcBuffer first to\n> match the usual source -> destination logical order, but that's a\n> detail.\n>\n> > + const libcamera::FrameBuffer *srcBuffer;\n> > + Camera3RequestDescriptor *request;\n> > };\n> >\n\nCould you add a document about these in .cpp?\nIt becomes more and more difficult to track\n\n> > Camera3RequestDescriptor(libcamera::Camera *camera,\n> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > index ca25c4db..0e268cdf 100644\n> > --- a/src/android/camera_stream.cpp\n> > +++ b/src/android/camera_stream.cpp\n> > @@ -147,8 +147,7 @@ int CameraStream::waitFence(int fence)\n> > }\n> >\n> > int CameraStream::process(const FrameBuffer &source,\n> > - Camera3RequestDescriptor::StreamBuffer &dest,\n> > - Camera3RequestDescriptor *request)\n> > + Camera3RequestDescriptor::StreamBuffer &dest)\n> > {\n> > /* Handle waiting on fences on the destination buffer. */\n> > if (dest.fence != -1) {\n> > @@ -170,14 +169,17 @@ int CameraStream::process(const FrameBuffer &source,\n> > * separate thread.\n> > */\n> > const StreamConfiguration &output = configuration();\n> > - CameraBuffer destBuffer(*dest.camera3Buffer, output.pixelFormat,\n> > - output.size, PROT_READ | PROT_WRITE);\n> > - if (!destBuffer.isValid()) {\n> > + dest.destBuffer = std::make_unique(\n> > + *dest.camera3Buffer, output.pixelFormat, output.size,\n> > + PROT_READ | PROT_WRITE);\n> > + if (!dest.destBuffer->isValid()) {\n> > LOG(HAL, Error) << \"Failed to create destination buffer\";\n> > return -EINVAL;\n> > }\n> >\n> > - return postProcessor_->process(source, &destBuffer, request);\n> > + dest.srcBuffer = &source;\n> > +\n> > + return postProcessor_->process(&dest);\n> > }\n> >\n> > FrameBuffer *CameraStream::getBuffer()\n> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> > index f242336e..e9c320b1 100644\n> > --- a/src/android/camera_stream.h\n> > +++ b/src/android/camera_stream.h\n> > @@ -122,8 +122,7 @@ public:\n> >\n> > int configure();\n> > int process(const libcamera::FrameBuffer &source,\n> > - Camera3RequestDescriptor::StreamBuffer &dest,\n> > - Camera3RequestDescriptor *request);\n> > + Camera3RequestDescriptor::StreamBuffer &dest);\n>\n\ndest seems to be no longer a proper name.\nAs Laurent said, if we set streamBuffer.srcBuffer to source in a\ncaller side, perhaps shall this renamed to streamBuffer?\n\nReviewed-by: Hirokazu Honda \n\n> You're passing a reference here, and a pointer in\n> PostProcessor::process(). Could you pick one of the two and use it\n> consistently ?\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > libcamera::FrameBuffer *getBuffer();\n> > void 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 49483836..da71f113 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > @@ -98,15 +98,17 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> > }\n> > }\n> >\n> > -int PostProcessorJpeg::process(const FrameBuffer &source,\n> > - CameraBuffer *destination,\n> > - Camera3RequestDescriptor *request)\n> > +int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)\n> > {\n> > ASSERT(encoder_);\n> > +\n> > + const FrameBuffer &source = *streamBuffer->srcBuffer;\n> > + CameraBuffer *destination = streamBuffer->destBuffer.get();\n> > +\n> > ASSERT(destination->numPlanes() == 1);\n> >\n> > - const CameraMetadata &requestMetadata = request->settings_;\n> > - CameraMetadata *resultMetadata = request->resultMetadata_.get();\n> > + const CameraMetadata &requestMetadata = streamBuffer->request->settings_;\n> > + CameraMetadata *resultMetadata = streamBuffer->request->resultMetadata_.get();\n> > camera_metadata_ro_entry_t entry;\n> > int ret;\n> >\n> > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> > index 0184d77e..92385548 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.h\n> > +++ b/src/android/jpeg/post_processor_jpeg.h\n> > @@ -22,9 +22,7 @@ public:\n> >\n> > int configure(const libcamera::StreamConfiguration &incfg,\n> > const libcamera::StreamConfiguration &outcfg) override;\n> > - int process(const libcamera::FrameBuffer &source,\n> > - CameraBuffer *destination,\n> > - Camera3RequestDescriptor *request) override;\n> > + int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) override;\n> >\n> > private:\n> > void generateThumbnail(const libcamera::FrameBuffer &source,\n> > diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> > index 27eaef88..128161c8 100644\n> > --- a/src/android/post_processor.h\n> > +++ b/src/android/post_processor.h\n> > @@ -11,8 +11,7 @@\n> > #include \n> >\n> > #include \"camera_buffer.h\"\n> > -\n> > -class Camera3RequestDescriptor;\n> > +#include \"camera_request.h\"\n> >\n> > class PostProcessor\n> > {\n> > @@ -21,9 +20,7 @@ public:\n> >\n> > virtual int configure(const libcamera::StreamConfiguration &inCfg,\n> > const libcamera::StreamConfiguration &outCfg) = 0;\n> > - virtual int process(const libcamera::FrameBuffer &source,\n> > - CameraBuffer *destination,\n> > - Camera3RequestDescriptor *request) = 0;\n> > + virtual int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 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 8110a1f1..eeb8f1f4 100644\n> > --- a/src/android/yuv/post_processor_yuv.cpp\n> > +++ b/src/android/yuv/post_processor_yuv.cpp\n> > @@ -49,10 +49,11 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg,\n> > return 0;\n> > }\n> >\n> > -int PostProcessorYuv::process(const FrameBuffer &source,\n> > - CameraBuffer *destination,\n> > - [[maybe_unused]] Camera3RequestDescriptor *request)\n> > +int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)\n> > {\n> > + const FrameBuffer &source = *streamBuffer->srcBuffer;\n> > + CameraBuffer *destination = streamBuffer->destBuffer.get();\n> > +\n> > if (!isValidBuffers(source, *destination))\n> > return -EINVAL;\n> >\n> > diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h\n> > index a4e0ff5d..5954e11b 100644\n> > --- a/src/android/yuv/post_processor_yuv.h\n> > +++ b/src/android/yuv/post_processor_yuv.h\n> > @@ -18,9 +18,7 @@ public:\n> >\n> > int configure(const libcamera::StreamConfiguration &incfg,\n> > const libcamera::StreamConfiguration &outcfg) override;\n> > - int process(const libcamera::FrameBuffer &source,\n> > - CameraBuffer *destination,\n> > - Camera3RequestDescriptor *request) override;\n> > + int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) override;\n> >\n> > private:\n> > bool isValidBuffers(const libcamera::FrameBuffer &source,\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 80C97BDB1C\n\tfor ;\n\tMon, 25 Oct 2021 11:13:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EA8F764872;\n\tMon, 25 Oct 2021 13:13:27 +0200 (CEST)","from mail-ed1-x52d.google.com (mail-ed1-x52d.google.com\n\t[IPv6:2a00:1450:4864:20::52d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E184C60124\n\tfor ;\n\tMon, 25 Oct 2021 13:13:25 +0200 (CEST)","by mail-ed1-x52d.google.com with SMTP id s1so15768103edd.3\n\tfor ;\n\tMon, 25 Oct 2021 04:13:25 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"JJLFvN7y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=lUXi2Up9sY8MsCpYg0v+Bvdz9Bne8iFo9PWkRs8Ung8=;\n\tb=JJLFvN7yPh8cyLKuMHfeFQVF0V6+QAeW70sMyMBrfFMB328ucxvsEsTTruQl8s/w8p\n\tRcN5vOW7PHbmqMo3o07qFHG2C7rSd1CEYY2WsK2YajUm5r8hLyHBPGf0K8rQe6AgWEaZ\n\tvkPAFWSDvZZerB4fNH0RV1D9plRtsIT4fymJg=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=lUXi2Up9sY8MsCpYg0v+Bvdz9Bne8iFo9PWkRs8Ung8=;\n\tb=3Cj3FGdqW+lMjyiibXC1CDWVcdKKOSv55ux8R+taw+z3F/zD1e9cc9HPTQgCHFdziY\n\tZfV3KUvA3oQ+GGYi4SBAnGHhqZlew5Hq4+26wSZPKW65S7dl6cVEcZF3KogF29qXaeVg\n\tYl4hFAIs3bTCnFhvinwhFqPNmszVC3CU1OmwmoUH7ENrU1409NM+kCzhupn32PLUh5VZ\n\tEi1ZSlL3UW+2s64gB9AbresVySCJxl/Y/T/REOXe/nYj5Onb7Ta3Bi8uSGpmV+TQrsFM\n\tfumiCoBOG7e1BlI8gDAsjk8bJxc+j1HCh9HRQBQweTHsseOXvolXgjF8eqnb5MF5ko/W\n\tX2Og==","X-Gm-Message-State":"AOAM532rIgVX21pyrOJMAy/BG/dzd4rapy4dGTk+ootRBadbNljCIKUn\n\tig3FahYzG27XPaYDeo4ls5Fv6sS05c2KX3ptxg3E/g==","X-Google-Smtp-Source":"ABdhPJz5p3epir6DvPFdcW37Rzrm4KnN9jt+YS+J/XOVeo6KG7QCzPWjpup887f1rMwoj/W9YGN4tpo5nCqGgnMn6oM=","X-Received":"by 2002:a17:906:5801:: with SMTP id\n\tm1mr22178322ejq.296.1635160405338; \n\tMon, 25 Oct 2021 04:13:25 -0700 (PDT)","MIME-Version":"1.0","References":"<20211023103302.152769-1-umang.jain@ideasonboard.com>\n\t<20211023103302.152769-5-umang.jain@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 25 Oct 2021 20:13:14 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v6 4/7] android: post_processor:\n\tConsolidate contextual information","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20435,"web_url":"https://patchwork.libcamera.org/comment/20435/","msgid":"","date":"2021-10-25T13:00:41","subject":"Re: [libcamera-devel] [PATCH v6 4/7] android: post_processor:\n\tConsolidate contextual information","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 10/25/21 10:53 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Sat, Oct 23, 2021 at 04:02:59PM +0530, Umang Jain wrote:\n>> Save and provide the context for post-processor of a camera stream\n>> via Camera3RequestDescriptor::StreamBuffer. We extend the structure\n>> to include source and destination buffers for the post processor, along\n>> with CameraStream::Type::Internal buffer pointer (if any). In addition\n>> to that, a back pointer to Camera3RequestDescriptor is convienient to\n>> get access to overall descriptor (status, metadata settings etc.)\n>>\n>> Also, migrate PostProcessor::process() signature to use\n>> Camera3RequestDescriptor::StreamBuffer only. This will be helpful\n>> when we move to async post-processing in subsequent commits.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/camera_device.cpp | 11 ++++++-----\n>> src/android/camera_request.cpp | 3 ++-\n>> src/android/camera_request.h | 5 +++++\n>> src/android/camera_stream.cpp | 14 ++++++++------\n>> src/android/camera_stream.h | 3 +--\n>> src/android/jpeg/post_processor_jpeg.cpp | 12 +++++++-----\n>> src/android/jpeg/post_processor_jpeg.h | 4 +---\n>> src/android/post_processor.h | 7 ++-----\n>> src/android/yuv/post_processor_yuv.cpp | 7 ++++---\n>> src/android/yuv/post_processor_yuv.h | 4 +---\n>> 10 files changed, 37 insertions(+), 33 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index f4d4fb09..2a98a2e6 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -953,6 +953,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>> \t\t\t * once it has been processed.\n>> \t\t\t */\n>> \t\t\tframeBuffer = cameraStream->getBuffer();\n>> +\t\t\tbuffer.internalBuffer = frameBuffer;\n>> \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n>> \t\t\tbreak;\n>> \t\t}\n>> @@ -1134,14 +1135,14 @@ void CameraDevice::requestComplete(Request *request)\n>> \t\t\tcontinue;\n>> \t\t}\n>> \n> Should we store src in buffer here instead of in\n> CameraStream::process(), to make the signature of all process()\n> functions the same ?\n\n\nThat's a good point.\n\nI'll try to address it in this series, but see if it's easy enough \nchangeset for a fixup! patch? If I see major conflicts, would it be OK \non top?\n\n>\n>> -\t\tint ret = stream->process(*src, buffer, descriptor);\n>> +\t\tint ret = stream->process(*src, buffer);\n>> \n>> \t\t/*\n>> -\t\t * Return the FrameBuffer to the CameraStream now that we're\n>> -\t\t * done processing it.\n>> +\t\t * If the framebuffer is internal to CameraStream return it back\n>> +\t\t * now that we're done processing it.\n>> \t\t */\n>> -\t\tif (stream->type() == CameraStream::Type::Internal)\n>> -\t\t\tstream->putBuffer(src);\n>> +\t\tif (buffer.internalBuffer)\n>> +\t\t\tstream->putBuffer(buffer.internalBuffer);\n>> \n>> \t\tif (ret) {\n>> \t\t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n>> diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp\n>> index 16cf266f..fd927167 100644\n>> --- a/src/android/camera_request.cpp\n>> +++ b/src/android/camera_request.cpp\n>> @@ -36,7 +36,8 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n>> \t\t\tstatic_cast(buffer.stream->priv);\n>> \n>> \t\tbuffers_.push_back({ stream, buffer.buffer, nullptr,\n>> -\t\t\t\t buffer.acquire_fence, Status::Success });\n>> +\t\t\t\t buffer.acquire_fence, Status::Success,\n>> +\t\t\t\t nullptr, nullptr, nullptr, this });\n> A constructor will be nice at some point :-)\n>\n>> \t}\n>> \n>> \t/* Clone the controls associated with the camera3 request. */\n>> diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n>> index 904886da..c4bc5d6e 100644\n>> --- a/src/android/camera_request.h\n>> +++ b/src/android/camera_request.h\n>> @@ -17,6 +17,7 @@\n>> \n>> #include \n>> \n>> +#include \"camera_buffer.h\"\n> Can you forward-declare CameraBuffer instead of including the header\n> here ?\n>\n>> #include \"camera_metadata.h\"\n>> #include \"camera_worker.h\"\n>> \n>> @@ -36,6 +37,10 @@ public:\n>> \t\tstd::unique_ptr frameBuffer;\n>> \t\tint fence;\n>> \t\tStatus status;\n>> +\t\tlibcamera::FrameBuffer *internalBuffer;\n>> +\t\tstd::unique_ptr destBuffer;\n> Maybe dstBuffer to match srcBuffer ? I'd also place srcBuffer first to\n> match the usual source -> destination logical order, but that's a\n> detail.\n\n\nYes, renaming that should be fine.\n\n>\n>> +\t\tconst libcamera::FrameBuffer *srcBuffer;\n>> +\t\tCamera3RequestDescriptor *request;\n>> \t};\n>> \n>> \tCamera3RequestDescriptor(libcamera::Camera *camera,\n>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n>> index ca25c4db..0e268cdf 100644\n>> --- a/src/android/camera_stream.cpp\n>> +++ b/src/android/camera_stream.cpp\n>> @@ -147,8 +147,7 @@ int CameraStream::waitFence(int fence)\n>> }\n>> \n>> int CameraStream::process(const FrameBuffer &source,\n>> -\t\t\t Camera3RequestDescriptor::StreamBuffer &dest,\n>> -\t\t\t Camera3RequestDescriptor *request)\n>> +\t\t\t Camera3RequestDescriptor::StreamBuffer &dest)\n>> {\n>> \t/* Handle waiting on fences on the destination buffer. */\n>> \tif (dest.fence != -1) {\n>> @@ -170,14 +169,17 @@ int CameraStream::process(const FrameBuffer &source,\n>> \t * separate thread.\n>> \t */\n>> \tconst StreamConfiguration &output = configuration();\n>> -\tCameraBuffer destBuffer(*dest.camera3Buffer, output.pixelFormat,\n>> -\t\t\t\toutput.size, PROT_READ | PROT_WRITE);\n>> -\tif (!destBuffer.isValid()) {\n>> +\tdest.destBuffer = std::make_unique(\n>> +\t\t*dest.camera3Buffer, output.pixelFormat, output.size,\n>> +\t\tPROT_READ | PROT_WRITE);\n>> +\tif (!dest.destBuffer->isValid()) {\n>> \t\tLOG(HAL, Error) << \"Failed to create destination buffer\";\n>> \t\treturn -EINVAL;\n>> \t}\n>> \n>> -\treturn postProcessor_->process(source, &destBuffer, request);\n>> +\tdest.srcBuffer = &source;\n>> +\n>> +\treturn postProcessor_->process(&dest);\n>> }\n>> \n>> FrameBuffer *CameraStream::getBuffer()\n>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n>> index f242336e..e9c320b1 100644\n>> --- a/src/android/camera_stream.h\n>> +++ b/src/android/camera_stream.h\n>> @@ -122,8 +122,7 @@ public:\n>> \n>> \tint configure();\n>> \tint process(const libcamera::FrameBuffer &source,\n>> -\t\t Camera3RequestDescriptor::StreamBuffer &dest,\n>> -\t\t Camera3RequestDescriptor *request);\n>> +\t\t Camera3RequestDescriptor::StreamBuffer &dest);\n> You're passing a reference here, and a pointer in\n> PostProcessor::process(). Could you pick one of the two and use it\n> consistently ?\n\n\nAh right, I see that now. I will probably make  use of pointers.\n\n>\n> Reviewed-by: Laurent Pinchart \n>\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 49483836..da71f113 100644\n>> --- a/src/android/jpeg/post_processor_jpeg.cpp\n>> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n>> @@ -98,15 +98,17 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n>> \t}\n>> }\n>> \n>> -int PostProcessorJpeg::process(const FrameBuffer &source,\n>> -\t\t\t CameraBuffer *destination,\n>> -\t\t\t Camera3RequestDescriptor *request)\n>> +int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)\n>> {\n>> \tASSERT(encoder_);\n>> +\n>> +\tconst FrameBuffer &source = *streamBuffer->srcBuffer;\n>> +\tCameraBuffer *destination = streamBuffer->destBuffer.get();\n>> +\n>> \tASSERT(destination->numPlanes() == 1);\n>> \n>> -\tconst CameraMetadata &requestMetadata = request->settings_;\n>> -\tCameraMetadata *resultMetadata = request->resultMetadata_.get();\n>> +\tconst CameraMetadata &requestMetadata = streamBuffer->request->settings_;\n>> +\tCameraMetadata *resultMetadata = streamBuffer->request->resultMetadata_.get();\n>> \tcamera_metadata_ro_entry_t entry;\n>> \tint ret;\n>> \n>> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n>> index 0184d77e..92385548 100644\n>> --- a/src/android/jpeg/post_processor_jpeg.h\n>> +++ b/src/android/jpeg/post_processor_jpeg.h\n>> @@ -22,9 +22,7 @@ public:\n>> \n>> \tint configure(const libcamera::StreamConfiguration &incfg,\n>> \t\t const libcamera::StreamConfiguration &outcfg) override;\n>> -\tint process(const libcamera::FrameBuffer &source,\n>> -\t\t CameraBuffer *destination,\n>> -\t\t Camera3RequestDescriptor *request) override;\n>> +\tint process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) 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 27eaef88..128161c8 100644\n>> --- a/src/android/post_processor.h\n>> +++ b/src/android/post_processor.h\n>> @@ -11,8 +11,7 @@\n>> #include \n>> \n>> #include \"camera_buffer.h\"\n>> -\n>> -class Camera3RequestDescriptor;\n>> +#include \"camera_request.h\"\n>> \n>> class PostProcessor\n>> {\n>> @@ -21,9 +20,7 @@ public:\n>> \n>> \tvirtual int configure(const libcamera::StreamConfiguration &inCfg,\n>> \t\t\t const libcamera::StreamConfiguration &outCfg) = 0;\n>> -\tvirtual int process(const libcamera::FrameBuffer &source,\n>> -\t\t\t CameraBuffer *destination,\n>> -\t\t\t Camera3RequestDescriptor *request) = 0;\n>> +\tvirtual int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 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 8110a1f1..eeb8f1f4 100644\n>> --- a/src/android/yuv/post_processor_yuv.cpp\n>> +++ b/src/android/yuv/post_processor_yuv.cpp\n>> @@ -49,10 +49,11 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg,\n>> \treturn 0;\n>> }\n>> \n>> -int PostProcessorYuv::process(const FrameBuffer &source,\n>> -\t\t\t CameraBuffer *destination,\n>> -\t\t\t [[maybe_unused]] Camera3RequestDescriptor *request)\n>> +int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)\n>> {\n>> +\tconst FrameBuffer &source = *streamBuffer->srcBuffer;\n>> +\tCameraBuffer *destination = streamBuffer->destBuffer.get();\n>> +\n>> \tif (!isValidBuffers(source, *destination))\n>> \t\treturn -EINVAL;\n>> \n>> diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h\n>> index a4e0ff5d..5954e11b 100644\n>> --- a/src/android/yuv/post_processor_yuv.h\n>> +++ b/src/android/yuv/post_processor_yuv.h\n>> @@ -18,9 +18,7 @@ public:\n>> \n>> \tint configure(const libcamera::StreamConfiguration &incfg,\n>> \t\t const libcamera::StreamConfiguration &outcfg) override;\n>> -\tint process(const libcamera::FrameBuffer &source,\n>> -\t\t CameraBuffer *destination,\n>> -\t\t Camera3RequestDescriptor *request) override;\n>> +\tint process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) override;\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 8899DBF415\n\tfor ;\n\tMon, 25 Oct 2021 13:00:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DD7156486E;\n\tMon, 25 Oct 2021 15:00:47 +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 7026D60124\n\tfor ;\n\tMon, 25 Oct 2021 15:00:46 +0200 (CEST)","from [192.168.1.106] (unknown [103.251.226.211])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7ED373F0;\n\tMon, 25 Oct 2021 15:00:45 +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=\"sHLKlpTb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635166846;\n\tbh=zpHf1lWguHwpkffiTo9ll5iXvm+F9gi+d4xqmvz2Asc=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=sHLKlpTbh9b5rSu8t2Qa5aLI0YyTm/khLP/jX84X5Iix9kDdZ0W9CnLHZ4YdFlY0N\n\tBo3HsoBLRlYUSK3mQwA1yG8pnaw/zDBPaqu8w83guCC6KPa/FVxnLv/aLTg4wLcpAI\n\tM6ayBARBpY6P6N522JTRlfwJoQfUJzx8NhLqukhc=","To":"Laurent Pinchart ","References":"<20211023103302.152769-1-umang.jain@ideasonboard.com>\n\t<20211023103302.152769-5-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"","Date":"Mon, 25 Oct 2021 18:30:41 +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":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v6 4/7] android: post_processor:\n\tConsolidate contextual information","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":20438,"web_url":"https://patchwork.libcamera.org/comment/20438/","msgid":"","date":"2021-10-25T13:26:21","subject":"Re: [libcamera-devel] [PATCH v6 4/7] android: post_processor:\n\tConsolidate contextual information","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, Oct 25, 2021 at 06:30:41PM +0530, Umang Jain wrote:\n> On 10/25/21 10:53 AM, Laurent Pinchart wrote:\n> > On Sat, Oct 23, 2021 at 04:02:59PM +0530, Umang Jain wrote:\n> >> Save and provide the context for post-processor of a camera stream\n> >> via Camera3RequestDescriptor::StreamBuffer. We extend the structure\n> >> to include source and destination buffers for the post processor, along\n> >> with CameraStream::Type::Internal buffer pointer (if any). In addition\n> >> to that, a back pointer to Camera3RequestDescriptor is convienient to\n> >> get access to overall descriptor (status, metadata settings etc.)\n> >>\n> >> Also, migrate PostProcessor::process() signature to use\n> >> Camera3RequestDescriptor::StreamBuffer only. This will be helpful\n> >> when we move to async post-processing in subsequent commits.\n> >>\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> src/android/camera_device.cpp | 11 ++++++-----\n> >> src/android/camera_request.cpp | 3 ++-\n> >> src/android/camera_request.h | 5 +++++\n> >> src/android/camera_stream.cpp | 14 ++++++++------\n> >> src/android/camera_stream.h | 3 +--\n> >> src/android/jpeg/post_processor_jpeg.cpp | 12 +++++++-----\n> >> src/android/jpeg/post_processor_jpeg.h | 4 +---\n> >> src/android/post_processor.h | 7 ++-----\n> >> src/android/yuv/post_processor_yuv.cpp | 7 ++++---\n> >> src/android/yuv/post_processor_yuv.h | 4 +---\n> >> 10 files changed, 37 insertions(+), 33 deletions(-)\n> >>\n> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >> index f4d4fb09..2a98a2e6 100644\n> >> --- a/src/android/camera_device.cpp\n> >> +++ b/src/android/camera_device.cpp\n> >> @@ -953,6 +953,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >> \t\t\t * once it has been processed.\n> >> \t\t\t */\n> >> \t\t\tframeBuffer = cameraStream->getBuffer();\n> >> +\t\t\tbuffer.internalBuffer = frameBuffer;\n> >> \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n> >> \t\t\tbreak;\n> >> \t\t}\n> >> @@ -1134,14 +1135,14 @@ void CameraDevice::requestComplete(Request *request)\n> >> \t\t\tcontinue;\n> >> \t\t}\n> >> \n> >\n> > Should we store src in buffer here instead of in\n> > CameraStream::process(), to make the signature of all process()\n> > functions the same ?\n> \n> That's a good point.\n> \n> I'll try to address it in this series, but see if it's easy enough \n> changeset for a fixup! patch? If I see major conflicts, would it be OK \n> on top?\n\nIf it's too difficult I think it would be ok on top.\n\n> >> -\t\tint ret = stream->process(*src, buffer, descriptor);\n> >> +\t\tint ret = stream->process(*src, buffer);\n> >> \n> >> \t\t/*\n> >> -\t\t * Return the FrameBuffer to the CameraStream now that we're\n> >> -\t\t * done processing it.\n> >> +\t\t * If the framebuffer is internal to CameraStream return it back\n> >> +\t\t * now that we're done processing it.\n> >> \t\t */\n> >> -\t\tif (stream->type() == CameraStream::Type::Internal)\n> >> -\t\t\tstream->putBuffer(src);\n> >> +\t\tif (buffer.internalBuffer)\n> >> +\t\t\tstream->putBuffer(buffer.internalBuffer);\n> >> \n> >> \t\tif (ret) {\n> >> \t\t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n> >> diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp\n> >> index 16cf266f..fd927167 100644\n> >> --- a/src/android/camera_request.cpp\n> >> +++ b/src/android/camera_request.cpp\n> >> @@ -36,7 +36,8 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> >> \t\t\tstatic_cast(buffer.stream->priv);\n> >> \n> >> \t\tbuffers_.push_back({ stream, buffer.buffer, nullptr,\n> >> -\t\t\t\t buffer.acquire_fence, Status::Success });\n> >> +\t\t\t\t buffer.acquire_fence, Status::Success,\n> >> +\t\t\t\t nullptr, nullptr, nullptr, this });\n> >\n> > A constructor will be nice at some point :-)\n> >\n> >> \t}\n> >> \n> >> \t/* Clone the controls associated with the camera3 request. */\n> >> diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> >> index 904886da..c4bc5d6e 100644\n> >> --- a/src/android/camera_request.h\n> >> +++ b/src/android/camera_request.h\n> >> @@ -17,6 +17,7 @@\n> >> \n> >> #include \n> >> \n> >> +#include \"camera_buffer.h\"\n> >\n> > Can you forward-declare CameraBuffer instead of including the header\n> > here ?\n> >\n> >> #include \"camera_metadata.h\"\n> >> #include \"camera_worker.h\"\n> >> \n> >> @@ -36,6 +37,10 @@ public:\n> >> \t\tstd::unique_ptr frameBuffer;\n> >> \t\tint fence;\n> >> \t\tStatus status;\n> >> +\t\tlibcamera::FrameBuffer *internalBuffer;\n> >> +\t\tstd::unique_ptr destBuffer;\n> >\n> > Maybe dstBuffer to match srcBuffer ? I'd also place srcBuffer first to\n> > match the usual source -> destination logical order, but that's a\n> > detail.\n> \n> Yes, renaming that should be fine.\n> \n> >> +\t\tconst libcamera::FrameBuffer *srcBuffer;\n> >> +\t\tCamera3RequestDescriptor *request;\n> >> \t};\n> >> \n> >> \tCamera3RequestDescriptor(libcamera::Camera *camera,\n> >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> >> index ca25c4db..0e268cdf 100644\n> >> --- a/src/android/camera_stream.cpp\n> >> +++ b/src/android/camera_stream.cpp\n> >> @@ -147,8 +147,7 @@ int CameraStream::waitFence(int fence)\n> >> }\n> >> \n> >> int CameraStream::process(const FrameBuffer &source,\n> >> -\t\t\t Camera3RequestDescriptor::StreamBuffer &dest,\n> >> -\t\t\t Camera3RequestDescriptor *request)\n> >> +\t\t\t Camera3RequestDescriptor::StreamBuffer &dest)\n> >> {\n> >> \t/* Handle waiting on fences on the destination buffer. */\n> >> \tif (dest.fence != -1) {\n> >> @@ -170,14 +169,17 @@ int CameraStream::process(const FrameBuffer &source,\n> >> \t * separate thread.\n> >> \t */\n> >> \tconst StreamConfiguration &output = configuration();\n> >> -\tCameraBuffer destBuffer(*dest.camera3Buffer, output.pixelFormat,\n> >> -\t\t\t\toutput.size, PROT_READ | PROT_WRITE);\n> >> -\tif (!destBuffer.isValid()) {\n> >> +\tdest.destBuffer = std::make_unique(\n> >> +\t\t*dest.camera3Buffer, output.pixelFormat, output.size,\n> >> +\t\tPROT_READ | PROT_WRITE);\n> >> +\tif (!dest.destBuffer->isValid()) {\n> >> \t\tLOG(HAL, Error) << \"Failed to create destination buffer\";\n> >> \t\treturn -EINVAL;\n> >> \t}\n> >> \n> >> -\treturn postProcessor_->process(source, &destBuffer, request);\n> >> +\tdest.srcBuffer = &source;\n> >> +\n> >> +\treturn postProcessor_->process(&dest);\n> >> }\n> >> \n> >> FrameBuffer *CameraStream::getBuffer()\n> >> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> >> index f242336e..e9c320b1 100644\n> >> --- a/src/android/camera_stream.h\n> >> +++ b/src/android/camera_stream.h\n> >> @@ -122,8 +122,7 @@ public:\n> >> \n> >> \tint configure();\n> >> \tint process(const libcamera::FrameBuffer &source,\n> >> -\t\t Camera3RequestDescriptor::StreamBuffer &dest,\n> >> -\t\t Camera3RequestDescriptor *request);\n> >> +\t\t Camera3RequestDescriptor::StreamBuffer &dest);\n> >\n> > You're passing a reference here, and a pointer in\n> > PostProcessor::process(). Could you pick one of the two and use it\n> > consistently ?\n> \n> Ah right, I see that now. I will probably make  use of pointers.\n> \n> > Reviewed-by: Laurent Pinchart \n> >\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 49483836..da71f113 100644\n> >> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> >> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> >> @@ -98,15 +98,17 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> >> \t}\n> >> }\n> >> \n> >> -int PostProcessorJpeg::process(const FrameBuffer &source,\n> >> -\t\t\t CameraBuffer *destination,\n> >> -\t\t\t Camera3RequestDescriptor *request)\n> >> +int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)\n> >> {\n> >> \tASSERT(encoder_);\n> >> +\n> >> +\tconst FrameBuffer &source = *streamBuffer->srcBuffer;\n> >> +\tCameraBuffer *destination = streamBuffer->destBuffer.get();\n> >> +\n> >> \tASSERT(destination->numPlanes() == 1);\n> >> \n> >> -\tconst CameraMetadata &requestMetadata = request->settings_;\n> >> -\tCameraMetadata *resultMetadata = request->resultMetadata_.get();\n> >> +\tconst CameraMetadata &requestMetadata = streamBuffer->request->settings_;\n> >> +\tCameraMetadata *resultMetadata = streamBuffer->request->resultMetadata_.get();\n> >> \tcamera_metadata_ro_entry_t entry;\n> >> \tint ret;\n> >> \n> >> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> >> index 0184d77e..92385548 100644\n> >> --- a/src/android/jpeg/post_processor_jpeg.h\n> >> +++ b/src/android/jpeg/post_processor_jpeg.h\n> >> @@ -22,9 +22,7 @@ public:\n> >> \n> >> \tint configure(const libcamera::StreamConfiguration &incfg,\n> >> \t\t const libcamera::StreamConfiguration &outcfg) override;\n> >> -\tint process(const libcamera::FrameBuffer &source,\n> >> -\t\t CameraBuffer *destination,\n> >> -\t\t Camera3RequestDescriptor *request) override;\n> >> +\tint process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) 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 27eaef88..128161c8 100644\n> >> --- a/src/android/post_processor.h\n> >> +++ b/src/android/post_processor.h\n> >> @@ -11,8 +11,7 @@\n> >> #include \n> >> \n> >> #include \"camera_buffer.h\"\n> >> -\n> >> -class Camera3RequestDescriptor;\n> >> +#include \"camera_request.h\"\n> >> \n> >> class PostProcessor\n> >> {\n> >> @@ -21,9 +20,7 @@ public:\n> >> \n> >> \tvirtual int configure(const libcamera::StreamConfiguration &inCfg,\n> >> \t\t\t const libcamera::StreamConfiguration &outCfg) = 0;\n> >> -\tvirtual int process(const libcamera::FrameBuffer &source,\n> >> -\t\t\t CameraBuffer *destination,\n> >> -\t\t\t Camera3RequestDescriptor *request) = 0;\n> >> +\tvirtual int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 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 8110a1f1..eeb8f1f4 100644\n> >> --- a/src/android/yuv/post_processor_yuv.cpp\n> >> +++ b/src/android/yuv/post_processor_yuv.cpp\n> >> @@ -49,10 +49,11 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg,\n> >> \treturn 0;\n> >> }\n> >> \n> >> -int PostProcessorYuv::process(const FrameBuffer &source,\n> >> -\t\t\t CameraBuffer *destination,\n> >> -\t\t\t [[maybe_unused]] Camera3RequestDescriptor *request)\n> >> +int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)\n> >> {\n> >> +\tconst FrameBuffer &source = *streamBuffer->srcBuffer;\n> >> +\tCameraBuffer *destination = streamBuffer->destBuffer.get();\n> >> +\n> >> \tif (!isValidBuffers(source, *destination))\n> >> \t\treturn -EINVAL;\n> >> \n> >> diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h\n> >> index a4e0ff5d..5954e11b 100644\n> >> --- a/src/android/yuv/post_processor_yuv.h\n> >> +++ b/src/android/yuv/post_processor_yuv.h\n> >> @@ -18,9 +18,7 @@ public:\n> >> \n> >> \tint configure(const libcamera::StreamConfiguration &incfg,\n> >> \t\t const libcamera::StreamConfiguration &outcfg) override;\n> >> -\tint process(const libcamera::FrameBuffer &source,\n> >> -\t\t CameraBuffer *destination,\n> >> -\t\t Camera3RequestDescriptor *request) override;\n> >> +\tint process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) override;\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 E9348BF415\n\tfor ;\n\tMon, 25 Oct 2021 13:26:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AB32F6486E;\n\tMon, 25 Oct 2021 15:26:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0667B60124\n\tfor ;\n\tMon, 25 Oct 2021 15:26:43 +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 8B2F3E0A;\n\tMon, 25 Oct 2021 15:26:42 +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=\"fmk5NyBQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635168402;\n\tbh=kcwEwzppeTxqmNnXLYS1xt4leljiekcAtO01WsYakiw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fmk5NyBQcUOtmDA6vajtRs9sWGX+OEycroDTt6SLAbH9bjkp/p1ocD6yYwTAFFInD\n\tSTTbYZ7W4V41i9CMLQBmSPGoL7XO9fZieBs9HqrIAyksAm2jGCeyNpxZAKNdyxwd6O\n\tyKpBDxlZTcgRsq+t+b5tkHqLM1hgQ/Zi3EzA1O1Y=","Date":"Mon, 25 Oct 2021 16:26:21 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211023103302.152769-1-umang.jain@ideasonboard.com>\n\t<20211023103302.152769-5-umang.jain@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6 4/7] android: post_processor:\n\tConsolidate contextual information","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\" "}}]