[{"id":14879,"web_url":"https://patchwork.libcamera.org/comment/14879/","msgid":"<20210201110031.ij4mkhdy6xwnewxu@uno.localdomain>","date":"2021-02-01T11:00:31","subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: post_processor:\n\tChange the type destination in process()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Thu, Jan 28, 2021 at 10:42:15PM +0000, Hirokazu Honda wrote:\n> The type of the destination buffer in PostProcessor::process() is\n> libcamera::Span. libcamera::Span is used for one dimension buffer\n> (e.g. blob buffer). The destination can be multiple dimensions\n> buffer (e.g. yuv frame). Therefore, this changes the type of the\n> destination buffer to MappedFrameBuffer.\n\nThanks, this will make the recalculation of the JPEG buffer size\neasier.\n\n>\n> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n>\n> ---\n>  src/android/camera_stream.cpp            | 4 ++--\n>  src/android/camera_stream.h              | 6 ++++--\n>  src/android/jpeg/post_processor_jpeg.cpp | 6 +++---\n>  src/android/jpeg/post_processor_jpeg.h   | 2 +-\n>  src/android/post_processor.h             | 7 ++++---\n>  5 files changed, 14 insertions(+), 11 deletions(-)\n>\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index 4c8020e5..611ec0d1 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -96,14 +96,14 @@ int CameraStream::configure()\n>  }\n>\n>  int CameraStream::process(const libcamera::FrameBuffer &source,\n> -\t\t\t  MappedCamera3Buffer *dest,\n> +\t\t\t  libcamera::MappedBuffer *destination,\n>  \t\t\t  const CameraMetadata &requestMetadata,\n>  \t\t\t  CameraMetadata *resultMetadata)\n>  {\n>  \tif (!postProcessor_)\n>  \t\treturn 0;\n>\n> -\treturn postProcessor_->process(source, dest->maps()[0],\n> +\treturn postProcessor_->process(source, destination,\n>  \t\t\t\t       requestMetadata, resultMetadata);\n>  }\n>\n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index 298ffbf6..73bac0ba 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -19,9 +19,10 @@\n>  #include <libcamera/geometry.h>\n>  #include <libcamera/pixel_format.h>\n>\n> +#include <libcamera/internal/buffer.h>\n> +\n>  class CameraDevice;\n>  class CameraMetadata;\n> -class MappedCamera3Buffer;\n>  class PostProcessor;\n>\n>  class CameraStream\n> @@ -119,9 +120,10 @@ public:\n>\n>  \tint configure();\n>  \tint process(const libcamera::FrameBuffer &source,\n> -\t\t    MappedCamera3Buffer *dest,\n> +\t\t    libcamera::MappedBuffer *destination,\n>  \t\t    const CameraMetadata &requestMetadata,\n>  \t\t    CameraMetadata *resultMetadata);\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 cac0087b..74e81c6f 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -83,7 +83,7 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n>  }\n>\n>  int PostProcessorJpeg::process(const FrameBuffer &source,\n> -\t\t\t       Span<uint8_t> destination,\n> +\t\t\t       libcamera::MappedBuffer *destination,\n>  \t\t\t       const CameraMetadata &requestMetadata,\n>  \t\t\t       CameraMetadata *resultMetadata)\n>  {\n> @@ -171,8 +171,8 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n>  \tret = requestMetadata.getEntry(ANDROID_JPEG_QUALITY, &entry);\n>  \tconst uint8_t quality = ret ? *entry.data.u8 : 95;\n>  \tresultMetadata->addEntry(ANDROID_JPEG_QUALITY, &quality, 1);\n> +\tint jpeg_size = encoder_->encode(source, destination->maps()[0], exif.data(), quality);\n\nI would not move this\n\n>\n> -\tint jpeg_size = encoder_->encode(source, destination, exif.data(), quality);\n>  \tif (jpeg_size < 0) {\n>  \t\tLOG(JPEG, Error) << \"Failed to encode stream image\";\n>  \t\treturn jpeg_size;\n> @@ -190,7 +190,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n>  \t * \\todo Investigate if the buffer size mismatch is an issue or\n>  \t * expected behaviour.\n>  \t */\n> -\tuint8_t *resultPtr = destination.data() +\n> +\tuint8_t *resultPtr = destination->maps()[0].data() +\n>  \t\t\t     cameraDevice_->maxJpegBufferSize() -\n>  \t\t\t     sizeof(struct camera3_jpeg_blob);\n>  \tauto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr);\n> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> index d2dfa450..7689de73 100644\n> --- a/src/android/jpeg/post_processor_jpeg.h\n> +++ b/src/android/jpeg/post_processor_jpeg.h\n> @@ -25,7 +25,7 @@ public:\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    libcamera::Span<uint8_t> destination,\n> +\t\t    libcamera::MappedBuffer *destination,\n>  \t\t    const CameraMetadata &requestMetadata,\n>  \t\t    CameraMetadata *resultMetadata) override;\n>\n> diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> index bda93bb4..f5c99f03 100644\n> --- a/src/android/post_processor.h\n> +++ b/src/android/post_processor.h\n> @@ -8,9 +8,10 @@\n>  #define __ANDROID_POST_PROCESSOR_H__\n>\n>  #include <libcamera/buffer.h>\n> -#include <libcamera/span.h>\n>  #include <libcamera/stream.h>\n>\n> +#include <libcamera/internal/buffer.h>\n> +\n>  class CameraMetadata;\n>\n>  class PostProcessor\n> @@ -21,9 +22,9 @@ public:\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    libcamera::Span<uint8_t> destination,\n> +\t\t\t    libcamera::MappedBuffer *destination,\n>  \t\t\t    const CameraMetadata &requestMetadata,\n> -\t\t\t    CameraMetadata *resultMetadata) = 0;\n> +\t\t\t    CameraMetadata *metadata) = 0;\n\nUnrelated and not updated in the sub-classes. I would drop.\n\nI think both can be fixed when applying, the rest looks good\nReviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n\nThanks\n  j\n\n>  };\n>\n>  #endif /* __ANDROID_POST_PROCESSOR_H__ */\n> --\n> 2.30.0.365.g02bc693789-goog\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 02A88C33BB\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  1 Feb 2021 11:00:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CDC1468400;\n\tMon,  1 Feb 2021 12:00:14 +0100 (CET)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F411D68374\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  1 Feb 2021 12:00:13 +0100 (CET)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 1DB5610001B;\n\tMon,  1 Feb 2021 11:00:10 +0000 (UTC)"],"Date":"Mon, 1 Feb 2021 12:00:31 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Hirokazu Honda <hiroh@chromium.org>","Message-ID":"<20210201110031.ij4mkhdy6xwnewxu@uno.localdomain>","References":"<20210128224217.2919790-1-hiroh@chromium.org>\n\t<20210128224217.2919790-2-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210128224217.2919790-2-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: post_processor:\n\tChange the type destination in process()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":14910,"web_url":"https://patchwork.libcamera.org/comment/14910/","msgid":"<20210202181810.axfxhihf6m4g7bk7@uno.localdomain>","date":"2021-02-02T18:18:10","subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: post_processor:\n\tChange the type destination in process()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello,\n\nOn Mon, Feb 01, 2021 at 12:00:31PM +0100, Jacopo Mondi wrote:\n> Hi Hiro,\n>\n> On Thu, Jan 28, 2021 at 10:42:15PM +0000, Hirokazu Honda wrote:\n> > The type of the destination buffer in PostProcessor::process() is\n> > libcamera::Span. libcamera::Span is used for one dimension buffer\n> > (e.g. blob buffer). The destination can be multiple dimensions\n> > buffer (e.g. yuv frame). Therefore, this changes the type of the\n> > destination buffer to MappedFrameBuffer.\n>\n> Thanks, this will make the recalculation of the JPEG buffer size\n> easier.\n>\n> >\n> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> >\n> > ---\n> >  src/android/camera_stream.cpp            | 4 ++--\n> >  src/android/camera_stream.h              | 6 ++++--\n> >  src/android/jpeg/post_processor_jpeg.cpp | 6 +++---\n> >  src/android/jpeg/post_processor_jpeg.h   | 2 +-\n> >  src/android/post_processor.h             | 7 ++++---\n> >  5 files changed, 14 insertions(+), 11 deletions(-)\n> >\n> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > index 4c8020e5..611ec0d1 100644\n> > --- a/src/android/camera_stream.cpp\n> > +++ b/src/android/camera_stream.cpp\n> > @@ -96,14 +96,14 @@ int CameraStream::configure()\n> >  }\n> >\n> >  int CameraStream::process(const libcamera::FrameBuffer &source,\n> > -\t\t\t  MappedCamera3Buffer *dest,\n> > +\t\t\t  libcamera::MappedBuffer *destination,\n> >  \t\t\t  const CameraMetadata &requestMetadata,\n> >  \t\t\t  CameraMetadata *resultMetadata)\n> >  {\n> >  \tif (!postProcessor_)\n> >  \t\treturn 0;\n> >\n> > -\treturn postProcessor_->process(source, dest->maps()[0],\n> > +\treturn postProcessor_->process(source, destination,\n> >  \t\t\t\t       requestMetadata, resultMetadata);\n> >  }\n> >\n> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> > index 298ffbf6..73bac0ba 100644\n> > --- a/src/android/camera_stream.h\n> > +++ b/src/android/camera_stream.h\n> > @@ -19,9 +19,10 @@\n> >  #include <libcamera/geometry.h>\n> >  #include <libcamera/pixel_format.h>\n> >\n> > +#include <libcamera/internal/buffer.h>\n> > +\n> >  class CameraDevice;\n> >  class CameraMetadata;\n> > -class MappedCamera3Buffer;\n> >  class PostProcessor;\n> >\n> >  class CameraStream\n> > @@ -119,9 +120,10 @@ public:\n> >\n> >  \tint configure();\n> >  \tint process(const libcamera::FrameBuffer &source,\n> > -\t\t    MappedCamera3Buffer *dest,\n> > +\t\t    libcamera::MappedBuffer *destination,\n> >  \t\t    const CameraMetadata &requestMetadata,\n> >  \t\t    CameraMetadata *resultMetadata);\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 cac0087b..74e81c6f 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > @@ -83,7 +83,7 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> >  }\n> >\n> >  int PostProcessorJpeg::process(const FrameBuffer &source,\n> > -\t\t\t       Span<uint8_t> destination,\n> > +\t\t\t       libcamera::MappedBuffer *destination,\n> >  \t\t\t       const CameraMetadata &requestMetadata,\n> >  \t\t\t       CameraMetadata *resultMetadata)\n> >  {\n> > @@ -171,8 +171,8 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> >  \tret = requestMetadata.getEntry(ANDROID_JPEG_QUALITY, &entry);\n> >  \tconst uint8_t quality = ret ? *entry.data.u8 : 95;\n> >  \tresultMetadata->addEntry(ANDROID_JPEG_QUALITY, &quality, 1);\n> > +\tint jpeg_size = encoder_->encode(source, destination->maps()[0], exif.data(), quality);\n>\n> I would not move this\n>\n> >\n> > -\tint jpeg_size = encoder_->encode(source, destination, exif.data(), quality);\n> >  \tif (jpeg_size < 0) {\n> >  \t\tLOG(JPEG, Error) << \"Failed to encode stream image\";\n> >  \t\treturn jpeg_size;\n> > @@ -190,7 +190,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> >  \t * \\todo Investigate if the buffer size mismatch is an issue or\n> >  \t * expected behaviour.\n> >  \t */\n> > -\tuint8_t *resultPtr = destination.data() +\n> > +\tuint8_t *resultPtr = destination->maps()[0].data() +\n> >  \t\t\t     cameraDevice_->maxJpegBufferSize() -\n> >  \t\t\t     sizeof(struct camera3_jpeg_blob);\n> >  \tauto *blob = reinterpret_cast<struct camera3_jpeg_blob *>(resultPtr);\n> > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> > index d2dfa450..7689de73 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.h\n> > +++ b/src/android/jpeg/post_processor_jpeg.h\n> > @@ -25,7 +25,7 @@ public:\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    libcamera::Span<uint8_t> destination,\n> > +\t\t    libcamera::MappedBuffer *destination,\n> >  \t\t    const CameraMetadata &requestMetadata,\n> >  \t\t    CameraMetadata *resultMetadata) override;\n> >\n> > diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> > index bda93bb4..f5c99f03 100644\n> > --- a/src/android/post_processor.h\n> > +++ b/src/android/post_processor.h\n> > @@ -8,9 +8,10 @@\n> >  #define __ANDROID_POST_PROCESSOR_H__\n> >\n> >  #include <libcamera/buffer.h>\n> > -#include <libcamera/span.h>\n> >  #include <libcamera/stream.h>\n> >\n> > +#include <libcamera/internal/buffer.h>\n> > +\n> >  class CameraMetadata;\n> >\n> >  class PostProcessor\n> > @@ -21,9 +22,9 @@ public:\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    libcamera::Span<uint8_t> destination,\n> > +\t\t\t    libcamera::MappedBuffer *destination,\n> >  \t\t\t    const CameraMetadata &requestMetadata,\n> > -\t\t\t    CameraMetadata *resultMetadata) = 0;\n> > +\t\t\t    CameraMetadata *metadata) = 0;\n>\n> Unrelated and not updated in the sub-classes. I would drop.\n>\n> I think both can be fixed when applying, the rest looks good\n> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n\nI've applied this one with the minors pointed out here above.\n\nThanks\n  j\n\n>\n> Thanks\n>   j\n>\n> >  };\n> >\n> >  #endif /* __ANDROID_POST_PROCESSOR_H__ */\n> > --\n> > 2.30.0.365.g02bc693789-goog\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 141D7BD160\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue,  2 Feb 2021 18:17:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A29756842F;\n\tTue,  2 Feb 2021 19:17:50 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5AFD760307\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  2 Feb 2021 19:17:49 +0100 (CET)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id B83CC40006;\n\tTue,  2 Feb 2021 18:17:48 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Tue, 2 Feb 2021 19:18:10 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Hirokazu Honda <hiroh@chromium.org>","Message-ID":"<20210202181810.axfxhihf6m4g7bk7@uno.localdomain>","References":"<20210128224217.2919790-1-hiroh@chromium.org>\n\t<20210128224217.2919790-2-hiroh@chromium.org>\n\t<20210201110031.ij4mkhdy6xwnewxu@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210201110031.ij4mkhdy6xwnewxu@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: post_processor:\n\tChange the type destination in process()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","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\" <libcamera-devel-bounces@lists.libcamera.org>"}}]