[{"id":20022,"web_url":"https://patchwork.libcamera.org/comment/20022/","msgid":"","date":"2021-10-04T03:03:06","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Check if Stream\n\tconfigurations were generated correctly","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Javier, thank you for the patch.\n\nOn Sat, Oct 2, 2021 at 6:16 AM Javier Martinez Canillas\n wrote:\n>\n> The libcamera Android Camera HAL generates camera configurations for the\n> StillCapture, Raw and ViewFinder stream roles. But there is only a check\n> if the configuration generation failed, for the StillCapture stream role.\n>\n> This could lead to a NULL pointer dereference if a pipeline handler fails\n> to generate a default configuration for one of the other two stream roles.\n>\n> Signed-off-by: Javier Martinez Canillas \n> ---\n>\n> Changes in v2:\n> - Fix typos in commit message.\n>\n> src/android/camera_capabilities.cpp | 13 ++++++++++++-\n> 1 file changed, 12 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index 87a6e1c6f26..baeedc11500 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -408,6 +408,11 @@ CameraCapabilities::initializeYUVResolutions(const PixelFormat &pixelFormat,\n> std::vector supportedResolutions;\n> std::unique_ptr cameraConfig =\n> camera_->generateConfiguration({ StreamRole::Viewfinder });\n> + if (!cameraConfig) {\n> + LOG(HAL, Error) << \"Failed to get supported YUV resolutions\";\n> + return supportedResolutions;\n> + }\n> +\n> StreamConfiguration &cfg = cameraConfig->at(0);\n>\n> for (const Size &res : resolutions) {\n> @@ -431,11 +436,17 @@ CameraCapabilities::initializeYUVResolutions(const PixelFormat &pixelFormat,\n> std::vector\n> CameraCapabilities::initializeRawResolutions(const PixelFormat &pixelFormat)\n> {\n> + std::vector supportedResolutions;\n> std::unique_ptr cameraConfig =\n> camera_->generateConfiguration({ StreamRole::Raw });\n> + if (!cameraConfig) {\n> + LOG(HAL, Error) << \"Failed to get supported Raw resolutions\";\n> + return supportedResolutions;\n\nreturn {} may work?\nSo declaring supportedResolution above is not necessary.\n\nWith the nit,\nReviewed-by: Hirokazu Honda \n\n> + }\n> +\n> StreamConfiguration &cfg = cameraConfig->at(0);\n> const StreamFormats &formats = cfg.formats();\n> - std::vector supportedResolutions = formats.sizes(pixelFormat);\n> + supportedResolutions = formats.sizes(pixelFormat);\n>\n> return supportedResolutions;\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 80498BDC71\n\tfor ;\n\tMon, 4 Oct 2021 03:03:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E74F8691B9;\n\tMon, 4 Oct 2021 05:03:15 +0200 (CEST)","from mail-ed1-x529.google.com (mail-ed1-x529.google.com\n\t[IPv6:2a00:1450:4864:20::529])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 77C396023E\n\tfor ;\n\tMon, 4 Oct 2021 05:03:14 +0200 (CEST)","by mail-ed1-x529.google.com with SMTP id p13so30909114edw.0\n\tfor ;\n\tSun, 03 Oct 2021 20:03:14 -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=\"fnISLgRw\"; 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=e5t3n4GkdEiCFogsEISnudFy3+yncTglZS1OORdoO1E=;\n\tb=fnISLgRwcXpbNNZ35ddOAWSCYyuN+UmGu9RDGkhVwmkl1L8I/DzZjPSg9bzT8Jv3+4\n\te7120Eb7RsEM4FOT48hnZ+U8hd+U9ALiIYwU08B19mZ1ei6xYBtLrL2NEcQKDsyQBdPz\n\tT2RdnylUpKsQhhe857Qtbp8weqPAz+JesSA7k=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=e5t3n4GkdEiCFogsEISnudFy3+yncTglZS1OORdoO1E=;\n\tb=z5q5Rla3ElNutwJzUfhnXePN1JA1ls9Q2zFktl0kviVSaEgvT0UPf7W4GUPkq6yp+Y\n\t+3uBsWXGnuPXNeEZ8HSqEIdR0OoRk3NoEe1Rioov/0Y2AbOoyRmYDkMc4pwJV+EBr6RN\n\tFyRo0PpgMJfkG552iQhlVgXPbC1DRnESQJXGQvAAJ/JpTy0u3wxYroO99lOPKiY8aCgi\n\twi8gmX2KCoL/Srz2uC/fhqj4WW3BZqizsKpEGxLKgvTOCOfRqLiEsotC3EIKxKziJrmz\n\txoZnM9UOcJ46T31ZEUO0eyUJj2q92/m9BdoWJNjaZeiTCGrbYrPYlL0L8IgHg8tCUhi/\n\tmwOg==","X-Gm-Message-State":"AOAM533gKJMIeG7dLlNfej8gcUGge7bb7urYbD2WC80M6SmwMBjGLkrE\n\tgqW7fNvTWqycp5KPRIWq3lqw0QUsYLjhWjMcfMDTHQ==","X-Google-Smtp-Source":"ABdhPJwW4dyGEnw9Vq+l7fhGFDG7s/SV43h1TkAB5OxEXqbh7Hj9p6LnfAR2NI6CL7+7urS7x1TgsXe/+/Smy62u6as=","X-Received":"by 2002:aa7:c143:: with SMTP id r3mr5451774edp.220.1633316593927;\n\tSun, 03 Oct 2021 20:03:13 -0700 (PDT)","MIME-Version":"1.0","References":"<20211001211525.1423725-1-javierm@redhat.com>\n\t<20211001211525.1423725-2-javierm@redhat.com>","In-Reply-To":"<20211001211525.1423725-2-javierm@redhat.com>","From":"Hirokazu Honda ","Date":"Mon, 4 Oct 2021 12:03:06 +0900","Message-ID":"","To":"Javier Martinez Canillas ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Check if Stream\n\tconfigurations were generated correctly","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":20030,"web_url":"https://patchwork.libcamera.org/comment/20030/","msgid":"<20211004084142.eyjf7p4rrb4c5m75@uno.localdomain>","date":"2021-10-04T08:41:42","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Check if Stream\n\tconfigurations were generated correctly","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Javier,\n\nOn Fri, Oct 01, 2021 at 11:15:25PM +0200, Javier Martinez Canillas wrote:\n> The libcamera Android Camera HAL generates camera configurations for the\n> StillCapture, Raw and ViewFinder stream roles. But there is only a check\n> if the configuration generation failed, for the StillCapture stream role.\n>\n> This could lead to a NULL pointer dereference if a pipeline handler fails\n> to generate a default configuration for one of the other two stream roles.\n\nI don't think that's strictly necessary.\n\nThe path towards which we get to genreate a config for a role is:\n\n for (androidFormat : AndroidRequiredFormats) {\n\n PixelFormat mappedFormat;\n for (libcameraFormat : androidFormat.compatibleFmts) {\n cameraConfig.pixelFormat = libcameraFormat;\n\n status = cameraConfig.validate()l\n if (status == ok) {\n mappedFormat = libameraFormat;\n break;\n }\n }\n\n if (!mappedFormat.valid())\n continue;\n\n if (mappedFormat == RAW)\n initializeRawResolutions()\n\n if (mappedFormat == YUV || mappedFormat == RGB)\n initializeYUVResolutions()\n }\n\nHence, if we get to generate a config for a role we know there's a\nsupported compatible format.\n\nHowever, as the association between roles and supported pixel formats\nis a little fuzzy in pipeline handlers, I guess the better safe than\nsorry approach is the correct one\n\n>\n> Signed-off-by: Javier Martinez Canillas \n\nReviewed-by: Jacopo Mondi \n\n> ---\n>\n> Changes in v2:\n> - Fix typos in commit message.\n>\n> src/android/camera_capabilities.cpp | 13 ++++++++++++-\n> 1 file changed, 12 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index 87a6e1c6f26..baeedc11500 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -408,6 +408,11 @@ CameraCapabilities::initializeYUVResolutions(const PixelFormat &pixelFormat,\n> \tstd::vector supportedResolutions;\n> \tstd::unique_ptr cameraConfig =\n> \t\tcamera_->generateConfiguration({ StreamRole::Viewfinder });\n> +\tif (!cameraConfig) {\n> +\t\tLOG(HAL, Error) << \"Failed to get supported YUV resolutions\";\n> +\t\treturn supportedResolutions;\n> +\t}\n> +\n> \tStreamConfiguration &cfg = cameraConfig->at(0);\n>\n> \tfor (const Size &res : resolutions) {\n> @@ -431,11 +436,17 @@ CameraCapabilities::initializeYUVResolutions(const PixelFormat &pixelFormat,\n> std::vector\n> CameraCapabilities::initializeRawResolutions(const PixelFormat &pixelFormat)\n> {\n> +\tstd::vector supportedResolutions;\n> \tstd::unique_ptr cameraConfig =\n> \t\tcamera_->generateConfiguration({ StreamRole::Raw });\n> +\tif (!cameraConfig) {\n> +\t\tLOG(HAL, Error) << \"Failed to get supported Raw resolutions\";\n> +\t\treturn supportedResolutions;\n> +\t}\n> +\n> \tStreamConfiguration &cfg = cameraConfig->at(0);\n> \tconst StreamFormats &formats = cfg.formats();\n> -\tstd::vector supportedResolutions = formats.sizes(pixelFormat);\n> +\tsupportedResolutions = formats.sizes(pixelFormat);\n\nHiro's right, not required, but I prefer your way as it makes\ninitializeRawResolutions more similar to initializeYUVResolutions\n\nUp to you!\n\nThanks\n j\n\n\n>\n> \treturn supportedResolutions;\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 6269DBDC71\n\tfor ;\n\tMon, 4 Oct 2021 08:40:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CFA51691B9;\n\tMon, 4 Oct 2021 10:40:57 +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 7EB98602DC\n\tfor ;\n\tMon, 4 Oct 2021 10:40:56 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 4D45F200008;\n\tMon, 4 Oct 2021 08:40:54 +0000 (UTC)"],"Date":"Mon, 4 Oct 2021 10:41:42 +0200","From":"Jacopo Mondi ","To":"Javier Martinez Canillas ","Message-ID":"<20211004084142.eyjf7p4rrb4c5m75@uno.localdomain>","References":"<20211001211525.1423725-1-javierm@redhat.com>\n\t<20211001211525.1423725-2-javierm@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211001211525.1423725-2-javierm@redhat.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Check if Stream\n\tconfigurations were generated correctly","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":20049,"web_url":"https://patchwork.libcamera.org/comment/20049/","msgid":"","date":"2021-10-05T06:04:18","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Check if Stream\n\tconfigurations were generated correctly","submitter":{"id":95,"url":"https://patchwork.libcamera.org/api/people/95/","name":"Javier Martinez Canillas","email":"javierm@redhat.com"},"content":"Thanks Hiro and Jacobo for the feedback.\n\nOn 10/4/21 10:41, Jacopo Mondi wrote:\n> Hi Javier,\n\n[snip]\n\n> \n> Hence, if we get to generate a config for a role we know there's a\n> supported compatible format.\n>\n\nAgreed, that's why I used \"could lead to...\" (I first wrote \"will lead\")\nbut the function should not attempt to dereference a pointer that may be\nNULL and rely on the caller to only call it when is safe.\n\n[snip]\n\n>> +\n>> \tStreamConfiguration &cfg = cameraConfig->at(0);\n>> \tconst StreamFormats &formats = cfg.formats();\n>> -\tstd::vector supportedResolutions = formats.sizes(pixelFormat);\n>> +\tsupportedResolutions = formats.sizes(pixelFormat);\n> \n> Hiro's right, not required, but I prefer your way as it makes\n> initializeRawResolutions more similar to initializeYUVResolutions\n>\n\nYes, I did if for consistency between the two functions. I don't have a\nstrong opinion really, I'm happy to change if you believe that's better.\n\nBest regards,","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 08051C3243\n\tfor ;\n\tTue, 5 Oct 2021 06:04:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4D01069189;\n\tTue, 5 Oct 2021 08:04:26 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 271986023D\n\tfor ;\n\tTue, 5 Oct 2021 08:04:24 +0200 (CEST)","from mail-wm1-f69.google.com (mail-wm1-f69.google.com\n\t[209.85.128.69]) (Using TLS) by relay.mimecast.com with ESMTP id\n\tus-mta-428-zNLqfBoCPeSswIEpeBUdcg-1; Tue, 05 Oct 2021 02:04:21 -0400","by mail-wm1-f69.google.com with SMTP id\n\t5-20020a1c00050000b02902e67111d9f0so8742795wma.4\n\tfor ;\n\tMon, 04 Oct 2021 23:04:21 -0700 (PDT)","from [192.168.1.101] ([92.176.231.106])\n\tby smtp.gmail.com with ESMTPSA id\n\tz5sm772044wmp.26.2021.10.04.23.04.18\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tMon, 04 Oct 2021 23:04:19 -0700 (PDT)"],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"OU+yJQQn\"; dkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=javierm@redhat.com"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1633413862;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=nt5I2UeBMQ73O3RYxZCtwPgRxu64r7F42ckzHN4Cfvc=;\n\tb=OU+yJQQndOa8ElyWKdLgrXWZHecNd1/9EksJX6Btqw6IH99RCuabLUUyRCSZnwImAShfFK\n\tVOJbli0zAiWKMBwmEFyHcTfC7or6BmVyl7mtv34m4nM2/ik95jlioc37ShhZ3/jL7fwTzQ\n\tMsuZlPOTdLFBMOLb61RVUr+Bg7yNhSU=","X-MC-Unique":"zNLqfBoCPeSswIEpeBUdcg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:message-id:date:mime-version:user-agent:subject\n\t:content-language:to:cc:references:from:in-reply-to\n\t:content-transfer-encoding;\n\tbh=nt5I2UeBMQ73O3RYxZCtwPgRxu64r7F42ckzHN4Cfvc=;\n\tb=v0e2YnQKpwHQMZILW6Z6M4D8IiS8lBlF/Vcgm5UusRJYLGQ2j6y59q/vCqjR8vESra\n\tFM/b2BfeAPxUMnVPcoXOGd6WWTwDvkAGJ8iGs7BbyCcFa35nGyVjq30fZrIXmD94kkk7\n\t4SeRZCO6fyyZO65mynilpZTKNbC0YTcxnj1GegHgtMK22J9s1erTjxlxebeYfgy8Cb/h\n\tcIyOcbUMnfgXV9lNjOdvTJ3TLiYJY4x1SCw9HbRN47Xy1GNO+sBAXqidPIfOdbOMsbd9\n\tfpO8vVIt6ofRcRXmH105sQSLdrQUixBYyGGT1VHiDRx7Q7Evz1fDx0qloj+xbgOSX5Lq\n\ts7Ew==","X-Gm-Message-State":"AOAM531aXQI7vG5R07xQmeRvWxAR1qryk8DTfj8UqUeGStcVkuo4Z0/Y\n\tOutiVeJIAilKZIqDWF4vtm4+3vwKmft0lWXmbRBDkrXF7NREltJek4fHGsT+2j3Tzz9V2uL6KqH\n\t/DwGA/Ew6KD8byaljmfXa84nLcwfZYojJzA==","X-Received":["by 2002:adf:dc0d:: with SMTP id\n\tt13mr18939733wri.158.1633413860058; \n\tMon, 04 Oct 2021 23:04:20 -0700 (PDT)","by 2002:adf:dc0d:: with SMTP id\n\tt13mr18939707wri.158.1633413859832; \n\tMon, 04 Oct 2021 23:04:19 -0700 (PDT)"],"X-Google-Smtp-Source":"ABdhPJyfCKkVN3rEDlwTR/OJvfMAMLijeRpXZKIQgMaTa6bCp0YVxe0ShDDTDKj5D3qongnI43zKfg==","Message-ID":"","Date":"Tue, 5 Oct 2021 08:04:18 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.1.0","To":"Jacopo Mondi ","References":"<20211001211525.1423725-1-javierm@redhat.com>\n\t<20211001211525.1423725-2-javierm@redhat.com>\n\t<20211004084142.eyjf7p4rrb4c5m75@uno.localdomain>","From":"Javier Martinez Canillas ","In-Reply-To":"<20211004084142.eyjf7p4rrb4c5m75@uno.localdomain>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Check if Stream\n\tconfigurations were generated correctly","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\" "}}]