[{"id":16008,"web_url":"https://patchwork.libcamera.org/comment/16008/","msgid":"<20210329091343.3kdevw7fdgfaz56n@uno.localdomain>","date":"2021-03-29T09:13:43","subject":"Re: [libcamera-devel] [PATCH v3 2/2] android: CameraDevice: Add\n\tstop()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Mon, Mar 29, 2021 at 03:11:19PM +0900, Hirokazu Honda wrote:\n> This adds CameraDevice::stop(), which cleans up the member\n> variables of CameraDevice. It is called in CameraDevice::close()\n> and CameraDevice::configureStreams().\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/camera_device.cpp | 17 +++++++++--------\n> src/android/camera_device.h | 2 ++\n> 2 files changed, 11 insertions(+), 8 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 02d3bfb2..d5447d8e 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -659,12 +659,17 @@ int CameraDevice::open(const hw_module_t *hardwareModule)\n>\n> void CameraDevice::close()\n> {\n> -\tstreams_.clear();\n> +\tstop();\n> +\n> +\tcamera_->release();\n> +}\n>\n> +void CameraDevice::stop()\n> +{\n> \tworker_.stop();\n> \tcamera_->stop();\n\nI think the camera shall be stopped only if (running_). Otherwise\nyou'll get an error message from the Camera state maching if I'm not\nmistaken.\n\n> -\tcamera_->release();\n>\n> +\tdescriptors_.clear();\n\nIdeally, this should be done as:\n1) Introduce stop() in CameraDevice\n2) Introduce descriptors_ and clear it in stop()\n\nAs this bundle two changes in one patch.\n\nA minor though.\n\nThe patch is fine, but I think the discussion with Laurent on what has\ntriggered this issue in first place is still a bit on-going, right ?\n\n> \trunning_ = false;\n> }\n>\n> @@ -1547,12 +1552,8 @@ PixelFormat CameraDevice::toPixelFormat(int format) const\n> */\n> int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> {\n> -\t/* Before any configuration attempt, stop the camera if it's running. */\n> -\tif (running_) {\n> -\t\tworker_.stop();\n> -\t\tcamera_->stop();\n> -\t\trunning_ = false;\n> -\t}\n> +\t/* Before any configuration attempt, stop the camera. */\n> +\tstop();\n>\n> \t/*\n> \t * Generate an empty configuration, and construct a StreamConfiguration\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 5a2d22d6..cc12fe20 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -87,6 +87,8 @@ private:\n> \t\tint androidFormat;\n> \t};\n>\n> +\tvoid stop();\n> +\n> \tint initializeStreamConfigurations();\n> \tstd::vector\n> \tgetYUVResolutions(libcamera::CameraConfiguration *cameraConfig,\n> --\n> 2.31.0.291.g576ba9dcdaf-goog\n>\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 9B9D6C32EF\n\tfor ;\n\tMon, 29 Mar 2021 09:13:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C62EE68783;\n\tMon, 29 Mar 2021 11:13:10 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 327566877E\n\tfor ;\n\tMon, 29 Mar 2021 11:13:09 +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 relay6-d.mail.gandi.net (Postfix) with ESMTPSA id A88EAC0007;\n\tMon, 29 Mar 2021 09:13:08 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 29 Mar 2021 11:13:43 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210329091343.3kdevw7fdgfaz56n@uno.localdomain>","References":"<20210329061119.135513-1-hiroh@chromium.org>\n\t<20210329061119.135513-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210329061119.135513-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] android: CameraDevice: Add\n\tstop()","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16052,"web_url":"https://patchwork.libcamera.org/comment/16052/","msgid":"","date":"2021-03-30T05:41:30","subject":"Re: [libcamera-devel] [PATCH v3 2/2] android: CameraDevice: Add\n\tstop()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi, Jacopo, thanks for reviewing.\n\nOn Mon, Mar 29, 2021 at 6:13 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Mon, Mar 29, 2021 at 03:11:19PM +0900, Hirokazu Honda wrote:\n> > This adds CameraDevice::stop(), which cleans up the member\n> > variables of CameraDevice. It is called in CameraDevice::close()\n> > and CameraDevice::configureStreams().\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/android/camera_device.cpp | 17 +++++++++--------\n> > src/android/camera_device.h | 2 ++\n> > 2 files changed, 11 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 02d3bfb2..d5447d8e 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -659,12 +659,17 @@ int CameraDevice::open(const hw_module_t *hardwareModule)\n> >\n> > void CameraDevice::close()\n> > {\n> > - streams_.clear();\n> > + stop();\n> > +\n> > + camera_->release();\n> > +}\n> >\n> > +void CameraDevice::stop()\n> > +{\n> > worker_.stop();\n> > camera_->stop();\n>\n> I think the camera shall be stopped only if (running_). Otherwise\n> you'll get an error message from the Camera state maching if I'm not\n> mistaken.\n>\n> > - camera_->release();\n> >\n> > + descriptors_.clear();\n>\n> Ideally, this should be done as:\n> 1) Introduce stop() in CameraDevice\n> 2) Introduce descriptors_ and clear it in stop()\n>\n> As this bundle two changes in one patch.\n>\n> A minor though.\n>\n\nOkay, I reorder the patches to first one of adding stop() and the\nsecond one of fixing leakage.\n\n\n> The patch is fine, but I think the discussion with Laurent on what has\n> triggered this issue in first place is still a bit on-going, right ?\n>\n\nWe agree, regardless of issues, this patch should go as avoiding the leakage.\nThe issue is trivial, and we are discussing to select the most proper\nway of fixing it.\nThat is, we shall merge this patch without minding the discussion.\n\nRegards,\n-Hiro\n> > running_ = false;\n> > }\n> >\n> > @@ -1547,12 +1552,8 @@ PixelFormat CameraDevice::toPixelFormat(int format) const\n> > */\n> > int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > {\n> > - /* Before any configuration attempt, stop the camera if it's running. */\n> > - if (running_) {\n> > - worker_.stop();\n> > - camera_->stop();\n> > - running_ = false;\n> > - }\n> > + /* Before any configuration attempt, stop the camera. */\n> > + stop();\n> >\n> > /*\n> > * Generate an empty configuration, and construct a StreamConfiguration\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index 5a2d22d6..cc12fe20 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -87,6 +87,8 @@ private:\n> > int androidFormat;\n> > };\n> >\n> > + void stop();\n> > +\n> > int initializeStreamConfigurations();\n> > std::vector\n> > getYUVResolutions(libcamera::CameraConfiguration *cameraConfig,\n> > --\n> > 2.31.0.291.g576ba9dcdaf-goog\n> >\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 E64A6C32F0\n\tfor ;\n\tTue, 30 Mar 2021 05:41:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 46A1A68782;\n\tTue, 30 Mar 2021 07:41:45 +0200 (CEST)","from mail-ed1-x530.google.com (mail-ed1-x530.google.com\n\t[IPv6:2a00:1450:4864:20::530])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 510AD602D7\n\tfor ;\n\tTue, 30 Mar 2021 07:41:43 +0200 (CEST)","by mail-ed1-x530.google.com with SMTP id o19so16726246edc.3\n\tfor ;\n\tMon, 29 Mar 2021 22:41:43 -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=\"JzcamAnW\"; 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=EtuaZV7O+0M46Tv/og6D37vE75jJQxjZKM1DpXZpo84=;\n\tb=JzcamAnWrb5J9lOd3Hvjx6R5kcHEKstUQ8DWUB52yx3dP2Mwl0IH6MsbLkJqcbWSmx\n\tpjERC5W3SIdQwWSYnAxPibydQ9ix04M7mObPX2fbgdt7pDUfiImO5OCiJ2jJj67f+wcD\n\tnnP9YwkiuTVNH5pN1+QtJ29mAxlMue2XSgf2U=","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=EtuaZV7O+0M46Tv/og6D37vE75jJQxjZKM1DpXZpo84=;\n\tb=r8TaopRTzSLXS33kOBrcsJ1yZ39QPrLM8ZRS9lYbrRMG9Tuiaq3S21xvWVenOTne1U\n\tfMkIzL+IeAGMeqcCEQOf9acCEFNy5puRrVGVJKxnL1TMf5SlLitF3t56+mCxt/gPP8EE\n\tig0fUn33J423NYj4BkYahJyFDofiH8L5c+EDal+acry4JTci6/whuTVXmicRGbRrqG8d\n\tZlniKZEgQov2zeP2gwmfgjxfbpq5IkJ9KasVSga5rEEaGv3PdvMSaAP237+km2VQmsRZ\n\ta+dOE1ELCGl7pM1+r4dt5MO0B7gE9PCcNSNEv7g9vf7mDX62BHSypqnd6aWtCO5DoVGL\n\ttNXQ==","X-Gm-Message-State":"AOAM531HLtbfX16cZ14C3M7QyDB8ogXy8E+meFRgD2OV4kJtklSmQS6L\n\tqvBeAPeRHY8KyTXUoYAXEWhlYVXXrpWkQRZgAHAksg==","X-Google-Smtp-Source":"ABdhPJzS4/Qa4l9LlbelF6xS3IiIaETR6mPCDCCmkP8XklnjO41nEfRaWZm6aWmziX0Ih4Z1GyG3sIopOq7AbqTCZhk=","X-Received":"by 2002:aa7:c4cc:: with SMTP id\n\tp12mr31251074edr.325.1617082903001; \n\tMon, 29 Mar 2021 22:41:43 -0700 (PDT)","MIME-Version":"1.0","References":"<20210329061119.135513-1-hiroh@chromium.org>\n\t<20210329061119.135513-3-hiroh@chromium.org>\n\t<20210329091343.3kdevw7fdgfaz56n@uno.localdomain>","In-Reply-To":"<20210329091343.3kdevw7fdgfaz56n@uno.localdomain>","From":"Hirokazu Honda ","Date":"Tue, 30 Mar 2021 14:41:30 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] android: CameraDevice: Add\n\tstop()","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]