[{"id":19460,"web_url":"https://patchwork.libcamera.org/comment/19460/","msgid":"","date":"2021-09-06T13:56:26","subject":"Re: [libcamera-devel] [PATCH v2 28/27] android: jpeg: Support\n\tmulti-planar buffers","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Mon, Sep 6, 2021 at 10:40 PM Laurent Pinchart\n wrote:\n>\n> The JPEG post-processor uses MappedFrameBuffer to access pixel data, but\n> only uses data from the first plane. Pass the vector of planes to the\n> encode() function to correctly handle multi-planar formats (currently\n> limited to NV12).\n>\n> Signed-off-by: Laurent Pinchart \n\nI am not against this patch.\nBut is this necessary in this series?\nI have done this in\nhttps://patchwork.libcamera.org/project/libcamera/list/?series=2423.\n\n-Hiro\n> ---\n> src/android/jpeg/encoder_libjpeg.cpp | 18 +++++++++---------\n> src/android/jpeg/encoder_libjpeg.h | 8 +++++---\n> src/android/jpeg/post_processor_jpeg.cpp | 2 +-\n> src/android/jpeg/thumbnailer.cpp | 4 ++--\n> 4 files changed, 17 insertions(+), 15 deletions(-)\n>\n> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> index b8b01e20a001..7f63f36e793e 100644\n> --- a/src/android/jpeg/encoder_libjpeg.cpp\n> +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> @@ -103,9 +103,9 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n> return 0;\n> }\n>\n> -void EncoderLibJpeg::compressRGB(Span frame)\n> +void EncoderLibJpeg::compressRGB(const std::vector> &planes)\n> {\n> - unsigned char *src = const_cast(frame.data());\n> + unsigned char *src = const_cast(planes[0].data());\n> /* \\todo Stride information should come from buffer configuration. */\n> unsigned int stride = pixelFormatInfo_->stride(compress_.image_width, 0);\n>\n> @@ -121,7 +121,7 @@ void EncoderLibJpeg::compressRGB(Span frame)\n> * Compress the incoming buffer from a supported NV format.\n> * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg.\n> */\n> -void EncoderLibJpeg::compressNV(Span frame)\n> +void EncoderLibJpeg::compressNV(const std::vector> &planes)\n> {\n> uint8_t tmprowbuf[compress_.image_width * 3];\n>\n> @@ -133,7 +133,6 @@ void EncoderLibJpeg::compressNV(Span frame)\n> * Possible hints at:\n> * https://sourceforge.net/p/libjpeg/mailman/message/30815123/\n> */\n> - unsigned int y_stride = pixelFormatInfo_->stride(compress_.image_width, 0);\n> unsigned int c_stride = pixelFormatInfo_->stride(compress_.image_width, 1);\n>\n> unsigned int horzSubSample = 2 * compress_.image_width / c_stride;\n> @@ -143,8 +142,8 @@ void EncoderLibJpeg::compressNV(Span frame)\n> unsigned int cb_pos = nvSwap_ ? 1 : 0;\n> unsigned int cr_pos = nvSwap_ ? 0 : 1;\n>\n> - const unsigned char *src = frame.data();\n> - const unsigned char *src_c = src + y_stride * compress_.image_height;\n> + const unsigned char *src = planes[0].data();\n> + const unsigned char *src_c = planes[1].data();\n>\n> JSAMPROW row_pointer[1];\n> row_pointer[0] = &tmprowbuf[0];\n> @@ -188,11 +187,12 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, Span dest,\n> return frame.error();\n> }\n>\n> - return encode(frame.planes()[0], dest, exifData, quality);\n> + return encode(frame.planes(), dest, exifData, quality);\n> }\n>\n> -int EncoderLibJpeg::encode(Span src, Span dest,\n> - Span exifData, unsigned int quality)\n> +int EncoderLibJpeg::encode(const std::vector> &src,\n> + Span dest, Span exifData,\n> + unsigned int quality)\n> {\n> unsigned char *destination = dest.data();\n> unsigned long size = dest.size();\n> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> index 61fbd1a69278..45ffbd7fae5d 100644\n> --- a/src/android/jpeg/encoder_libjpeg.h\n> +++ b/src/android/jpeg/encoder_libjpeg.h\n> @@ -9,6 +9,8 @@\n>\n> #include \"encoder.h\"\n>\n> +#include \n> +\n> #include \"libcamera/internal/formats.h\"\n>\n> #include \n> @@ -24,14 +26,14 @@ public:\n> libcamera::Span destination,\n> libcamera::Span exifData,\n> unsigned int quality) override;\n> - int encode(libcamera::Span source,\n> + int encode(const std::vector> &planes,\n> libcamera::Span destination,\n> libcamera::Span exifData,\n> unsigned int quality);\n>\n> private:\n> - void compressRGB(libcamera::Span frame);\n> - void compressNV(libcamera::Span frame);\n> + void compressRGB(const std::vector> &planes);\n> + void compressNV(const std::vector> &planes);\n>\n> struct jpeg_compress_struct compress_;\n> struct jpeg_error_mgr jerr_;\n> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> index 3160a784419c..68d74842925d 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -72,7 +72,7 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> */\n> thumbnail->resize(rawThumbnail.size());\n>\n> - int jpeg_size = thumbnailEncoder_.encode(rawThumbnail,\n> + int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail },\n> *thumbnail, {}, quality);\n> thumbnail->resize(jpeg_size);\n>\n> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n> index 043c7b33f06a..2c341535de9e 100644\n> --- a/src/android/jpeg/thumbnailer.cpp\n> +++ b/src/android/jpeg/thumbnailer.cpp\n> @@ -62,8 +62,8 @@ void Thumbnailer::createThumbnail(const FrameBuffer &source,\n> ASSERT(tw % 2 == 0 && th % 2 == 0);\n>\n> /* Image scaling block implementing nearest-neighbour algorithm. */\n> - unsigned char *src = static_cast(frame.planes()[0].data());\n> - unsigned char *srcC = src + sh * sw;\n> + unsigned char *src = frame.planes()[0].data();\n> + unsigned char *srcC = frame.planes()[1].data();\n> unsigned char *srcCb, *srcCr;\n> unsigned char *dstY, *srcY;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 F0B77BD87D\n\tfor ;\n\tMon, 6 Sep 2021 13:56:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 51FAE69167;\n\tMon, 6 Sep 2021 15:56:39 +0200 (CEST)","from mail-ej1-x62e.google.com (mail-ej1-x62e.google.com\n\t[IPv6:2a00:1450:4864:20::62e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B179960137\n\tfor ;\n\tMon, 6 Sep 2021 15:56:37 +0200 (CEST)","by mail-ej1-x62e.google.com with SMTP id u14so13642635ejf.13\n\tfor ;\n\tMon, 06 Sep 2021 06:56:37 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"PtliPUCZ\"; 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=x6mg/S905IeYmlzwIwM0acmLKmDG6l5RN9mGSY2thAw=;\n\tb=PtliPUCZtTpZ+z00KcJnBZkGZ31/uVYGDzE2+kh+q+yWg2q7EjLtlZCh0qG2JUy8PQ\n\tCsR4nZX8IhpnqUMDsY5Q1EM+FsmiOUNz03CUUHIzgvf5QutvTw2udsvsr55R5RroJ3t8\n\tqDp7/tAIKv/giCxT8Hgwj89CWDIVm0CjZAfxc=","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=x6mg/S905IeYmlzwIwM0acmLKmDG6l5RN9mGSY2thAw=;\n\tb=i8qeyt8FnxQOK2xbo+QZPDvf8xzg5kmRXziH41jYB+NXfGiH/jrID9iESUec/YJAlN\n\tfcOq10OKEEJzrhKn4GxLc1I8zbrVbAFc1LOnMtR5leZozFGX+RPlrvmv/a7U+q6AQRiD\n\tO2dmw1y7M0GpV3OZXjTT6F81v7lNr8262Pk+J6/aE+NxJTaBgt0AlJ53FohWRfnGSBaR\n\ty2+QoVEBbUWgHzM5Ippm5hz06XaS3In3IX6HK3wYEJDWxJKLSU8DxYuSDHUsue4GmDHc\n\tEotNyRfay3vfUyqysOKlRP9ge2F+aB9/r2AByWKPMZ4Zrpka8Wac/eHHLzMAeqssAWpb\n\tLYbQ==","X-Gm-Message-State":"AOAM530+xn+T0mqPGK3sR7jveh3GJ8dvHpIhF1etSCD8aW9PSOidY1oJ\n\tzU32CuEfA3hS8AH0Z45I51NLW4URn4ySks6YCKefQqCylKj7iA==","X-Google-Smtp-Source":"ABdhPJzD17uElqw6DGENPdlvfYS7ollGD975d/J391PMeDIRS4Zgp0Yp8JFT7pMSuc4sHiz3TKS1kwV7mrR6VKoKLgs=","X-Received":"by 2002:a17:906:f289:: with SMTP id\n\tgu9mr13478678ejb.559.1630936597182; \n\tMon, 06 Sep 2021 06:56:37 -0700 (PDT)","MIME-Version":"1.0","References":"<20210906020100.14430-1-laurent.pinchart@ideasonboard.com>\n\t<20210906134004.23485-1-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20210906134004.23485-1-laurent.pinchart@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Mon, 6 Sep 2021 22:56:26 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 28/27] android: jpeg: Support\n\tmulti-planar buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19461,"web_url":"https://patchwork.libcamera.org/comment/19461/","msgid":"<8ea9b2e2-e747-eeae-15d4-44fc5de73927@ideasonboard.com>","date":"2021-09-06T14:00:40","subject":"Re: [libcamera-devel] [PATCH v2 28/27] android: jpeg: Support\n\tmulti-planar buffers","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 9/6/21 7:10 PM, Laurent Pinchart wrote:\n> The JPEG post-processor uses MappedFrameBuffer to access pixel data, but\n> only uses data from the first plane. Pass the vector of planes to the\n> encode() function to correctly handle multi-planar formats (currently\n> limited to NV12).\n>\n> Signed-off-by: Laurent Pinchart \n\n\nThe diff looks good to me but I haven't the entire series yet. Also, \nthis fixes the segfault observed with the series while trying to click a \npicture so,\n\nAcked-by: Umang Jain \n\nTested-by: Umang Jain \n\n> ---\n> src/android/jpeg/encoder_libjpeg.cpp | 18 +++++++++---------\n> src/android/jpeg/encoder_libjpeg.h | 8 +++++---\n> src/android/jpeg/post_processor_jpeg.cpp | 2 +-\n> src/android/jpeg/thumbnailer.cpp | 4 ++--\n> 4 files changed, 17 insertions(+), 15 deletions(-)\n>\n> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> index b8b01e20a001..7f63f36e793e 100644\n> --- a/src/android/jpeg/encoder_libjpeg.cpp\n> +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> @@ -103,9 +103,9 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n> \treturn 0;\n> }\n> \n> -void EncoderLibJpeg::compressRGB(Span frame)\n> +void EncoderLibJpeg::compressRGB(const std::vector> &planes)\n> {\n> -\tunsigned char *src = const_cast(frame.data());\n> +\tunsigned char *src = const_cast(planes[0].data());\n> \t/* \\todo Stride information should come from buffer configuration. */\n> \tunsigned int stride = pixelFormatInfo_->stride(compress_.image_width, 0);\n> \n> @@ -121,7 +121,7 @@ void EncoderLibJpeg::compressRGB(Span frame)\n> * Compress the incoming buffer from a supported NV format.\n> * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg.\n> */\n> -void EncoderLibJpeg::compressNV(Span frame)\n> +void EncoderLibJpeg::compressNV(const std::vector> &planes)\n> {\n> \tuint8_t tmprowbuf[compress_.image_width * 3];\n> \n> @@ -133,7 +133,6 @@ void EncoderLibJpeg::compressNV(Span frame)\n> \t * Possible hints at:\n> \t * https://sourceforge.net/p/libjpeg/mailman/message/30815123/\n> \t */\n> -\tunsigned int y_stride = pixelFormatInfo_->stride(compress_.image_width, 0);\n> \tunsigned int c_stride = pixelFormatInfo_->stride(compress_.image_width, 1);\n> \n> \tunsigned int horzSubSample = 2 * compress_.image_width / c_stride;\n> @@ -143,8 +142,8 @@ void EncoderLibJpeg::compressNV(Span frame)\n> \tunsigned int cb_pos = nvSwap_ ? 1 : 0;\n> \tunsigned int cr_pos = nvSwap_ ? 0 : 1;\n> \n> -\tconst unsigned char *src = frame.data();\n> -\tconst unsigned char *src_c = src + y_stride * compress_.image_height;\n> +\tconst unsigned char *src = planes[0].data();\n> +\tconst unsigned char *src_c = planes[1].data();\n> \n> \tJSAMPROW row_pointer[1];\n> \trow_pointer[0] = &tmprowbuf[0];\n> @@ -188,11 +187,12 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, Span dest,\n> \t\treturn frame.error();\n> \t}\n> \n> -\treturn encode(frame.planes()[0], dest, exifData, quality);\n> +\treturn encode(frame.planes(), dest, exifData, quality);\n> }\n> \n> -int EncoderLibJpeg::encode(Span src, Span dest,\n> -\t\t\t Span exifData, unsigned int quality)\n> +int EncoderLibJpeg::encode(const std::vector> &src,\n> +\t\t\t Span dest, Span exifData,\n> +\t\t\t unsigned int quality)\n> {\n> \tunsigned char *destination = dest.data();\n> \tunsigned long size = dest.size();\n> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> index 61fbd1a69278..45ffbd7fae5d 100644\n> --- a/src/android/jpeg/encoder_libjpeg.h\n> +++ b/src/android/jpeg/encoder_libjpeg.h\n> @@ -9,6 +9,8 @@\n> \n> #include \"encoder.h\"\n> \n> +#include \n> +\n> #include \"libcamera/internal/formats.h\"\n> \n> #include \n> @@ -24,14 +26,14 @@ public:\n> \t\t libcamera::Span destination,\n> \t\t libcamera::Span exifData,\n> \t\t unsigned int quality) override;\n> -\tint encode(libcamera::Span source,\n> +\tint encode(const std::vector> &planes,\n> \t\t libcamera::Span destination,\n> \t\t libcamera::Span exifData,\n> \t\t unsigned int quality);\n> \n> private:\n> -\tvoid compressRGB(libcamera::Span frame);\n> -\tvoid compressNV(libcamera::Span frame);\n> +\tvoid compressRGB(const std::vector> &planes);\n> +\tvoid compressNV(const std::vector> &planes);\n> \n> \tstruct jpeg_compress_struct compress_;\n> \tstruct jpeg_error_mgr jerr_;\n> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> index 3160a784419c..68d74842925d 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -72,7 +72,7 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> \t\t */\n> \t\tthumbnail->resize(rawThumbnail.size());\n> \n> -\t\tint jpeg_size = thumbnailEncoder_.encode(rawThumbnail,\n> +\t\tint jpeg_size = thumbnailEncoder_.encode({ rawThumbnail },\n> \t\t\t\t\t\t\t *thumbnail, {}, quality);\n> \t\tthumbnail->resize(jpeg_size);\n> \n> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n> index 043c7b33f06a..2c341535de9e 100644\n> --- a/src/android/jpeg/thumbnailer.cpp\n> +++ b/src/android/jpeg/thumbnailer.cpp\n> @@ -62,8 +62,8 @@ void Thumbnailer::createThumbnail(const FrameBuffer &source,\n> \tASSERT(tw % 2 == 0 && th % 2 == 0);\n> \n> \t/* Image scaling block implementing nearest-neighbour algorithm. */\n> -\tunsigned char *src = static_cast(frame.planes()[0].data());\n> -\tunsigned char *srcC = src + sh * sw;\n> +\tunsigned char *src = frame.planes()[0].data();\n> +\tunsigned char *srcC = frame.planes()[1].data();\n> \tunsigned char *srcCb, *srcCr;\n> \tunsigned char *dstY, *srcY;\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 821D9BDC71\n\tfor ;\n\tMon, 6 Sep 2021 14:00:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0B2C86916A;\n\tMon, 6 Sep 2021 16:00:48 +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 985C760137\n\tfor ;\n\tMon, 6 Sep 2021 16:00:46 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.180])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 875B38AD;\n\tMon, 6 Sep 2021 16:00:45 +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=\"InB/6s7t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630936846;\n\tbh=IieWuvclPOmW6LHg5WZ1iCzKzWFU5ZqJ7pu5WOw1Dcw=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=InB/6s7tNI9p5YWPCmWyJwwo+W/dpqLOlWBPsUJ0MlsQUsyaKcMKImfmChTZEMWpd\n\tWcHmDri23/i/Y4X6xFE8RtO1PJBc/xBMxlOhvZZXJJNdX1tAsdAPkGgDGsLaQmzjQB\n\tuVCUQSliqT4UFRG6lmfTuvWMF7vaAkPdLwRNHGt0=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210906020100.14430-1-laurent.pinchart@ideasonboard.com>\n\t<20210906134004.23485-1-laurent.pinchart@ideasonboard.com>","From":"Umang Jain ","Message-ID":"<8ea9b2e2-e747-eeae-15d4-44fc5de73927@ideasonboard.com>","Date":"Mon, 6 Sep 2021 19:30:40 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210906134004.23485-1-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2 28/27] android: jpeg: Support\n\tmulti-planar buffers","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19463,"web_url":"https://patchwork.libcamera.org/comment/19463/","msgid":"","date":"2021-09-06T14:08:26","subject":"Re: [libcamera-devel] [PATCH v2 28/27] android: jpeg: Support\n\tmulti-planar buffers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Mon, Sep 06, 2021 at 10:56:26PM +0900, Hirokazu Honda wrote:\n> On Mon, Sep 6, 2021 at 10:40 PM Laurent Pinchart wrote:\n> >\n> > The JPEG post-processor uses MappedFrameBuffer to access pixel data, but\n> > only uses data from the first plane. Pass the vector of planes to the\n> > encode() function to correctly handle multi-planar formats (currently\n> > limited to NV12).\n> >\n> > Signed-off-by: Laurent Pinchart \n> \n> I am not against this patch.\n> But is this necessary in this series?\n\nWithout it the JPEG encoder crashes when capturing a still image, so I'd\nconsider it as necessary :-)\n\n> I have done this in\n> https://patchwork.libcamera.org/project/libcamera/list/?series=2423.\n\nI've given this more thoughts over the weekend, and I think we need to\nintroduce an Image class. It won't be trivial, there are quite a few\nissues to handle to make it suitable for the libcamera public API. Once\nthis series gets merged I'll provide feedback on that.\n\n> > ---\n> > src/android/jpeg/encoder_libjpeg.cpp | 18 +++++++++---------\n> > src/android/jpeg/encoder_libjpeg.h | 8 +++++---\n> > src/android/jpeg/post_processor_jpeg.cpp | 2 +-\n> > src/android/jpeg/thumbnailer.cpp | 4 ++--\n> > 4 files changed, 17 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> > index b8b01e20a001..7f63f36e793e 100644\n> > --- a/src/android/jpeg/encoder_libjpeg.cpp\n> > +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> > @@ -103,9 +103,9 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n> > return 0;\n> > }\n> >\n> > -void EncoderLibJpeg::compressRGB(Span frame)\n> > +void EncoderLibJpeg::compressRGB(const std::vector> &planes)\n> > {\n> > - unsigned char *src = const_cast(frame.data());\n> > + unsigned char *src = const_cast(planes[0].data());\n> > /* \\todo Stride information should come from buffer configuration. */\n> > unsigned int stride = pixelFormatInfo_->stride(compress_.image_width, 0);\n> >\n> > @@ -121,7 +121,7 @@ void EncoderLibJpeg::compressRGB(Span frame)\n> > * Compress the incoming buffer from a supported NV format.\n> > * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg.\n> > */\n> > -void EncoderLibJpeg::compressNV(Span frame)\n> > +void EncoderLibJpeg::compressNV(const std::vector> &planes)\n> > {\n> > uint8_t tmprowbuf[compress_.image_width * 3];\n> >\n> > @@ -133,7 +133,6 @@ void EncoderLibJpeg::compressNV(Span frame)\n> > * Possible hints at:\n> > * https://sourceforge.net/p/libjpeg/mailman/message/30815123/\n> > */\n> > - unsigned int y_stride = pixelFormatInfo_->stride(compress_.image_width, 0);\n> > unsigned int c_stride = pixelFormatInfo_->stride(compress_.image_width, 1);\n> >\n> > unsigned int horzSubSample = 2 * compress_.image_width / c_stride;\n> > @@ -143,8 +142,8 @@ void EncoderLibJpeg::compressNV(Span frame)\n> > unsigned int cb_pos = nvSwap_ ? 1 : 0;\n> > unsigned int cr_pos = nvSwap_ ? 0 : 1;\n> >\n> > - const unsigned char *src = frame.data();\n> > - const unsigned char *src_c = src + y_stride * compress_.image_height;\n> > + const unsigned char *src = planes[0].data();\n> > + const unsigned char *src_c = planes[1].data();\n> >\n> > JSAMPROW row_pointer[1];\n> > row_pointer[0] = &tmprowbuf[0];\n> > @@ -188,11 +187,12 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, Span dest,\n> > return frame.error();\n> > }\n> >\n> > - return encode(frame.planes()[0], dest, exifData, quality);\n> > + return encode(frame.planes(), dest, exifData, quality);\n> > }\n> >\n> > -int EncoderLibJpeg::encode(Span src, Span dest,\n> > - Span exifData, unsigned int quality)\n> > +int EncoderLibJpeg::encode(const std::vector> &src,\n> > + Span dest, Span exifData,\n> > + unsigned int quality)\n> > {\n> > unsigned char *destination = dest.data();\n> > unsigned long size = dest.size();\n> > diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> > index 61fbd1a69278..45ffbd7fae5d 100644\n> > --- a/src/android/jpeg/encoder_libjpeg.h\n> > +++ b/src/android/jpeg/encoder_libjpeg.h\n> > @@ -9,6 +9,8 @@\n> >\n> > #include \"encoder.h\"\n> >\n> > +#include \n> > +\n> > #include \"libcamera/internal/formats.h\"\n> >\n> > #include \n> > @@ -24,14 +26,14 @@ public:\n> > libcamera::Span destination,\n> > libcamera::Span exifData,\n> > unsigned int quality) override;\n> > - int encode(libcamera::Span source,\n> > + int encode(const std::vector> &planes,\n> > libcamera::Span destination,\n> > libcamera::Span exifData,\n> > unsigned int quality);\n> >\n> > private:\n> > - void compressRGB(libcamera::Span frame);\n> > - void compressNV(libcamera::Span frame);\n> > + void compressRGB(const std::vector> &planes);\n> > + void compressNV(const std::vector> &planes);\n> >\n> > struct jpeg_compress_struct compress_;\n> > struct jpeg_error_mgr jerr_;\n> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > index 3160a784419c..68d74842925d 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > @@ -72,7 +72,7 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> > */\n> > thumbnail->resize(rawThumbnail.size());\n> >\n> > - int jpeg_size = thumbnailEncoder_.encode(rawThumbnail,\n> > + int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail },\n> > *thumbnail, {}, quality);\n> > thumbnail->resize(jpeg_size);\n> >\n> > diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n> > index 043c7b33f06a..2c341535de9e 100644\n> > --- a/src/android/jpeg/thumbnailer.cpp\n> > +++ b/src/android/jpeg/thumbnailer.cpp\n> > @@ -62,8 +62,8 @@ void Thumbnailer::createThumbnail(const FrameBuffer &source,\n> > ASSERT(tw % 2 == 0 && th % 2 == 0);\n> >\n> > /* Image scaling block implementing nearest-neighbour algorithm. */\n> > - unsigned char *src = static_cast(frame.planes()[0].data());\n> > - unsigned char *srcC = src + sh * sw;\n> > + unsigned char *src = frame.planes()[0].data();\n> > + unsigned char *srcC = frame.planes()[1].data();\n> > unsigned char *srcCb, *srcCr;\n> > unsigned char *dstY, *srcY;\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 5E727BD87D\n\tfor ;\n\tMon, 6 Sep 2021 14:08:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CFD8F6916A;\n\tMon, 6 Sep 2021 16:08:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E515C60137\n\tfor ;\n\tMon, 6 Sep 2021 16:08:45 +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 5D4948AD;\n\tMon, 6 Sep 2021 16:08:45 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"IuPbMOyL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630937325;\n\tbh=q0TtxdkY8HthC8VW9nr/SZ5FhEM1fdFMZa85xGwtdV0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IuPbMOyLU0kmhl3mhFH4wfWOvcUy4PtfI2ajsEqqCWgW8gxPer1xSD5kJKNePvfBG\n\tTtNSvfOWTJzCVIpc5/SZFHpAv08e9SaiZeZ4wPQ5S4qqGRskF4GVBvePFnTMmn9Elx\n\tqwIlQr6sDhq7XguI23inGJZqPuaN0UcglTPkD6Ec=","Date":"Mon, 6 Sep 2021 17:08:26 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210906020100.14430-1-laurent.pinchart@ideasonboard.com>\n\t<20210906134004.23485-1-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 28/27] android: jpeg: Support\n\tmulti-planar buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19466,"web_url":"https://patchwork.libcamera.org/comment/19466/","msgid":"","date":"2021-09-06T14:22:52","subject":"Re: [libcamera-devel] [PATCH v2 28/27] android: jpeg: Support\n\tmulti-planar buffers","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Mon, Sep 6, 2021 at 11:08 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Mon, Sep 06, 2021 at 10:56:26PM +0900, Hirokazu Honda wrote:\n> > On Mon, Sep 6, 2021 at 10:40 PM Laurent Pinchart wrote:\n> > >\n> > > The JPEG post-processor uses MappedFrameBuffer to access pixel data, but\n> > > only uses data from the first plane. Pass the vector of planes to the\n> > > encode() function to correctly handle multi-planar formats (currently\n> > > limited to NV12).\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> >\n> > I am not against this patch.\n> > But is this necessary in this series?\n>\n> Without it the JPEG encoder crashes when capturing a still image, so I'd\n> consider it as necessary :-)\n\nGot it.\n\n>\n> > I have done this in\n> > https://patchwork.libcamera.org/project/libcamera/list/?series=2423.\n>\n> I've given this more thoughts over the weekend, and I think we need to\n> introduce an Image class. It won't be trivial, there are quite a few\n> issues to handle to make it suitable for the libcamera public API. Once\n> this series gets merged I'll provide feedback on that.\n>\n\nLook forward to it!\n\n> > > ---\n> > > src/android/jpeg/encoder_libjpeg.cpp | 18 +++++++++---------\n> > > src/android/jpeg/encoder_libjpeg.h | 8 +++++---\n> > > src/android/jpeg/post_processor_jpeg.cpp | 2 +-\n> > > src/android/jpeg/thumbnailer.cpp | 4 ++--\n> > > 4 files changed, 17 insertions(+), 15 deletions(-)\n> > >\n> > > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> > > index b8b01e20a001..7f63f36e793e 100644\n> > > --- a/src/android/jpeg/encoder_libjpeg.cpp\n> > > +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> > > @@ -103,9 +103,9 @@ int EncoderLibJpeg::configure(const StreamConfiguration &cfg)\n> > > return 0;\n> > > }\n> > >\n> > > -void EncoderLibJpeg::compressRGB(Span frame)\n> > > +void EncoderLibJpeg::compressRGB(const std::vector> &planes)\n> > > {\n> > > - unsigned char *src = const_cast(frame.data());\n> > > + unsigned char *src = const_cast(planes[0].data());\n> > > /* \\todo Stride information should come from buffer configuration. */\n> > > unsigned int stride = pixelFormatInfo_->stride(compress_.image_width, 0);\n> > >\n> > > @@ -121,7 +121,7 @@ void EncoderLibJpeg::compressRGB(Span frame)\n> > > * Compress the incoming buffer from a supported NV format.\n> > > * This naively unpacks the semi-planar NV12 to a YUV888 format for libjpeg.\n> > > */\n> > > -void EncoderLibJpeg::compressNV(Span frame)\n> > > +void EncoderLibJpeg::compressNV(const std::vector> &planes)\n> > > {\n> > > uint8_t tmprowbuf[compress_.image_width * 3];\n> > >\n> > > @@ -133,7 +133,6 @@ void EncoderLibJpeg::compressNV(Span frame)\n> > > * Possible hints at:\n> > > * https://sourceforge.net/p/libjpeg/mailman/message/30815123/\n> > > */\n> > > - unsigned int y_stride = pixelFormatInfo_->stride(compress_.image_width, 0);\n\ny_stride should be used in computing src_y later in while-loop.\n\nReviewed-by: Hirokazu Honda \n-Hiro\n> > > unsigned int c_stride = pixelFormatInfo_->stride(compress_.image_width, 1);\n> > >\n> > > unsigned int horzSubSample = 2 * compress_.image_width / c_stride;\n> > > @@ -143,8 +142,8 @@ void EncoderLibJpeg::compressNV(Span frame)\n> > > unsigned int cb_pos = nvSwap_ ? 1 : 0;\n> > > unsigned int cr_pos = nvSwap_ ? 0 : 1;\n> > >\n> > > - const unsigned char *src = frame.data();\n> > > - const unsigned char *src_c = src + y_stride * compress_.image_height;\n> > > + const unsigned char *src = planes[0].data();\n> > > + const unsigned char *src_c = planes[1].data();\n> > >\n> > > JSAMPROW row_pointer[1];\n> > > row_pointer[0] = &tmprowbuf[0];\n> > > @@ -188,11 +187,12 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, Span dest,\n> > > return frame.error();\n> > > }\n> > >\n> > > - return encode(frame.planes()[0], dest, exifData, quality);\n> > > + return encode(frame.planes(), dest, exifData, quality);\n> > > }\n> > >\n> > > -int EncoderLibJpeg::encode(Span src, Span dest,\n> > > - Span exifData, unsigned int quality)\n> > > +int EncoderLibJpeg::encode(const std::vector> &src,\n> > > + Span dest, Span exifData,\n> > > + unsigned int quality)\n> > > {\n> > > unsigned char *destination = dest.data();\n> > > unsigned long size = dest.size();\n> > > diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> > > index 61fbd1a69278..45ffbd7fae5d 100644\n> > > --- a/src/android/jpeg/encoder_libjpeg.h\n> > > +++ b/src/android/jpeg/encoder_libjpeg.h\n> > > @@ -9,6 +9,8 @@\n> > >\n> > > #include \"encoder.h\"\n> > >\n> > > +#include \n> > > +\n> > > #include \"libcamera/internal/formats.h\"\n> > >\n> > > #include \n> > > @@ -24,14 +26,14 @@ public:\n> > > libcamera::Span destination,\n> > > libcamera::Span exifData,\n> > > unsigned int quality) override;\n> > > - int encode(libcamera::Span source,\n> > > + int encode(const std::vector> &planes,\n> > > libcamera::Span destination,\n> > > libcamera::Span exifData,\n> > > unsigned int quality);\n> > >\n> > > private:\n> > > - void compressRGB(libcamera::Span frame);\n> > > - void compressNV(libcamera::Span frame);\n> > > + void compressRGB(const std::vector> &planes);\n> > > + void compressNV(const std::vector> &planes);\n> > >\n> > > struct jpeg_compress_struct compress_;\n> > > struct jpeg_error_mgr jerr_;\n> > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > > index 3160a784419c..68d74842925d 100644\n> > > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > > @@ -72,7 +72,7 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> > > */\n> > > thumbnail->resize(rawThumbnail.size());\n> > >\n> > > - int jpeg_size = thumbnailEncoder_.encode(rawThumbnail,\n> > > + int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail },\n> > > *thumbnail, {}, quality);\n> > > thumbnail->resize(jpeg_size);\n> > >\n> > > diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n> > > index 043c7b33f06a..2c341535de9e 100644\n> > > --- a/src/android/jpeg/thumbnailer.cpp\n> > > +++ b/src/android/jpeg/thumbnailer.cpp\n> > > @@ -62,8 +62,8 @@ void Thumbnailer::createThumbnail(const FrameBuffer &source,\n> > > ASSERT(tw % 2 == 0 && th % 2 == 0);\n> > >\n> > > /* Image scaling block implementing nearest-neighbour algorithm. */\n> > > - unsigned char *src = static_cast(frame.planes()[0].data());\n> > > - unsigned char *srcC = src + sh * sw;\n> > > + unsigned char *src = frame.planes()[0].data();\n> > > + unsigned char *srcC = frame.planes()[1].data();\n> > > unsigned char *srcCb, *srcCr;\n> > > unsigned char *dstY, *srcY;\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 2C0FBBD87D\n\tfor ;\n\tMon, 6 Sep 2021 14:23:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7028F6916A;\n\tMon, 6 Sep 2021 16:23:04 +0200 (CEST)","from mail-ej1-x629.google.com (mail-ej1-x629.google.com\n\t[IPv6:2a00:1450:4864:20::629])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9A34D60137\n\tfor ;\n\tMon, 6 Sep 2021 16:23:03 +0200 (CEST)","by mail-ej1-x629.google.com with SMTP id lc21so13820996ejc.7\n\tfor ;\n\tMon, 06 Sep 2021 07:23:03 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"cG2clW6U\"; 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=KfcgccCzxHid8MZA5IzrBnA04F/4dttPTqKcxltOXiQ=;\n\tb=cG2clW6UQBPDsle2XTVfBFsgyyMUnMOJybiTySEKrXGYuAf2bTTcVZYlWzbTd/I+66\n\tEh8ZeXagc2B/e6jVXNXFFk8CmEgKMesXrP1R3txkB+iyPXRUUe9EXvtUfZf4OmrJBail\n\tfvjxQ7coCmS3PsAaLVtcUn3lIKKlnqEzeikdQ=","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=KfcgccCzxHid8MZA5IzrBnA04F/4dttPTqKcxltOXiQ=;\n\tb=AM+EgKmWEVBJL2zyvFq45JoMWq54HtUSRKGJckkHAL9dXc7u4/fNG8L3t/es60+qIJ\n\tJsLt3Da8iTeh/10NnRu4ITwWSv94ZYUeO51Dxuc/kIMcpi6D0MCj+mncam25J6zf5cMM\n\tZcWWYA9GKcy6LwAX8DZPSXnLGW+g00R7ZAcawOgnoL/u1B3FKFdZ9ccC8xCq4Bl5oiUw\n\t4HdpRvn1DscxRIfCP1Ycjn9mt9kgKYVPzgw2ziFkGOGNf7vsncgGBHXAJj6MzPp9GCyK\n\tz8hO7O0JtXEVgopK43IzrlQ7jvUVRKk9wC1rFGHpP6CaXavTy+eceJo3fdRQ9HbkJyYk\n\twvag==","X-Gm-Message-State":"AOAM531OjByBSA7Ec7WaIzQTssrkmXV3tMH4QqRWcA3KnqeBGGqEnZN3\n\tj2xaBaz5hj9gdcN5Q3qZh0iLmdYDe1mqjX6XI5k2CuSgJZvzWw==","X-Google-Smtp-Source":"ABdhPJy38MAbG6QzIYKSKWlhal+1sBpZHlMUUrkx6GOvoCAH79HaztCFwb+HT9C5kCpz7+9WH/5DAYysilPduBmP6xc=","X-Received":"by 2002:a17:906:4fd6:: with SMTP id\n\ti22mr13987752ejw.92.1630938183068; \n\tMon, 06 Sep 2021 07:23:03 -0700 (PDT)","MIME-Version":"1.0","References":"<20210906020100.14430-1-laurent.pinchart@ideasonboard.com>\n\t<20210906134004.23485-1-laurent.pinchart@ideasonboard.com>\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 6 Sep 2021 23:22:52 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 28/27] android: jpeg: Support\n\tmulti-planar buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]