[{"id":19533,"web_url":"https://patchwork.libcamera.org/comment/19533/","msgid":"","date":"2021-09-08T07:50:54","subject":"Re: [libcamera-devel] [PATCH] android: jpeg: Pass the thumbnail\n\tplanes correctly to encoder","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang, thank you for the patch.\n\nOn Wed, Sep 8, 2021 at 3:23 PM Umang Jain wrote:\n>\n> After multi-planar support was introduced for jpeg encoding as well,\n> EncoderLibJpeg::encode() expects a vector of planes as the source of\n> framebuffer to be encoded. Currently, this is broken for thumbnail\n> encoding as the thumbnail generated is directly passed as a single\n> plane.\n>\n> Hence, piece the thumbnail data as planes if the parent Framebuffer is\n> multi-planar. This fixes the encoding of the corresponding thumbnail.\n>\n> Fixes: 894ca69f6043(\"android: jpeg: Support multi-planar buffers\")\n> Signed-off-by: Umang Jain \n> ---\n> src/android/jpeg/post_processor_jpeg.cpp | 17 +++++++++++------\n> 1 file changed, 11 insertions(+), 6 deletions(-)\n>\n> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> index 68d74842..d896581f 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -66,13 +66,18 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> int ret = thumbnailEncoder_.configure(thCfg);\n>\n> if (!rawThumbnail.empty() && !ret) {\n> - /*\n> - * \\todo Avoid value-initialization of all elements of the\n> - * vector.\n> - */\n> - thumbnail->resize(rawThumbnail.size());\n> + std::vector> thumbnailPlanes;\n> + for (int i = 0; i < source.planes().size(); i++) {\n> + unsigned int thumbnailSize =\n> + targetSize.height * targetSize.width;\n> + Span plane{\n> + rawThumbnail.data() + (i * thumbnailSize),\n> + thumbnailSize\n\nThis is not correct. Since rawThumbanil is NV12, the second plane size\nis a half of the first plane size.\n\n> + };\n> + thumbnailPlanes.push_back(plane);\n> + }\n\nUhm, I would avoid introducing the dependency how rawThumbnail is\ncreated in thumbnailer_.\nSince this will be resolved by my patch series [1] later, I am fine\nwith this as a temporary solution.\nBy the way, this is related to Launrent's change, should this be a\npart of the series?\n\n+Laurent Pinchart for visibility.\n\n[1] https://patchwork.libcamera.org/project/libcamera/list/?series=2423\n\n-Hiro\n>\n> - int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail },\n> + int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes,\n> *thumbnail, {}, quality);\n> thumbnail->resize(jpeg_size);\n>\n> --\n> 2.31.1\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 33801BDC71\n\tfor ;\n\tWed, 8 Sep 2021 07:51:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B02706916E;\n\tWed, 8 Sep 2021 09:51:05 +0200 (CEST)","from mail-ej1-x630.google.com (mail-ej1-x630.google.com\n\t[IPv6:2a00:1450:4864:20::630])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 063266024D\n\tfor ;\n\tWed, 8 Sep 2021 09:51:05 +0200 (CEST)","by mail-ej1-x630.google.com with SMTP id h9so2358193ejs.4\n\tfor ;\n\tWed, 08 Sep 2021 00:51:04 -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=\"eatqTNxT\"; 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=9P+qUQCSFHo7lZfmMqNWs18CtTgL997dgeKhO7owML0=;\n\tb=eatqTNxTQXagA48UNuSW5/JfVCqtgUfMdl99u62cf1owhq+hfz+/RAeCkEb2fXnsNL\n\tU/HE6GzjQ8XefCgap/C/cewoq+JMUM53cqXNNypm3VbRJVgfQ1ahaLBiI9LtwBmM/VuC\n\tQ6PtmSuGczkh+RVN66CRcWYJD+Swew4UzvJPk=","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=9P+qUQCSFHo7lZfmMqNWs18CtTgL997dgeKhO7owML0=;\n\tb=h24knwvloB7U+fxACABaTZd3N/tUx7OB/QH22iA63GUeyXqpGtn/t7y5oqbJsucnru\n\tAi+bndNGbymO7yE6AKFAr2MPB0mHPG/MV5RQ8rBgktL4n3XXYlJnRWquj2REiW4Fk8Vj\n\tVLCm8gm02McWp3QF8hJOtKm2CsgxIfQnS923+90qQ/8ALCx6ptDe5ZT0VQPOBgQyUo9I\n\t49ReIu0dEu4/dWsKgiPwSBkUUFGNGeOsLM9k1jrLV/KhiAXMJ5Ywsc+VbvGuL0SUodzP\n\tCE1rO1Cm8fMS1knbq4x6Oq0dSfpxtMdSMycBJiIcmnB2pHBMM/sIV2SZE+wppYVUGlo4\n\tHgRA==","X-Gm-Message-State":"AOAM53280O9600GZp4LzbVxbFTo3MJaVgbigX3MMuhha2C6w1zr52Pk1\n\tN38Wh+mJxwaDvbW/yRi1eAlwzNvCXkLmOyngOS+FcQ==","X-Google-Smtp-Source":"ABdhPJxbbUfSKDeGz3iIewFQDr+OF/GpLpF0Ww1qr7DXykdj85e2vto0FOivFzb3yr7Kq7Dybclsaax+gaxWIuQBtMQ=","X-Received":"by 2002:a17:906:265a:: with SMTP id\n\ti26mr2602321ejc.522.1631087464623; \n\tWed, 08 Sep 2021 00:51:04 -0700 (PDT)","MIME-Version":"1.0","References":"<20210908062316.49466-1-umang.jain@ideasonboard.com>","In-Reply-To":"<20210908062316.49466-1-umang.jain@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Wed, 8 Sep 2021 16:50:54 +0900","Message-ID":"","To":"Umang Jain , \n\tLaurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] android: jpeg: Pass the thumbnail\n\tplanes correctly to encoder","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":19534,"web_url":"https://patchwork.libcamera.org/comment/19534/","msgid":"<2f1f9f19-1ebc-76fa-540e-9fa1aea71394@ideasonboard.com>","date":"2021-09-08T07:54:08","subject":"Re: [libcamera-devel] [PATCH] android: jpeg: Pass the thumbnail\n\tplanes correctly to encoder","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Hiro,\n\nOn 9/8/21 1:20 PM, Hirokazu Honda wrote:\n> Hi Umang, thank you for the patch.\n>\n> On Wed, Sep 8, 2021 at 3:23 PM Umang Jain wrote:\n>> After multi-planar support was introduced for jpeg encoding as well,\n>> EncoderLibJpeg::encode() expects a vector of planes as the source of\n>> framebuffer to be encoded. Currently, this is broken for thumbnail\n>> encoding as the thumbnail generated is directly passed as a single\n>> plane.\n>>\n>> Hence, piece the thumbnail data as planes if the parent Framebuffer is\n>> multi-planar. This fixes the encoding of the corresponding thumbnail.\n>>\n>> Fixes: 894ca69f6043(\"android: jpeg: Support multi-planar buffers\")\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/jpeg/post_processor_jpeg.cpp | 17 +++++++++++------\n>> 1 file changed, 11 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n>> index 68d74842..d896581f 100644\n>> --- a/src/android/jpeg/post_processor_jpeg.cpp\n>> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n>> @@ -66,13 +66,18 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n>> int ret = thumbnailEncoder_.configure(thCfg);\n>>\n>> if (!rawThumbnail.empty() && !ret) {\n>> - /*\n>> - * \\todo Avoid value-initialization of all elements of the\n>> - * vector.\n>> - */\n>> - thumbnail->resize(rawThumbnail.size());\n>> + std::vector> thumbnailPlanes;\n>> + for (int i = 0; i < source.planes().size(); i++) {\n>> + unsigned int thumbnailSize =\n>> + targetSize.height * targetSize.width;\n>> + Span plane{\n>> + rawThumbnail.data() + (i * thumbnailSize),\n>> + thumbnailSize\n> This is not correct. Since rawThumbanil is NV12, the second plane size\n> is a half of the first plane size.\n\n\nOh oops, yes you are right. Forgot to take into consideration :(\n\nWill need to think more a bit hmm!\n\n>> + };\n>> + thumbnailPlanes.push_back(plane);\n>> + }\n> Uhm, I would avoid introducing the dependency how rawThumbnail is\n> created in thumbnailer_.\n> Since this will be resolved by my patch series [1] later, I am fine\n> with this as a temporary solution.\n> By the way, this is related to Launrent's change, should this be a\n> part of the series?\n\n\nThe series is merged as far as I can see. And thumbnailing is broken on \nmaster. So, we need to fix it\n\n>\n> +Laurent Pinchart for visibility.\n>\n> [1] https://patchwork.libcamera.org/project/libcamera/list/?series=2423\n>\n> -Hiro\n>> - int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail },\n>> + int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes,\n>> *thumbnail, {}, quality);\n>> thumbnail->resize(jpeg_size);\n>>\n>> --\n>> 2.31.1\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 B660EBDB1D\n\tfor ;\n\tWed, 8 Sep 2021 07:54:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4C0C760250;\n\tWed, 8 Sep 2021 09:54:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5626F6024D\n\tfor ;\n\tWed, 8 Sep 2021 09:54:17 +0200 (CEST)","from [IPv6:2405:204:8105:83f6:23fd:f036:32ba:a608] (unknown\n\t[IPv6:2405:204:8105:83f6:23fd:f036:32ba:a608])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 58820993;\n\tWed, 8 Sep 2021 09:54:13 +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=\"LpRXQiSm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631087656;\n\tbh=pS9yH8f6rrMXooK14pZENvNu4G/20toIK3hMQ/q5M7c=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=LpRXQiSmAMvG0p4GueoJcpTrPBOXeomJjUbtzkwClAZL/TdgRUY8gaPwip2IIr/w3\n\tE4S3rGSTY5bG2N6s0PcA8UPqUmudVPu+vNEWc9sgVzMgaN/wQRx1SbT0pOUMZeS3vV\n\tqSCQGYG3qVI5mWSy9FHbIpazTM+8BO5mmxiccT2A=","To":"Hirokazu Honda ,\n\tLaurent Pinchart ","References":"<20210908062316.49466-1-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"<2f1f9f19-1ebc-76fa-540e-9fa1aea71394@ideasonboard.com>","Date":"Wed, 8 Sep 2021 13:24:08 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH] android: jpeg: Pass the thumbnail\n\tplanes correctly to encoder","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":19535,"web_url":"https://patchwork.libcamera.org/comment/19535/","msgid":"","date":"2021-09-08T08:04:02","subject":"Re: [libcamera-devel] [PATCH] android: jpeg: Pass the thumbnail\n\tplanes correctly to encoder","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang,\n\nOn Wed, Sep 8, 2021 at 4:54 PM Umang Jain wrote:\n>\n> Hi Hiro,\n>\n> On 9/8/21 1:20 PM, Hirokazu Honda wrote:\n> > Hi Umang, thank you for the patch.\n> >\n> > On Wed, Sep 8, 2021 at 3:23 PM Umang Jain wrote:\n> >> After multi-planar support was introduced for jpeg encoding as well,\n> >> EncoderLibJpeg::encode() expects a vector of planes as the source of\n> >> framebuffer to be encoded. Currently, this is broken for thumbnail\n> >> encoding as the thumbnail generated is directly passed as a single\n> >> plane.\n> >>\n> >> Hence, piece the thumbnail data as planes if the parent Framebuffer is\n> >> multi-planar. This fixes the encoding of the corresponding thumbnail.\n> >>\n> >> Fixes: 894ca69f6043(\"android: jpeg: Support multi-planar buffers\")\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> src/android/jpeg/post_processor_jpeg.cpp | 17 +++++++++++------\n> >> 1 file changed, 11 insertions(+), 6 deletions(-)\n> >>\n> >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> >> index 68d74842..d896581f 100644\n> >> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> >> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> >> @@ -66,13 +66,18 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> >> int ret = thumbnailEncoder_.configure(thCfg);\n> >>\n> >> if (!rawThumbnail.empty() && !ret) {\n> >> - /*\n> >> - * \\todo Avoid value-initialization of all elements of the\n> >> - * vector.\n> >> - */\n> >> - thumbnail->resize(rawThumbnail.size());\n> >> + std::vector> thumbnailPlanes;\n> >> + for (int i = 0; i < source.planes().size(); i++) {\n> >> + unsigned int thumbnailSize =\n> >> + targetSize.height * targetSize.width;\n> >> + Span plane{\n> >> + rawThumbnail.data() + (i * thumbnailSize),\n> >> + thumbnailSize\n> > This is not correct. Since rawThumbanil is NV12, the second plane size\n> > is a half of the first plane size.\n>\n>\n> Oh oops, yes you are right. Forgot to take into consideration :(\n>\n> Will need to think more a bit hmm!\n>\n> >> + };\n> >> + thumbnailPlanes.push_back(plane);\n> >> + }\n> > Uhm, I would avoid introducing the dependency how rawThumbnail is\n> > created in thumbnailer_.\n> > Since this will be resolved by my patch series [1] later, I am fine\n> > with this as a temporary solution.\n> > By the way, this is related to Launrent's change, should this be a\n> > part of the series?\n>\n>\n> The series is merged as far as I can see. And thumbnailing is broken on\n> master. So, we need to fix it\n>\n\nOh I didn't realize it. :-)\nLet's go ahead then.\n\n-Hiro\n> >\n> > +Laurent Pinchart for visibility.\n> >\n> > [1] https://patchwork.libcamera.org/project/libcamera/list/?series=2423\n> >\n> > -Hiro\n> >> - int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail },\n> >> + int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes,\n> >> *thumbnail, {}, quality);\n> >> thumbnail->resize(jpeg_size);\n> >>\n> >> --\n> >> 2.31.1\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 B93E4BDC71\n\tfor ;\n\tWed, 8 Sep 2021 08:04:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 14CAF6916E;\n\tWed, 8 Sep 2021 10:04:15 +0200 (CEST)","from mail-ed1-x536.google.com (mail-ed1-x536.google.com\n\t[IPv6:2a00:1450:4864:20::536])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 96AB26024D\n\tfor ;\n\tWed, 8 Sep 2021 10:04:13 +0200 (CEST)","by mail-ed1-x536.google.com with SMTP id s25so1767239edw.0\n\tfor ;\n\tWed, 08 Sep 2021 01:04:13 -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=\"jM9IfC2T\"; 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=6w5GeUsK2LKyiFJ39o+yzMnSVlf4rXhKpk2MDdmlJKM=;\n\tb=jM9IfC2T68xjlonXDfS7k9kIee56oacSEjs0yTAiNLKQcN8/OjoLsfjvOcEw1DqWHW\n\t02s+pgXPauaxbca8PFZy6v6Fr4jTPyPJMvReIf95BtOQMAK6p2nEjO85ZJQe8dKbwPvv\n\t+hgndSSXaPyxvbzF1BAec3VAtAlwm43IeyGcM=","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=6w5GeUsK2LKyiFJ39o+yzMnSVlf4rXhKpk2MDdmlJKM=;\n\tb=o31rbwOROpcdFqz65M00+7Hb1PhBAPxVCtdQK7UBM40BOfvvjIvy5BibP21FcYYyn3\n\ta7fwVBJV8J2VkoTMkCBIRPpmI5ubd/VNbn93F5tIamUBOu0xR6H4wl7c72aXr5bKtbP/\n\t+eV2F3QauHM5Vv7I98T8H71cmm4KT7j+LgB3FrrENVg7X0vdGuH1avLIFJikS84vNFUB\n\tldVXRfSqyMyDAvA8LZw6QlpNwJYOFwOZakFlDJwx1U1G4e/3ZKJGzjs2y+7ULCTGIhqb\n\tMyMCXI/eGyy2CNET3gItZ80bJzaFB0vVe+Bax/BuUanHK598uHa0Q1BTtucimorlwu7q\n\t1PdQ==","X-Gm-Message-State":"AOAM5338udVwoicFjqcGPzRY0TEOtM/Q6yrHhfjRhFMoNeA9JdC/496J\n\tLtfrX0u4bnjXCMdHtEeRJ3RHV2ITiXSeGyrTbKHOaWfoT2Q=","X-Google-Smtp-Source":"ABdhPJz7O5Vqfn0xhulOBBbupuImPJgoP9VztD0234qVypg+T0Yswx4eH7E9cDOCrF0Y+HZtjdc8Il5us1Gns7zYRMM=","X-Received":"by 2002:a05:6402:5163:: with SMTP id\n\td3mr2582672ede.220.1631088253229; \n\tWed, 08 Sep 2021 01:04:13 -0700 (PDT)","MIME-Version":"1.0","References":"<20210908062316.49466-1-umang.jain@ideasonboard.com>\n\t\n\t<2f1f9f19-1ebc-76fa-540e-9fa1aea71394@ideasonboard.com>","In-Reply-To":"<2f1f9f19-1ebc-76fa-540e-9fa1aea71394@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Wed, 8 Sep 2021 17:04:02 +0900","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] android: jpeg: Pass the thumbnail\n\tplanes correctly to encoder","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":19536,"web_url":"https://patchwork.libcamera.org/comment/19536/","msgid":"<20210908080503.njbp3afkvv6kkpz3@uno.localdomain>","date":"2021-09-08T08:05:03","subject":"Re: [libcamera-devel] [PATCH] android: jpeg: Pass the thumbnail\n\tplanes correctly to encoder","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang\n\nOn Wed, Sep 08, 2021 at 11:53:16AM +0530, Umang Jain wrote:\n> After multi-planar support was introduced for jpeg encoding as well,\n> EncoderLibJpeg::encode() expects a vector of planes as the source of\n> framebuffer to be encoded. Currently, this is broken for thumbnail\n> encoding as the thumbnail generated is directly passed as a single\n> plane.\n>\n> Hence, piece the thumbnail data as planes if the parent Framebuffer is\n\nDid you mean 'place', as in 'to place' as a verb ?\n\n> multi-planar. This fixes the encoding of the corresponding thumbnail.\n>\n> Fixes: 894ca69f6043(\"android: jpeg: Support multi-planar buffers\")\n> Signed-off-by: Umang Jain \n> ---\n> src/android/jpeg/post_processor_jpeg.cpp | 17 +++++++++++------\n> 1 file changed, 11 insertions(+), 6 deletions(-)\n>\n> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> index 68d74842..d896581f 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -66,13 +66,18 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> \tint ret = thumbnailEncoder_.configure(thCfg);\n>\n> \tif (!rawThumbnail.empty() && !ret) {\n> -\t\t/*\n> -\t\t * \\todo Avoid value-initialization of all elements of the\n> -\t\t * vector.\n> -\t\t */\n> -\t\tthumbnail->resize(rawThumbnail.size());\n> +\t\tstd::vector> thumbnailPlanes;\n> +\t\tfor (int i = 0; i < source.planes().size(); i++) {\n> +\t\t\tunsigned int thumbnailSize =\n> +\t\t\t\ttargetSize.height * targetSize.width;\n> +\t\t\tSpan plane{\n> +\t\t\t\trawThumbnail.data() + (i * thumbnailSize),\n> +\t\t\t\tthumbnailSize\n> +\t\t\t};\n\nSo, I never really understood the thumbnailer bits but this looks\nsurprising to me. Are all the planes of the same size ? Aren't we\nassuming NV12 as the format of the rawThumbnail ? Shouldn't the CbCr\nplane be half in size ?\n\nSo, I haven't followed much the fallout of the multi-planar support\nbut what I see here is that now the libjpeg encoder requires the\nY and CbCr planes to be stored in different 'planes()' so I\nunderstand you have to artificially create those planes as a\nconsequence of the fact that createThumbnail() scales-down the image\ninto a single span of chars\n\n\t/* Stores the raw scaled-down thumbnail bytes. */\n\tstd::vector rawThumbnail;\n\tthumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);\n\nWouldn't it be better to:\n\t/* Stores the raw scaled-down thumbnail bytes. */\n\tstd::vector> rawThumbnail;\n\tthumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);\n\nAnd have the raw thumbnail already displaced in two planes ?\nThe code seems to be there\n\n dstC[(y / 2) * tw + x + 0] = srcCb[(sourceX / 2) * 2];\n dstC[(y / 2) * tw + x + 1] = srcCr[(sourceX / 2) * 2];\n\nYou just need to point dstC to the second plane if I'm not mistaken.\n\nThanks\n j\n\n\n\n> +\t\t\tthumbnailPlanes.push_back(plane);\n> +\t\t}\n>\n> -\t\tint jpeg_size = thumbnailEncoder_.encode({ rawThumbnail },\n> +\t\tint jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes,\n> \t\t\t\t\t\t\t *thumbnail, {}, quality);\n> \t\tthumbnail->resize(jpeg_size);\n>\n> --\n> 2.31.1\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 00B77BDC71\n\tfor ;\n\tWed, 8 Sep 2021 08:04:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CA35269170;\n\tWed, 8 Sep 2021 10:04:18 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6802D6024D\n\tfor ;\n\tWed, 8 Sep 2021 10:04:17 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id EE8FDFF812;\n\tWed, 8 Sep 2021 08:04:16 +0000 (UTC)"],"Date":"Wed, 8 Sep 2021 10:05:03 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210908080503.njbp3afkvv6kkpz3@uno.localdomain>","References":"<20210908062316.49466-1-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210908062316.49466-1-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] android: jpeg: Pass the thumbnail\n\tplanes correctly to encoder","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19537,"web_url":"https://patchwork.libcamera.org/comment/19537/","msgid":"","date":"2021-09-08T08:36:45","subject":"Re: [libcamera-devel] [PATCH] android: jpeg: Pass the thumbnail\n\tplanes correctly to encoder","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Sep 08, 2021 at 10:05:03AM +0200, Jacopo Mondi wrote:\n> On Wed, Sep 08, 2021 at 11:53:16AM +0530, Umang Jain wrote:\n> > After multi-planar support was introduced for jpeg encoding as well,\n> > EncoderLibJpeg::encode() expects a vector of planes as the source of\n> > framebuffer to be encoded. Currently, this is broken for thumbnail\n> > encoding as the thumbnail generated is directly passed as a single\n> > plane.\n> >\n> > Hence, piece the thumbnail data as planes if the parent Framebuffer is\n> \n> Did you mean 'place', as in 'to place' as a verb ?\n> \n> > multi-planar. This fixes the encoding of the corresponding thumbnail.\n> >\n> > Fixes: 894ca69f6043(\"android: jpeg: Support multi-planar buffers\")\n> > Signed-off-by: Umang Jain \n> > ---\n> > src/android/jpeg/post_processor_jpeg.cpp | 17 +++++++++++------\n> > 1 file changed, 11 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > index 68d74842..d896581f 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > @@ -66,13 +66,18 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> > \tint ret = thumbnailEncoder_.configure(thCfg);\n> >\n> > \tif (!rawThumbnail.empty() && !ret) {\n> > -\t\t/*\n> > -\t\t * \\todo Avoid value-initialization of all elements of the\n> > -\t\t * vector.\n> > -\t\t */\n> > -\t\tthumbnail->resize(rawThumbnail.size());\n> > +\t\tstd::vector> thumbnailPlanes;\n> > +\t\tfor (int i = 0; i < source.planes().size(); i++) {\n> > +\t\t\tunsigned int thumbnailSize =\n> > +\t\t\t\ttargetSize.height * targetSize.width;\n> > +\t\t\tSpan plane{\n> > +\t\t\t\trawThumbnail.data() + (i * thumbnailSize),\n> > +\t\t\t\tthumbnailSize\n> > +\t\t\t};\n> \n> So, I never really understood the thumbnailer bits but this looks\n> surprising to me. Are all the planes of the same size ? Aren't we\n> assuming NV12 as the format of the rawThumbnail ? Shouldn't the CbCr\n> plane be half in size ?\n\nYou're right. Hiro has reported the same :-)\n\n> So, I haven't followed much the fallout of the multi-planar support\n> but what I see here is that now the libjpeg encoder requires the\n> Y and CbCr planes to be stored in different 'planes()' so I\n> understand you have to artificially create those planes as a\n> consequence of the fact that createThumbnail() scales-down the image\n> into a single span of chars\n> \n> \t/* Stores the raw scaled-down thumbnail bytes. */\n> \tstd::vector rawThumbnail;\n> \tthumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);\n> \n> Wouldn't it be better to:\n> \t/* Stores the raw scaled-down thumbnail bytes. */\n> \tstd::vector> rawThumbnail;\n> \tthumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);\n> \n> And have the raw thumbnail already displaced in two planes ?\n\nThe NV12 thumbnail doesn't have to be stored in two separate memory\nbuffers, it's just that the encoder now needs to be told where the two\nplanes are, even if they're contiguous.\n\nWe could split the planes, but I'm not sure it's worth it. I'd go for\nthe simplest solution for now, it will be cleaned up once we get an\nImage class in libcamera.\n\n> The code seems to be there\n> \n> dstC[(y / 2) * tw + x + 0] = srcCb[(sourceX / 2) * 2];\n> dstC[(y / 2) * tw + x + 1] = srcCr[(sourceX / 2) * 2];\n> \n> You just need to point dstC to the second plane if I'm not mistaken.\n> \n> > +\t\t\tthumbnailPlanes.push_back(plane);\n> > +\t\t}\n> >\n> > -\t\tint jpeg_size = thumbnailEncoder_.encode({ rawThumbnail },\n> > +\t\tint jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes,\n> > \t\t\t\t\t\t\t *thumbnail, {}, quality);\n> > \t\tthumbnail->resize(jpeg_size);\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 EEE6ABDB1D\n\tfor ;\n\tWed, 8 Sep 2021 08:37:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3A5016916E;\n\tWed, 8 Sep 2021 10:37:07 +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 B0B826024D\n\tfor ;\n\tWed, 8 Sep 2021 10:37:05 +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 1DFB3993;\n\tWed, 8 Sep 2021 10:37:05 +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=\"ROKIfdwq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631090225;\n\tbh=4GpQABOVZZHPqzp0QPcEtxZtPf4ToC5RN03wCM68UYI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ROKIfdwqlzcWBZkrXM/7V1RV1CKvTWQIrZhy7MYf/dShSTsuFCqYJY3bl8NI70e43\n\tMBgeECpAHeLy88VMsWHId68dIAFn9ND4vhkKvrkYsdXDD/PgZoXpOQHvS5Zfyxz5vd\n\tlqm1S5aDpcUk7d7Cc4fF9dBmurDp1y40EbPo/53c=","Date":"Wed, 8 Sep 2021 11:36:45 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210908062316.49466-1-umang.jain@ideasonboard.com>\n\t<20210908080503.njbp3afkvv6kkpz3@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210908080503.njbp3afkvv6kkpz3@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH] android: jpeg: Pass the thumbnail\n\tplanes correctly to encoder","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19546,"web_url":"https://patchwork.libcamera.org/comment/19546/","msgid":"<401a0bc5-f720-b3ec-4049-71b6aae67675@ideasonboard.com>","date":"2021-09-08T10:01:18","subject":"Re: [libcamera-devel] [PATCH] android: jpeg: Pass the thumbnail\n\tplanes correctly to encoder","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 9/8/21 1:35 PM, Jacopo Mondi wrote:\n> Hi Umang\n>\n> On Wed, Sep 08, 2021 at 11:53:16AM +0530, Umang Jain wrote:\n>> After multi-planar support was introduced for jpeg encoding as well,\n>> EncoderLibJpeg::encode() expects a vector of planes as the source of\n>> framebuffer to be encoded. Currently, this is broken for thumbnail\n>> encoding as the thumbnail generated is directly passed as a single\n>> plane.\n>>\n>> Hence, piece the thumbnail data as planes if the parent Framebuffer is\n> Did you mean 'place', as in 'to place' as a verb ?\n\n\nplace? I wrote 'piece'. I was going to write 'chop', okay, nevermind...\n\n>\n>> multi-planar. This fixes the encoding of the corresponding thumbnail.\n>>\n>> Fixes: 894ca69f6043(\"android: jpeg: Support multi-planar buffers\")\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/jpeg/post_processor_jpeg.cpp | 17 +++++++++++------\n>> 1 file changed, 11 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n>> index 68d74842..d896581f 100644\n>> --- a/src/android/jpeg/post_processor_jpeg.cpp\n>> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n>> @@ -66,13 +66,18 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n>> \tint ret = thumbnailEncoder_.configure(thCfg);\n>>\n>> \tif (!rawThumbnail.empty() && !ret) {\n>> -\t\t/*\n>> -\t\t * \\todo Avoid value-initialization of all elements of the\n>> -\t\t * vector.\n>> -\t\t */\n>> -\t\tthumbnail->resize(rawThumbnail.size());\n>> +\t\tstd::vector> thumbnailPlanes;\n>> +\t\tfor (int i = 0; i < source.planes().size(); i++) {\n>> +\t\t\tunsigned int thumbnailSize =\n>> +\t\t\t\ttargetSize.height * targetSize.width;\n>> +\t\t\tSpan plane{\n>> +\t\t\t\trawThumbnail.data() + (i * thumbnailSize),\n>> +\t\t\t\tthumbnailSize\n>> +\t\t\t};\n> So, I never really understood the thumbnailer bits but this looks\n> surprising to me. Are all the planes of the same size ? Aren't we\n> assuming NV12 as the format of the rawThumbnail ? Shouldn't the CbCr\n> plane be half in size ?\n>\n> So, I haven't followed much the fallout of the multi-planar support\n> but what I see here is that now the libjpeg encoder requires the\n> Y and CbCr planes to be stored in different 'planes()' so I\n> understand you have to artificially create those planes as a\n> consequence of the fact that createThumbnail() scales-down the image\n> into a single span of chars\n>\n> \t/* Stores the raw scaled-down thumbnail bytes. */\n> \tstd::vector rawThumbnail;\n> \tthumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);\n>\n> Wouldn't it be better to:\n> \t/* Stores the raw scaled-down thumbnail bytes. */\n> \tstd::vector> rawThumbnail;\n> \tthumbnailer_.createThumbnail(source, targetSize, &rawThumbnail);\n\n\nYes this is definitely better. I would also need to ensure correct Span \nis used to store the scaled down bits - which will probably change quite \na bit of pointer arithmetic in the Thumbnailer code. It's not hard to do \nper say, but I guess Laurent is in favor of a quick and simpler patch \nfor now (and I too).\n\nI have added your suggestion as a \\todo though in v2, so the idea \ndoesn't gets lost in future.\n\n>\n> And have the raw thumbnail already displaced in two planes ?\n> The code seems to be there\n>\n> dstC[(y / 2) * tw + x + 0] = srcCb[(sourceX / 2) * 2];\n> dstC[(y / 2) * tw + x + 1] = srcCr[(sourceX / 2) * 2];\n>\n> You just need to point dstC to the second plane if I'm not mistaken.\n>\n> Thanks\n> j\n>\n>\n>\n>> +\t\t\tthumbnailPlanes.push_back(plane);\n>> +\t\t}\n>>\n>> -\t\tint jpeg_size = thumbnailEncoder_.encode({ rawThumbnail },\n>> +\t\tint jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes,\n>> \t\t\t\t\t\t\t *thumbnail, {}, quality);\n>> \t\tthumbnail->resize(jpeg_size);\n>>\n>> --\n>> 2.31.1\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 C3763BDC71\n\tfor ;\n\tWed, 8 Sep 2021 10:01:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 851506916E;\n\tWed, 8 Sep 2021 12:01:25 +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 A9A0160503\n\tfor ;\n\tWed, 8 Sep 2021 12:01:23 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.149])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ADB51993;\n\tWed, 8 Sep 2021 12:01:22 +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=\"arDZMckg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631095283;\n\tbh=DMc5KpobxaBL6AodId+rSDVecqQQgR+3CZhJymrCWxE=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=arDZMckg1G905P6n5j0ncTRTZT9wDfVTPAw/2gNerI2lQ78zqghi6RRnoO55bvkOk\n\tEWTGJs/QwL4ieYP16SXafrFSZzqHRcYdGR14waJtAFgvur1HyECjFDCVzTO5eYv7R0\n\tOw2PG5EMSQdEHhUIieVWiBvZKQ4IGPgFnWlMrx2g=","To":"Jacopo Mondi ","References":"<20210908062316.49466-1-umang.jain@ideasonboard.com>\n\t<20210908080503.njbp3afkvv6kkpz3@uno.localdomain>","From":"Umang Jain ","Message-ID":"<401a0bc5-f720-b3ec-4049-71b6aae67675@ideasonboard.com>","Date":"Wed, 8 Sep 2021 15:31:18 +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":"<20210908080503.njbp3afkvv6kkpz3@uno.localdomain>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH] android: jpeg: Pass the thumbnail\n\tplanes correctly to encoder","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]