[{"id":13250,"web_url":"https://patchwork.libcamera.org/comment/13250/","msgid":"<0a009843-abea-2b14-2b6b-dd58caedfff0@uajain.com>","date":"2020-10-19T11:24:27","subject":"Re: [libcamera-devel] [PATCH] android: Modify PostProcessor\n\tinterface","submitter":{"id":1,"url":"https://patchwork.libcamera.org/api/people/1/","name":"Umang Jain","email":"email@uajain.com"},"content":"Hi Hiro,\n\nOn 10/19/20 1:33 PM, Hirokazu Honda wrote:\n> This modifies PostProcessor interface and polishes the code\n> around the PostProcessor.\n>\n> Signed-off-by: Hirokazu Honda \nChanges looks good, albeit a few comments. Also, I think the patch could \nalso be split down further to address the per Span pass-by-value,  per \n*-type reference to &-type, etc.\n> ---\n> src/android/camera_stream.cpp | 9 ++++-----\n> src/android/camera_stream.h | 8 ++++----\n> src/android/jpeg/encoder.h | 6 +++---\n> src/android/jpeg/encoder_libjpeg.cpp | 6 +++---\n> src/android/jpeg/encoder_libjpeg.h | 4 ++--\n> src/android/jpeg/post_processor_jpeg.cpp | 8 +++++---\n> src/android/jpeg/post_processor_jpeg.h | 6 +++---\n> src/android/post_processor.h | 4 ++--\n> 8 files changed, 26 insertions(+), 25 deletions(-)\n>\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index 1b8afa8..cc8691d 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -40,11 +40,10 @@ LOG_DECLARE_CATEGORY(HAL);\n> \n> CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,\n> \t\t\t camera3_stream_t *camera3Stream, unsigned int index)\n> -\t: cameraDevice_(cameraDevice), type_(type),\n> +\t: cameraDevice_(cameraDevice),\n> +\t config_(cameraDevice_->cameraConfiguration()), type_(type),\nWill cameraDevice_ will be initialized by this point? I am not sure.\n> \t camera3Stream_(camera3Stream), index_(index)\n> {\n> -\tconfig_ = cameraDevice_->cameraConfiguration();\n> -\n> \tif (type_ == Type::Internal || type_ == Type::Mapped) {\n> \t\t/*\n> \t\t * \\todo There might be multiple post-processors. The logic\n> @@ -52,7 +51,7 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,\n> \t\t * future. For now, we only have PostProcessorJpeg and that\n> \t\t * is what we instantiate here.\n> \t\t */\n> -\t\tpostProcessor_ = std::make_unique(cameraDevice_);\n> +\t\tpostProcessor_ = std::make_unique(cameraDevice);\nWhat does this change achieve?\n> \t}\n> \n> \tif (type == Type::Internal) {\n> @@ -102,7 +101,7 @@ int CameraStream::process(const libcamera::FrameBuffer &source,\n> \tif (!postProcessor_)\n> \t\treturn 0;\n> \n> -\treturn postProcessor_->process(&source, dest->maps()[0], metadata);\n> +\treturn postProcessor_->process(source, dest->maps()[0], metadata);\n> }\n> \n> FrameBuffer *CameraStream::getBuffer()\n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index c367a5f..24e66bb 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -124,11 +124,11 @@ public:\n> \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n> \n> private:\n> -\tCameraDevice *cameraDevice_;\n> -\tlibcamera::CameraConfiguration *config_;\n> -\tType type_;\n> +\tCameraDevice const *cameraDevice_;\n> +\tconst libcamera::CameraConfiguration *config_;\n> +\tconst Type type_;\n> \tcamera3_stream_t *camera3Stream_;\n> -\tunsigned int index_;\n> +\tconst unsigned int index_;\n> \n> \tstd::unique_ptr allocator_;\n> \tstd::vector buffers_;\n> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n> index 4483153..270ea60 100644\n> --- a/src/android/jpeg/encoder.h\n> +++ b/src/android/jpeg/encoder.h\n> @@ -14,11 +14,11 @@\n> class Encoder\n> {\n> public:\n> -\tvirtual ~Encoder() {};\n> +\tvirtual ~Encoder() {}\n> \n> \tvirtual int configure(const libcamera::StreamConfiguration &cfg) = 0;\n> -\tvirtual int encode(const libcamera::FrameBuffer *source,\n> -\t\t\t const libcamera::Span &destination,\n> +\tvirtual int encode(const libcamera::FrameBuffer &source,\n> +\t\t\t libcamera::Span destination,\n> \t\t\t const libcamera::Span &exifData) = 0;\n> };\n> \n> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> index 8995ba5..4236c9d 100644\n> --- a/src/android/jpeg/encoder_libjpeg.cpp\n> +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> @@ -179,11 +179,11 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n> \t}\n> }\n> \n> -int EncoderLibJpeg::encode(const FrameBuffer *source,\n> -\t\t\t const libcamera::Span &dest,\n> +int EncoderLibJpeg::encode(const FrameBuffer &source,\n> +\t\t\t libcamera::Span dest,\n> \t\t\t const libcamera::Span &exifData)\n> {\n> -\tMappedFrameBuffer frame(source, PROT_READ);\n> +\tMappedFrameBuffer frame(&source, PROT_READ);\n> \tif (!frame.isValid()) {\n> \t\tLOG(JPEG, Error) << \"Failed to map FrameBuffer : \"\n> \t\t\t\t << strerror(frame.error());\n> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> index 934caef..391a53c 100644\n> --- a/src/android/jpeg/encoder_libjpeg.h\n> +++ b/src/android/jpeg/encoder_libjpeg.h\n> @@ -21,8 +21,8 @@ public:\n> \t~EncoderLibJpeg();\n> \n> \tint configure(const libcamera::StreamConfiguration &cfg) override;\n> -\tint encode(const libcamera::FrameBuffer *source,\n> -\t\t const libcamera::Span &destination,\n> +\tint encode(const libcamera::FrameBuffer &source,\n> +\t\t libcamera::Span destination,\n> \t\t const libcamera::Span &exifData) override;\n> \n> private:\n> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> index 753c28e..4ded3e3 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -44,8 +44,8 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n> \treturn encoder_->configure(inCfg);\n> }\n> \n> -int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,\n> -\t\t\t const libcamera::Span &destination,\n> +int PostProcessorJpeg::process(const libcamera::FrameBuffer &source,\n> +\t\t\t libcamera::Span destination,\n> \t\t\t CameraMetadata *metadata)\n> {\n> \tif (!encoder_)\n> @@ -64,8 +64,10 @@ int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,\n> \t * second, it is good enough.\n> \t */\n> \texif.setTimestamp(std::time(nullptr));\n> -\tif (exif.generate() != 0)\n> +\tif (exif.generate() != 0) {\n> \t\tLOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n> +\t\treturn -EINVAL;\n> +\t}\n> \nPersonally speaking, I don't think we should return FAIL here. So my \npreference is to skip this change.\n> \tint jpeg_size = encoder_->encode(source, destination, exif.data());\n> \tif (jpeg_size < 0) {\n> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> index 62c8650..4d8edf5 100644\n> --- a/src/android/jpeg/post_processor_jpeg.h\n> +++ b/src/android/jpeg/post_processor_jpeg.h\n> @@ -23,12 +23,12 @@ 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 const libcamera::Span &destination,\n> +\tint process(const libcamera::FrameBuffer &source,\n> +\t\t libcamera::Span destination,\n> \t\t CameraMetadata *metadata) override;\n> \n> private:\n> -\tCameraDevice *cameraDevice_;\n> +\tCameraDevice const *cameraDevice_;\n> \tstd::unique_ptr encoder_;\n> \tlibcamera::Size streamSize_;\n> };\n> diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> index a891c43..5f87a5d 100644\n> --- a/src/android/post_processor.h\n> +++ b/src/android/post_processor.h\n> @@ -20,8 +20,8 @@ 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 const libcamera::Span &destination,\n> +\tvirtual int process(const libcamera::FrameBuffer &source,\n> +\t\t\t libcamera::Span destination,\n> \t\t\t CameraMetadata *metadata) = 0;\n> };\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 82BC9BDB13\n\tfor ;\n\tMon, 19 Oct 2020 11:24:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0F329613AD;\n\tMon, 19 Oct 2020 13:24:33 +0200 (CEST)","from mail.uajain.com (static.126.159.217.95.clients.your-server.de\n\t[95.217.159.126])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 05534607B8\n\tfor ;\n\tMon, 19 Oct 2020 13:24:31 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=uajain.com header.i=@uajain.com\n\theader.b=\"sjgilTaJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=uajain.com; s=mail;\n\tt=1603106670; bh=bJa0hW07TwaFEdJmHDVksjj6s/pxotmHgjX2Sv8gFSY=;\n\th=Subject:To:Cc:References:From:In-Reply-To;\n\tb=sjgilTaJXlY2oHKAbkhfHCeDuXjpXm2acwNs12gT9BfC0Y4fAaJ6sx+F83g/TvLmO\n\tVKRJ/hndI4Xj+OK5Er3/waUxw1hCAkItOSRSXYXw07NoQOOldQXlQFgPWs944XbA6Y\n\t13APhieU2QXNZeHq7j2ZDEWnLh1kKCfT3n7M3wmezzPZ1h+wgh3S0osRii6c62wn9y\n\tLgjbGFbvU59iO9I3SkCfUyCtCBZhQs9xm/wZn3/Z+NcPwmXT9967A4x7VX7OMGjdu/\n\tKP0dxv0aO4lRnqHFCNGtqfBt8+NZNr6FaJAESATcECAKYEDcD9M+oThb1R2Kk1b43F\n\tMv6GGKI7ewZgQ==","To":"Hirokazu Honda , libcamera-devel@lists.libcamera.org","References":"<20201019080323.2236548-1-hiroh@chromium.org>","From":"Umang Jain ","Message-ID":"<0a009843-abea-2b14-2b6b-dd58caedfff0@uajain.com>","Date":"Mon, 19 Oct 2020 16:54:27 +0530","Mime-Version":"1.0","In-Reply-To":"<20201019080323.2236548-1-hiroh@chromium.org>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH] android: Modify PostProcessor\n\tinterface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Transfer-Encoding":"base64","Content-Type":"text/plain; charset=\"utf-8\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13251,"web_url":"https://patchwork.libcamera.org/comment/13251/","msgid":"","date":"2020-10-19T11:44:24","subject":"Re: [libcamera-devel] [PATCH] android: Modify PostProcessor\n\tinterface","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang,\n\nOn Mon, Oct 19, 2020 at 8:24 PM Umang Jain wrote:\n>\n> Hi Hiro,\n>\n> On 10/19/20 1:33 PM, Hirokazu Honda wrote:\n> > This modifies PostProcessor interface and polishes the code\n> > around the PostProcessor.\n> >\n> > Signed-off-by: Hirokazu Honda \n> Changes looks good, albeit a few comments. Also, I think the patch could\n> also be split down further to address the per Span pass-by-value, per\n> *-type reference to &-type, etc.\n\nAck. I will do it in the next patch series.\n\n> > ---\n> > src/android/camera_stream.cpp | 9 ++++-----\n> > src/android/camera_stream.h | 8 ++++----\n> > src/android/jpeg/encoder.h | 6 +++---\n> > src/android/jpeg/encoder_libjpeg.cpp | 6 +++---\n> > src/android/jpeg/encoder_libjpeg.h | 4 ++--\n> > src/android/jpeg/post_processor_jpeg.cpp | 8 +++++---\n> > src/android/jpeg/post_processor_jpeg.h | 6 +++---\n> > src/android/post_processor.h | 4 ++--\n> > 8 files changed, 26 insertions(+), 25 deletions(-)\n> >\n> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > index 1b8afa8..cc8691d 100644\n> > --- a/src/android/camera_stream.cpp\n> > +++ b/src/android/camera_stream.cpp\n> > @@ -40,11 +40,10 @@ LOG_DECLARE_CATEGORY(HAL);\n> >\n> > CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,\n> > camera3_stream_t *camera3Stream, unsigned int index)\n> > - : cameraDevice_(cameraDevice), type_(type),\n> > + : cameraDevice_(cameraDevice),\n> > + config_(cameraDevice_->cameraConfiguration()), type_(type),\n> Will cameraDevice_ will be initialized by this point? I am not sure.\n\nI expect moving here out of body of constructor doesn't make a\ndifference and thus the answer to your question should be true.\n\n> > camera3Stream_(camera3Stream), index_(index)\n> > {\n> > - config_ = cameraDevice_->cameraConfiguration();\n> > -\n> > if (type_ == Type::Internal || type_ == Type::Mapped) {\n> > /*\n> > * \\todo There might be multiple post-processors. The logic\n> > @@ -52,7 +51,7 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,\n> > * future. For now, we only have PostProcessorJpeg and that\n> > * is what we instantiate here.\n> > */\n> > - postProcessor_ = std::make_unique(cameraDevice_);\n> > + postProcessor_ = std::make_unique(cameraDevice);\n> What does this change achieve?\n\nSince I change CameraDevice_ to const raw pointer, I need to pass\nnon-const raw pointer (cameraDevice, not cameraDevice\"_\") to\nPostProcessorJpeg.\nWe can change PostProcessor and CameraStream constructors to receive\nconst raw pointer.\n\n> > }\n> >\n> > if (type == Type::Internal) {\n> > @@ -102,7 +101,7 @@ int CameraStream::process(const libcamera::FrameBuffer &source,\n> > if (!postProcessor_)\n> > return 0;\n> >\n> > - return postProcessor_->process(&source, dest->maps()[0], metadata);\n> > + return postProcessor_->process(source, dest->maps()[0], metadata);\n> > }\n> >\n> > FrameBuffer *CameraStream::getBuffer()\n> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> > index c367a5f..24e66bb 100644\n> > --- a/src/android/camera_stream.h\n> > +++ b/src/android/camera_stream.h\n> > @@ -124,11 +124,11 @@ public:\n> > void putBuffer(libcamera::FrameBuffer *buffer);\n> >\n> > private:\n> > - CameraDevice *cameraDevice_;\n> > - libcamera::CameraConfiguration *config_;\n> > - Type type_;\n> > + CameraDevice const *cameraDevice_;\n> > + const libcamera::CameraConfiguration *config_;\n> > + const Type type_;\n> > camera3_stream_t *camera3Stream_;\n> > - unsigned int index_;\n> > + const unsigned int index_;\n> >\n> > std::unique_ptr allocator_;\n> > std::vector buffers_;\n> > diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n> > index 4483153..270ea60 100644\n> > --- a/src/android/jpeg/encoder.h\n> > +++ b/src/android/jpeg/encoder.h\n> > @@ -14,11 +14,11 @@\n> > class Encoder\n> > {\n> > public:\n> > - virtual ~Encoder() {};\n> > + virtual ~Encoder() {}\n> >\n> > virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;\n> > - virtual int encode(const libcamera::FrameBuffer *source,\n> > - const libcamera::Span &destination,\n> > + virtual int encode(const libcamera::FrameBuffer &source,\n> > + libcamera::Span destination,\n> > const libcamera::Span &exifData) = 0;\n> > };\n> >\n> > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> > index 8995ba5..4236c9d 100644\n> > --- a/src/android/jpeg/encoder_libjpeg.cpp\n> > +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> > @@ -179,11 +179,11 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n> > }\n> > }\n> >\n> > -int EncoderLibJpeg::encode(const FrameBuffer *source,\n> > - const libcamera::Span &dest,\n> > +int EncoderLibJpeg::encode(const FrameBuffer &source,\n> > + libcamera::Span dest,\n> > const libcamera::Span &exifData)\n> > {\n> > - MappedFrameBuffer frame(source, PROT_READ);\n> > + MappedFrameBuffer frame(&source, PROT_READ);\n> > if (!frame.isValid()) {\n> > LOG(JPEG, Error) << \"Failed to map FrameBuffer : \"\n> > << strerror(frame.error());\n> > diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> > index 934caef..391a53c 100644\n> > --- a/src/android/jpeg/encoder_libjpeg.h\n> > +++ b/src/android/jpeg/encoder_libjpeg.h\n> > @@ -21,8 +21,8 @@ public:\n> > ~EncoderLibJpeg();\n> >\n> > int configure(const libcamera::StreamConfiguration &cfg) override;\n> > - int encode(const libcamera::FrameBuffer *source,\n> > - const libcamera::Span &destination,\n> > + int encode(const libcamera::FrameBuffer &source,\n> > + libcamera::Span destination,\n> > const libcamera::Span &exifData) override;\n> >\n> > private:\n> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > index 753c28e..4ded3e3 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > @@ -44,8 +44,8 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n> > return encoder_->configure(inCfg);\n> > }\n> >\n> > -int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,\n> > - const libcamera::Span &destination,\n> > +int PostProcessorJpeg::process(const libcamera::FrameBuffer &source,\n> > + libcamera::Span destination,\n> > CameraMetadata *metadata)\n> > {\n> > if (!encoder_)\n> > @@ -64,8 +64,10 @@ int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,\n> > * second, it is good enough.\n> > */\n> > exif.setTimestamp(std::time(nullptr));\n> > - if (exif.generate() != 0)\n> > + if (exif.generate() != 0) {\n> > LOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n> > + return -EINVAL;\n> > + }\n> >\n> Personally speaking, I don't think we should return FAIL here. So my\n> preference is to skip this change.\n\nThanks. Kieran also told me that in another mail thread.\nI will do so in the next patch series.\n\nRegards,\n-Hiro\n\n> > int jpeg_size = encoder_->encode(source, destination, exif.data());\n> > if (jpeg_size < 0) {\n> > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> > index 62c8650..4d8edf5 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.h\n> > +++ b/src/android/jpeg/post_processor_jpeg.h\n> > @@ -23,12 +23,12 @@ public:\n> >\n> > int configure(const libcamera::StreamConfiguration &incfg,\n> > const libcamera::StreamConfiguration &outcfg) override;\n> > - int process(const libcamera::FrameBuffer *source,\n> > - const libcamera::Span &destination,\n> > + int process(const libcamera::FrameBuffer &source,\n> > + libcamera::Span destination,\n> > CameraMetadata *metadata) override;\n> >\n> > private:\n> > - CameraDevice *cameraDevice_;\n> > + CameraDevice const *cameraDevice_;\n> > std::unique_ptr encoder_;\n> > libcamera::Size streamSize_;\n> > };\n> > diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> > index a891c43..5f87a5d 100644\n> > --- a/src/android/post_processor.h\n> > +++ b/src/android/post_processor.h\n> > @@ -20,8 +20,8 @@ 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> > - const libcamera::Span &destination,\n> > + virtual int process(const libcamera::FrameBuffer &source,\n> > + libcamera::Span destination,\n> > CameraMetadata *metadata) = 0;\n> > };\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9A2E0BDB1F\n\tfor ;\n\tMon, 19 Oct 2020 11:44:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A6B061373;\n\tMon, 19 Oct 2020 13:44:37 +0200 (CEST)","from mail-ej1-x643.google.com (mail-ej1-x643.google.com\n\t[IPv6:2a00:1450:4864:20::643])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 46668607B8\n\tfor ;\n\tMon, 19 Oct 2020 13:44:36 +0200 (CEST)","by mail-ej1-x643.google.com with SMTP id u8so13459556ejg.1\n\tfor ;\n\tMon, 19 Oct 2020 04:44:36 -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=\"JGr0Z0Za\"; 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=HCRrw29GSyJRUrfD+xIOBy4jwpmaLs4FtdQYO8aAa5o=;\n\tb=JGr0Z0ZaDX4NkszuORIh+9pEJMz0D1H/k8izoRi6E31UAzvNz3o1GqrnaWCt8UxpqT\n\t4nbT/h7uTvCTE6AAHFCT65kKqETnyIgCtT6rQU2tg7TrpujaCyM2TMN60iOeB9nbSL6r\n\t6OCuBmqLQyTxUafHTkaDTzDgaTXfleqCpq64E=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=HCRrw29GSyJRUrfD+xIOBy4jwpmaLs4FtdQYO8aAa5o=;\n\tb=mWMR4YH5RZJKjPRyRefh5kplRPcU6U0Vezh8NuI6CetbDI91S7mVOh3m4u3BEhces0\n\tjmt2f1Fko3XI7N8vdgDAfSOoMxccywcK0qvun+8sdaseH7BBTTyjB4j9LcjszWkU3+qj\n\te+09ttutx7IfnSS4cJfoRD417aGM3EdldH3WUur+j1eIO66SRt6I0GJepveIJCwu03e2\n\tFehwnti2IeGCqcd9Zh67lpIv7u+YlMJOhBJgj5vbeszjs3+cFnciCYMV8X5+nfcVarFd\n\t4navLjF03EZS8+i3xQjwD2H+xGhYzz2gRWjwZRNbTf74txkRSn5zaSMfJ+pJ6HffWo6X\n\tRt0w==","X-Gm-Message-State":"AOAM5334kysCpPig4Gb+1ULyjcapkj9SqzR2Eld08c+7mtfY/d9m/nwZ\n\tis1nLMIKvSeZIO+dgHxj9vEOXlUG43TcyLSKVKwBFw==","X-Google-Smtp-Source":"ABdhPJw2K9nHHDlYKmKW8RfIZg4DHwzEMgWqpMkrnPxPQGGsUo0VTz2+mGzG0BdTrOFJ9bsdJ3FYzJ6mugHM+yLcU48=","X-Received":"by 2002:a17:906:1955:: with SMTP id\n\tb21mr17413164eje.42.1603107875755; \n\tMon, 19 Oct 2020 04:44:35 -0700 (PDT)","MIME-Version":"1.0","References":"<20201019080323.2236548-1-hiroh@chromium.org>\n\t<0a009843-abea-2b14-2b6b-dd58caedfff0@uajain.com>","In-Reply-To":"<0a009843-abea-2b14-2b6b-dd58caedfff0@uajain.com>","From":"Hirokazu Honda ","Date":"Mon, 19 Oct 2020 20:44:24 +0900","Message-ID":"","To":"Umang Jain ","Subject":"Re: [libcamera-devel] [PATCH] android: Modify PostProcessor\n\tinterface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13252,"web_url":"https://patchwork.libcamera.org/comment/13252/","msgid":"","date":"2020-10-19T11:55:38","subject":"Re: [libcamera-devel] [PATCH] android: Modify PostProcessor\n\tinterface","submitter":{"id":9,"url":"https://patchwork.libcamera.org/api/people/9/","name":"Tomasz Figa","email":"tfiga@chromium.org"},"content":"On Mon, Oct 19, 2020 at 1:24 PM Umang Jain wrote:\n>\n> Hi Hiro,\n>\n> On 10/19/20 1:33 PM, Hirokazu Honda wrote:\n> > This modifies PostProcessor interface and polishes the code\n> > around the PostProcessor.\n> >\n> > Signed-off-by: Hirokazu Honda \n> Changes looks good, albeit a few comments. Also, I think the patch could\n> also be split down further to address the per Span pass-by-value, per\n> *-type reference to &-type, etc.\n\n+1\n\nI'd also see rationale explained for each change.\n\n> > ---\n> > src/android/camera_stream.cpp | 9 ++++-----\n> > src/android/camera_stream.h | 8 ++++----\n> > src/android/jpeg/encoder.h | 6 +++---\n> > src/android/jpeg/encoder_libjpeg.cpp | 6 +++---\n> > src/android/jpeg/encoder_libjpeg.h | 4 ++--\n> > src/android/jpeg/post_processor_jpeg.cpp | 8 +++++---\n> > src/android/jpeg/post_processor_jpeg.h | 6 +++---\n> > src/android/post_processor.h | 4 ++--\n> > 8 files changed, 26 insertions(+), 25 deletions(-)\n> >\n> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > index 1b8afa8..cc8691d 100644\n> > --- a/src/android/camera_stream.cpp\n> > +++ b/src/android/camera_stream.cpp\n> > @@ -40,11 +40,10 @@ LOG_DECLARE_CATEGORY(HAL);\n> >\n> > CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,\n> > camera3_stream_t *camera3Stream, unsigned int index)\n> > - : cameraDevice_(cameraDevice), type_(type),\n> > + : cameraDevice_(cameraDevice),\n> > + config_(cameraDevice_->cameraConfiguration()), type_(type),\n> Will cameraDevice_ will be initialized by this point? I am not sure.\n> > camera3Stream_(camera3Stream), index_(index)\n> > {\n> > - config_ = cameraDevice_->cameraConfiguration();\n> > -\n> > if (type_ == Type::Internal || type_ == Type::Mapped) {\n> > /*\n> > * \\todo There might be multiple post-processors. The logic\n> > @@ -52,7 +51,7 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,\n> > * future. For now, we only have PostProcessorJpeg and that\n> > * is what we instantiate here.\n> > */\n> > - postProcessor_ = std::make_unique(cameraDevice_);\n> > + postProcessor_ = std::make_unique(cameraDevice);\n> What does this change achieve?\n> > }\n> >\n> > if (type == Type::Internal) {\n> > @@ -102,7 +101,7 @@ int CameraStream::process(const libcamera::FrameBuffer &source,\n> > if (!postProcessor_)\n> > return 0;\n> >\n> > - return postProcessor_->process(&source, dest->maps()[0], metadata);\n> > + return postProcessor_->process(source, dest->maps()[0], metadata);\n> > }\n> >\n> > FrameBuffer *CameraStream::getBuffer()\n> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> > index c367a5f..24e66bb 100644\n> > --- a/src/android/camera_stream.h\n> > +++ b/src/android/camera_stream.h\n> > @@ -124,11 +124,11 @@ public:\n> > void putBuffer(libcamera::FrameBuffer *buffer);\n> >\n> > private:\n> > - CameraDevice *cameraDevice_;\n> > - libcamera::CameraConfiguration *config_;\n> > - Type type_;\n> > + CameraDevice const *cameraDevice_;\n> > + const libcamera::CameraConfiguration *config_;\n> > + const Type type_;\n> > camera3_stream_t *camera3Stream_;\n> > - unsigned int index_;\n> > + const unsigned int index_;\n> >\n> > std::unique_ptr allocator_;\n> > std::vector buffers_;\n> > diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n> > index 4483153..270ea60 100644\n> > --- a/src/android/jpeg/encoder.h\n> > +++ b/src/android/jpeg/encoder.h\n> > @@ -14,11 +14,11 @@\n> > class Encoder\n> > {\n> > public:\n> > - virtual ~Encoder() {};\n> > + virtual ~Encoder() {}\n> >\n> > virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;\n> > - virtual int encode(const libcamera::FrameBuffer *source,\n> > - const libcamera::Span &destination,\n> > + virtual int encode(const libcamera::FrameBuffer &source,\n> > + libcamera::Span destination,\n> > const libcamera::Span &exifData) = 0;\n> > };\n> >\n> > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> > index 8995ba5..4236c9d 100644\n> > --- a/src/android/jpeg/encoder_libjpeg.cpp\n> > +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> > @@ -179,11 +179,11 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n> > }\n> > }\n> >\n> > -int EncoderLibJpeg::encode(const FrameBuffer *source,\n> > - const libcamera::Span &dest,\n> > +int EncoderLibJpeg::encode(const FrameBuffer &source,\n> > + libcamera::Span dest,\n> > const libcamera::Span &exifData)\n> > {\n> > - MappedFrameBuffer frame(source, PROT_READ);\n> > + MappedFrameBuffer frame(&source, PROT_READ);\n> > if (!frame.isValid()) {\n> > LOG(JPEG, Error) << \"Failed to map FrameBuffer : \"\n> > << strerror(frame.error());\n> > diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> > index 934caef..391a53c 100644\n> > --- a/src/android/jpeg/encoder_libjpeg.h\n> > +++ b/src/android/jpeg/encoder_libjpeg.h\n> > @@ -21,8 +21,8 @@ public:\n> > ~EncoderLibJpeg();\n> >\n> > int configure(const libcamera::StreamConfiguration &cfg) override;\n> > - int encode(const libcamera::FrameBuffer *source,\n> > - const libcamera::Span &destination,\n> > + int encode(const libcamera::FrameBuffer &source,\n> > + libcamera::Span destination,\n> > const libcamera::Span &exifData) override;\n> >\n> > private:\n> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > index 753c28e..4ded3e3 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > @@ -44,8 +44,8 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n> > return encoder_->configure(inCfg);\n> > }\n> >\n> > -int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,\n> > - const libcamera::Span &destination,\n> > +int PostProcessorJpeg::process(const libcamera::FrameBuffer &source,\n> > + libcamera::Span destination,\n> > CameraMetadata *metadata)\n> > {\n> > if (!encoder_)\n> > @@ -64,8 +64,10 @@ int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,\n> > * second, it is good enough.\n> > */\n> > exif.setTimestamp(std::time(nullptr));\n> > - if (exif.generate() != 0)\n> > + if (exif.generate() != 0) {\n> > LOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n> > + return -EINVAL;\n> > + }\n> >\n> Personally speaking, I don't think we should return FAIL here. So my\n> preference is to skip this change.\n\nI'd consider this to be governed by Android expectations.\n\nHiro, did you base on some kind of documented expectations when making\nthis change? If not, would you mind researching what's required for\ncompliance here? Thanks!\n\nBest regards,\nTomasz","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 C3171BDB13\n\tfor ;\n\tMon, 19 Oct 2020 11:55:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 573BD61373;\n\tMon, 19 Oct 2020 13:55:52 +0200 (CEST)","from mail-ed1-x542.google.com (mail-ed1-x542.google.com\n\t[IPv6:2a00:1450:4864:20::542])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5133C607B8\n\tfor ;\n\tMon, 19 Oct 2020 13:55:51 +0200 (CEST)","by mail-ed1-x542.google.com with SMTP id cm24so9955209edb.0\n\tfor ;\n\tMon, 19 Oct 2020 04:55:51 -0700 (PDT)","from mail-wm1-f43.google.com (mail-wm1-f43.google.com.\n\t[209.85.128.43]) by smtp.gmail.com with ESMTPSA id\n\tw22sm10795225edu.15.2020.10.19.04.55.49\n\tfor \n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tMon, 19 Oct 2020 04:55:49 -0700 (PDT)","by mail-wm1-f43.google.com with SMTP id q5so12535681wmq.0\n\tfor ;\n\tMon, 19 Oct 2020 04:55:49 -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=\"MizCP3op\"; 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=eQjXjQbjsehg9jjbtQ4F4aS33U0fdKue4HXIluEeQGg=;\n\tb=MizCP3oplX2603IuOD/PcPdlK9nOsSiyeW94ZiAi2jI01kMhqDUBwZHJnmJHKZzMfV\n\tEGAs4dYgdC9K5GcT8+sMpffTVwt6Jh8O61WW2GyRnbPi+/SzC5TIvWvwEbKA0M7XncvE\n\thT7ksK9je6ML/BQrue7D09QhR8YP5lE8yVck8=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=eQjXjQbjsehg9jjbtQ4F4aS33U0fdKue4HXIluEeQGg=;\n\tb=d7zOX93u8aE4s2KtepfZOcGK9Y+5hhki16N7n0xAQO9H0SH9NUPkyweDfi6NIZsOaF\n\tIUouGNH766a1DQj8i9BKuzugKaDDXfeX6y1BLPF4kKF9vYyOxOGl12YO6C1G53vNiORi\n\tggdHbKqLWpQBUl27ff/7qJjk2Ziv2nVZBodJkM0s6GlRxH8KKCrpYdLBtcCn+piTR4yi\n\t4fI4SumwlBV7EmgDrPmO9242xJRsJ/ghu0Za7xY9nTuCGF+RzEQd9x7tKDFhNcL7nJ5X\n\tLAMB8rg95CfiGiYnHEl5olVgaGow03yab4tBxppOiumeJdg10iLGz436/m1FyTxy/x58\n\tqe6g==","X-Gm-Message-State":"AOAM533bGIMdZscNh93HzhcFerp7DFT+/MKzeHiafhbUIbwMkdLjnIVE\n\tHL0Q/S0J6TydyPt0+RM3t0PtPtsp+8+6Ag==","X-Google-Smtp-Source":"ABdhPJzPwrH9Zaqot1DZnzuTaFcNV7TieO3oUwi/IO2mqpaLnqZ7SZdVEJfyWO5qSGYDTyJTZAnCSg==","X-Received":["by 2002:a05:6402:124a:: with SMTP id\n\tl10mr17431025edw.99.1603108550532; \n\tMon, 19 Oct 2020 04:55:50 -0700 (PDT)","by 2002:a1c:8057:: with SMTP id\n\tb84mr17005331wmd.116.1603108549005; \n\tMon, 19 Oct 2020 04:55:49 -0700 (PDT)"],"MIME-Version":"1.0","References":"<20201019080323.2236548-1-hiroh@chromium.org>\n\t<0a009843-abea-2b14-2b6b-dd58caedfff0@uajain.com>","In-Reply-To":"<0a009843-abea-2b14-2b6b-dd58caedfff0@uajain.com>","From":"Tomasz Figa ","Date":"Mon, 19 Oct 2020 13:55:38 +0200","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"Umang Jain , Hirokazu Honda ","Subject":"Re: [libcamera-devel] [PATCH] android: Modify PostProcessor\n\tinterface","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13253,"web_url":"https://patchwork.libcamera.org/comment/13253/","msgid":"<20201019125743.GC3936@pendragon.ideasonboard.com>","date":"2020-10-19T12:57:43","subject":"Re: [libcamera-devel] [PATCH] android: Modify PostProcessor\n\tinterface","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello everybody,\n\nHiro-san, thank you for the patch.\n\nOn Mon, Oct 19, 2020 at 04:54:27PM +0530, Umang Jain wrote:\n> Hi Hiro,\n> \n> On 10/19/20 1:33 PM, Hirokazu Honda wrote:\n> > This modifies PostProcessor interface and polishes the code\n> > around the PostProcessor.\n> >\n> > Signed-off-by: Hirokazu Honda \n>\n> Changes looks good, albeit a few comments. Also, I think the patch could \n> also be split down further to address the per Span pass-by-value,  per \n> *-type reference to &-type, etc.\n\nAgreed, an explanation of the rationale is useful. It may appear\nstraightforward today, but when we will read the code in the future we\nmay not remember the context, so it should be captured in the commit\nmessage.\n\n> > ---\n> > src/android/camera_stream.cpp | 9 ++++-----\n> > src/android/camera_stream.h | 8 ++++----\n> > src/android/jpeg/encoder.h | 6 +++---\n> > src/android/jpeg/encoder_libjpeg.cpp | 6 +++---\n> > src/android/jpeg/encoder_libjpeg.h | 4 ++--\n> > src/android/jpeg/post_processor_jpeg.cpp | 8 +++++---\n> > src/android/jpeg/post_processor_jpeg.h | 6 +++---\n> > src/android/post_processor.h | 4 ++--\n> > 8 files changed, 26 insertions(+), 25 deletions(-)\n> >\n> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > index 1b8afa8..cc8691d 100644\n> > --- a/src/android/camera_stream.cpp\n> > +++ b/src/android/camera_stream.cpp\n> > @@ -40,11 +40,10 @@ LOG_DECLARE_CATEGORY(HAL);\n> > \n> > CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,\n> > \t\t\t camera3_stream_t *camera3Stream, unsigned int index)\n> > -\t: cameraDevice_(cameraDevice), type_(type),\n> > +\t: cameraDevice_(cameraDevice),\n> > +\t config_(cameraDevice_->cameraConfiguration()), type_(type),\n>\n> Will cameraDevice_ will be initialized by this point? I am not sure.\n\nC++ initializes fields in the order they are defined in the class. The\ncompiler warns if the initialization list here is in a different order,\nso the two are guaranteed to match. cameraDevice_ is thus guaranteed to\nbe initialized here.\n\n> > \t camera3Stream_(camera3Stream), index_(index)\n> > {\n> > -\tconfig_ = cameraDevice_->cameraConfiguration();\n> > -\n> > \tif (type_ == Type::Internal || type_ == Type::Mapped) {\n> > \t\t/*\n> > \t\t * \\todo There might be multiple post-processors. The logic\n> > @@ -52,7 +51,7 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,\n> > \t\t * future. For now, we only have PostProcessorJpeg and that\n> > \t\t * is what we instantiate here.\n> > \t\t */\n> > -\t\tpostProcessor_ = std::make_unique(cameraDevice_);\n> > +\t\tpostProcessor_ = std::make_unique(cameraDevice);\n>\n> What does this change achieve?\n>\n> > \t}\n> > \n> > \tif (type == Type::Internal) {\n> > @@ -102,7 +101,7 @@ int CameraStream::process(const libcamera::FrameBuffer &source,\n> > \tif (!postProcessor_)\n> > \t\treturn 0;\n> > \n> > -\treturn postProcessor_->process(&source, dest->maps()[0], metadata);\n> > +\treturn postProcessor_->process(source, dest->maps()[0], metadata);\n> > }\n> > \n> > FrameBuffer *CameraStream::getBuffer()\n> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> > index c367a5f..24e66bb 100644\n> > --- a/src/android/camera_stream.h\n> > +++ b/src/android/camera_stream.h\n> > @@ -124,11 +124,11 @@ public:\n> > \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n> > \n> > private:\n> > -\tCameraDevice *cameraDevice_;\n> > -\tlibcamera::CameraConfiguration *config_;\n> > -\tType type_;\n> > +\tCameraDevice const *cameraDevice_;\n> > +\tconst libcamera::CameraConfiguration *config_;\n> > +\tconst Type type_;\n> > \tcamera3_stream_t *camera3Stream_;\n> > -\tunsigned int index_;\n> > +\tconst unsigned int index_;\n> > \n> > \tstd::unique_ptr allocator_;\n> > \tstd::vector buffers_;\n> > diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n> > index 4483153..270ea60 100644\n> > --- a/src/android/jpeg/encoder.h\n> > +++ b/src/android/jpeg/encoder.h\n> > @@ -14,11 +14,11 @@\n> > class Encoder\n> > {\n> > public:\n> > -\tvirtual ~Encoder() {};\n> > +\tvirtual ~Encoder() {}\n\nThis should also be split in a separate patch, it's an easy one.\n\n> > \n> > \tvirtual int configure(const libcamera::StreamConfiguration &cfg) = 0;\n> > -\tvirtual int encode(const libcamera::FrameBuffer *source,\n> > -\t\t\t const libcamera::Span &destination,\n> > +\tvirtual int encode(const libcamera::FrameBuffer &source,\n> > +\t\t\t libcamera::Span destination,\n> > \t\t\t const libcamera::Span &exifData) = 0;\n> > };\n> > \n> > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> > index 8995ba5..4236c9d 100644\n> > --- a/src/android/jpeg/encoder_libjpeg.cpp\n> > +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> > @@ -179,11 +179,11 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n> > \t}\n> > }\n> > \n> > -int EncoderLibJpeg::encode(const FrameBuffer *source,\n> > -\t\t\t const libcamera::Span &dest,\n> > +int EncoderLibJpeg::encode(const FrameBuffer &source,\n> > +\t\t\t libcamera::Span dest,\n> > \t\t\t const libcamera::Span &exifData)\n> > {\n> > -\tMappedFrameBuffer frame(source, PROT_READ);\n> > +\tMappedFrameBuffer frame(&source, PROT_READ);\n> > \tif (!frame.isValid()) {\n> > \t\tLOG(JPEG, Error) << \"Failed to map FrameBuffer : \"\n> > \t\t\t\t << strerror(frame.error());\n> > diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> > index 934caef..391a53c 100644\n> > --- a/src/android/jpeg/encoder_libjpeg.h\n> > +++ b/src/android/jpeg/encoder_libjpeg.h\n> > @@ -21,8 +21,8 @@ public:\n> > \t~EncoderLibJpeg();\n> > \n> > \tint configure(const libcamera::StreamConfiguration &cfg) override;\n> > -\tint encode(const libcamera::FrameBuffer *source,\n> > -\t\t const libcamera::Span &destination,\n> > +\tint encode(const libcamera::FrameBuffer &source,\n> > +\t\t libcamera::Span destination,\n> > \t\t const libcamera::Span &exifData) override;\n> > \n> > private:\n> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > index 753c28e..4ded3e3 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > @@ -44,8 +44,8 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n> > \treturn encoder_->configure(inCfg);\n> > }\n> > \n> > -int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,\n> > -\t\t\t const libcamera::Span &destination,\n> > +int PostProcessorJpeg::process(const libcamera::FrameBuffer &source,\n> > +\t\t\t libcamera::Span destination,\n> > \t\t\t CameraMetadata *metadata)\n> > {\n> > \tif (!encoder_)\n> > @@ -64,8 +64,10 @@ int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,\n> > \t * second, it is good enough.\n> > \t */\n> > \texif.setTimestamp(std::time(nullptr));\n> > -\tif (exif.generate() != 0)\n> > +\tif (exif.generate() != 0) {\n> > \t\tLOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > \n>\n> Personally speaking, I don't think we should return FAIL here. So my \n> preference is to skip this change.\n\nTomasz' recommendation of aligning this with Android's requirements\nmakes sense I think.\n\n> > \tint jpeg_size = encoder_->encode(source, destination, exif.data());\n> > \tif (jpeg_size < 0) {\n> > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> > index 62c8650..4d8edf5 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.h\n> > +++ b/src/android/jpeg/post_processor_jpeg.h\n> > @@ -23,12 +23,12 @@ 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 const libcamera::Span &destination,\n> > +\tint process(const libcamera::FrameBuffer &source,\n> > +\t\t libcamera::Span destination,\n> > \t\t CameraMetadata *metadata) override;\n> > \n> > private:\n> > -\tCameraDevice *cameraDevice_;\n> > +\tCameraDevice const *cameraDevice_;\n> > \tstd::unique_ptr encoder_;\n> > \tlibcamera::Size streamSize_;\n> > };\n> > diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> > index a891c43..5f87a5d 100644\n> > --- a/src/android/post_processor.h\n> > +++ b/src/android/post_processor.h\n> > @@ -20,8 +20,8 @@ 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 const libcamera::Span &destination,\n> > +\tvirtual int process(const libcamera::FrameBuffer &source,\n> > +\t\t\t libcamera::Span destination,\n> > \t\t\t CameraMetadata *metadata) = 0;\n> > };\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A6342BDB13\n\tfor ;\n\tMon, 19 Oct 2020 12:58:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1519A61373;\n\tMon, 19 Oct 2020 14:58:32 +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 B3A8C607B8\n\tfor ;\n\tMon, 19 Oct 2020 14:58:30 +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 E7AC848;\n\tMon, 19 Oct 2020 14:58:29 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"YpLh2+pg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603112310;\n\tbh=MXxJQ+kUo95ATTga60hhIpgliqSJuI570gYk1GMAZGc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YpLh2+pg+4IV27AbKZlSMR/z9cp4gE+reEv5EDBuR1g4rLrO5eIJHkjOYT0UooTRN\n\tmqxAm4gd3iTlh9bfeEXEemwpeMcy7zT0e0oPMRM/3HsxEd9cCvmtGKPXkuN6/cJWRS\n\te4tbcSvbBWazRXtOkvbAPL1aoY/tUOxwzqxWumD4=","Date":"Mon, 19 Oct 2020 15:57:43 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20201019125743.GC3936@pendragon.ideasonboard.com>","References":"<20201019080323.2236548-1-hiroh@chromium.org>\n\t<0a009843-abea-2b14-2b6b-dd58caedfff0@uajain.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<0a009843-abea-2b14-2b6b-dd58caedfff0@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH] android: Modify PostProcessor\n\tinterface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13269,"web_url":"https://patchwork.libcamera.org/comment/13269/","msgid":"","date":"2020-10-20T06:51:25","subject":"Re: [libcamera-devel] [PATCH] android: Modify PostProcessor\n\tinterface","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Thanks Laurent and Tomasz,\n\nOn Mon, Oct 19, 2020 at 9:58 PM Laurent Pinchart\n wrote:\n>\n> Hello everybody,\n>\n> Hiro-san, thank you for the patch.\n>\n> On Mon, Oct 19, 2020 at 04:54:27PM +0530, Umang Jain wrote:\n> > Hi Hiro,\n> >\n> > On 10/19/20 1:33 PM, Hirokazu Honda wrote:\n> > > This modifies PostProcessor interface and polishes the code\n> > > around the PostProcessor.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> >\n> > Changes looks good, albeit a few comments. Also, I think the patch could\n> > also be split down further to address the per Span pass-by-value, per\n> > *-type reference to &-type, etc.\n>\n> Agreed, an explanation of the rationale is useful. It may appear\n> straightforward today, but when we will read the code in the future we\n> may not remember the context, so it should be captured in the commit\n> message.\n>\n> > > ---\n> > > src/android/camera_stream.cpp | 9 ++++-----\n> > > src/android/camera_stream.h | 8 ++++----\n> > > src/android/jpeg/encoder.h | 6 +++---\n> > > src/android/jpeg/encoder_libjpeg.cpp | 6 +++---\n> > > src/android/jpeg/encoder_libjpeg.h | 4 ++--\n> > > src/android/jpeg/post_processor_jpeg.cpp | 8 +++++---\n> > > src/android/jpeg/post_processor_jpeg.h | 6 +++---\n> > > src/android/post_processor.h | 4 ++--\n> > > 8 files changed, 26 insertions(+), 25 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > > index 1b8afa8..cc8691d 100644\n> > > --- a/src/android/camera_stream.cpp\n> > > +++ b/src/android/camera_stream.cpp\n> > > @@ -40,11 +40,10 @@ LOG_DECLARE_CATEGORY(HAL);\n> > >\n> > > CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,\n> > > camera3_stream_t *camera3Stream, unsigned int index)\n> > > - : cameraDevice_(cameraDevice), type_(type),\n> > > + : cameraDevice_(cameraDevice),\n> > > + config_(cameraDevice_->cameraConfiguration()), type_(type),\n> >\n> > Will cameraDevice_ will be initialized by this point? I am not sure.\n>\n> C++ initializes fields in the order they are defined in the class. The\n> compiler warns if the initialization list here is in a different order,\n> so the two are guaranteed to match. cameraDevice_ is thus guaranteed to\n> be initialized here.\n>\n> > > camera3Stream_(camera3Stream), index_(index)\n> > > {\n> > > - config_ = cameraDevice_->cameraConfiguration();\n> > > -\n> > > if (type_ == Type::Internal || type_ == Type::Mapped) {\n> > > /*\n> > > * \\todo There might be multiple post-processors. The logic\n> > > @@ -52,7 +51,7 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,\n> > > * future. For now, we only have PostProcessorJpeg and that\n> > > * is what we instantiate here.\n> > > */\n> > > - postProcessor_ = std::make_unique(cameraDevice_);\n> > > + postProcessor_ = std::make_unique(cameraDevice);\n> >\n> > What does this change achieve?\n> >\n> > > }\n> > >\n> > > if (type == Type::Internal) {\n> > > @@ -102,7 +101,7 @@ int CameraStream::process(const libcamera::FrameBuffer &source,\n> > > if (!postProcessor_)\n> > > return 0;\n> > >\n> > > - return postProcessor_->process(&source, dest->maps()[0], metadata);\n> > > + return postProcessor_->process(source, dest->maps()[0], metadata);\n> > > }\n> > >\n> > > FrameBuffer *CameraStream::getBuffer()\n> > > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> > > index c367a5f..24e66bb 100644\n> > > --- a/src/android/camera_stream.h\n> > > +++ b/src/android/camera_stream.h\n> > > @@ -124,11 +124,11 @@ public:\n> > > void putBuffer(libcamera::FrameBuffer *buffer);\n> > >\n> > > private:\n> > > - CameraDevice *cameraDevice_;\n> > > - libcamera::CameraConfiguration *config_;\n> > > - Type type_;\n> > > + CameraDevice const *cameraDevice_;\n> > > + const libcamera::CameraConfiguration *config_;\n> > > + const Type type_;\n> > > camera3_stream_t *camera3Stream_;\n> > > - unsigned int index_;\n> > > + const unsigned int index_;\n> > >\n> > > std::unique_ptr allocator_;\n> > > std::vector buffers_;\n> > > diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n> > > index 4483153..270ea60 100644\n> > > --- a/src/android/jpeg/encoder.h\n> > > +++ b/src/android/jpeg/encoder.h\n> > > @@ -14,11 +14,11 @@\n> > > class Encoder\n> > > {\n> > > public:\n> > > - virtual ~Encoder() {};\n> > > + virtual ~Encoder() {}\n>\n> This should also be split in a separate patch, it's an easy one.\n>\n\nI submitted a patch series of removing extra semicolons widely in libcamera.\nI will not touch this in this patch therefore.\n\n> > >\n> > > virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;\n> > > - virtual int encode(const libcamera::FrameBuffer *source,\n> > > - const libcamera::Span &destination,\n> > > + virtual int encode(const libcamera::FrameBuffer &source,\n> > > + libcamera::Span destination,\n> > > const libcamera::Span &exifData) = 0;\n> > > };\n> > >\n> > > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> > > index 8995ba5..4236c9d 100644\n> > > --- a/src/android/jpeg/encoder_libjpeg.cpp\n> > > +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> > > @@ -179,11 +179,11 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n> > > }\n> > > }\n> > >\n> > > -int EncoderLibJpeg::encode(const FrameBuffer *source,\n> > > - const libcamera::Span &dest,\n> > > +int EncoderLibJpeg::encode(const FrameBuffer &source,\n> > > + libcamera::Span dest,\n> > > const libcamera::Span &exifData)\n> > > {\n> > > - MappedFrameBuffer frame(source, PROT_READ);\n> > > + MappedFrameBuffer frame(&source, PROT_READ);\n> > > if (!frame.isValid()) {\n> > > LOG(JPEG, Error) << \"Failed to map FrameBuffer : \"\n> > > << strerror(frame.error());\n> > > diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> > > index 934caef..391a53c 100644\n> > > --- a/src/android/jpeg/encoder_libjpeg.h\n> > > +++ b/src/android/jpeg/encoder_libjpeg.h\n> > > @@ -21,8 +21,8 @@ public:\n> > > ~EncoderLibJpeg();\n> > >\n> > > int configure(const libcamera::StreamConfiguration &cfg) override;\n> > > - int encode(const libcamera::FrameBuffer *source,\n> > > - const libcamera::Span &destination,\n> > > + int encode(const libcamera::FrameBuffer &source,\n> > > + libcamera::Span destination,\n> > > const libcamera::Span &exifData) override;\n> > >\n> > > private:\n> > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > > index 753c28e..4ded3e3 100644\n> > > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > > @@ -44,8 +44,8 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n> > > return encoder_->configure(inCfg);\n> > > }\n> > >\n> > > -int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,\n> > > - const libcamera::Span &destination,\n> > > +int PostProcessorJpeg::process(const libcamera::FrameBuffer &source,\n> > > + libcamera::Span destination,\n> > > CameraMetadata *metadata)\n> > > {\n> > > if (!encoder_)\n> > > @@ -64,8 +64,10 @@ int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,\n> > > * second, it is good enough.\n> > > */\n> > > exif.setTimestamp(std::time(nullptr));\n> > > - if (exif.generate() != 0)\n> > > + if (exif.generate() != 0) {\n> > > LOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n> > > + return -EINVAL;\n> > > + }\n> > >\n> >\n> > Personally speaking, I don't think we should return FAIL here. So my\n> > preference is to skip this change.\n>\n> Tomasz' recommendation of aligning this with Android's requirements\n> makes sense I think.\n>\n\nRe Android standard, I saw in CaptureRequest documentation [1] that\nthe exif values affect the aspects of the captured image.\nIn fact, there is a CTS test to check exif metadata [2].\nSearching the android codebase, a couple of implementations [3, 4]\nhandle the exif failure as a capture fatal BUT a few implementations\n[5, 6] also ignore the failure.\nI consider this should be treated as failure?\n\n[1] https://developer.android.com/reference/android/hardware/camera2/CaptureRequest\n[2] https://cs.android.com/android/platform/superproject/+/master:cts/tests/camera/src/android/hardware/camera2/cts/StillCaptureTest.java;l=690;drc=master\n[3] https://cs.android.com/android/platform/superproject/+/master:hardware/libhardware/modules/camera/3_4/arc/image_processor.cpp;l=381;drc=9dae907ea07692118698a242fe4a7427283bd10a\n[4] https://cs.android.com/android/platform/superproject/+/master:frameworks/av/services/camera/libcameraservice/api2/HeicCompositeStream.cpp;l=999;drc=991b7b672203d79b66ba7fcb672cb885d7947ae1\n[5] https://cs.android.com/android/platform/superproject/+/master:frameworks/av/services/camera/libcameraservice/common/DepthPhotoProcessor.cpp;l=211;drc=29e9ec182d20f44ee2e8a15de1940cb4ef29663e\n[6] https://cs.android.com/android/platform/superproject/+/master:hardware/google/camera/devices/EmulatedCamera/hwl/JpegCompressor.cpp;l=314;drc=master\n\nBest Regards,\n-Hiro\n> > > int jpeg_size = encoder_->encode(source, destination, exif.data());\n> > > if (jpeg_size < 0) {\n> > > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> > > index 62c8650..4d8edf5 100644\n> > > --- a/src/android/jpeg/post_processor_jpeg.h\n> > > +++ b/src/android/jpeg/post_processor_jpeg.h\n> > > @@ -23,12 +23,12 @@ public:\n> > >\n> > > int configure(const libcamera::StreamConfiguration &incfg,\n> > > const libcamera::StreamConfiguration &outcfg) override;\n> > > - int process(const libcamera::FrameBuffer *source,\n> > > - const libcamera::Span &destination,\n> > > + int process(const libcamera::FrameBuffer &source,\n> > > + libcamera::Span destination,\n> > > CameraMetadata *metadata) override;\n> > >\n> > > private:\n> > > - CameraDevice *cameraDevice_;\n> > > + CameraDevice const *cameraDevice_;\n> > > std::unique_ptr encoder_;\n> > > libcamera::Size streamSize_;\n> > > };\n> > > diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> > > index a891c43..5f87a5d 100644\n> > > --- a/src/android/post_processor.h\n> > > +++ b/src/android/post_processor.h\n> > > @@ -20,8 +20,8 @@ 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> > > - const libcamera::Span &destination,\n> > > + virtual int process(const libcamera::FrameBuffer &source,\n> > > + libcamera::Span destination,\n> > > CameraMetadata *metadata) = 0;\n> > > };\n> > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 689ABBDB1F\n\tfor ;\n\tTue, 20 Oct 2020 06:51:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A9F4C61373;\n\tTue, 20 Oct 2020 08:51:38 +0200 (CEST)","from mail-ej1-x643.google.com (mail-ej1-x643.google.com\n\t[IPv6:2a00:1450:4864:20::643])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 69CFA60351\n\tfor ;\n\tTue, 20 Oct 2020 08:51:37 +0200 (CEST)","by mail-ej1-x643.google.com with SMTP id e22so1067784ejr.4\n\tfor ;\n\tMon, 19 Oct 2020 23:51:37 -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=\"Bla4ppGE\"; 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=J4CXDDfOltk+DQxeIdWzJDSd9pq+IZtx9duX/MgGvpg=;\n\tb=Bla4ppGEixus14zP31m18dttOnNorT3L3BphhEwyIilEzzCVTCVyCerDcVwlgPP1c5\n\tzW8KexZG+I04rhwf6ARy9pwSUKkb/du2AVKtqJCB+t7AFMgRqd4c81bMkYTXRcMFPHUX\n\tU14BZOIgq7qCRXIMKC2M6rrgoff9uHwhOxyMc=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=J4CXDDfOltk+DQxeIdWzJDSd9pq+IZtx9duX/MgGvpg=;\n\tb=WoIqsET5ug3YG7ZoD7y2wpPVds5x8fmCCBiua4LuDv9ZpV5HSOeSB7GFzFsO3Go0CZ\n\ttEO78iA7n+yi67LX4ba8m1st1+cny+8YcNc6h5DBEIhEimHQjScUceCSZD+IfgSnlzCy\n\tQ2PR2mvl7uvc4lLreiNU9ThQkbrDGW1u4l7L/dJlfWp8hQ7xsc8dZQN8c+WwzsYG1KI7\n\tQbN8/k+1HBBZH9wCMZbxRIMfaMN5x2qQX1EfFRLaANFYGKOfPivIBYLumnTu8evAqFzf\n\t403dhBhLi4mJwy1lkmqT+iGygMWnpjDMUB6/HkhMWSE5HgzhEe1oMesUqgtW5NJkssf9\n\t2VTw==","X-Gm-Message-State":"AOAM532Fe/wKDZuUBrx3mH1qvMtAmklNuP0fus0QviNpFhoi9LsskrnE\n\ttf5rvTMipK71ed0qjeiWxIH0bTfV57iESFYaSb60ow==","X-Google-Smtp-Source":"ABdhPJwES7+EPwcWuwl1W20/6LpAYQZlwmDUsKK0pO2xYNtWemX+1D/+/9O3nMrYOUYiu9g+GKeamtVKZobrHZxKKjg=","X-Received":"by 2002:a17:906:b110:: with SMTP id\n\tu16mr1635956ejy.55.1603176696697; \n\tMon, 19 Oct 2020 23:51:36 -0700 (PDT)","MIME-Version":"1.0","References":"<20201019080323.2236548-1-hiroh@chromium.org>\n\t<0a009843-abea-2b14-2b6b-dd58caedfff0@uajain.com>\n\t<20201019125743.GC3936@pendragon.ideasonboard.com>","In-Reply-To":"<20201019125743.GC3936@pendragon.ideasonboard.com>","From":"Hirokazu Honda ","Date":"Tue, 20 Oct 2020 15:51:25 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH] android: Modify PostProcessor\n\tinterface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13393,"web_url":"https://patchwork.libcamera.org/comment/13393/","msgid":"<20201022041507.GG3942@pendragon.ideasonboard.com>","date":"2020-10-22T04:15:07","subject":"Re: [libcamera-devel] [PATCH] android: Modify PostProcessor\n\tinterface","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro-san,\n\nOn Tue, Oct 20, 2020 at 03:51:25PM +0900, Hirokazu Honda wrote:\n> On Mon, Oct 19, 2020 at 9:58 PM Laurent Pinchart wrote:\n> >\n> > Hello everybody,\n> >\n> > Hiro-san, thank you for the patch.\n> >\n> > On Mon, Oct 19, 2020 at 04:54:27PM +0530, Umang Jain wrote:\n> > > Hi Hiro,\n> > >\n> > > On 10/19/20 1:33 PM, Hirokazu Honda wrote:\n> > > > This modifies PostProcessor interface and polishes the code\n> > > > around the PostProcessor.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > >\n> > > Changes looks good, albeit a few comments. Also, I think the patch could\n> > > also be split down further to address the per Span pass-by-value, per\n> > > *-type reference to &-type, etc.\n> >\n> > Agreed, an explanation of the rationale is useful. It may appear\n> > straightforward today, but when we will read the code in the future we\n> > may not remember the context, so it should be captured in the commit\n> > message.\n> >\n> > > > ---\n> > > > src/android/camera_stream.cpp | 9 ++++-----\n> > > > src/android/camera_stream.h | 8 ++++----\n> > > > src/android/jpeg/encoder.h | 6 +++---\n> > > > src/android/jpeg/encoder_libjpeg.cpp | 6 +++---\n> > > > src/android/jpeg/encoder_libjpeg.h | 4 ++--\n> > > > src/android/jpeg/post_processor_jpeg.cpp | 8 +++++---\n> > > > src/android/jpeg/post_processor_jpeg.h | 6 +++---\n> > > > src/android/post_processor.h | 4 ++--\n> > > > 8 files changed, 26 insertions(+), 25 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > > > index 1b8afa8..cc8691d 100644\n> > > > --- a/src/android/camera_stream.cpp\n> > > > +++ b/src/android/camera_stream.cpp\n> > > > @@ -40,11 +40,10 @@ LOG_DECLARE_CATEGORY(HAL);\n> > > >\n> > > > CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,\n> > > > camera3_stream_t *camera3Stream, unsigned int index)\n> > > > - : cameraDevice_(cameraDevice), type_(type),\n> > > > + : cameraDevice_(cameraDevice),\n> > > > + config_(cameraDevice_->cameraConfiguration()), type_(type),\n> > >\n> > > Will cameraDevice_ will be initialized by this point? I am not sure.\n> >\n> > C++ initializes fields in the order they are defined in the class. The\n> > compiler warns if the initialization list here is in a different order,\n> > so the two are guaranteed to match. cameraDevice_ is thus guaranteed to\n> > be initialized here.\n> >\n> > > > camera3Stream_(camera3Stream), index_(index)\n> > > > {\n> > > > - config_ = cameraDevice_->cameraConfiguration();\n> > > > -\n> > > > if (type_ == Type::Internal || type_ == Type::Mapped) {\n> > > > /*\n> > > > * \\todo There might be multiple post-processors. The logic\n> > > > @@ -52,7 +51,7 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,\n> > > > * future. For now, we only have PostProcessorJpeg and that\n> > > > * is what we instantiate here.\n> > > > */\n> > > > - postProcessor_ = std::make_unique(cameraDevice_);\n> > > > + postProcessor_ = std::make_unique(cameraDevice);\n> > >\n> > > What does this change achieve?\n> > >\n> > > > }\n> > > >\n> > > > if (type == Type::Internal) {\n> > > > @@ -102,7 +101,7 @@ int CameraStream::process(const libcamera::FrameBuffer &source,\n> > > > if (!postProcessor_)\n> > > > return 0;\n> > > >\n> > > > - return postProcessor_->process(&source, dest->maps()[0], metadata);\n> > > > + return postProcessor_->process(source, dest->maps()[0], metadata);\n> > > > }\n> > > >\n> > > > FrameBuffer *CameraStream::getBuffer()\n> > > > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> > > > index c367a5f..24e66bb 100644\n> > > > --- a/src/android/camera_stream.h\n> > > > +++ b/src/android/camera_stream.h\n> > > > @@ -124,11 +124,11 @@ public:\n> > > > void putBuffer(libcamera::FrameBuffer *buffer);\n> > > >\n> > > > private:\n> > > > - CameraDevice *cameraDevice_;\n> > > > - libcamera::CameraConfiguration *config_;\n> > > > - Type type_;\n> > > > + CameraDevice const *cameraDevice_;\n> > > > + const libcamera::CameraConfiguration *config_;\n> > > > + const Type type_;\n> > > > camera3_stream_t *camera3Stream_;\n> > > > - unsigned int index_;\n> > > > + const unsigned int index_;\n> > > >\n> > > > std::unique_ptr allocator_;\n> > > > std::vector buffers_;\n> > > > diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n> > > > index 4483153..270ea60 100644\n> > > > --- a/src/android/jpeg/encoder.h\n> > > > +++ b/src/android/jpeg/encoder.h\n> > > > @@ -14,11 +14,11 @@\n> > > > class Encoder\n> > > > {\n> > > > public:\n> > > > - virtual ~Encoder() {};\n> > > > + virtual ~Encoder() {}\n> >\n> > This should also be split in a separate patch, it's an easy one.\n> >\n> \n> I submitted a patch series of removing extra semicolons widely in libcamera.\n> I will not touch this in this patch therefore.\n> \n> > > >\n> > > > virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;\n> > > > - virtual int encode(const libcamera::FrameBuffer *source,\n> > > > - const libcamera::Span &destination,\n> > > > + virtual int encode(const libcamera::FrameBuffer &source,\n> > > > + libcamera::Span destination,\n> > > > const libcamera::Span &exifData) = 0;\n> > > > };\n> > > >\n> > > > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> > > > index 8995ba5..4236c9d 100644\n> > > > --- a/src/android/jpeg/encoder_libjpeg.cpp\n> > > > +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> > > > @@ -179,11 +179,11 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n> > > > }\n> > > > }\n> > > >\n> > > > -int EncoderLibJpeg::encode(const FrameBuffer *source,\n> > > > - const libcamera::Span &dest,\n> > > > +int EncoderLibJpeg::encode(const FrameBuffer &source,\n> > > > + libcamera::Span dest,\n> > > > const libcamera::Span &exifData)\n> > > > {\n> > > > - MappedFrameBuffer frame(source, PROT_READ);\n> > > > + MappedFrameBuffer frame(&source, PROT_READ);\n> > > > if (!frame.isValid()) {\n> > > > LOG(JPEG, Error) << \"Failed to map FrameBuffer : \"\n> > > > << strerror(frame.error());\n> > > > diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> > > > index 934caef..391a53c 100644\n> > > > --- a/src/android/jpeg/encoder_libjpeg.h\n> > > > +++ b/src/android/jpeg/encoder_libjpeg.h\n> > > > @@ -21,8 +21,8 @@ public:\n> > > > ~EncoderLibJpeg();\n> > > >\n> > > > int configure(const libcamera::StreamConfiguration &cfg) override;\n> > > > - int encode(const libcamera::FrameBuffer *source,\n> > > > - const libcamera::Span &destination,\n> > > > + int encode(const libcamera::FrameBuffer &source,\n> > > > + libcamera::Span destination,\n> > > > const libcamera::Span &exifData) override;\n> > > >\n> > > > private:\n> > > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > > > index 753c28e..4ded3e3 100644\n> > > > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > > > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > > > @@ -44,8 +44,8 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n> > > > return encoder_->configure(inCfg);\n> > > > }\n> > > >\n> > > > -int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,\n> > > > - const libcamera::Span &destination,\n> > > > +int PostProcessorJpeg::process(const libcamera::FrameBuffer &source,\n> > > > + libcamera::Span destination,\n> > > > CameraMetadata *metadata)\n> > > > {\n> > > > if (!encoder_)\n> > > > @@ -64,8 +64,10 @@ int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,\n> > > > * second, it is good enough.\n> > > > */\n> > > > exif.setTimestamp(std::time(nullptr));\n> > > > - if (exif.generate() != 0)\n> > > > + if (exif.generate() != 0) {\n> > > > LOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > >\n> > >\n> > > Personally speaking, I don't think we should return FAIL here. So my\n> > > preference is to skip this change.\n> >\n> > Tomasz' recommendation of aligning this with Android's requirements\n> > makes sense I think.\n> \n> Re Android standard, I saw in CaptureRequest documentation [1] that\n> the exif values affect the aspects of the captured image.\n> In fact, there is a CTS test to check exif metadata [2].\n> Searching the android codebase, a couple of implementations [3, 4]\n> handle the exif failure as a capture fatal BUT a few implementations\n> [5, 6] also ignore the failure.\n> I consider this should be treated as failure?\n\nThis shouldn't fail in normal circumstances, so I don't really see a\nneed for a degraded mode without Exif data as different parts of the\nAndroid stack may choke on this. On the other hand, as it's not more\ncostly for us to handle the failure gracefully, and as it may still\nwork, we could consider this as non-fatal. I'm fine with either option.\n\n> [1] https://developer.android.com/reference/android/hardware/camera2/CaptureRequest\n> [2] https://cs.android.com/android/platform/superproject/+/master:cts/tests/camera/src/android/hardware/camera2/cts/StillCaptureTest.java;l=690;drc=master\n> [3] https://cs.android.com/android/platform/superproject/+/master:hardware/libhardware/modules/camera/3_4/arc/image_processor.cpp;l=381;drc=9dae907ea07692118698a242fe4a7427283bd10a\n> [4] https://cs.android.com/android/platform/superproject/+/master:frameworks/av/services/camera/libcameraservice/api2/HeicCompositeStream.cpp;l=999;drc=991b7b672203d79b66ba7fcb672cb885d7947ae1\n> [5] https://cs.android.com/android/platform/superproject/+/master:frameworks/av/services/camera/libcameraservice/common/DepthPhotoProcessor.cpp;l=211;drc=29e9ec182d20f44ee2e8a15de1940cb4ef29663e\n> [6] https://cs.android.com/android/platform/superproject/+/master:hardware/google/camera/devices/EmulatedCamera/hwl/JpegCompressor.cpp;l=314;drc=master\n> \n> > > > int jpeg_size = encoder_->encode(source, destination, exif.data());\n> > > > if (jpeg_size < 0) {\n> > > > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> > > > index 62c8650..4d8edf5 100644\n> > > > --- a/src/android/jpeg/post_processor_jpeg.h\n> > > > +++ b/src/android/jpeg/post_processor_jpeg.h\n> > > > @@ -23,12 +23,12 @@ public:\n> > > >\n> > > > int configure(const libcamera::StreamConfiguration &incfg,\n> > > > const libcamera::StreamConfiguration &outcfg) override;\n> > > > - int process(const libcamera::FrameBuffer *source,\n> > > > - const libcamera::Span &destination,\n> > > > + int process(const libcamera::FrameBuffer &source,\n> > > > + libcamera::Span destination,\n> > > > CameraMetadata *metadata) override;\n> > > >\n> > > > private:\n> > > > - CameraDevice *cameraDevice_;\n> > > > + CameraDevice const *cameraDevice_;\n> > > > std::unique_ptr encoder_;\n> > > > libcamera::Size streamSize_;\n> > > > };\n> > > > diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> > > > index a891c43..5f87a5d 100644\n> > > > --- a/src/android/post_processor.h\n> > > > +++ b/src/android/post_processor.h\n> > > > @@ -20,8 +20,8 @@ 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> > > > - const libcamera::Span &destination,\n> > > > + virtual int process(const libcamera::FrameBuffer &source,\n> > > > + libcamera::Span destination,\n> > > > CameraMetadata *metadata) = 0;\n> > > > };\n> > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2AF9FC3D3C\n\tfor ;\n\tThu, 22 Oct 2020 04:15:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D59E61059;\n\tThu, 22 Oct 2020 06:15:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 01DCE6034F\n\tfor ;\n\tThu, 22 Oct 2020 06:15:54 +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 3E85C53;\n\tThu, 22 Oct 2020 06:15:54 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"SIlw+3+3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603340154;\n\tbh=4wiMaXJEjhUztqISdNy1qPMRoNOLE4AbvrmOAnmgLsw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SIlw+3+3oBmbhJ28+X6lZ7qcf+1K37f0ReMd3sUum4kOTKPFSDoTqQvTXk3XwUalv\n\tpwqjaBZwXekA/T7UIqCCQFjNr5LSprDsD56jfIi1vzyAte5Lgq89sKG9WVFKa941KX\n\tmVDqpV+93fPNBM8e+0oLE9SEscMQ42E3Err5Lbdo=","Date":"Thu, 22 Oct 2020 07:15:07 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"<20201022041507.GG3942@pendragon.ideasonboard.com>","References":"<20201019080323.2236548-1-hiroh@chromium.org>\n\t<0a009843-abea-2b14-2b6b-dd58caedfff0@uajain.com>\n\t<20201019125743.GC3936@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] android: Modify PostProcessor\n\tinterface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13395,"web_url":"https://patchwork.libcamera.org/comment/13395/","msgid":"","date":"2020-10-22T04:51:24","subject":"Re: [libcamera-devel] [PATCH] android: Modify PostProcessor\n\tinterface","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Thu, Oct 22, 2020 at 1:15 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro-san,\n>\n> On Tue, Oct 20, 2020 at 03:51:25PM +0900, Hirokazu Honda wrote:\n> > On Mon, Oct 19, 2020 at 9:58 PM Laurent Pinchart wrote:\n> > >\n> > > Hello everybody,\n> > >\n> > > Hiro-san, thank you for the patch.\n> > >\n> > > On Mon, Oct 19, 2020 at 04:54:27PM +0530, Umang Jain wrote:\n> > > > Hi Hiro,\n> > > >\n> > > > On 10/19/20 1:33 PM, Hirokazu Honda wrote:\n> > > > > This modifies PostProcessor interface and polishes the code\n> > > > > around the PostProcessor.\n> > > > >\n> > > > > Signed-off-by: Hirokazu Honda \n> > > >\n> > > > Changes looks good, albeit a few comments. Also, I think the patch could\n> > > > also be split down further to address the per Span pass-by-value, per\n> > > > *-type reference to &-type, etc.\n> > >\n> > > Agreed, an explanation of the rationale is useful. It may appear\n> > > straightforward today, but when we will read the code in the future we\n> > > may not remember the context, so it should be captured in the commit\n> > > message.\n> > >\n> > > > > ---\n> > > > > src/android/camera_stream.cpp | 9 ++++-----\n> > > > > src/android/camera_stream.h | 8 ++++----\n> > > > > src/android/jpeg/encoder.h | 6 +++---\n> > > > > src/android/jpeg/encoder_libjpeg.cpp | 6 +++---\n> > > > > src/android/jpeg/encoder_libjpeg.h | 4 ++--\n> > > > > src/android/jpeg/post_processor_jpeg.cpp | 8 +++++---\n> > > > > src/android/jpeg/post_processor_jpeg.h | 6 +++---\n> > > > > src/android/post_processor.h | 4 ++--\n> > > > > 8 files changed, 26 insertions(+), 25 deletions(-)\n> > > > >\n> > > > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > > > > index 1b8afa8..cc8691d 100644\n> > > > > --- a/src/android/camera_stream.cpp\n> > > > > +++ b/src/android/camera_stream.cpp\n> > > > > @@ -40,11 +40,10 @@ LOG_DECLARE_CATEGORY(HAL);\n> > > > >\n> > > > > CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,\n> > > > > camera3_stream_t *camera3Stream, unsigned int index)\n> > > > > - : cameraDevice_(cameraDevice), type_(type),\n> > > > > + : cameraDevice_(cameraDevice),\n> > > > > + config_(cameraDevice_->cameraConfiguration()), type_(type),\n> > > >\n> > > > Will cameraDevice_ will be initialized by this point? I am not sure.\n> > >\n> > > C++ initializes fields in the order they are defined in the class. The\n> > > compiler warns if the initialization list here is in a different order,\n> > > so the two are guaranteed to match. cameraDevice_ is thus guaranteed to\n> > > be initialized here.\n> > >\n> > > > > camera3Stream_(camera3Stream), index_(index)\n> > > > > {\n> > > > > - config_ = cameraDevice_->cameraConfiguration();\n> > > > > -\n> > > > > if (type_ == Type::Internal || type_ == Type::Mapped) {\n> > > > > /*\n> > > > > * \\todo There might be multiple post-processors. The logic\n> > > > > @@ -52,7 +51,7 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,\n> > > > > * future. For now, we only have PostProcessorJpeg and that\n> > > > > * is what we instantiate here.\n> > > > > */\n> > > > > - postProcessor_ = std::make_unique(cameraDevice_);\n> > > > > + postProcessor_ = std::make_unique(cameraDevice);\n> > > >\n> > > > What does this change achieve?\n> > > >\n> > > > > }\n> > > > >\n> > > > > if (type == Type::Internal) {\n> > > > > @@ -102,7 +101,7 @@ int CameraStream::process(const libcamera::FrameBuffer &source,\n> > > > > if (!postProcessor_)\n> > > > > return 0;\n> > > > >\n> > > > > - return postProcessor_->process(&source, dest->maps()[0], metadata);\n> > > > > + return postProcessor_->process(source, dest->maps()[0], metadata);\n> > > > > }\n> > > > >\n> > > > > FrameBuffer *CameraStream::getBuffer()\n> > > > > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> > > > > index c367a5f..24e66bb 100644\n> > > > > --- a/src/android/camera_stream.h\n> > > > > +++ b/src/android/camera_stream.h\n> > > > > @@ -124,11 +124,11 @@ public:\n> > > > > void putBuffer(libcamera::FrameBuffer *buffer);\n> > > > >\n> > > > > private:\n> > > > > - CameraDevice *cameraDevice_;\n> > > > > - libcamera::CameraConfiguration *config_;\n> > > > > - Type type_;\n> > > > > + CameraDevice const *cameraDevice_;\n> > > > > + const libcamera::CameraConfiguration *config_;\n> > > > > + const Type type_;\n> > > > > camera3_stream_t *camera3Stream_;\n> > > > > - unsigned int index_;\n> > > > > + const unsigned int index_;\n> > > > >\n> > > > > std::unique_ptr allocator_;\n> > > > > std::vector buffers_;\n> > > > > diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n> > > > > index 4483153..270ea60 100644\n> > > > > --- a/src/android/jpeg/encoder.h\n> > > > > +++ b/src/android/jpeg/encoder.h\n> > > > > @@ -14,11 +14,11 @@\n> > > > > class Encoder\n> > > > > {\n> > > > > public:\n> > > > > - virtual ~Encoder() {};\n> > > > > + virtual ~Encoder() {}\n> > >\n> > > This should also be split in a separate patch, it's an easy one.\n> > >\n> >\n> > I submitted a patch series of removing extra semicolons widely in libcamera.\n> > I will not touch this in this patch therefore.\n> >\n> > > > >\n> > > > > virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;\n> > > > > - virtual int encode(const libcamera::FrameBuffer *source,\n> > > > > - const libcamera::Span &destination,\n> > > > > + virtual int encode(const libcamera::FrameBuffer &source,\n> > > > > + libcamera::Span destination,\n> > > > > const libcamera::Span &exifData) = 0;\n> > > > > };\n> > > > >\n> > > > > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> > > > > index 8995ba5..4236c9d 100644\n> > > > > --- a/src/android/jpeg/encoder_libjpeg.cpp\n> > > > > +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> > > > > @@ -179,11 +179,11 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n> > > > > }\n> > > > > }\n> > > > >\n> > > > > -int EncoderLibJpeg::encode(const FrameBuffer *source,\n> > > > > - const libcamera::Span &dest,\n> > > > > +int EncoderLibJpeg::encode(const FrameBuffer &source,\n> > > > > + libcamera::Span dest,\n> > > > > const libcamera::Span &exifData)\n> > > > > {\n> > > > > - MappedFrameBuffer frame(source, PROT_READ);\n> > > > > + MappedFrameBuffer frame(&source, PROT_READ);\n> > > > > if (!frame.isValid()) {\n> > > > > LOG(JPEG, Error) << \"Failed to map FrameBuffer : \"\n> > > > > << strerror(frame.error());\n> > > > > diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> > > > > index 934caef..391a53c 100644\n> > > > > --- a/src/android/jpeg/encoder_libjpeg.h\n> > > > > +++ b/src/android/jpeg/encoder_libjpeg.h\n> > > > > @@ -21,8 +21,8 @@ public:\n> > > > > ~EncoderLibJpeg();\n> > > > >\n> > > > > int configure(const libcamera::StreamConfiguration &cfg) override;\n> > > > > - int encode(const libcamera::FrameBuffer *source,\n> > > > > - const libcamera::Span &destination,\n> > > > > + int encode(const libcamera::FrameBuffer &source,\n> > > > > + libcamera::Span destination,\n> > > > > const libcamera::Span &exifData) override;\n> > > > >\n> > > > > private:\n> > > > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > > > > index 753c28e..4ded3e3 100644\n> > > > > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > > > > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > > > > @@ -44,8 +44,8 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n> > > > > return encoder_->configure(inCfg);\n> > > > > }\n> > > > >\n> > > > > -int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,\n> > > > > - const libcamera::Span &destination,\n> > > > > +int PostProcessorJpeg::process(const libcamera::FrameBuffer &source,\n> > > > > + libcamera::Span destination,\n> > > > > CameraMetadata *metadata)\n> > > > > {\n> > > > > if (!encoder_)\n> > > > > @@ -64,8 +64,10 @@ int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,\n> > > > > * second, it is good enough.\n> > > > > */\n> > > > > exif.setTimestamp(std::time(nullptr));\n> > > > > - if (exif.generate() != 0)\n> > > > > + if (exif.generate() != 0) {\n> > > > > LOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > >\n> > > >\n> > > > Personally speaking, I don't think we should return FAIL here. So my\n> > > > preference is to skip this change.\n> > >\n> > > Tomasz' recommendation of aligning this with Android's requirements\n> > > makes sense I think.\n> >\n> > Re Android standard, I saw in CaptureRequest documentation [1] that\n> > the exif values affect the aspects of the captured image.\n> > In fact, there is a CTS test to check exif metadata [2].\n> > Searching the android codebase, a couple of implementations [3, 4]\n> > handle the exif failure as a capture fatal BUT a few implementations\n> > [5, 6] also ignore the failure.\n> > I consider this should be treated as failure?\n>\n> This shouldn't fail in normal circumstances, so I don't really see a\n> need for a degraded mode without Exif data as different parts of the\n> Android stack may choke on this. On the other hand, as it's not more\n> costly for us to handle the failure gracefully, and as it may still\n> work, we could consider this as non-fatal. I'm fine with either option.\n>\n\nI got it.\nI don't treat it as fatal.\n\nBest Regards,\n-Hiro\n> > [1] https://developer.android.com/reference/android/hardware/camera2/CaptureRequest\n> > [2] https://cs.android.com/android/platform/superproject/+/master:cts/tests/camera/src/android/hardware/camera2/cts/StillCaptureTest.java;l=690;drc=master\n> > [3] https://cs.android.com/android/platform/superproject/+/master:hardware/libhardware/modules/camera/3_4/arc/image_processor.cpp;l=381;drc=9dae907ea07692118698a242fe4a7427283bd10a\n> > [4] https://cs.android.com/android/platform/superproject/+/master:frameworks/av/services/camera/libcameraservice/api2/HeicCompositeStream.cpp;l=999;drc=991b7b672203d79b66ba7fcb672cb885d7947ae1\n> > [5] https://cs.android.com/android/platform/superproject/+/master:frameworks/av/services/camera/libcameraservice/common/DepthPhotoProcessor.cpp;l=211;drc=29e9ec182d20f44ee2e8a15de1940cb4ef29663e\n> > [6] https://cs.android.com/android/platform/superproject/+/master:hardware/google/camera/devices/EmulatedCamera/hwl/JpegCompressor.cpp;l=314;drc=master\n> >\n> > > > > int jpeg_size = encoder_->encode(source, destination, exif.data());\n> > > > > if (jpeg_size < 0) {\n> > > > > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> > > > > index 62c8650..4d8edf5 100644\n> > > > > --- a/src/android/jpeg/post_processor_jpeg.h\n> > > > > +++ b/src/android/jpeg/post_processor_jpeg.h\n> > > > > @@ -23,12 +23,12 @@ public:\n> > > > >\n> > > > > int configure(const libcamera::StreamConfiguration &incfg,\n> > > > > const libcamera::StreamConfiguration &outcfg) override;\n> > > > > - int process(const libcamera::FrameBuffer *source,\n> > > > > - const libcamera::Span &destination,\n> > > > > + int process(const libcamera::FrameBuffer &source,\n> > > > > + libcamera::Span destination,\n> > > > > CameraMetadata *metadata) override;\n> > > > >\n> > > > > private:\n> > > > > - CameraDevice *cameraDevice_;\n> > > > > + CameraDevice const *cameraDevice_;\n> > > > > std::unique_ptr encoder_;\n> > > > > libcamera::Size streamSize_;\n> > > > > };\n> > > > > diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> > > > > index a891c43..5f87a5d 100644\n> > > > > --- a/src/android/post_processor.h\n> > > > > +++ b/src/android/post_processor.h\n> > > > > @@ -20,8 +20,8 @@ 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> > > > > - const libcamera::Span &destination,\n> > > > > + virtual int process(const libcamera::FrameBuffer &source,\n> > > > > + libcamera::Span destination,\n> > > > > CameraMetadata *metadata) = 0;\n> > > > > };\n> > > > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E4A74C3D3C\n\tfor ;\n\tThu, 22 Oct 2020 04:51:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7657661059;\n\tThu, 22 Oct 2020 06:51:37 +0200 (CEST)","from mail-ej1-x644.google.com (mail-ej1-x644.google.com\n\t[IPv6:2a00:1450:4864:20::644])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 825306034F\n\tfor ;\n\tThu, 22 Oct 2020 06:51:35 +0200 (CEST)","by mail-ej1-x644.google.com with SMTP id t25so296287ejd.13\n\tfor ;\n\tWed, 21 Oct 2020 21:51:35 -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=\"dwj1qNU+\"; 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=Be6Nywg0LB9n1ZFtCIKY2xFp1YlCKH18Ehz2TxI5Pn8=;\n\tb=dwj1qNU+mwoCBS5BPp82Nf9ZJI+f97r93D+dW8ad9rPIVXwd62mWxyC9NhOVnGpUFf\n\tq3UUCtK5S8jtxQuZ2poSvyRJI+4VlI33VSjXY2upF3MtjiXeDC6aIF7NoxMkW+QX/zcR\n\tM7ZheJ7UB3+tZ94Kx6L+/T3/EuIC4sSeOzShI=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Be6Nywg0LB9n1ZFtCIKY2xFp1YlCKH18Ehz2TxI5Pn8=;\n\tb=C/6o66leixtcYhXtWiC2SSz7vHbq1hPkDZOxLKLXZPBhn5qAhXO+PNyLBJmDnT5wv8\n\toeipBq02I5aNtx6woExJqpVzs/aEzYaCHy6cu8yH3dqNsfkB8Z0BCyHSjm4NEPe5vac9\n\tqnGDeOgTdbqMtWmphNJLbUOf2tNZdoW3tZ4PTvvzu0B3g7fiTpjOYywdK3F0RoUN2Wzh\n\twta6OusG3n1UpOjSTMjmAogrHCyZ5oBa5J2vy8Vj3v0H5zFjwIYAGHyCfbpNe+CLw5cz\n\tmsbklNa29KnU3k4UAEE8NO/3LfJIpW9nGrJ8/g9pvznPahcXcReo9ZgoUZXNL1D4I1Tg\n\t20xQ==","X-Gm-Message-State":"AOAM530Qyh0+iMd2j3CxsuiUCPsSlnkn2coUc+ShWCMZMnvwUiF0a5jL\n\t5m9rPNGGCEFSAF5Qx/jfHN92tsbjtASNPhNkzH4JEA==","X-Google-Smtp-Source":"ABdhPJzJGaOqpRBN+3qnwMhEQA2IGqsN/+/Iok5IW5wo7+/GeSk5NaAwy+57mIxx60ztv+enunmPgTBx4ySnNImjQ14=","X-Received":"by 2002:a17:906:c152:: with SMTP id\n\tdp18mr587554ejc.243.1603342294985; \n\tWed, 21 Oct 2020 21:51:34 -0700 (PDT)","MIME-Version":"1.0","References":"<20201019080323.2236548-1-hiroh@chromium.org>\n\t<0a009843-abea-2b14-2b6b-dd58caedfff0@uajain.com>\n\t<20201019125743.GC3936@pendragon.ideasonboard.com>\n\t\n\t<20201022041507.GG3942@pendragon.ideasonboard.com>","In-Reply-To":"<20201022041507.GG3942@pendragon.ideasonboard.com>","From":"Hirokazu Honda ","Date":"Thu, 22 Oct 2020 13:51:24 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH] android: Modify PostProcessor\n\tinterface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]