[{"id":18381,"web_url":"https://patchwork.libcamera.org/comment/18381/","msgid":"","date":"2021-07-27T14:56:49","subject":"Re: [libcamera-devel] [PATCH v3 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 Tue, Jul 27, 2021 at 01:05:19PM +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> src/android/camera_capabilities.cpp | 37 +++++++++++++++++------------\n> src/android/camera_capabilities.h | 1 +\n> 2 files changed, 23 insertions(+), 15 deletions(-)\n> \n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index 15e54192adff..709bfb2a4a19 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,28 @@ 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\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\telse\n> +\t\t\tbreak;\n> +\n> +\t\tcase PixelFormatInfo::ColourEncodingYUV:\n> +\t\tcase PixelFormatInfo::ColourEncodingRGB:\n> +\t\t\t/*\n> +\t\t\t * We support enumerating RGB streams here to allow\n> +\t\t\t * mapping IMPLEMENTATION_DEFINED format to RGB.\n> +\t\t\t */\n\nThanks for the comment, that makes me feel better.\n\n> \t\t\tresolutions = initializeYUVResolutions(mappedFormat,\n> \t\t\t\t\t\t\t cameraResolutions);\n> +\t\t\tbreak;\n> +\n> +\t\tdefault:\n> +\t\t\tcontinue;\n\nDo you get a warning if you don't add the default case ? If not, I'd\ndrop it, in order to get a warning if we ever add another colour\nencoding.\n\n> +\t\t}\n> \n> \t\tfor (const Size &res : resolutions) {\n> \t\t\tstreamConfigurations_.push_back({ res, androidFormat });\n> @@ -866,22 +884,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\nWith one less indentation level,\n\nReviewed-by: Laurent Pinchart \n\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 8735FC0109\n\tfor ;\n\tTue, 27 Jul 2021 14:56:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D6D80687C4;\n\tTue, 27 Jul 2021 16:56:56 +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 DD1D560272\n\tfor ;\n\tTue, 27 Jul 2021 16:56:55 +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 56A6EEE;\n\tTue, 27 Jul 2021 16:56:55 +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=\"ein/ceYF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627397815;\n\tbh=NPJC1ED+dEoIKi6w7MC7Lb4WgmVW8ajZlOvDWzD4fXQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ein/ceYFrb+6ZJVX78ueCGf/jqnzeHcT8g7eybAqPLRHh2vP/d5S3mrzZUS12SWou\n\tvWWYTYH0cMg2tmR5nQK2z3ef+VxpyLJ3lKueVfWUl6TsNFthuLGmdyzHEEcDSJXyj9\n\t0Nbo8yosOBNxudebzW9+fChhmiS0ElsqvQBu+oNo=","Date":"Tue, 27 Jul 2021 17:56:49 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210727110519.11587-1-jacopo@jmondi.org>\n\t<20210727110519.11587-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210727110519.11587-4-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 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":18384,"web_url":"https://patchwork.libcamera.org/comment/18384/","msgid":"<20210727153634.d3j5ie3sn7tm5osx@uno.localdomain>","date":"2021-07-27T15:36:34","subject":"Re: [libcamera-devel] [PATCH v3 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 Tue, Jul 27, 2021 at 05:56:49PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Tue, Jul 27, 2021 at 01:05:19PM +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> > src/android/camera_capabilities.cpp | 37 +++++++++++++++++------------\n> > src/android/camera_capabilities.h | 1 +\n> > 2 files changed, 23 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > index 15e54192adff..709bfb2a4a19 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,28 @@ 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\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\telse\n> > +\t\t\tbreak;\n> > +\n> > +\t\tcase PixelFormatInfo::ColourEncodingYUV:\n> > +\t\tcase PixelFormatInfo::ColourEncodingRGB:\n> > +\t\t\t/*\n> > +\t\t\t * We support enumerating RGB streams here to allow\n> > +\t\t\t * mapping IMPLEMENTATION_DEFINED format to RGB.\n> > +\t\t\t */\n>\n> Thanks for the comment, that makes me feel better.\n>\n\n:)\n\n> > \t\t\tresolutions = initializeYUVResolutions(mappedFormat,\n> > \t\t\t\t\t\t\t cameraResolutions);\n> > +\t\t\tbreak;\n> > +\n> > +\t\tdefault:\n> > +\t\t\tcontinue;\n>\n> Do you get a warning if you don't add the default case ? If not, I'd\n> drop it, in order to get a warning if we ever add another colour\n> encoding.\n>\n\nNo I don't, but I thought it would be cleaner to just continue, as\n'resolutions' would be empty and there's no point in going to the\nfollowing loop. But your argument about getting a warning if we forget\na format is more solid, so I'll drop\n\n> > +\t\t}\n> >\n> > \t\tfor (const Size &res : resolutions) {\n> > \t\t\tstreamConfigurations_.push_back({ res, androidFormat });\n> > @@ -866,22 +884,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>\n> With one less indentation level,\n>\n\nOuch, I had missed it, sorry!\n\n> Reviewed-by: Laurent Pinchart \n\nThanks\n j\n\n>\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 07715C0109\n\tfor ;\n\tTue, 27 Jul 2021 15:35:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E581687B6;\n\tTue, 27 Jul 2021 17:35:49 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 88FF960272\n\tfor ;\n\tTue, 27 Jul 2021 17:35:47 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 0E5D12000D;\n\tTue, 27 Jul 2021 15:35:46 +0000 (UTC)"],"Date":"Tue, 27 Jul 2021 17:36:34 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210727153634.d3j5ie3sn7tm5osx@uno.localdomain>","References":"<20210727110519.11587-1-jacopo@jmondi.org>\n\t<20210727110519.11587-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 v3 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\" "}}]