[{"id":17552,"web_url":"https://patchwork.libcamera.org/comment/17552/","msgid":"","date":"2021-06-15T06:10:01","subject":"Re: [libcamera-devel] [PATCH] android;\n\tcamera_device: Reset config_ if Camera::configure() fails","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent, thank you for the patch.\n\nOn Tue, Jun 15, 2021 at 9:21 AM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> The config_ pointer is reset in all error paths of the\n> CameraDevice::configureStreams() function, except when\n> Camera::configure() fails. Fix it by using a local unique pointer to\n> store the configuration until the end of the function, to avoid similar\n> issues in the future.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/android/camera_device.cpp | 17 ++++++++---------\n> 1 file changed, 8 insertions(+), 9 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index f3fd38690e63..f0b5f87bc38f 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1716,8 +1716,8 @@ int\n> CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> * Generate an empty configuration, and construct a\n> StreamConfiguration\n> * for each camera3_stream to add to it.\n> */\n> - config_ = camera_->generateConfiguration();\n> - if (!config_) {\n> + std::unique_ptr config =\n> camera_->generateConfiguration();\n> + if (!config) {\n> LOG(HAL, Error) << \"Failed to generate camera\n> configuration\";\n> return -EINVAL;\n> }\n> @@ -1855,29 +1855,27 @@ int\n> CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n>\n> sortCamera3StreamConfigs(streamConfigs, jpegStream);\n> for (const auto &streamConfig : streamConfigs) {\n> - config_->addConfiguration(streamConfig.config);\n> + config->addConfiguration(streamConfig.config);\n>\n> for (auto &stream : streamConfig.streams) {\n> streams_.emplace_back(this, stream.type,\n> stream.stream,\n> - config_->size() - 1);\n> + config->size() - 1);\n\n stream.stream->priv = static_cast *>(&streams_.back());\n\n }\n> }\n>\n> - switch (config_->validate()) {\n> + switch (config->validate()) {\n> case CameraConfiguration::Valid:\n> break;\n> case CameraConfiguration::Adjusted:\n> LOG(HAL, Info) << \"Camera configuration adjusted\";\n>\n> - for (const StreamConfiguration &cfg : *config_)\n> + for (const StreamConfiguration &cfg : *config)\n> LOG(HAL, Info) << \" - \" << cfg.toString();\n>\n> - config_.reset();\n> return -EINVAL;\n> case CameraConfiguration::Invalid:\n> LOG(HAL, Info) << \"Camera configuration invalid\";\n> - config_.reset();\n> return -EINVAL;\n> }\n>\n> @@ -1885,7 +1883,7 @@ int\n> CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> * Once the CameraConfiguration has been adjusted/validated\n> * it can be applied to the camera.\n> */\n> - int ret = camera_->configure(config_.get());\n> + int ret = camera_->configure(config.get());\n> if (ret) {\n> LOG(HAL, Error) << \"Failed to configure camera '\"\n> << camera_->id() << \"'\";\n> @@ -1905,6 +1903,7 @@ int\n> CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> }\n> }\n>\n> + config_ = std::move(config);\n> return 0;\n> }\n>\n>\nShall we also clear streams_ on failure?\n\nReviewed-by: Hirokazu Honda \n\n\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\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 96004C3218\n\tfor ;\n\tTue, 15 Jun 2021 06:10:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E4A0968944;\n\tTue, 15 Jun 2021 08:10:13 +0200 (CEST)","from mail-ej1-x636.google.com (mail-ej1-x636.google.com\n\t[IPv6:2a00:1450:4864:20::636])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E0A3C6892E\n\tfor ;\n\tTue, 15 Jun 2021 08:10:12 +0200 (CEST)","by mail-ej1-x636.google.com with SMTP id gt18so2029614ejc.11\n\tfor ;\n\tMon, 14 Jun 2021 23:10:12 -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=\"jxTsyzTQ\"; 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=5uSTX778ChbG3kFspeDtNCQhPzwdUFQ71l/L6uuEAYs=;\n\tb=jxTsyzTQ3qyzIVSbL8WK3iZlauqubHMP/TFgNKPBt4NTbJXRJDIRWQ1uQVloSlsHd3\n\tas/Etk54W1yMEzi7rHWvJKmmgnMCYk7E9IkTV+Ddp0GcXIP73gKORZKqKk9NM1MwDOlI\n\tZP4unIEfCh4PYbM6X/SN/GrkII9v+A4tTvkc8=","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=5uSTX778ChbG3kFspeDtNCQhPzwdUFQ71l/L6uuEAYs=;\n\tb=SzZYIbzVhhoyOjiC9/gKzOOCtQVQJBd6A66Y/UpSilJHiusRhqyH0KzL7VI4JyFoDr\n\tR/U8CoMnNHQOkd0eBRE8NEjNprJFC8aITqEDIteLagVu+sODpH9WeuWBoSTUShS2JsFv\n\trPAyhcSDFjnJCMlj8+Hnu12E8kP7mHNP+bKJELWARae1rro1jjhSA16WV9fOJR/I2nD0\n\tdEmjphMS2MwcZEu/RFBaJLKyLi1G5imCFer7Vrt8m9ytTOBBEYpaEG8XGq+1eOV2ZmdC\n\t2BH80UD1bQdnwqAX1a7xJSiqJPSf1tTdAQ24XqYmQqkcFLTCKZ4nrq7zmwn2VuzHZtDF\n\tqqkw==","X-Gm-Message-State":"AOAM533S4b6KWhVEd0XGcT04zf2u5lNmjJ53xdHVFiZSz3QZF8C/vtAw\n\tuTipRTYvfXRC9buYNkla6v3ON7037835EQnV/aj91YTgWM1OCw==","X-Google-Smtp-Source":"ABdhPJxxS7f0s9aA6LLYNGvVr9Pwb8y77UkyNRduXAbRliD5GNSPIbCDP1iZVgoSkDIqtViogA5ee0MDgSgOGZC4HBc=","X-Received":"by 2002:a17:906:ccc3:: with SMTP id\n\tot3mr818504ejb.475.1623737412411; \n\tMon, 14 Jun 2021 23:10:12 -0700 (PDT)","MIME-Version":"1.0","References":"<20210615002047.25393-1-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20210615002047.25393-1-laurent.pinchart@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Tue, 15 Jun 2021 15:10:01 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000acf54405c4c7d300\"","Subject":"Re: [libcamera-devel] [PATCH] android;\n\tcamera_device: Reset config_ if Camera::configure() fails","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":17554,"web_url":"https://patchwork.libcamera.org/comment/17554/","msgid":"","date":"2021-06-15T07:17:36","subject":"Re: [libcamera-devel] [PATCH] android;\n\tcamera_device: Reset config_ if Camera::configure() fails","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 6/15/21 5:50 AM, Laurent Pinchart wrote:\n> The config_ pointer is reset in all error paths of the\n> CameraDevice::configureStreams() function, except when\n> Camera::configure() fails. Fix it by using a local unique pointer to\n> store the configuration until the end of the function, to avoid similar\n> issues in the future.\n>\n> Signed-off-by: Laurent Pinchart \nReviewed-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 17 ++++++++---------\n> 1 file changed, 8 insertions(+), 9 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index f3fd38690e63..f0b5f87bc38f 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1716,8 +1716,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \t * Generate an empty configuration, and construct a StreamConfiguration\n> \t * for each camera3_stream to add to it.\n> \t */\n> -\tconfig_ = camera_->generateConfiguration();\n> -\tif (!config_) {\n> +\tstd::unique_ptr config = camera_->generateConfiguration();\n> +\tif (!config) {\n> \t\tLOG(HAL, Error) << \"Failed to generate camera configuration\";\n> \t\treturn -EINVAL;\n> \t}\n> @@ -1855,29 +1855,27 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \n> \tsortCamera3StreamConfigs(streamConfigs, jpegStream);\n> \tfor (const auto &streamConfig : streamConfigs) {\n> -\t\tconfig_->addConfiguration(streamConfig.config);\n> +\t\tconfig->addConfiguration(streamConfig.config);\n> \n> \t\tfor (auto &stream : streamConfig.streams) {\n> \t\t\tstreams_.emplace_back(this, stream.type, stream.stream,\n> -\t\t\t\t\t config_->size() - 1);\n> +\t\t\t\t\t config->size() - 1);\n> \t\t\tstream.stream->priv = static_cast(&streams_.back());\n> \t\t}\n> \t}\n> \n> -\tswitch (config_->validate()) {\n> +\tswitch (config->validate()) {\n> \tcase CameraConfiguration::Valid:\n> \t\tbreak;\n> \tcase CameraConfiguration::Adjusted:\n> \t\tLOG(HAL, Info) << \"Camera configuration adjusted\";\n> \n> -\t\tfor (const StreamConfiguration &cfg : *config_)\n> +\t\tfor (const StreamConfiguration &cfg : *config)\n> \t\t\tLOG(HAL, Info) << \" - \" << cfg.toString();\n> \n> -\t\tconfig_.reset();\n> \t\treturn -EINVAL;\n> \tcase CameraConfiguration::Invalid:\n> \t\tLOG(HAL, Info) << \"Camera configuration invalid\";\n> -\t\tconfig_.reset();\n> \t\treturn -EINVAL;\n> \t}\n> \n> @@ -1885,7 +1883,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \t * Once the CameraConfiguration has been adjusted/validated\n> \t * it can be applied to the camera.\n> \t */\n> -\tint ret = camera_->configure(config_.get());\n> +\tint ret = camera_->configure(config.get());\n> \tif (ret) {\n> \t\tLOG(HAL, Error) << \"Failed to configure camera '\"\n> \t\t\t\t<< camera_->id() << \"'\";\n> @@ -1905,6 +1903,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \t\t}\n> \t}\n> \n> +\tconfig_ = std::move(config);\n> \treturn 0;\n> }\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 B89B3C3218\n\tfor ;\n\tTue, 15 Jun 2021 07:17:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3E11E6892D;\n\tTue, 15 Jun 2021 09:17:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B58CC60299\n\tfor ;\n\tTue, 15 Jun 2021 09:17:40 +0200 (CEST)","from [192.168.0.107] (unknown [103.238.109.26])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B08B0436;\n\tTue, 15 Jun 2021 09:17:39 +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=\"c3MT2Rk6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623741460;\n\tbh=aRJC9MCNqoPbYMcrYx0uLNXEOzDUzNCQQto3KsVrDH8=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=c3MT2Rk6+sVJD1M0nAvOejYnKS9Eexeg7N4HDi5jHLZ9Mw/U6H7z4/TqbkEaBMB3A\n\td5ALnA1w7VyCdfIQFY+iwrngpyaC9o0Mo2AwvbY7WaEDrxsUMhBpRDW4ZwoQvoYU8o\n\tJghi1iQqSFEgC8JVxWr6p+EyeSKO9OabtxBrlqTY=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210615002047.25393-1-laurent.pinchart@ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Tue, 15 Jun 2021 12:47:36 +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":"<20210615002047.25393-1-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH] android;\n\tcamera_device: Reset config_ if Camera::configure() fails","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17847,"web_url":"https://patchwork.libcamera.org/comment/17847/","msgid":"","date":"2021-06-28T04:01:34","subject":"Re: [libcamera-devel] [PATCH] android;\n\tcamera_device: Reset config_ if Camera::configure() fails","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Tue, Jun 15, 2021 at 03:10:01PM +0900, Hirokazu Honda wrote:\n> On Tue, Jun 15, 2021 at 9:21 AM Laurent Pinchart wrote:\n> > The config_ pointer is reset in all error paths of the\n> > CameraDevice::configureStreams() function, except when\n> > Camera::configure() fails. Fix it by using a local unique pointer to\n> > store the configuration until the end of the function, to avoid similar\n> > issues in the future.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/android/camera_device.cpp | 17 ++++++++---------\n> > 1 file changed, 8 insertions(+), 9 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index f3fd38690e63..f0b5f87bc38f 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -1716,8 +1716,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > * Generate an empty configuration, and construct a StreamConfiguration\n> > * for each camera3_stream to add to it.\n> > */\n> > - config_ = camera_->generateConfiguration();\n> > - if (!config_) {\n> > + std::unique_ptr config = camera_->generateConfiguration();\n> > + if (!config) {\n> > LOG(HAL, Error) << \"Failed to generate camera configuration\";\n> > return -EINVAL;\n> > }\n> > @@ -1855,29 +1855,27 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> >\n> > sortCamera3StreamConfigs(streamConfigs, jpegStream);\n> > for (const auto &streamConfig : streamConfigs) {\n> > - config_->addConfiguration(streamConfig.config);\n> > + config->addConfiguration(streamConfig.config);\n> >\n> > for (auto &stream : streamConfig.streams) {\n> > streams_.emplace_back(this, stream.type, stream.stream,\n> > - config_->size() - 1);\n> > + config->size() - 1);\n> \n> stream.stream->priv = static_cast(&streams_.back());\n> \n> }\n> > }\n> >\n> > - switch (config_->validate()) {\n> > + switch (config->validate()) {\n> > case CameraConfiguration::Valid:\n> > break;\n> > case CameraConfiguration::Adjusted:\n> > LOG(HAL, Info) << \"Camera configuration adjusted\";\n> >\n> > - for (const StreamConfiguration &cfg : *config_)\n> > + for (const StreamConfiguration &cfg : *config)\n> > LOG(HAL, Info) << \" - \" << cfg.toString();\n> >\n> > - config_.reset();\n> > return -EINVAL;\n> > case CameraConfiguration::Invalid:\n> > LOG(HAL, Info) << \"Camera configuration invalid\";\n> > - config_.reset();\n> > return -EINVAL;\n> > }\n> >\n> > @@ -1885,7 +1883,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > * Once the CameraConfiguration has been adjusted/validated\n> > * it can be applied to the camera.\n> > */\n> > - int ret = camera_->configure(config_.get());\n> > + int ret = camera_->configure(config.get());\n> > if (ret) {\n> > LOG(HAL, Error) << \"Failed to configure camera '\"\n> > << camera_->id() << \"'\";\n> > @@ -1905,6 +1903,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > }\n> > }\n> >\n> > + config_ = std::move(config);\n> > return 0;\n> > }\n>\n> Shall we also clear streams_ on failure?\n\nPossibly, but we clear it at the beginning of\nCameraDevice::configureStreams(), so it should be fine.\n\nLooking at it, it seems that streams_ is only used in this function.\nShould it be turned into a local variable ? That's a candidate for a\nseparate patch.\n\n> Reviewed-by: Hirokazu Honda ","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 131B9C321A\n\tfor ;\n\tMon, 28 Jun 2021 04:01:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6BDE6684D5;\n\tMon, 28 Jun 2021 06:01:37 +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 4B2D86028C\n\tfor ;\n\tMon, 28 Jun 2021 06:01:36 +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 C7F66268;\n\tMon, 28 Jun 2021 06:01:35 +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=\"hTSG0gE0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624852896;\n\tbh=HvtL2RFXi4oruJsh/H/1r/KX02OlL9Z7xlHKDO8KY6Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hTSG0gE0KiwSbqfWzjF2u1Z3munlo+2ln8uF/VsG1r4NaFpQytdr2+mEpdOpbAVlw\n\tCbZE8B/VEHrWHAe3VBdJ/FO6PmjKOIG5xvS/7yMAy9xUi3J3kPu5Zz5926r6hnbj/P\n\txi5YJf62I21Rie+AcmhE6+eBdS8MT4ujYALTSpT0=","Date":"Mon, 28 Jun 2021 07:01:34 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210615002047.25393-1-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] android;\n\tcamera_device: Reset config_ if Camera::configure() fails","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":17848,"web_url":"https://patchwork.libcamera.org/comment/17848/","msgid":"","date":"2021-06-28T04:22:46","subject":"Re: [libcamera-devel] [PATCH] android;\n\tcamera_device: Reset config_ if Camera::configure() fails","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Mon, Jun 28, 2021 at 1:01 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Tue, Jun 15, 2021 at 03:10:01PM +0900, Hirokazu Honda wrote:\n> > On Tue, Jun 15, 2021 at 9:21 AM Laurent Pinchart wrote:\n> > > The config_ pointer is reset in all error paths of the\n> > > CameraDevice::configureStreams() function, except when\n> > > Camera::configure() fails. Fix it by using a local unique pointer to\n> > > store the configuration until the end of the function, to avoid similar\n> > > issues in the future.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/android/camera_device.cpp | 17 ++++++++---------\n> > > 1 file changed, 8 insertions(+), 9 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index f3fd38690e63..f0b5f87bc38f 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -1716,8 +1716,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > * Generate an empty configuration, and construct a StreamConfiguration\n> > > * for each camera3_stream to add to it.\n> > > */\n> > > - config_ = camera_->generateConfiguration();\n> > > - if (!config_) {\n> > > + std::unique_ptr config = camera_->generateConfiguration();\n> > > + if (!config) {\n> > > LOG(HAL, Error) << \"Failed to generate camera configuration\";\n> > > return -EINVAL;\n> > > }\n> > > @@ -1855,29 +1855,27 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > >\n> > > sortCamera3StreamConfigs(streamConfigs, jpegStream);\n> > > for (const auto &streamConfig : streamConfigs) {\n> > > - config_->addConfiguration(streamConfig.config);\n> > > + config->addConfiguration(streamConfig.config);\n> > >\n> > > for (auto &stream : streamConfig.streams) {\n> > > streams_.emplace_back(this, stream.type, stream.stream,\n> > > - config_->size() - 1);\n> > > + config->size() - 1);\n> >\n> > stream.stream->priv = static_cast(&streams_.back());\n> >\n> > }\n> > > }\n> > >\n> > > - switch (config_->validate()) {\n> > > + switch (config->validate()) {\n> > > case CameraConfiguration::Valid:\n> > > break;\n> > > case CameraConfiguration::Adjusted:\n> > > LOG(HAL, Info) << \"Camera configuration adjusted\";\n> > >\n> > > - for (const StreamConfiguration &cfg : *config_)\n> > > + for (const StreamConfiguration &cfg : *config)\n> > > LOG(HAL, Info) << \" - \" << cfg.toString();\n> > >\n> > > - config_.reset();\n> > > return -EINVAL;\n> > > case CameraConfiguration::Invalid:\n> > > LOG(HAL, Info) << \"Camera configuration invalid\";\n> > > - config_.reset();\n> > > return -EINVAL;\n> > > }\n> > >\n> > > @@ -1885,7 +1883,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > * Once the CameraConfiguration has been adjusted/validated\n> > > * it can be applied to the camera.\n> > > */\n> > > - int ret = camera_->configure(config_.get());\n> > > + int ret = camera_->configure(config.get());\n> > > if (ret) {\n> > > LOG(HAL, Error) << \"Failed to configure camera '\"\n> > > << camera_->id() << \"'\";\n> > > @@ -1905,6 +1903,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > }\n> > > }\n> > >\n> > > + config_ = std::move(config);\n> > > return 0;\n> > > }\n> >\n> > Shall we also clear streams_ on failure?\n>\n> Possibly, but we clear it at the beginning of\n> CameraDevice::configureStreams(), so it should be fine.\n>\n> Looking at it, it seems that streams_ is only used in this function.\n> Should it be turned into a local variable ? That's a candidate for a\n> separate patch.\n>\n\nNo, if configureStreams() is successful, streams_ will be used in\nother places (e.g. processCaptureRequest()) as\ncamera3_stream_buffer_t.stream->priv.\nWe couldn't make it a local variable.\n\n-Hiro\n> > Reviewed-by: Hirokazu Honda \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 849A3C321D\n\tfor ;\n\tMon, 28 Jun 2021 04:22:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BEDBD684D5;\n\tMon, 28 Jun 2021 06:22:58 +0200 (CEST)","from mail-ed1-x52e.google.com (mail-ed1-x52e.google.com\n\t[IPv6:2a00:1450:4864:20::52e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1221F6028C\n\tfor ;\n\tMon, 28 Jun 2021 06:22:57 +0200 (CEST)","by mail-ed1-x52e.google.com with SMTP id i24so23703869edx.4\n\tfor ;\n\tSun, 27 Jun 2021 21:22:57 -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=\"iE6snKiV\"; 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=2LiYQSg+ayyGZt4dt5+d/TOptRq7BYo9fj+MW6W5cDw=;\n\tb=iE6snKiVLKbabH7TMTFPfAOntpTIdohxaNQpPl0IurV2/LmXnsN50xBWGLn6u/vW0y\n\tmyiYFkzDjUUd47BkvcXb850QAeNZQq502s1zIVz8rq7E97S/odmGb6D57+T410rsxHpT\n\twfeFWvc1xmNkuM/lIm7RHfyzr25AQeHuzSsmQ=","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=2LiYQSg+ayyGZt4dt5+d/TOptRq7BYo9fj+MW6W5cDw=;\n\tb=Ty5681ZrXT1yYZ6WjGdEVOC7MvXrzu5Q38EqI9gZMC85hQPx87IIfa69e/X7LDOPyj\n\tFbgNxtpJUr6DBCv6SRZSLSFOkJUplXmaDF1ecR0uYMs0fdX+SFE0Vu4ruKHK8z/TqRek\n\tzVIKDIuZ/92TOu8Tclp28G7mXBU5ZmwBxYWJ6dpIZSS9Ihcg7mLjjmJyKzl9tjVrhUDw\n\tG05yK1QJ9ML6cokkjB/S8KNKXI561IVWCs6sghv8qls435xxLBLpXSqqXVRNE/iGVqGJ\n\ts+yrDsxP9sWuN8c2VnQUKOpLi9MvEHk1qIpGzrjTXUczNXAYMJ3naL8bAM4L3dAhTvwm\n\tX/Pg==","X-Gm-Message-State":"AOAM533/dMTO1LKJbwX3NPSsj938eoN+W2RQ4UbnjG43lTT9p4NXkXwv\n\tm+ltlArlwbnZYFDm4pJfD5pF0ALQGHkfuBhr3xEPhQ==","X-Google-Smtp-Source":"ABdhPJwxRz/qIIvpqFoQFSc0GpsLCXM7s9lmstjRf/NxPWRKhysQzgE0RjE10UWU8ZwtHqprqe/ALHJLJA8cezWUWK4=","X-Received":"by 2002:a05:6402:3548:: with SMTP id\n\tf8mr30276861edd.116.1624854176633; \n\tSun, 27 Jun 2021 21:22:56 -0700 (PDT)","MIME-Version":"1.0","References":"<20210615002047.25393-1-laurent.pinchart@ideasonboard.com>\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 28 Jun 2021 13:22:46 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] android;\n\tcamera_device: Reset config_ if Camera::configure() fails","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":17854,"web_url":"https://patchwork.libcamera.org/comment/17854/","msgid":"","date":"2021-06-28T04:32:56","subject":"Re: [libcamera-devel] [PATCH] android;\n\tcamera_device: Reset config_ if Camera::configure() fails","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Mon, Jun 28, 2021 at 01:22:46PM +0900, Hirokazu Honda wrote:\n> On Mon, Jun 28, 2021 at 1:01 PM Laurent Pinchart wrote:\n> > On Tue, Jun 15, 2021 at 03:10:01PM +0900, Hirokazu Honda wrote:\n> > > On Tue, Jun 15, 2021 at 9:21 AM Laurent Pinchart wrote:\n> > > > The config_ pointer is reset in all error paths of the\n> > > > CameraDevice::configureStreams() function, except when\n> > > > Camera::configure() fails. Fix it by using a local unique pointer to\n> > > > store the configuration until the end of the function, to avoid similar\n> > > > issues in the future.\n> > > >\n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > src/android/camera_device.cpp | 17 ++++++++---------\n> > > > 1 file changed, 8 insertions(+), 9 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index f3fd38690e63..f0b5f87bc38f 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -1716,8 +1716,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > > * Generate an empty configuration, and construct a StreamConfiguration\n> > > > * for each camera3_stream to add to it.\n> > > > */\n> > > > - config_ = camera_->generateConfiguration();\n> > > > - if (!config_) {\n> > > > + std::unique_ptr config = camera_->generateConfiguration();\n> > > > + if (!config) {\n> > > > LOG(HAL, Error) << \"Failed to generate camera configuration\";\n> > > > return -EINVAL;\n> > > > }\n> > > > @@ -1855,29 +1855,27 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > >\n> > > > sortCamera3StreamConfigs(streamConfigs, jpegStream);\n> > > > for (const auto &streamConfig : streamConfigs) {\n> > > > - config_->addConfiguration(streamConfig.config);\n> > > > + config->addConfiguration(streamConfig.config);\n> > > >\n> > > > for (auto &stream : streamConfig.streams) {\n> > > > streams_.emplace_back(this, stream.type, stream.stream,\n> > > > - config_->size() - 1);\n> > > > + config->size() - 1);\n> > >\n> > > stream.stream->priv = static_cast(&streams_.back());\n> > >\n> > > }\n> > > > }\n> > > >\n> > > > - switch (config_->validate()) {\n> > > > + switch (config->validate()) {\n> > > > case CameraConfiguration::Valid:\n> > > > break;\n> > > > case CameraConfiguration::Adjusted:\n> > > > LOG(HAL, Info) << \"Camera configuration adjusted\";\n> > > >\n> > > > - for (const StreamConfiguration &cfg : *config_)\n> > > > + for (const StreamConfiguration &cfg : *config)\n> > > > LOG(HAL, Info) << \" - \" << cfg.toString();\n> > > >\n> > > > - config_.reset();\n> > > > return -EINVAL;\n> > > > case CameraConfiguration::Invalid:\n> > > > LOG(HAL, Info) << \"Camera configuration invalid\";\n> > > > - config_.reset();\n> > > > return -EINVAL;\n> > > > }\n> > > >\n> > > > @@ -1885,7 +1883,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > > * Once the CameraConfiguration has been adjusted/validated\n> > > > * it can be applied to the camera.\n> > > > */\n> > > > - int ret = camera_->configure(config_.get());\n> > > > + int ret = camera_->configure(config.get());\n> > > > if (ret) {\n> > > > LOG(HAL, Error) << \"Failed to configure camera '\"\n> > > > << camera_->id() << \"'\";\n> > > > @@ -1905,6 +1903,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > > }\n> > > > }\n> > > >\n> > > > + config_ = std::move(config);\n> > > > return 0;\n> > > > }\n> > >\n> > > Shall we also clear streams_ on failure?\n> >\n> > Possibly, but we clear it at the beginning of\n> > CameraDevice::configureStreams(), so it should be fine.\n> >\n> > Looking at it, it seems that streams_ is only used in this function.\n> > Should it be turned into a local variable ? That's a candidate for a\n> > separate patch.\n> \n> No, if configureStreams() is successful, streams_ will be used in\n> other places (e.g. processCaptureRequest()) as\n> camera3_stream_buffer_t.stream->priv.\n> We couldn't make it a local variable.\n\nOf course. My bad, sorry.\n\n> > > Reviewed-by: Hirokazu Honda ","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 44A25C321D\n\tfor ;\n\tMon, 28 Jun 2021 04:33:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0768B684D5;\n\tMon, 28 Jun 2021 06:33:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 034726028C\n\tfor ;\n\tMon, 28 Jun 2021 06:32:58 +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 8649FB8A;\n\tMon, 28 Jun 2021 06:32:57 +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=\"IVn7Vg4T\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624854777;\n\tbh=0YoFBn0dRkF8metolJiwP+BbsObrkieTNXaOVvFtAcc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IVn7Vg4TVCZoUP4Ufus8vnWiHECdbWqeIt094+0l2FmxTsiigCUVH8uaUyzOxhHep\n\tXmVRhf2nlOv/JMmvx+cb0pwzlRKbcVyEGTnXzEFb3GlDpUdvs+GP1QgGDLtbWpwX1S\n\tOM33nsGcljHaR4A+vSVfQfG0TlcewbTT/pvSqSyU=","Date":"Mon, 28 Jun 2021 07:32:56 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210615002047.25393-1-laurent.pinchart@ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] android;\n\tcamera_device: Reset config_ if Camera::configure() fails","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\" "}}]