[{"id":12316,"web_url":"https://patchwork.libcamera.org/comment/12316/","msgid":"<20200905180021.GG6319@pendragon.ideasonboard.com>","date":"2020-09-05T18:00:21","subject":"Re: [libcamera-devel] [PATCH v2 04/12] android: camera_device:\n\tBreak out size calculation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Wed, Sep 02, 2020 at 05:22:28PM +0200, Jacopo Mondi wrote:\n> As the RAW stream sizes needs to be calculated differently from the\n> processed one, break out break out the procedure to calculate the\n\ns/break out break out/break out/\n\n> processed (RGB/YUV) resolutions from initializeStreamConfigurations() in\n> order to prepare for RAW sizes calculation.\n> \n> Reviewed-by: Kieran Bingham \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 55 ++++++++++++++++++++++++-----------\n> src/android/camera_device.h | 5 ++++\n> 2 files changed, 43 insertions(+), 17 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 493d6cecde72..58b2ad27c5e2 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -283,6 +283,37 @@ int CameraDevice::initialize()\n> \treturn ret;\n> }\n> \n> +std::vector CameraDevice::filterYUVResolutions(CameraConfiguration *cameraConfig,\n> +\t\t\t\t\t\t const PixelFormat &pixelFormat,\n> +\t\t\t\t\t\t const std::vector &resolutions)\n> +{\n> +\tstd::vector supportedResolutions;\n> +\n> +\tStreamConfiguration &cfg = cameraConfig->at(0);\n> +\tfor (const Size &res : resolutions) {\n> +\t\tcfg.pixelFormat = pixelFormat;\n> +\t\tcfg.size = res;\n> +\n> +\t\tstd::stringstream ss;\n> +\t\tss << \"Tested (\" << res.toString() << \")[\"\n\nTesting became Tested ? :-)\n\n> +\t\t << pixelFormat.toString() << \"]: \";\n> +\n> +\t\tCameraConfiguration::Status status = cameraConfig->validate();\n> +\t\tif (status != CameraConfiguration::Valid) {\n> +\t\t\tss << \" not supported\";\n> +\t\t\tLOG(HAL, Debug) << ss.str();\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tss << \" supported\";\n> +\t\tLOG(HAL, Debug) << ss.str();\n> +\n> +\t\tsupportedResolutions.push_back(res);\n> +\t}\n> +\n> +\treturn supportedResolutions;\n> +}\n> +\n> /*\n> * Initialize the format conversion map to translate from Android format\n> * identifier to libcamera pixel formats and fill in the list of supported\n> @@ -435,24 +466,14 @@ int CameraDevice::initializeStreamConfigurations()\n> \t\t\t\t<< camera3Format.name << \" to: \"\n> \t\t\t\t<< mappedFormat.toString();\n> \n> -\t\tfor (const Size &res : cameraResolutions) {\n> -\t\t\tcfg.pixelFormat = mappedFormat;\n> -\t\t\tcfg.size = res;\n> -\n> -\t\t\tstd::stringstream ss;\n> -\t\t\tss << \"Testing (\" << res.toString() << \")[\"\n> -\t\t\t << mappedFormat.toString() << \"]: \";\n> -\n> -\t\t\tCameraConfiguration::Status status = cameraConfig->validate();\n> -\t\t\tif (status != CameraConfiguration::Valid) {\n> -\t\t\t\tss << \" not supported\";\n> -\t\t\t\tLOG(HAL, Debug) << ss.str();\n> -\t\t\t\tcontinue;\n> -\t\t\t}\n> -\n> -\t\t\tss << \" supported\";\n> -\t\t\tLOG(HAL, Debug) << ss.str();\n> +\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(mappedFormat);\n> +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> +\t\t\tcontinue;\n\nI'd move this to patch 05/12, as it introduces a change not described in\nthe commit message (and breaks bisection unless I'm mistaken).\n\n> \n> +\t\tstd::vector resolutions = filterYUVResolutions(cameraConfig.get(),\n> +\t\t\t\t\t\t\t\t mappedFormat,\n> +\t\t\t\t\t\t\t\t cameraResolutions);\n> +\t\tfor (const Size &res : resolutions) {\n> \t\t\tstreamConfigurations_.push_back({ res, androidFormat });\n> \n> \t\t\t/*\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 3934f194f1b5..359a163ebab9 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -93,6 +93,11 @@ private:\n> \t};\n> \n> \tint initializeStreamConfigurations();\n> +\tstd::vector\n> +\tfilterYUVResolutions(libcamera::CameraConfiguration *cameraConfig,\n> +\t\t\t const libcamera::PixelFormat &pixelFormat,\n> +\t\t\t const std::vector &resolutions);\n\nYou can make this a static function.\n\nReviewed-by: Laurent Pinchart \n\n> +\n> \tstd::tuple calculateStaticMetadataSize();\n> \tlibcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> \tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);","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 A6507BDB1D\n\tfor ;\n\tSat, 5 Sep 2020 18:00:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 45B1E629B6;\n\tSat, 5 Sep 2020 20:00:49 +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 AB3E162901\n\tfor ;\n\tSat, 5 Sep 2020 20:00:47 +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 11C90335;\n\tSat, 5 Sep 2020 20:00:44 +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=\"W0lMcgnD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599328845;\n\tbh=eQ5gfERBuSu/upKA8N0grSQC2rAQ2xmn0CIMpCxGsGg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=W0lMcgnDe/l1n3qQJip2bIr6upl1OLmamSWeimoe90qTFiV7Cid6m8MLLy/B3/q1Z\n\tJOua+13iG8jOZWux6tBUIeqPwVeH48E+m9BHphalhNPBM7WyohNHWLGA0A7I+mkEmR\n\tG+axZLcBZh4t7vx6bqH3crs1nvyu4VAH2MokWHqw=","Date":"Sat, 5 Sep 2020 21:00:21 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200905180021.GG6319@pendragon.ideasonboard.com>","References":"<20200902152236.69770-1-jacopo@jmondi.org>\n\t<20200902152236.69770-5-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200902152236.69770-5-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 04/12] android: camera_device:\n\tBreak out size calculation","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":"tfiga@google.com, libcamera-devel@lists.libcamera.org, hiroh@google.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12345,"web_url":"https://patchwork.libcamera.org/comment/12345/","msgid":"<20200907083200.hj7tw3jq526mixik@uno.localdomain>","date":"2020-09-07T08:32:00","subject":"Re: [libcamera-devel] [PATCH v2 04/12] android: camera_device:\n\tBreak out size calculation","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sat, Sep 05, 2020 at 09:00:21PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Sep 02, 2020 at 05:22:28PM +0200, Jacopo Mondi wrote:\n> > As the RAW stream sizes needs to be calculated differently from the\n> > processed one, break out break out the procedure to calculate the\n>\n> s/break out break out/break out/\n>\n> > processed (RGB/YUV) resolutions from initializeStreamConfigurations() in\n> > order to prepare for RAW sizes calculation.\n> >\n> > Reviewed-by: Kieran Bingham \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_device.cpp | 55 ++++++++++++++++++++++++-----------\n> > src/android/camera_device.h | 5 ++++\n> > 2 files changed, 43 insertions(+), 17 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 493d6cecde72..58b2ad27c5e2 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -283,6 +283,37 @@ int CameraDevice::initialize()\n> > \treturn ret;\n> > }\n> >\n> > +std::vector CameraDevice::filterYUVResolutions(CameraConfiguration *cameraConfig,\n> > +\t\t\t\t\t\t const PixelFormat &pixelFormat,\n> > +\t\t\t\t\t\t const std::vector &resolutions)\n> > +{\n> > +\tstd::vector supportedResolutions;\n> > +\n> > +\tStreamConfiguration &cfg = cameraConfig->at(0);\n> > +\tfor (const Size &res : resolutions) {\n> > +\t\tcfg.pixelFormat = pixelFormat;\n> > +\t\tcfg.size = res;\n> > +\n> > +\t\tstd::stringstream ss;\n> > +\t\tss << \"Tested (\" << res.toString() << \")[\"\n>\n> Testing became Tested ? :-)\n>\n> > +\t\t << pixelFormat.toString() << \"]: \";\n> > +\n> > +\t\tCameraConfiguration::Status status = cameraConfig->validate();\n> > +\t\tif (status != CameraConfiguration::Valid) {\n> > +\t\t\tss << \" not supported\";\n> > +\t\t\tLOG(HAL, Debug) << ss.str();\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > +\n> > +\t\tss << \" supported\";\n> > +\t\tLOG(HAL, Debug) << ss.str();\n> > +\n> > +\t\tsupportedResolutions.push_back(res);\n> > +\t}\n> > +\n> > +\treturn supportedResolutions;\n> > +}\n> > +\n> > /*\n> > * Initialize the format conversion map to translate from Android format\n> > * identifier to libcamera pixel formats and fill in the list of supported\n> > @@ -435,24 +466,14 @@ int CameraDevice::initializeStreamConfigurations()\n> > \t\t\t\t<< camera3Format.name << \" to: \"\n> > \t\t\t\t<< mappedFormat.toString();\n> >\n> > -\t\tfor (const Size &res : cameraResolutions) {\n> > -\t\t\tcfg.pixelFormat = mappedFormat;\n> > -\t\t\tcfg.size = res;\n> > -\n> > -\t\t\tstd::stringstream ss;\n> > -\t\t\tss << \"Testing (\" << res.toString() << \")[\"\n> > -\t\t\t << mappedFormat.toString() << \"]: \";\n> > -\n> > -\t\t\tCameraConfiguration::Status status = cameraConfig->validate();\n> > -\t\t\tif (status != CameraConfiguration::Valid) {\n> > -\t\t\t\tss << \" not supported\";\n> > -\t\t\t\tLOG(HAL, Debug) << ss.str();\n> > -\t\t\t\tcontinue;\n> > -\t\t\t}\n> > -\n> > -\t\t\tss << \" supported\";\n> > -\t\t\tLOG(HAL, Debug) << ss.str();\n> > +\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(mappedFormat);\n> > +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> > +\t\t\tcontinue;\n>\n> I'd move this to patch 05/12, as it introduces a change not described in\n> the commit message (and breaks bisection unless I'm mistaken).\n>\n\nYes, that's probably better. Let's keep the original behaviour of not\nsupporting RAW resolutions properly here and introduce them in 5/12.\n\nThanks\n j\n\n> >\n> > +\t\tstd::vector resolutions = filterYUVResolutions(cameraConfig.get(),\n> > +\t\t\t\t\t\t\t\t mappedFormat,\n> > +\t\t\t\t\t\t\t\t cameraResolutions);\n> > +\t\tfor (const Size &res : resolutions) {\n> > \t\t\tstreamConfigurations_.push_back({ res, androidFormat });\n> >\n> > \t\t\t/*\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index 3934f194f1b5..359a163ebab9 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -93,6 +93,11 @@ private:\n> > \t};\n> >\n> > \tint initializeStreamConfigurations();\n> > +\tstd::vector\n> > +\tfilterYUVResolutions(libcamera::CameraConfiguration *cameraConfig,\n> > +\t\t\t const libcamera::PixelFormat &pixelFormat,\n> > +\t\t\t const std::vector &resolutions);\n>\n> You can make this a static function.\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > +\n> > \tstd::tuple calculateStaticMetadataSize();\n> > \tlibcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> > \tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\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 976B3BDB1C\n\tfor ;\n\tMon, 7 Sep 2020 08:28:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3E35E62B82;\n\tMon, 7 Sep 2020 10:28:14 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3CC3E629B6\n\tfor ;\n\tMon, 7 Sep 2020 10:28:13 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 2857A200011;\n\tMon, 7 Sep 2020 08:28:11 +0000 (UTC)"],"Date":"Mon, 7 Sep 2020 10:32:00 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20200907083200.hj7tw3jq526mixik@uno.localdomain>","References":"<20200902152236.69770-1-jacopo@jmondi.org>\n\t<20200902152236.69770-5-jacopo@jmondi.org>\n\t<20200905180021.GG6319@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200905180021.GG6319@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 04/12] android: camera_device:\n\tBreak out size calculation","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":"tfiga@google.com, libcamera-devel@lists.libcamera.org, hiroh@google.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12346,"web_url":"https://patchwork.libcamera.org/comment/12346/","msgid":"","date":"2020-09-07T08:32:44","subject":"Re: [libcamera-devel] [PATCH v2 04/12] android: camera_device:\n\tBreak out size calculation","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Sun, Sep 6, 2020 at 3:00 AM Laurent Pinchart\n wrote:\n>\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Sep 02, 2020 at 05:22:28PM +0200, Jacopo Mondi wrote:\n> > As the RAW stream sizes needs to be calculated differently from the\n> > processed one, break out break out the procedure to calculate the\n>\n> s/break out break out/break out/\n>\n> > processed (RGB/YUV) resolutions from initializeStreamConfigurations() in\n> > order to prepare for RAW sizes calculation.\n> >\n> > Reviewed-by: Kieran Bingham \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_device.cpp | 55 ++++++++++++++++++++++++-----------\n> > src/android/camera_device.h | 5 ++++\n> > 2 files changed, 43 insertions(+), 17 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 493d6cecde72..58b2ad27c5e2 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -283,6 +283,37 @@ int CameraDevice::initialize()\n> > return ret;\n> > }\n> >\n> > +std::vector CameraDevice::filterYUVResolutions(CameraConfiguration *cameraConfig,\n> > + const PixelFormat &pixelFormat,\n> > + const std::vector &resolutions)\n\nPass StreamConfiguration* rather than CameraConfiguration?\n\n> > +{\n> > + std::vector supportedResolutions;\n> > +\n> > + StreamConfiguration &cfg = cameraConfig->at(0);\n> > + for (const Size &res : resolutions) {\n> > + cfg.pixelFormat = pixelFormat;\n> > + cfg.size = res;\n> > +\n> > + std::stringstream ss;\n> > + ss << \"Tested (\" << res.toString() << \")[\"\n>\n> Testing became Tested ? :-)\n>\n> > + << pixelFormat.toString() << \"]: \";\n> > +\n> > + CameraConfiguration::Status status = cameraConfig->validate();\n> > + if (status != CameraConfiguration::Valid) {\n> > + ss << \" not supported\";\n> > + LOG(HAL, Debug) << ss.str();\n> > + continue;\n> > + }\n> > +\n> > + ss << \" supported\";\n> > + LOG(HAL, Debug) << ss.str();\n> > +\n> > + supportedResolutions.push_back(res);\n> > + }\n> > +\n> > + return supportedResolutions;\n> > +}\n> > +\n> > /*\n> > * Initialize the format conversion map to translate from Android format\n> > * identifier to libcamera pixel formats and fill in the list of supported\n> > @@ -435,24 +466,14 @@ int CameraDevice::initializeStreamConfigurations()\n> > << camera3Format.name << \" to: \"\n> > << mappedFormat.toString();\n> >\n> > - for (const Size &res : cameraResolutions) {\n> > - cfg.pixelFormat = mappedFormat;\n> > - cfg.size = res;\n> > -\n> > - std::stringstream ss;\n> > - ss << \"Testing (\" << res.toString() << \")[\"\n> > - << mappedFormat.toString() << \"]: \";\n> > -\n> > - CameraConfiguration::Status status = cameraConfig->validate();\n> > - if (status != CameraConfiguration::Valid) {\n> > - ss << \" not supported\";\n> > - LOG(HAL, Debug) << ss.str();\n> > - continue;\n> > - }\n> > -\n> > - ss << \" supported\";\n> > - LOG(HAL, Debug) << ss.str();\n> > + const PixelFormatInfo &info = PixelFormatInfo::info(mappedFormat);\n> > + if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> > + continue;\n>\n> I'd move this to patch 05/12, as it introduces a change not described in\n> the commit message (and breaks bisection unless I'm mistaken).\n>\n\nI would also add a comment description here about why we breakout in the case.\n\nReviewed-by: Hirokazu Honda \n> >\n> > + std::vector resolutions = filterYUVResolutions(cameraConfig.get(),\n> > + mappedFormat,\n> > + cameraResolutions);\n> > + for (const Size &res : resolutions) {\n> > streamConfigurations_.push_back({ res, androidFormat });\n> >\n> > /*\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index 3934f194f1b5..359a163ebab9 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -93,6 +93,11 @@ private:\n> > };\n> >\n> > int initializeStreamConfigurations();\n> > + std::vector\n> > + filterYUVResolutions(libcamera::CameraConfiguration *cameraConfig,\n> > + const libcamera::PixelFormat &pixelFormat,\n> > + const std::vector &resolutions);\n>\n> You can make this a static function.\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > +\n> > std::tuple calculateStaticMetadataSize();\n> > libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> > void notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 3EB42BDB1D\n\tfor ;\n\tMon, 7 Sep 2020 08:32:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C62DC62B58;\n\tMon, 7 Sep 2020 10:32:57 +0200 (CEST)","from mail-ed1-x544.google.com (mail-ed1-x544.google.com\n\t[IPv6:2a00:1450:4864:20::544])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C81FF629B6\n\tfor ;\n\tMon, 7 Sep 2020 10:32:56 +0200 (CEST)","by mail-ed1-x544.google.com with SMTP id t16so7131658edw.7\n\tfor ;\n\tMon, 07 Sep 2020 01:32:56 -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=\"Mac4jJ5n\"; 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=dS5rc7Kw5jmBszQjSnZVEOM/jMm/vEAV6uM/RK3ITXk=;\n\tb=Mac4jJ5nf1Foz9swZKPzlnU/dJH+poueBInHKuCWMcAC7PuBpwWgzAh/AJJNdyv4GT\n\t/iXZGplz11BBF/zWPJA7fYVVkhcEspyQIuatMsoju1ipEQ77vG+W9BKLO4wBLV+FLHXd\n\tdOn5PSL2sw85M1Qwz4fbWnAgXc5YwedEPsdGI=","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=dS5rc7Kw5jmBszQjSnZVEOM/jMm/vEAV6uM/RK3ITXk=;\n\tb=a8OQ70ujgAa2yhTQcmwyAMfCofQSLSlFetVm+sZwVOBU3hd9Ueqku0Dqsi7X5dqUUY\n\t+ebF4IBjJINPNIzRsXChj1fyq38oWkaCP0OW1jZD75lOWxlytk9f7+yWLXBhwf5Ydl/c\n\t+iTfvU5wmIpwmOnBz9PORR6rMIZYnLlI4rVhiHNBvvBOVYqYtdZ12frxb14SpDKG5iJA\n\tS/dv8n2gPuqkkCjh/10kvgF36LwIJcgLrjR154WZnNpzw3o24xz9t8gtUSjQyXZ16GB7\n\t+rCPSJ7vaRIb2wJBHXjZxJdpRs2MuWyYj1xaTczWFzQ6BqlXV9vxHyezvNTKSRmKAwoN\n\tN26w==","X-Gm-Message-State":"AOAM531Nvic1XWdjctYh5WlmWXIB69ypKJEXXfWLW5SvSKs+OR60MZJY\n\tIjcbft5u3+nOGX7jQ7KWSf0AI8C+/MY1sna/TLd8bw==","X-Google-Smtp-Source":"ABdhPJzFJ2knYsUjD1ix04o+kwhtRgP2fOHepGw7OSL+7mCIJB/0ANbbvXlJl7ml3uuolLSLAF1Yqova8oopM3hs774=","X-Received":"by 2002:aa7:d296:: with SMTP id\n\tw22mr20620470edq.327.1599467576429; \n\tMon, 07 Sep 2020 01:32:56 -0700 (PDT)","MIME-Version":"1.0","References":"<20200902152236.69770-1-jacopo@jmondi.org>\n\t<20200902152236.69770-5-jacopo@jmondi.org>\n\t<20200905180021.GG6319@pendragon.ideasonboard.com>","In-Reply-To":"<20200905180021.GG6319@pendragon.ideasonboard.com>","From":"Hirokazu Honda ","Date":"Mon, 7 Sep 2020 17:32:44 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 04/12] android: camera_device:\n\tBreak out size calculation","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":"tfiga@google.com, libcamera-devel@lists.libcamera.org,\n\tHirokazu Honda ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12352,"web_url":"https://patchwork.libcamera.org/comment/12352/","msgid":"<20200907090825.6l2vqw4blbxiuein@uno.localdomain>","date":"2020-09-07T09:08:25","subject":"Re: [libcamera-devel] [PATCH v2 04/12] android: camera_device:\n\tBreak out size calculation","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Mon, Sep 07, 2020 at 05:32:44PM +0900, Hirokazu Honda wrote:\n> On Sun, Sep 6, 2020 at 3:00 AM Laurent Pinchart\n> wrote:\n> >\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > On Wed, Sep 02, 2020 at 05:22:28PM +0200, Jacopo Mondi wrote:\n> > > As the RAW stream sizes needs to be calculated differently from the\n> > > processed one, break out break out the procedure to calculate the\n> >\n> > s/break out break out/break out/\n> >\n> > > processed (RGB/YUV) resolutions from initializeStreamConfigurations() in\n> > > order to prepare for RAW sizes calculation.\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/android/camera_device.cpp | 55 ++++++++++++++++++++++++-----------\n> > > src/android/camera_device.h | 5 ++++\n> > > 2 files changed, 43 insertions(+), 17 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 493d6cecde72..58b2ad27c5e2 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -283,6 +283,37 @@ int CameraDevice::initialize()\n> > > return ret;\n> > > }\n> > >\n> > > +std::vector CameraDevice::filterYUVResolutions(CameraConfiguration *cameraConfig,\n> > > + const PixelFormat &pixelFormat,\n> > > + const std::vector &resolutions)\n>\n> Pass StreamConfiguration* rather than CameraConfiguration?\n>\n\nBut I need to call ....\n> > > +{\n> > > + std::vector supportedResolutions;\n> > > +\n> > > + StreamConfiguration &cfg = cameraConfig->at(0);\n> > > + for (const Size &res : resolutions) {\n> > > + cfg.pixelFormat = pixelFormat;\n> > > + cfg.size = res;\n> > > +\n> > > + std::stringstream ss;\n> > > + ss << \"Tested (\" << res.toString() << \")[\"\n> >\n> > Testing became Tested ? :-)\n> >\n> > > + << pixelFormat.toString() << \"]: \";\n> > > +\n> > > + CameraConfiguration::Status status = cameraConfig->validate();\n\n\n.... validate here on the camera configuration :/\n\n> > > + if (status != CameraConfiguration::Valid) {\n> > > + ss << \" not supported\";\n> > > + LOG(HAL, Debug) << ss.str();\n> > > + continue;\n> > > + }\n> > > +\n> > > + ss << \" supported\";\n> > > + LOG(HAL, Debug) << ss.str();\n> > > +\n> > > + supportedResolutions.push_back(res);\n> > > + }\n> > > +\n> > > + return supportedResolutions;\n> > > +}\n> > > +\n> > > /*\n> > > * Initialize the format conversion map to translate from Android format\n> > > * identifier to libcamera pixel formats and fill in the list of supported\n> > > @@ -435,24 +466,14 @@ int CameraDevice::initializeStreamConfigurations()\n> > > << camera3Format.name << \" to: \"\n> > > << mappedFormat.toString();\n> > >\n> > > - for (const Size &res : cameraResolutions) {\n> > > - cfg.pixelFormat = mappedFormat;\n> > > - cfg.size = res;\n> > > -\n> > > - std::stringstream ss;\n> > > - ss << \"Testing (\" << res.toString() << \")[\"\n> > > - << mappedFormat.toString() << \"]: \";\n> > > -\n> > > - CameraConfiguration::Status status = cameraConfig->validate();\n> > > - if (status != CameraConfiguration::Valid) {\n> > > - ss << \" not supported\";\n> > > - LOG(HAL, Debug) << ss.str();\n> > > - continue;\n> > > - }\n> > > -\n> > > - ss << \" supported\";\n> > > - LOG(HAL, Debug) << ss.str();\n> > > + const PixelFormatInfo &info = PixelFormatInfo::info(mappedFormat);\n> > > + if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> > > + continue;\n> >\n> > I'd move this to patch 05/12, as it introduces a change not described in\n> > the commit message (and breaks bisection unless I'm mistaken).\n> >\n>\n> I would also add a comment description here about why we breakout in the case.\n\nWe would break bisection as this subtly introduces a behavioural change as\nno RAW resolution is reported anymore. It's not a code breakages but\nmore a process one, so I don't think it's worth a comment if I got\nyour comment right. And I'll try to make sure this does not happen in\nthe next version of the patch.\n\n>\n> Reviewed-by: Hirokazu Honda \n\nThanks\n j\n\n> > >\n> > > + std::vector resolutions = filterYUVResolutions(cameraConfig.get(),\n> > > + mappedFormat,\n> > > + cameraResolutions);\n> > > + for (const Size &res : resolutions) {\n> > > streamConfigurations_.push_back({ res, androidFormat });\n> > >\n> > > /*\n> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > index 3934f194f1b5..359a163ebab9 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -93,6 +93,11 @@ private:\n> > > };\n> > >\n> > > int initializeStreamConfigurations();\n> > > + std::vector\n> > > + filterYUVResolutions(libcamera::CameraConfiguration *cameraConfig,\n> > > + const libcamera::PixelFormat &pixelFormat,\n> > > + const std::vector &resolutions);\n> >\n> > You can make this a static function.\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > > +\n> > > std::tuple calculateStaticMetadataSize();\n> > > libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> > > void notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","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 642E2BDB1C\n\tfor ;\n\tMon, 7 Sep 2020 09:04:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0731A62BAE;\n\tMon, 7 Sep 2020 11:04:39 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9322D62B58\n\tfor ;\n\tMon, 7 Sep 2020 11:04:38 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 45F5C1BF210;\n\tMon, 7 Sep 2020 09:04:36 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 7 Sep 2020 11:08:25 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20200907090825.6l2vqw4blbxiuein@uno.localdomain>","References":"<20200902152236.69770-1-jacopo@jmondi.org>\n\t<20200902152236.69770-5-jacopo@jmondi.org>\n\t<20200905180021.GG6319@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 04/12] android: camera_device:\n\tBreak out size calculation","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":"tfiga@google.com, libcamera-devel@lists.libcamera.org,\n\tHirokazu Honda ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]