[{"id":18367,"web_url":"https://patchwork.libcamera.org/comment/18367/","msgid":"","date":"2021-07-26T16:40:42","subject":"Re: [libcamera-devel] [PATCH v2 3/3] android: capabilities:\n\tCentralize RAW support check","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 Mon, Jul 26, 2021 at 05:31:00PM +0200, Jacopo Mondi wrote:\n> The validation of RAW stream support is performed in two different\n> places:\n> \n> - At initializeStreamConfigurations() time, by verifying that the\n> libcamera format associated with HAL_PIXEL_FORMAT_BLOB is a Raw format\n> and ensuring the Camera successfully validates it\n> - As initializeStaticMetadata() time by generating a CameraConfiguration\n> for the Raw stream role and ensuring it is a Raw format with a 16 bit\n> depth\n> \n> The first check is used to build the list of supported Raw stream\n> resolutions. The latter is used to register the\n> ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW capability.\n> \n> As building the list of supported Raw streams doesn't serve any\n> purpose if the RAW capability is not registered, centralize the Raw\n> format support verification at initializeStreamConfigurations() time by\n> ensuring the supported format is a Raw one with a 16 bit depth.\n> \n> Signed-off-by: Jacopo Mondi \n> Reviewed-by: Umang Jain \n> \n> ---\n> src/android/camera_capabilities.cpp | 25 ++++++++++---------------\n> src/android/camera_capabilities.h | 1 +\n> 2 files changed, 11 insertions(+), 15 deletions(-)\n> \n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index 15e54192adff..12ffda2ee0b2 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -122,6 +122,7 @@ int CameraCapabilities::initialize(std::shared_ptr camera,\n> \tcamera_ = camera;\n> \torientation_ = orientation;\n> \tfacing_ = facing;\n> +\trawStreamAvailable_ = false;\n> \n> \t/* Acquire the camera and initialize available stream configurations. */\n> \tint ret = camera_->acquire();\n> @@ -324,11 +325,16 @@ int CameraCapabilities::initializeStreamConfigurations()\n> \n> \t\tstd::vector resolutions;\n> \t\tconst PixelFormatInfo &info = PixelFormatInfo::info(mappedFormat);\n> -\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> +\t\t\tif (info.bitsPerPixel != 16)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\trawStreamAvailable_ = true;\n> \t\t\tresolutions = initializeRawResolutions(mappedFormat);\n> -\t\telse\n> +\t\t} else {\n> \t\t\tresolutions = initializeYUVResolutions(mappedFormat,\n> \t\t\t\t\t\t\t cameraResolutions);\n> +\t\t}\n\nWould a switch/case be more readable ?\n\n\t\tswitch (info.colourEncoding) {\n\t\tcase PixelFormatInfo::ColourEncodingRAW:\n\t\t\tif (info.bitsPerPixel != 16)\n\t\t\t\tcontinue;\n\n\t\t\trawStreamAvailable_ = true;\n\t\t\tresolutions = initializeRawResolutions(mappedFormat);\n\t\t\tbreak;\n\n\t\tcase PixelFormatInfo::ColourEncodingYUV:\n\t\tcase PixelFormatInfo::ColourEncodingRGB:\n\t\t\tresolutions = initializeYUVResolutions(mappedFormat,\n\t\t\t\t\t\t\t cameraResolutions);\n\t\t\tbreak;\n\t\t}\n\nThe initializeYUVResolutions() function name for RGB still bothers me a\nbit, but it's not the biggest deal.\n\n> \n> \t\tfor (const Size &res : resolutions) {\n> \t\t\tstreamConfigurations_.push_back({ res, androidFormat });\n> @@ -866,22 +872,11 @@ int CameraCapabilities::initializeStaticMetadata()\n> \t};\n> \n> \t/* Report if camera supports RAW. */\n> -\tbool rawStreamAvailable = false;\n> -\tstd::unique_ptr cameraConfig =\n> -\t\tcamera_->generateConfiguration({ StreamRole::Raw });\n> -\tif (cameraConfig && !cameraConfig->empty()) {\n> -\t\tconst PixelFormatInfo &info =\n> -\t\t\tPixelFormatInfo::info(cameraConfig->at(0).pixelFormat);\n> -\t\t/* Only advertise RAW support if RAW16 is possible. */\n> -\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW &&\n> -\t\t info.bitsPerPixel == 16) {\n> -\t\t\trawStreamAvailable = true;\n> +\tif (rawStreamAvailable_)\n> \t\t\tavailableCapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);\n> -\t\t}\n> -\t}\n> \n> \t/* Number of { RAW, YUV, JPEG } supported output streams */\n> -\tint32_t numOutStreams[] = { rawStreamAvailable, 2, 1 };\n> +\tint32_t numOutStreams[] = { rawStreamAvailable_, 2, 1 };\n> \tstaticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS,\n> \t\t\t\t numOutStreams);\n> \n> diff --git a/src/android/camera_capabilities.h b/src/android/camera_capabilities.h\n> index e7aa46c0a689..42a976d3b482 100644\n> --- a/src/android/camera_capabilities.h\n> +++ b/src/android/camera_capabilities.h\n> @@ -55,6 +55,7 @@ private:\n> \n> \tint facing_;\n> \tint orientation_;\n> +\tbool rawStreamAvailable_;\n> \n> \tstd::vector streamConfigurations_;\n> \tstd::map formatsMap_;","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 42E2FC0109\n\tfor ;\n\tMon, 26 Jul 2021 16:40:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A940B687C0;\n\tMon, 26 Jul 2021 18:40: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 D5CA3687AF\n\tfor ;\n\tMon, 26 Jul 2021 18:40: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 4AEA8332;\n\tMon, 26 Jul 2021 18:40:47 +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=\"o2Zps5mM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627317647;\n\tbh=3p00w+pohhrMiGJHBWgKEcf6pXiy6bHpxeY8R0ASuiM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=o2Zps5mMVcKsYSlwOHwzX9D696hD3oMCI6uK8aqd/UnUgmtyBeIKceQ6EIi2Q60B2\n\th9fOEw7Bv4MyUx6WQRe1E7DQNyjtKqmixtiZWtvZr6KhDS7EVBsqSlAdKGUbRYDHed\n\t8PgFGhQODpWHjmJBbA9orK5AOK+OFsRaeA+V4oWE=","Date":"Mon, 26 Jul 2021 19:40:42 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210726153100.27856-1-jacopo@jmondi.org>\n\t<20210726153100.27856-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210726153100.27856-4-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] android: capabilities:\n\tCentralize RAW support check","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":18380,"web_url":"https://patchwork.libcamera.org/comment/18380/","msgid":"<20210727110023.oz62bekdb4kjwnln@uno.localdomain>","date":"2021-07-27T11:00:23","subject":"Re: [libcamera-devel] [PATCH v2 3/3] android: capabilities:\n\tCentralize RAW support check","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Jul 26, 2021 at 07:40:42PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Mon, Jul 26, 2021 at 05:31:00PM +0200, Jacopo Mondi wrote:\n> > The validation of RAW stream support is performed in two different\n> > places:\n> >\n> > - At initializeStreamConfigurations() time, by verifying that the\n> > libcamera format associated with HAL_PIXEL_FORMAT_BLOB is a Raw format\n> > and ensuring the Camera successfully validates it\n> > - As initializeStaticMetadata() time by generating a CameraConfiguration\n> > for the Raw stream role and ensuring it is a Raw format with a 16 bit\n> > depth\n> >\n> > The first check is used to build the list of supported Raw stream\n> > resolutions. The latter is used to register the\n> > ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW capability.\n> >\n> > As building the list of supported Raw streams doesn't serve any\n> > purpose if the RAW capability is not registered, centralize the Raw\n> > format support verification at initializeStreamConfigurations() time by\n> > ensuring the supported format is a Raw one with a 16 bit depth.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > Reviewed-by: Umang Jain \n> >\n> > ---\n> > src/android/camera_capabilities.cpp | 25 ++++++++++---------------\n> > src/android/camera_capabilities.h | 1 +\n> > 2 files changed, 11 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > index 15e54192adff..12ffda2ee0b2 100644\n> > --- a/src/android/camera_capabilities.cpp\n> > +++ b/src/android/camera_capabilities.cpp\n> > @@ -122,6 +122,7 @@ int CameraCapabilities::initialize(std::shared_ptr camera,\n> > \tcamera_ = camera;\n> > \torientation_ = orientation;\n> > \tfacing_ = facing;\n> > +\trawStreamAvailable_ = false;\n> >\n> > \t/* Acquire the camera and initialize available stream configurations. */\n> > \tint ret = camera_->acquire();\n> > @@ -324,11 +325,16 @@ int CameraCapabilities::initializeStreamConfigurations()\n> >\n> > \t\tstd::vector resolutions;\n> > \t\tconst PixelFormatInfo &info = PixelFormatInfo::info(mappedFormat);\n> > -\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> > +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> > +\t\t\tif (info.bitsPerPixel != 16)\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\trawStreamAvailable_ = true;\n> > \t\t\tresolutions = initializeRawResolutions(mappedFormat);\n> > -\t\telse\n> > +\t\t} else {\n> > \t\t\tresolutions = initializeYUVResolutions(mappedFormat,\n> > \t\t\t\t\t\t\t cameraResolutions);\n> > +\t\t}\n>\n> Would a switch/case be more readable ?\n>\n\nYes indeed :)\n\n> \t\tswitch (info.colourEncoding) {\n> \t\tcase PixelFormatInfo::ColourEncodingRAW:\n> \t\t\tif (info.bitsPerPixel != 16)\n> \t\t\t\tcontinue;\n>\n> \t\t\trawStreamAvailable_ = true;\n> \t\t\tresolutions = initializeRawResolutions(mappedFormat);\n> \t\t\tbreak;\n>\n> \t\tcase PixelFormatInfo::ColourEncodingYUV:\n> \t\tcase PixelFormatInfo::ColourEncodingRGB:\n> \t\t\tresolutions = initializeYUVResolutions(mappedFormat,\n> \t\t\t\t\t\t\t cameraResolutions);\n> \t\t\tbreak;\n> \t\t}\n>\n> The initializeYUVResolutions() function name for RGB still bothers me a\n> bit, but it's not the biggest deal.\n>\n\nI can record in a comment that we allow so to support\nIMPLEMENTATION_DEFINED ?\n\nI would be in favour of dropping YUV from the function name, but what\nalternatives could I use ?\n\nThanks\n j\n\n> >\n> > \t\tfor (const Size &res : resolutions) {\n> > \t\t\tstreamConfigurations_.push_back({ res, androidFormat });\n> > @@ -866,22 +872,11 @@ int CameraCapabilities::initializeStaticMetadata()\n> > \t};\n> >\n> > \t/* Report if camera supports RAW. */\n> > -\tbool rawStreamAvailable = false;\n> > -\tstd::unique_ptr cameraConfig =\n> > -\t\tcamera_->generateConfiguration({ StreamRole::Raw });\n> > -\tif (cameraConfig && !cameraConfig->empty()) {\n> > -\t\tconst PixelFormatInfo &info =\n> > -\t\t\tPixelFormatInfo::info(cameraConfig->at(0).pixelFormat);\n> > -\t\t/* Only advertise RAW support if RAW16 is possible. */\n> > -\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW &&\n> > -\t\t info.bitsPerPixel == 16) {\n> > -\t\t\trawStreamAvailable = true;\n> > +\tif (rawStreamAvailable_)\n> > \t\t\tavailableCapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);\n> > -\t\t}\n> > -\t}\n> >\n> > \t/* Number of { RAW, YUV, JPEG } supported output streams */\n> > -\tint32_t numOutStreams[] = { rawStreamAvailable, 2, 1 };\n> > +\tint32_t numOutStreams[] = { rawStreamAvailable_, 2, 1 };\n> > \tstaticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS,\n> > \t\t\t\t numOutStreams);\n> >\n> > diff --git a/src/android/camera_capabilities.h b/src/android/camera_capabilities.h\n> > index e7aa46c0a689..42a976d3b482 100644\n> > --- a/src/android/camera_capabilities.h\n> > +++ b/src/android/camera_capabilities.h\n> > @@ -55,6 +55,7 @@ private:\n> >\n> > \tint facing_;\n> > \tint orientation_;\n> > +\tbool rawStreamAvailable_;\n> >\n> > \tstd::vector streamConfigurations_;\n> > \tstd::map formatsMap_;\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 F1F90C0109\n\tfor ;\n\tTue, 27 Jul 2021 10:59:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3AE5A687C3;\n\tTue, 27 Jul 2021 12:59:37 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1082C60272\n\tfor ;\n\tTue, 27 Jul 2021 12:59:35 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 7E8E6240003;\n\tTue, 27 Jul 2021 10:59:35 +0000 (UTC)"],"Date":"Tue, 27 Jul 2021 13:00:23 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210727110023.oz62bekdb4kjwnln@uno.localdomain>","References":"<20210726153100.27856-1-jacopo@jmondi.org>\n\t<20210726153100.27856-4-jacopo@jmondi.org>\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 3/3] android: capabilities:\n\tCentralize RAW support check","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\" "}}]